runc delete: fix for rootless cgroup + ro cgroupfs#4523
Merged
thaJeztah merged 3 commits intoopencontainers:mainfrom Nov 12, 2024
Merged
runc delete: fix for rootless cgroup + ro cgroupfs#4523thaJeztah merged 3 commits intoopencontainers:mainfrom
thaJeztah merged 3 commits intoopencontainers:mainfrom
Conversation
An issue with runc 1.2.0 was reported to buildkit, in which runc delete returns with an error, with the log saying: > unable to destroy container: unable to remove container's cgroup: open /sys/fs/cgroup/snschvixiy3s74w74fjantrdg: no such file or directory Apparently, what happens is runc is running with no cgroup access (because /sys/fs/cgroup is mounted read-only). In this case error to create a cgroup path (in runc create/run) is ignored, but cgroup removal (in runc delete) is not. This is caused by commit d3d7f7d, which changes the cgroup removal logic in RemovePath. In the current code, if the initial rmdir has failed (in this case with EROFS), but the subsequent os.ReadDir returns ENOENT, it is returned (instead of being ignored -- as the path does not exist and so there is nothing to remove). Here is the minimal fix for the issue. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If the sub-cgroup RemovePath has failed for any reason, return the error right away. This way, we don't have to check for err != nil before retrying rmdir. This is a cosmetic change and should not change any functionality. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Let's explain in greater details what's happening here and why. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Contributor
Author
|
As I said in #4518, ideally (long term) we should rework the code in the following way: IF runc failed to create the cgroup, but created the container (because Currently, |
Contributor
Author
|
As for the test case, I'm afraid we need a real read-only /sys/fs/cgroup. Maybe run the whole integration test thing in an unprivileged Docker container (which has /sys/fs/cgroup mounted read-only). Currently can't wrap my head around it. |
lifubang
approved these changes
Nov 12, 2024
This was referenced Nov 12, 2024
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
3 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 an alternative to #4520.
An issue with runc 1.2.0 was reported to buildkit and then here (#4518),
in which
runc deletereturns with an error, with the log saying:Apparently, what happens is runc is running with no cgroup access
(because /sys/fs/cgroup is mounted read-only). In this case error to
create a cgroup path (in runc create/run) is ignored, but cgroup removal
(in runc delete) is not.
This is caused by commit d3d7f7d, which changes the cgroup removal
logic in
RemovePath, and contains a bug. In the current code, if theinitial rmdir has failed (in this case with
EROFS), but the subsequentos.ReadDirreturnsENOENT, that error it is ultimately returned(instead of being ignored -- since the path does not exist, there is
nothing to remove).
This PR fixes the issue in 3 commits:
Only the first commit is changing the logic and fixing the issue -- the rest is mostly
cosmetic and "nice to have".
Fixes: #4518.
TODO: add a test case.