linux: add /sys/fs/cgroup if /sys is a bind mount #17034
Conversation
openshift-ci
bot
added
release-note-none
approved
| @@ -972,6 +972,9 @@ EOF | |||
| is $output "$randomname" "Should be able to find container by name" | |||
| run_podman rm "/$randomname" | |||
| run_podman 125 create --name "$randomname/" $IMAGE | |||
|
|
|||
test/system/030-run.bats
Outdated
| @@ -972,6 +972,9 @@ EOF | |||
| is $output "$randomname" "Should be able to find container by name" | |||
| run_podman rm "/$randomname" | |||
| run_podman 125 create --name "$randomname/" $IMAGE | |||
|
|
|||
| @test "podman run --net=host --cgroupns=host with read only cgroupfs" { | |||
| run_podman run --net=host --cgroupns=host --rm -ti $IMAGE grep "^.*/sys/fs/cgroup.*ro.*-.*$" /proc/self/mountinfo | |||
There was a problem hiding this comment.
This will pass on main, because the ro will match on the "ro" in "cgROups". A better grep/awk is needed here but I can't tell what that is because, with this patch, I get two cgroup lines from grep:
$ bin/podman run --net=host --cgroupns=host --rm $iii grep -E '/sys/fs/cgroup' /proc/self/mountinfo
1529 1527 0:26 / /sys/fs/cgroup rw,nosuid,nodev,noexec,relatime - cgroup2 cgroup2 rw,seclabel
1547 1529 0:26 / /sys/fs/cgroup ro,nosuid,nodev,noexec,relatime - cgroup2 cgroup2 rw,seclabel
^^-- this is the diff(rootless. Root, I get only the last line, the "ro" one).
Also, please remove the -ti, it's not needed
There was a problem hiding this comment.
thanks, I've really messed that up.
The simplest test would be "python -c 'import os; import stat; print(os.statvfs("/sys/fs/cgroup").f_flag & 1)'" but sadly there is no python in the image, and stat -f doesn't expose the f_flag attribute; so the best I could come with is to retrieve the last /sys/fs/cgroup mount and verify it is mounted with ro
|
Here's a working test: @test "podman run --net=host --cgroupns=host with read only cgroupfs" {
run_podman run --net=host --cgroupns=host --rm $IMAGE awk '$5 == "/sys/fs/cgroup" { print $6 }' /proc/self/mountinfo
# Rootless, the above will emit two lines. We only care about the last one.
# (Root, there's only one match, and it's always ro, even before this PR)
assert "${lines[-1]}" =~ ^ro "/sys/fs/cgroup is mounted read-only"
}This test fails on main, but only rootless. Root passes. You may want to amend your commit message to make it clear that this fixes a rootless-only bug. |
|
LGTM once the test problems and Ed's comments are worked out. |
|
LGTM |
if /sys is bind mounted from the host then also add an explicit mount for /sys/fs/cgroup so that 'ro' is honored. Signed-off-by: Giuseppe Scrivano <[email protected]>
giuseppe
force-pushed
the
bind-mount-sys-fs-cgroup-ro
branch
from
d7b0409 to
cf36470
Compare
| @test "podman run --net=host --cgroupns=host with read only cgroupfs" { | ||
| # verify that the last /sys/fs/cgroup mount is read-only | ||
| run_podman run --net=host --cgroupns=host --rm $IMAGE sh -c "grep ' / /sys/fs/cgroup ' /proc/self/mountinfo | tail -n 1 | grep '/sys/fs/cgroup ro'" | ||
| } |
There was a problem hiding this comment.
Can't do that. You can't run_podman | anything. I encourage you to use the test code I already wrote. And I especially encourage you to use hack/bats:
$ hack/bats 030:"read only cgr"...and to use it against a podman that does not include your fixes. If the test passes against /usr/bin/podman or against podman @ main, your test is broken.
There was a problem hiding this comment.
the pipes are part of the command that runs inside the container.
The test fails when I use an older podman version
There was a problem hiding this comment.
Oops - sorry, I misread it. I still wish the test was better commented, but I won't block over that.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, 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 |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
if /sys is bind mounted from the host then also add an explicit mount for /sys/fs/cgroup so that 'ro' is honored.
Signed-off-by: Giuseppe Scrivano [email protected]
Does this PR introduce a user-facing change?