systemd: switch to unified cgroup hierarchy by default #104094

flokli · 2020-11-17T21:16:53Z

Other distros, like Fedora 32 have already switched to it.

Users that need to keep using the old cgroup hierarchy (namely because
they want to run docker, and not some of the alternatives supporting
cgroupsv2 can re-enable it by setting systemd.enableUnifiedCgroupHierarchy = false;.

Fixes #73800

Motivation for this change

Things done

grahamc

I have this in my config:

   boot.kernelParams = (if config.virtualisation.docker.enable then [] else [ "cgroup_no_v1=all" "systemd.unified_cgroup_hierarchy=yes" ]);

maybe we should automatically specify this for the docker services?

nixos/doc/manual/release-notes/rl-2103.xml

pkgs/os-specific/linux/systemd/default.nix

flokli

I have this in my config:

boot.kernelParams = (if config.virtualisation.docker.enable then [] else [ "cgroup_no_v1=all" "systemd.unified_cgroup_hierarchy=yes" ]);

maybe we should automatically specify this for the docker services?

Yes, adding "systemd.unified_cgroup_hierarchy=0 if docker is enabled might make sense, at least as long as docker doesn't support cgroupv2.
We probably should add link to the upstream tracking issues.

It seems it's supported in runc: opencontainers/runc#2315 (comment) and should be supported in docker 20, which was "not officially released yet, and its ETA still unpublished" in September 2020: moby/moby#40360 (comment)

SuperSandro2000 · 2020-11-18T03:13:54Z

Docker 20.10.0-rc1 (https://github.com/moby/moby/releases) was released today which should add cgroupsv2 and apparmor 3.0-beta support. Maybe we can wait until it is released?

flokli · 2020-11-18T08:56:15Z

@SuperSandro2000 are you aware of any timelines on docker 20.* getting released? Would you be up to packaging their rc, to see how it behaves? ;-)

I surely want to add/extend some more tests about this before merging, and see how config options for other container runtimes might need to look like (if it requires special configuration).

ajs124 · 2020-11-18T22:25:55Z

This will make user@ services fail when using with the hardened profile. Specifically this line of the hardened profile and this issue marked as "not supported".

There are several solutions to this and it definitely should not block this, but I thought it was worth noting.

cc @joachifm (because of the hardened profile).

flokli · 2020-11-18T23:20:05Z

I pushed a new version exposing a systemd.unifiedCgroupHierarchy option (defaulting to true, adding the kernel cmdline if set to false), that is automatically set to false by hidepid and when docker is enabled.

I still want to some checks for IOAccounting and IPAccounting to our systemd tests.

flokli · 2020-11-19T00:15:40Z

I also did some more experiments:

cri-o works fine with unified cgroups.
k3s with docker as container runtime obviously still needs unified cgroups disabled
k3s with their bundled containerd doesn't work with cgroupsv2 either

I made sure the unified cgroup hierarchy is disabled in these cases and the tests succeed again.

flokli · 2020-11-19T01:34:44Z

I added some more tests on the accounting parts. I couldn't reliably get IO Accounting on individual units tested, but the test ensures system.slice has non-zero IO accounting values :-)

This is ready for review, PTAL.

SuperSandro2000 · 2020-11-19T01:44:02Z

are you aware of any timelines on docker 20.* getting released?

No but I assume it could take a month.

Would you be up to packaging their rc, to see how it behaves?

I can get it to build but for testing I miss experience.

flokli · 2020-11-19T01:58:18Z

are you aware of any timelines on docker 20.* getting released?

No but I assume it could take a month.

Okay. So I propose we keep this PR as-is (disable unified cgroup when you enable docker), and we can remove this once docker has landed and supports it.

Would you be up to packaging their rc, to see how it behaves?

I can get it to build but for testing I miss experience.

nixosTests.docker should probably be enough to verify basic functionality, and once that works, people could take a more closer look in that new PR :-)

SuperSandro2000 · 2020-11-19T09:28:46Z

nixosTests.docker should probably be enough to verify basic functionality, and once that works, people could take a more closer look in that new PR :-)

KVM does not want to work with nix tests on my machine. Not sure why.

ajs124

