cgroup2: split fs2 from fs #2169

AkihiroSuda · 2019-11-07T11:25:40Z

split fs2 package from fs, as mixing up fs and fs2 is very likely to result in
unmaintainable code.

Signed-off-by: Akihiro Suda [email protected]

This PR is large, but almost all changes are in libcontainer/cgroups/fs2 and libcontainer/cgroups/systemd/unified_hierarchy.go.
Other changes are just for deduplicating files across cgroups/fs and cgroups/fs2.

AkihiroSuda · 2019-11-07T11:50:26Z

Test script:

#!/bin/sh
x(){
    echo + "$@"
    "$@"
}

for runtime in /usr/local/bin/crun /usr/local/sbin/runc; do
    echo "=== RUNTIME $runtime ==="
    for manager in cgroupfs systemd; do
        echo "= MANAGER $manager ="
        x sudo podman --runtime=$runtime --cgroup-manager=$manager run -d --name tmp -v /:/host --device /dev/sda1 --cgroupns=host --pids-limit=10 alpine top
        x sudo podman exec tmp cat /proc/self/cgroup
        x sudo podman exec tmp sh -c 'echo should be 10; cat /sys/fs/cgroup$(cat /proc/self/cgroup | sed -e s/0:://g)/pids.max'
        x sudo podman exec tmp sh -c 'echo should fail; hexdump -C /host/dev/sda | head -n 1'
        x sudo podman exec tmp sh -c 'echo should succeed; hexdump -C /host/dev/sda1 | head -n 1'
        x sudo podman rm -f tmp
    done
    echo "= MANAGER cgroupfs (rootless) ="
    x podman --runtime=$runtime --cgroup-manager=cgroupfs run --rm --cgroupns=host alpine cat /proc/self/cgroup
    # runc+systemd+rootless is not supported yet (#2163)
done

result (working as expected):

