Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#786) Set required memory for container #791

Merged
merged 1 commit into from
Jun 1, 2024

Conversation

pauby
Copy link
Member

@pauby pauby commented Jul 21, 2023

Description Of Changes

Sets the required memory for the docs repo container to 4 GB. Note that this is the amount that I need to set it to. The issue #786 didn't specify a memory number.

Motivation and Context

Less than 4GB memory in the container, in my setup, causes the site not to build.

Testing

  • I have previewed these changes using the Docker Container or another method before submitting this pull request.

Change Types Made

  • Minor documentation fix (typos etc.).
  • Major documentation change (refactoring, reformatting or adding documentation to existing page).
  • New documentation page added.
  • The change I have made should have a video added, and I have raised an issue for this.
    • Issue #

Change Checklist

  • Requires a change to menu structure (top or left hand side)/
  • Menu structure has been updated

Related Issue

Fixes #786

@pauby pauby requested a review from corbob July 21, 2023 09:47
Copy link
Member

@corbob corbob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to say also that 4GB is what the base codespace that I have uses, but when running locally 4GB is fine 🤔 .

2023-07-21_06-18-31

For now I'd say we leave it with the 4, and once the parsing error is fixed up can run it in codespaces and confirm it works.

Looking closer at the docs, they show a whole collection of code spaces, while I only have two listed. Now I'm starting to wonder if the memory issue is intermittent and depends on if other codespaces are running at the same time 🤔

@@ -10,6 +10,9 @@
"NODE_VERSION": "16"
}
},
"hostRequirements": {
"memory": "4gb"
}
Copy link
Member

@corbob corbob Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
},

I think this is what the codespace error is saying is missing... Although not too sure 😂 Interestingly this is not a problem for vscode locally.

2023-07-21_06-03-27

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed this.

@corbob
Copy link
Member

corbob commented Jul 21, 2023

So it looks like... In codespaces, the container when given 4GB idles at about 1700 MB of free memory... locally it idles at about 2000... This is only 300 MB so seems like it shouldn't really be a concern... However, when Statiq is previewing the site locally, the container has around 100 MB free 😂 Locally it also seems to dip into swap which codespaces is a constant 0.

I'd be interested to see if bumping the memory to 8 GB resulted in adverse behaviours locally if you don't have the 8 GB assigned to docker.

Edit: Locally I've bumped this to 8GB, and reduced docker to 1GB... and it doesn't complain at all about not having enough memory (the preview of course fails, but the dev container is otherwise fine with it) 👍

@pauby
Copy link
Member Author

pauby commented Jul 21, 2023

Based on what you said, do we bump this from 4GB to 8GB?

@corbob
Copy link
Member

corbob commented Jul 21, 2023

Based on what you said, do we bump this from 4GB to 8GB?

@pauby I think so. At least for this PR, then can easily test that codespaces launches the proper container.

@pauby
Copy link
Member Author

pauby commented Jul 21, 2023

I've updated to use 8GB.

@corbob
Copy link
Member

corbob commented Jul 21, 2023

OK, Codespaces are hurting my brain. To my knowledge, all of the ones I create say they're being created under corbob, but with the memory set to 8, I get this message regarding not being an applicable SKU:
image

When I try to create any codespace, I have two options, one 2 core with 4 GB memory, and the other 4 core with 8GB memory. Would it make sense to try setting cpus to 4 and memory to 8gb?
2023-07-21_09-53-53

@gep13
Copy link
Member

gep13 commented Nov 27, 2023

@corbob @pauby this PR seems to have been forgotten about.

Can either of you please provide an update here on what needs to be done to get this completed?

@pauby
Copy link
Member Author

pauby commented Jun 1, 2024

Based on @corbob comment above I've added cpus and rebased. @gep13 this is ready for review / merge.

Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gep13 gep13 merged commit d1aed7e into chocolatey:master Jun 1, 2024
1 check passed
@gep13
Copy link
Member

gep13 commented Jun 1, 2024

Happy to see this merged in for just now, this requirement will likely not be needed once @st3phhays has finished her upcoming work with Astro.

choco-bot pushed a commit that referenced this pull request Jun 1, 2024
Merge pull request #791 from pauby/maint/container-memory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set the default codespaces container larger
3 participants