-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
stats: get mem limit from the cgroup #18630
stats: get mem limit from the cgroup #18630
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
90f9c1f
to
35b533d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test and add something like SkipIfCrun("crun x.y.z required")
then someone could enable it at a later point instead of having no test at all.
35b533d
to
b822c40
Compare
yes that is a better idea. Added |
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold
b822c40
to
89087bc
Compare
b25b330 introduced this behaviour. It was fine at the time because we didn't support "container update", so the limit could not be changed at runtime. Since it is not possible to change the memory limit at runtime, read the limit as reported from the cgroup. containers/crun#1217 is required for crun. Closes: containers#18621 Signed-off-by: Giuseppe Scrivano <[email protected]>
89087bc
to
7c53a46
Compare
/lgtm |
b25b330 introduced this behaviour.
It was fine at the time because we didn't support "container update", so the limit could not be changed at runtime. Since it is not possible to change the memory limit at runtime, read the limit as reported from the cgroup.
containers/crun#1217 is required for crun.
[NO NEW TESTS NEEDED] needs a new crun release
Closes: #18621
Does this PR introduce a user-facing change?