systemd cgroup v1 freeze fixes#3080
Closed
kolyshkin wants to merge 3 commits intoopencontainers:masterfrom
Closed
Conversation
If a control group is frozen, all its descendants will report FROZEN in freezer.state cgroup file. OTOH cgroup v2 cgroup.freeze is not reporting the cgroup as frozen unless it is frozen directly (i.e. not via an ancestor). Fix the discrepancy between v1 and v2 drivers behavior by looking into freezer.self_freezing cgroup file, which, according to kernel documentation, will show 1 iff the cgroup was frozen directly. Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com> Signed-off-by: Odin Ugedal <odin@uged.al> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
commented
Jul 9, 2021
Comment on lines
+283
to
+285
| if err == nil { | ||
| m.cgroups.Resources.Freezer = state | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I don't know if this is necessary, but am inclined to keep this just in case.
We do a temporary freeze/unfreeze around setting systemd unit properties. There are two minor issues around it: First, m.Freeze method changes m.cgroups.Resources.Frozen field, which it shouldn't. Fix this by adding/using a method which does not changes it. Second, the current code does freeze/thaw unconditionally, meaning it also freezes the already frozen cgroup (which is a no-op except it still requires some reads and writes to freezer.state), and thaws the cgroup which is about to be frozen again. Fix by making freeze/unfreeze conditional. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test the issues fixed by the two preceding commits. Co-Authored-By: Kir Kolyshkin <kolyshkin@gmail.com> Signed-off-by: Odin Ugedal <odin@uged.al> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Contributor
Author
|
As suggested in #3065 (comment), this can be further improved to avoid the freeze entirely if we're sure systemd won't set the deny-all device rule. Looking at |
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a combo of #3065 and #3072, fixing issues resulting in kubernetes revert of runc 1.0.0 vendoring.
Fix cgroup v1
GetFreezerStateto not report cgroup state as frozen if a parent cgroup is frozen.After the fix, we only report the cgroup is frozen if it is frozen directly (and not via an ancestor).
This is how cgroup v2 GetFreezerState works, so now there's no discrepancy between v1 and v2 behavior.
Fix cgroup/systemd v1 Set to not leave sub-cgroup frozen in case a parent cgroup is frozen.
This issue mostly stems from the
GetFreezerStatereporting incorrect state (fixed above).Cleanup cgroup/systemd v1 freeze/thaw code (do not overwrite cgroup resources freezer).
Avoid unnecessary freeze/thaw.
Tests for all the above issues.
Co-authored-by: @odinuge