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

config-linux: add memory.checkBeforeUpdate #1158

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

kolyshkin
Copy link
Contributor

This setting can be used to mimic cgroup v1 behavior on cgroup v2,
when setting the new memory limit during update operation.

In cgroup v1, a limit which is lower than the current usage is rejected.

In cgroup v2, such a low limit is causing an OOM kill.

Ref: opencontainers/runc#3509

This setting can be used to mimic cgroup v1 behavior on cgroup v2,
when setting the new memory limit during update operation.

In cgroup v1, a limit which is lower than the current usage is rejected.

In cgroup v2, such a low limit is causing an OOM kill.

Ref: opencontainers/runc#3509

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

If enabled (`true`), runtime MAY check if a new memory limit is lower than the current usage, and MUST
reject the new limit. Practically, when cgroup v1 is used, the kernel rejects the limit lower than the
current usage, and when cgroup v2 is used, an OOM killer is invoked. This setting can be used on
cgroup v2 to mimic the cgroup v1 behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call out that there is TOCTOU race possible here and it won't be able to exactly match cgroups v1 behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me this is an implementation detail, and this is merely a spec. In fact it should not even explain why we need it, but I guess some context would not hurt.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

Thanks @kolyshkin

@kolyshkin kolyshkin added this to the v1.1.0 milestone Sep 1, 2022
@kolyshkin kolyshkin merged commit 494a5a6 into opencontainers:main Sep 9, 2022
utam0k added a commit to utam0k/oci-spec-rs that referenced this pull request Sep 10, 2022
utam0k added a commit to utam0k/oci-spec-rs that referenced this pull request Sep 10, 2022
@AkihiroSuda AkihiroSuda mentioned this pull request Jan 24, 2023
@AkihiroSuda AkihiroSuda mentioned this pull request Jun 26, 2023
12 tasks
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.

5 participants