Repository navigation
Fix/cgroup2 bind mount shared cgroupns - #5267
xujihui1985 wants to merge 1 commit into
Conversation
9a08cc5 to
922bd42
Compare
5c685ee to
0b7a122
Compare
0b7a122 to
9653c5c
Compare
1360929 to
491bf97
Compare
|
Hi @kolyshkin would you like to move forward with this PR? |
491bf97 to
c8ed922
Compare
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
Forgot to submit pending review comments, my bad.
Overall it's almost ready.
3786615 to
3d9d5a7
Compare
|
@lifubang @kolyshkin I have moved the checkpoint-related commit into a new PR: #5318, and fixed the other review suggestions. |
3d9d5a7 to
0a1029c
Compare
6c3174f to
4e14d47
Compare
4e14d47 to
e8edba3
Compare
|
@lifubang @kolyshkin I think I have fixed all the issue listed above, anything I can improve here? |
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
This is a bit out of scope for this PR, but I think we should test that the host mounts are not changed after tests (or after each test) as, in general, runc should not change host side mounts (with some exceptions).
@kolyshkin yes, the second commit is suppose to do this check e8edba3 so do you think we should run this check for each test? |
e8edba3 to
f5442f7
Compare
|
@lifubang @kolyshkin Hi everyone, just checking in on this PR since it's been pending for a while. Let me know if there's anything else I need to improve, or if we're good to move forward |
f5442f7 to
387008e
Compare
387008e to
f29c93a
Compare
|
@lifubang @kolyshkin Hi, since it has been a long time since last review, I would like to know if there is anything I need to improve on this PR. |
kolyshkin
left a comment
kolyshkin
left a comment
There was a problem hiding this comment.
Sorry, this fell off my radar somehow.
Part of second commit (that changes the rootfs_linux.go) should be in the first one. Yet better, just squash the commits.
Otherwise LGTM
f29c93a to
13816cc
Compare
@kolyshkin no worry, thanks for your reply, I have update the PR squash the commits into one |
|
@lifubang PTAL |
8373deb to
671312e
Compare
|
ping @lifubang |
There was a problem hiding this comment.
🟡 Changes recommended
Explicit host cgroup-namespace joins can still alter the host’s global cgroup2 options, and the regression test may miss these changes because it reads an unreliable mountinfo field.
2 open issues
What changed in this PR
This PR addresses #5258 by using the existing cgroup v2 hierarchy when no cgroup namespace is configured, avoiding fresh mounts that can change host-wide options such as nsdelegate.
Changes:
- Uses a private bind mount for the no-cgroup-namespace path.
- Preserves mount-first behavior and existing fallback handling for configured cgroup namespaces.
- Adds a regression assertion that host cgroup mount options remain unchanged.
| File | Description |
|---|---|
| tests/integration/mounts.bats | Compares host cgroup mount options before and after container execution. |
| libcontainer/rootfs_linux.go | Selects bind mounting when no cgroup namespace is configured and disables host-side propagation. |
Review effort: Balanced
| }) | ||
| if err == nil || (!errors.Is(err, unix.EPERM) && !errors.Is(err, unix.EBUSY)) { | ||
| return err | ||
| if c.cgroupns { |
There was a problem hiding this comment.
I think this is paranoid, if you insist this change, I will change
| } | ||
|
|
||
| function get_cgroup_mount_flags() { | ||
| awk '$5 == "/sys/fs/cgroup" {print $10; exit}' /proc/self/mountinfo |
There was a problem hiding this comment.
@xujihui1985 you marked this as resolved but probably forgot to git push?
There was a problem hiding this comment.
noting the issue is still not fixed
| } | ||
|
|
||
| function get_cgroup_mount_flags() { | ||
| awk '$5 == "/sys/fs/cgroup" {print $10; exit}' /proc/self/mountinfo |
There was a problem hiding this comment.
noting the issue is still not fixed
671312e to
dc9d957
Compare
dc9d957 to
55bfccf
Compare
|
rebased to fix binary-size job |
When cgroup namespaces is host, mounting a new cgroup2 fs instance for /sys/fs/cgroup can affect the host-visible cgroupfs mount state, including options such as nsdelegate. Avoid that by preferring a bind mount of the existing cgroup v2 hierarchy when cgroupns is host. Keep the existing cgroup2 mount-first logic for private cgroup namespaces, including the EPERM/EBUSY fallback to a bind mount and the rootless ENOENT masking behavior. Signed-off-by: sean <xujihui1985@gmail.com>
55bfccf to
10ff8ad
Compare
When cgroup namespaces is host, mounting a new cgroup2 fs instance for /sys/fs/cgroup can affect the host-visible cgroupfs mount state, including options such as nsdelegate.
Avoid that by preferring a bind mount of the existing cgroup v2 hierarchy when cgroupns is host. Keep the existing cgroup2 mount-first logic for private cgroup namespaces, including the EPERM/EBUSY fallback to a bind mount and the rootless ENOENT masking behavior.
this PR fix issue #5258