=== RUNTIME /usr/local/bin/crun ===
= MANAGER cgroupfs =
+ sudo podman --runtime=/usr/local/bin/crun --cgroup-manager=cgroupfs run -d --name tmp -v /:/host --device /dev/sda1 --cgroupns=host --pids-limit=10 alpine top
5bd05442b7b72d4dbe16a8ded0c1c8ac1b2a2a22ca1f50363f23d66f2371347e
+ sudo podman exec tmp cat /proc/self/cgroup
0::/libpod_parent/libpod-5bd05442b7b72d4dbe16a8ded0c1c8ac1b2a2a22ca1f50363f23d66f2371347e
+ sudo podman exec tmp sh -c echo should be 10; cat /sys/fs/cgroup$(cat /proc/self/cgroup | sed -e s/0:://g)/pids.max
should be 10
10
+ sudo podman exec tmp sh -c echo should fail; hexdump -C /host/dev/sda | head -n 1
should fail
hexdump: /host/dev/sda: Operation not permitted
hexdump: /host/dev/sda: Bad file descriptor
+ sudo podman exec tmp sh -c echo should succeed; hexdump -C /host/dev/sda1 | head -n 1
should succeed
00000000  eb 58 90 6d 6b 66 73 2e  66 61 74 00 02 08 20 00  |.X.mkfs.fat... .|
+ sudo podman rm -f tmp
5bd05442b7b72d4dbe16a8ded0c1c8ac1b2a2a22ca1f50363f23d66f2371347e
= MANAGER systemd =
+ sudo podman --runtime=/usr/local/bin/crun --cgroup-manager=systemd run -d --name tmp -v /:/host --device /dev/sda1 --cgroupns=host --pids-limit=10 alpine top
b7f24f4c38258791ba7a7cccc2967e4e8bd3141c9043fb9755888d06e13dbee6
+ sudo podman exec tmp cat /proc/self/cgroup
0::/machine.slice/libpod-b7f24f4c38258791ba7a7cccc2967e4e8bd3141c9043fb9755888d06e13dbee6.scope
+ sudo podman exec tmp sh -c echo should be 10; cat /sys/fs/cgroup$(cat /proc/self/cgroup | sed -e s/0:://g)/pids.max
should be 10
10
+ sudo podman exec tmp sh -c echo should fail; hexdump -C /host/dev/sda | head -n 1
should fail
hexdump: /host/dev/sda: Operation not permitted
hexdump: /host/dev/sda: Bad file descriptor
+ sudo podman exec tmp sh -c echo should succeed; hexdump -C /host/dev/sda1 | head -n 1
should succeed
00000000  eb 58 90 6d 6b 66 73 2e  66 61 74 00 02 08 20 00  |.X.mkfs.fat... .|
+ sudo podman rm -f tmp
b7f24f4c38258791ba7a7cccc2967e4e8bd3141c9043fb9755888d06e13dbee6
= MANAGER cgroupfs (rootless) =
+ podman --runtime=/usr/local/bin/crun --cgroup-manager=cgroupfs run --rm --cgroupns=host alpine cat /proc/self/cgroup
0::/user.slice/user-1001.slice/[email protected]/user.slice/podman-40576.scope
=== RUNTIME /usr/local/sbin/runc ===
= MANAGER cgroupfs =
+ sudo podman --runtime=/usr/local/sbin/runc --cgroup-manager=cgroupfs run -d --name tmp -v /:/host --device /dev/sda1 --cgroupns=host --pids-limit=10 alpine top
d1ca9ac588f32ad168420348734ccf1323d8bdf37d8f1fcee33e0520fea40e28
+ sudo podman exec tmp cat /proc/self/cgroup
0::/libpod_parent/libpod-d1ca9ac588f32ad168420348734ccf1323d8bdf37d8f1fcee33e0520fea40e28
+ sudo podman exec tmp sh -c echo should be 10; cat /sys/fs/cgroup$(cat /proc/self/cgroup | sed -e s/0:://g)/pids.max
should be 10
10
+ sudo podman exec tmp sh -c echo should fail; hexdump -C /host/dev/sda | head -n 1
should fail
hexdump: /host/dev/sda: Operation not permitted
hexdump: /host/dev/sda: Bad file descriptor
+ sudo podman exec tmp sh -c echo should succeed; hexdump -C /host/dev/sda1 | head -n 1
should succeed
00000000  eb 58 90 6d 6b 66 73 2e  66 61 74 00 02 08 20 00  |.X.mkfs.fat... .|
+ sudo podman rm -f tmp
d1ca9ac588f32ad168420348734ccf1323d8bdf37d8f1fcee33e0520fea40e28
= MANAGER systemd =
+ sudo podman --runtime=/usr/local/sbin/runc --cgroup-manager=systemd run -d --name tmp -v /:/host --device /dev/sda1 --cgroupns=host --pids-limit=10 alpine top
e96d5cff6ac7a27cacd13bc24207c167e707b86bbdb42904a3b20659b3910ddf
+ sudo podman exec tmp cat /proc/self/cgroup
0::/machine.slice/libpod-e96d5cff6ac7a27cacd13bc24207c167e707b86bbdb42904a3b20659b3910ddf.scope
+ sudo podman exec tmp sh -c echo should be 10; cat /sys/fs/cgroup$(cat /proc/self/cgroup | sed -e s/0:://g)/pids.max
should be 10
10
+ sudo podman exec tmp sh -c echo should fail; hexdump -C /host/dev/sda | head -n 1
should fail
hexdump: /host/dev/sda: Operation not permitted
hexdump: /host/dev/sda: Bad file descriptor
+ sudo podman exec tmp sh -c echo should succeed; hexdump -C /host/dev/sda1 | head -n 1
should succeed
00000000  eb 58 90 6d 6b 66 73 2e  66 61 74 00 02 08 20 00  |.X.mkfs.fat... .|
+ sudo podman rm -f tmp
e96d5cff6ac7a27cacd13bc24207c167e707b86bbdb42904a3b20659b3910ddf
= MANAGER cgroupfs (rootless) =
+ podman --runtime=/usr/local/sbin/runc --cgroup-manager=cgroupfs run --rm --cgroupns=host alpine cat /proc/self/cgroup
0::/user.slice/user-1001.slice/[email protected]/user.slice/podman-41213.scope

AkihiroSuda · 2019-11-15T13:40:15Z

@giuseppe @filbranden PTAL?

This PR is large, but almost all changes are in libcontainer/cgroups/fs2 and libcontainer/cgroups/systemd/unified_hierarchy.go.
Other changes are just for deduplicating files across cgroups/fs and cgroups/fs2.

filbranden · 2019-11-15T18:33:04Z

Thanks @AkihiroSuda!

I'm out this weekend and I'll be back on Tuesday. I'll give this PR my full attention when I'm back.

Cheers,
Filipe

filbranden

I tested this a bit and it works for me.

Also took a closer look at the code. Overall, everything looks good, I have a few nitpicks but mostly minor.

I really wonder about memory.high vs. memory.low, but looks like that's pre-existing, we can probably address that one in a follow up if needed.

Cheers!
Filipe

libcontainer/cgroups/fs2/fs2.go

libcontainer/cgroups/fs2/defaultpath.go

libcontainer/cgroups/fs2/fs2.go

libcontainer/cgroups/fs2/memory.go

libcontainer/cgroups/fs2/pids.go

libcontainer/cgroups/systemd/unified_hierarchy.go

libcontainer/factory_linux.go

split fs2 package from fs, as mixing up fs and fs2 is very likely to result in unmaintainable code. Inspired by containerd/cgroups#109 Fix opencontainers#2157 Signed-off-by: Akihiro Suda <[email protected]>

Signed-off-by: Akihiro Suda <[email protected]>

h-vetinari · 2019-12-09T22:59:05Z

I guess the fix for #2175 should be split off from this PR (seeing that the CI is currently broken, resp. how long PRs can take to be reviewed)?

AkihiroSuda · 2019-12-10T04:48:15Z

I tried to do that, but it was hard because the v1 codes and the v2 codes are entangled complicatedly on the git master.
I think it is safe to merge this PR. The v1 codes are untouched except deduplication of util functions, and unlikely to cause regression.

…#2169 gets merged) CI seems to have been broken due to some bad rebase during merging multiple cgroup2 PRs: opencontainers#2175 The failure is being fixed in opencontainers#2169, but still waiting for review. This PR updates .travis.yml to ignore the failure as a workaround until opencontainers#2169 gets merged. This PR can be closed if opencontainers#2169 can get merged right now (and that's even better). Signed-off-by: Akihiro Suda <[email protected]>

libcontainer/cgroups/fs/blkio.go

cyphar · 2020-01-01T03:22:57Z

LGTM. I don't really have a setup that I can use to test this at the moment, but it looks fairly reasonable and cleans up quite a bit of the less-than-pretty parts of the cgroupv2 initial implementation.

AkihiroSuda · 2020-01-02T16:44:45Z

@mrunalp PTAL?

mrunalp · 2020-01-14T00:23:23Z

LGTM

A new method was added to the cgroup interface when opencontainers#2177 was merged. After opencontainers#2177 got merged, opencontainers#2169 was merged without rebase (sorry!) and compilation was failing: libcontainer/cgroups/fs2/fs2.go:208:22: container.Cgroup undefined (type *configs.Config has no field or method Cgroup) Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda · 2020-01-14T02:17:42Z

follow-up: #2206

A new method was added to the cgroup interface when opencontainers#2177 was merged. After opencontainers#2177 got merged, opencontainers#2169 was merged without rebase (sorry!) and compilation was failing: libcontainer/cgroups/fs2/fs2.go:208:22: container.Cgroup undefined (type *configs.Config has no field or method Cgroup) Signed-off-by: Akihiro Suda <[email protected]>

This was referenced Nov 8, 2019

cgroup2: does not work with rootless podman #2163

Closed

kubelet: add initial support for cgroupv2 kubernetes/kubernetes#85218

Merged

filbranden approved these changes Nov 20, 2019

View reviewed changes

AkihiroSuda force-pushed the split-fs branch 2 times, most recently from bcea75a to 2ddb057 Compare December 6, 2019 06:20

cgroup2: split fs2 from fs

88e8350

split fs2 package from fs, as mixing up fs and fs2 is very likely to result in unmaintainable code. Inspired by containerd/cgroups#109 Fix opencontainers#2157 Signed-off-by: Akihiro Suda <[email protected]>

AkihiroSuda force-pushed the split-fs branch from 2ddb057 to 88e8350 Compare December 6, 2019 06:42

fs2: support legacy device spec (to pass CI)

ec49f98

Signed-off-by: Akihiro Suda <[email protected]>

This was referenced Dec 6, 2019

cgroup2: CI failure (bad rebase #2165): process_linux.go:297: applying cgroup configuration for process caused "mountpoint for not found" #2175

Closed

.travis.yml: add Fedora 31 vagrant box (for cgroup2) #2165

Merged

AkihiroSuda mentioned this pull request Dec 17, 2019

support cgroup2 moby/moby#40174

Merged

AkihiroSuda mentioned this pull request Dec 31, 2019

.travis.yml: ignore cgroup2 failures (workaround until #2169 gets merged) #2195

Closed

cyphar reviewed Jan 1, 2020

View reviewed changes

libcontainer/cgroups/fs/blkio.go Show resolved Hide resolved

mrunalp merged commit 5cc0dea into opencontainers:master Jan 14, 2020

AkihiroSuda mentioned this pull request Jan 14, 2020

fix merging #2177 and #2169 #2206

Closed

odinuge mentioned this pull request Jan 28, 2020

Bump dependency opencontainers/[email protected] kubernetes/kubernetes#87516

Merged

kolyshkin mentioned this pull request Apr 27, 2021

Fix fsmanager.GetStats performance (regression in rc93) #2921

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cgroup2: split fs2 from fs #2169

cgroup2: split fs2 from fs #2169

AkihiroSuda commented Nov 7, 2019 •

edited

Loading

AkihiroSuda commented Nov 7, 2019

AkihiroSuda commented Nov 15, 2019

filbranden commented Nov 15, 2019

filbranden left a comment

h-vetinari commented Dec 9, 2019

AkihiroSuda commented Dec 10, 2019

cyphar commented Jan 1, 2020 •

edited by caniszczyk

Loading

AkihiroSuda commented Jan 2, 2020

mrunalp commented Jan 14, 2020 •

edited by caniszczyk

Loading

AkihiroSuda commented Jan 14, 2020

cgroup2: split fs2 from fs #2169

cgroup2: split fs2 from fs #2169

Conversation

AkihiroSuda commented Nov 7, 2019 • edited Loading

AkihiroSuda commented Nov 7, 2019

AkihiroSuda commented Nov 15, 2019

filbranden commented Nov 15, 2019

filbranden left a comment

Choose a reason for hiding this comment

h-vetinari commented Dec 9, 2019

AkihiroSuda commented Dec 10, 2019

cyphar commented Jan 1, 2020 • edited by caniszczyk Loading

AkihiroSuda commented Jan 2, 2020

mrunalp commented Jan 14, 2020 • edited by caniszczyk Loading

AkihiroSuda commented Jan 14, 2020

AkihiroSuda commented Nov 7, 2019 •

edited

Loading

cyphar commented Jan 1, 2020 •

edited by caniszczyk

Loading

mrunalp commented Jan 14, 2020 •

edited by caniszczyk

Loading