LGTM, in general. Some notes, although they're not too important.

nixos/modules/security/hidepid.nix

nixos/modules/services/cluster/k3s/default.nix

nixos/tests/systemd.nix

nixos/modules/system/boot/systemd.nix

See https://www.redhat.com/sysadmin/fedora-31-control-group-v2 for details on why this is desirable, and how it impacts containers. Users that need to keep using the old cgroup hierarchy can re-enable it by setting `systemd.unifiedCgroupHierarchy` to `false`. Well-known candidates not supporting that hierarchy, like docker and hidepid=… will disable it automatically. Fixes NixOS#73800

This gets automatically disabled by docker if the docker backend is used, but the bundled containerd also doesn't seem to support cgroupsv2, so disable it explicitly here, too.

For now, testing IO Accounting is skipped, as it seems to be either broken, or hard to reproduce in a VM.

zowoq · 2020-11-19T23:30:06Z

nixos/tests/podman.nix

-          podman.succeed(
-              su_cmd(
-                  "podman run --runtime=crun -d --name=sleeping -v /nix/store:/nix/store -v /run/current-system/sw/bin:/bin scratchimg /bin/sleep 10"
+                  "podman run -d --name=sleeping -v /nix/store:/nix/store -v /run/current-system/sw/bin:/bin scratchimg /bin/sleep 10"


Can we keep the explicit --runtime=crun/ Run container rootless with crun please?

Can you elaborate on why? podman seems to default to it with cgroupsv2, and if we view this test from a user perspective, they just want to podman run as non-root, without explicitly configuring a specific runtime.

also cc @adisbladis

I just want to say I agree with @flokli on this one. It's better that we test the users default config.
Also I seem to recall seeing some intent to switch back to runc by default at some point once it's gained the required cgroupsv2 support (don't quote me on this one).

Can we do this instead?

Edit: just realised that the original diff was incomplete.

diff --git a/nixos/tests/podman.nix b/nixos/tests/podman.nix index cd8c2b4308c..3b54d793b1b 100644 --- a/nixos/tests/podman.nix +++ b/nixos/tests/podman.nix @@ -53,15 +53,24 @@ import ./make-test-python.nix ( podman.succeed("podman stop sleeping") podman.succeed("podman rm sleeping") + with subtest("Run container as root with default runtime"): + podman.succeed("tar cv --files-from /dev/null | podman import - scratchimg") + podman.succeed( + "podman run -d --name=sleeping -v /nix/store:/nix/store -v /run/current-system/sw/bin:/bin scratchimg /bin/sleep 10" + ) + podman.succeed("podman ps | grep sleeping") + podman.succeed("podman stop sleeping") + podman.succeed("podman rm sleeping") + with subtest("Run container rootless with runc"): podman.succeed(su_cmd("tar cv --files-from /dev/null | podman import - scratchimg")) - podman.succeed( + podman.fail( su_cmd( "podman run --runtime=runc -d --name=sleeping -v /nix/store:/nix/store -v /run/current-system/sw/bin:/bin scratchimg /bin/sleep 10" ) ) - podman.succeed(su_cmd("podman ps | grep sleeping")) - podman.succeed(su_cmd("podman stop sleeping")) + podman.fail(su_cmd("podman ps | grep sleeping")) + podman.fail(su_cmd("podman stop sleeping")) podman.succeed(su_cmd("podman rm sleeping")) with subtest("Run container rootless with crun"): @@ -74,6 +83,17 @@ import ./make-test-python.nix ( podman.succeed(su_cmd("podman ps | grep sleeping")) podman.succeed(su_cmd("podman stop sleeping")) podman.succeed(su_cmd("podman rm sleeping")) + + with subtest("Run container rootless with default runtime"): + podman.succeed(su_cmd("tar cv --files-from /dev/null | podman import - scratchimg")) + podman.succeed( + su_cmd( + "podman run -d --name=sleeping -v /nix/store:/nix/store -v /run/current-system/sw/bin:/bin scratchimg /bin/sleep 10" + ) + ) + podman.succeed(su_cmd("podman ps | grep sleeping")) + podman.succeed(su_cmd("podman stop sleeping")) + podman.succeed(su_cmd("podman rm sleeping")) ''; } )

I don't think our test should suddenly fail if podmans runc backend starts working in rootless scenarios again.

I'm fine adding a test for the default backend in as-root scenarios, but made the rootless runc subtest a comment, which we can re-add once it gets implemented.

I don't think our test should suddenly fail if podmans runc backend starts working in rootless scenarios again.

I'm fine adding a test for the default backend in as-root scenarios, but made the rootless runc subtest a comment, which we can re-add once it gets implemented.

All of these packages have passthru.tests so the failure will be noticed when it does start working.

Can you move the default tests after the explict runtime tests please? If it does fail because of a runtime problem having it fail on the explict test would be clearer.

I can move the test order, sure, but I still don't think the test should suddenly fail if more things work. podman is still functional if a backend that was known to be broken starts working again. We shouldn't then fail the test.

I'd expect the maintainers of this package to read the release notes while bumping, and updating/extending the test accordingly once new features are added.

The runc backend doesn't work with unified cgroup hierarchy, and it failing is a known issue. However, the default backends should work in both rootless and as-root scenarios, so make sure we test these.

andir · 2020-11-20T19:27:36Z

nixos/modules/virtualisation/docker.nix

@@ -155,6 +155,9 @@ in
      users.groups.docker.gid = config.ids.gids.docker;
      systemd.packages = [ cfg.package ];

+      # TODO: remove once docker 20.10 is released
+      systemd.enableUnifiedCgroupHierarchy = false;


Would it make sense to make this conditional on the docker version? Users can set the docker package at runtime and defaulting this to the right version automatically might be a nice perk that we can implement rather quickly in NixOS?

We don't know yet if docker 20.10 really works. I'm happy to take a closer look at this once it is in Nixpkgs. There currently is no docker working with enabled unified cgroup hierarchy, so I'm not much in favor of code for the eventuality of it being there ;-)

flokli · 2020-11-22T21:35:38Z

Let's get this in. If there's other applications that don't really work with cgroupsv2, disabling it for them is pretty trivial by setting systemd.enableUnifiedCgroupHierarchy = false;.

Since NixOS#104094 (d22b3ed), NixOS is using the unified cgroup hierarchy by default (aka cgroupv2). This means the blkio controller isn't there, so we should test for something else (e.g. the presence of the io controller). Fixes NixOS#105581.

Since #104094 (d22b3ed), NixOS is using the unified cgroup hierarchy by default (aka cgroupv2). This means the blkio controller isn't there, so we should test for something else (e.g. the presence of the io controller). Fixes #105581.

flokli requested review from grahamc, arianvp, andir and picnoir November 17, 2020 21:16

flokli added this to To Do in systemd via automation Nov 17, 2020

grahamc reviewed Nov 17, 2020

View reviewed changes

nixos/doc/manual/release-notes/rl-2103.xml Outdated Show resolved Hide resolved

andir reviewed Nov 17, 2020

View reviewed changes

pkgs/os-specific/linux/systemd/default.nix Show resolved Hide resolved

ofborg bot added 6.topic: nixos 8.has: changelog 8.has: documentation labels Nov 17, 2020

flokli force-pushed the systemd-unified-cgroup-hierarchy branch from b50247e to 26541b9 Compare November 17, 2020 21:30

flokli commented Nov 17, 2020

View reviewed changes

ofborg bot requested review from andir and edolstra November 17, 2020 22:28

ofborg bot added 11.by: package-maintainer 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Nov 17, 2020

flokli force-pushed the systemd-unified-cgroup-hierarchy branch from 26541b9 to aa64c18 Compare November 18, 2020 23:18

ofborg bot added the 8.has: module (update) label Nov 18, 2020

flokli marked this pull request as ready for review November 19, 2020 01:33

flokli requested a review from joachifm as a code owner November 19, 2020 01:33

flokli mentioned this pull request Nov 19, 2020

systemd unit doesn't report IP traffic #104070

Closed

flokli requested review from dasJ and ajs124 November 19, 2020 13:39

ajs124 approved these changes Nov 19, 2020

View reviewed changes

nixos/modules/security/hidepid.nix Outdated Show resolved Hide resolved

nixos/modules/services/cluster/k3s/default.nix Show resolved Hide resolved

nixos/tests/systemd.nix Show resolved Hide resolved

joachifm removed their request for review November 19, 2020 15:43

arianvp reviewed Nov 19, 2020

View reviewed changes

nixos/modules/system/boot/systemd.nix Outdated Show resolved Hide resolved

flokli added 3 commits November 19, 2020 16:56

nixos/k3s: disable unifiedCgroupHierarchy

5d45f26

This gets automatically disabled by docker if the docker backend is used, but the bundled containerd also doesn't seem to support cgroupsv2, so disable it explicitly here, too.

nixosTests.systemd: increase accounting coverage

f683297

For now, testing IO Accounting is skipped, as it seems to be either broken, or hard to reproduce in a VM.

flokli force-pushed the systemd-unified-cgroup-hierarchy branch from d5d53d6 to f683297 Compare November 19, 2020 16:00

flokli requested a review from zowoq as a code owner November 19, 2020 23:18

zowoq reviewed Nov 19, 2020

View reviewed changes

flokli force-pushed the systemd-unified-cgroup-hierarchy branch from 16ef7f1 to 8de38fa Compare November 20, 2020 11:16

nixosTests.podman: run default backends, don't run runc rootless

90d5bdb

The runc backend doesn't work with unified cgroup hierarchy, and it failing is a known issue. However, the default backends should work in both rootless and as-root scenarios, so make sure we test these.

flokli force-pushed the systemd-unified-cgroup-hierarchy branch from 8de38fa to 90d5bdb Compare November 20, 2020 15:24

andir reviewed Nov 20, 2020

View reviewed changes

flokli merged commit c768913 into NixOS:staging Nov 22, 2020

systemd automation moved this from To Do to Done Nov 22, 2020

flokli deleted the systemd-unified-cgroup-hierarchy branch November 22, 2020 21:35

lukegb mentioned this pull request Dec 1, 2020

nixos/tests/misc: account for cgroupv2 #105594

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

systemd: switch to unified cgroup hierarchy by default #104094

systemd: switch to unified cgroup hierarchy by default #104094

flokli commented Nov 17, 2020 •

edited

grahamc left a comment

flokli left a comment •

edited

SuperSandro2000 commented Nov 18, 2020 •

edited

flokli commented Nov 18, 2020 •

edited

ajs124 commented Nov 18, 2020

flokli commented Nov 18, 2020

flokli commented Nov 19, 2020

flokli commented Nov 19, 2020

SuperSandro2000 commented Nov 19, 2020

flokli commented Nov 19, 2020 •

edited

SuperSandro2000 commented Nov 19, 2020

ajs124 left a comment

zowoq Nov 19, 2020

flokli Nov 19, 2020

adisbladis Nov 20, 2020

zowoq Nov 20, 2020 •

edited

flokli Nov 20, 2020

zowoq Nov 20, 2020

flokli Nov 20, 2020

flokli Nov 20, 2020

andir Nov 20, 2020

flokli Nov 20, 2020

flokli commented Nov 22, 2020

systemd: switch to unified cgroup hierarchy by default #104094

systemd: switch to unified cgroup hierarchy by default #104094

Conversation

flokli commented Nov 17, 2020 • edited

Motivation for this change

Things done

grahamc left a comment

Choose a reason for hiding this comment

flokli left a comment • edited

Choose a reason for hiding this comment

SuperSandro2000 commented Nov 18, 2020 • edited

flokli commented Nov 18, 2020 • edited

ajs124 commented Nov 18, 2020

flokli commented Nov 18, 2020

flokli commented Nov 19, 2020

flokli commented Nov 19, 2020

SuperSandro2000 commented Nov 19, 2020

flokli commented Nov 19, 2020 • edited

SuperSandro2000 commented Nov 19, 2020

ajs124 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zowoq Nov 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flokli commented Nov 22, 2020

flokli commented Nov 17, 2020 •

edited

flokli left a comment •

edited

SuperSandro2000 commented Nov 18, 2020 •

edited

flokli commented Nov 18, 2020 •

edited

flokli commented Nov 19, 2020 •

edited

zowoq Nov 20, 2020 •

edited