Conversation
FindCgroup() refused to work on a host that only mounts the unified
hierarchy, and CgroupV2 was a stub whose methods carried no receivers,
so the type never satisfied the Cgroup interface. Every validation test
that reads resource limits back out of the cgroup filesystem failed with
"cgroupv2 is not supported yet" on any current distribution.
Read the unified control files instead: memory.{max,low,swap.max},
cpu.max, cpuset.{cpus,mems}, pids.max, hugetlb.<size>.max and io.max.
memory.swap.max limits swap alone while the configuration field covers
memory plus swap, so the memory limit is added back. cpuset files fall
back to the effective set, which is where the unified hierarchy reports
an inherited value.
Values the unified hierarchy does not carry are left unset rather than
guessed, so that a caller can tell them apart from a real zero. That
covers swappiness, disableOOMKiller, useHierarchy, kernel, kernelTCP
and the block IO weights, and also cpu.shares, which runtimes map onto
cpu.weight through a lossy conversion the runtime-spec does not define.
The devices and network controllers have no unified counterpart at all:
device access is enforced by an eBPF program that cannot be read back,
and net_cls and net_prio were never ported, so both report that rather
than a missing file.
Version() lets a caller ask which hierarchy it is talking to.
Relates to opencontainers#807.
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
The cgroup test bundles ask for memory.swappiness, memory.disableOOMKiller, memory.kernel, memory.kernelTCP, blockIO.leafWeight and the block IO weights on every host. None of those exist in the unified hierarchy, and the create operation requires a runtime that cannot apply a property to generate an error and not create the container. A correct cgroup v2 only runtime therefore fails these tests by doing exactly what the spec asks of it. Set those fields only where the hierarchy can carry them. cpu.shares goes the same way: runtimes map it onto cpu.weight with a lossy conversion the runtime-spec does not define, so the configured value cannot be recovered and asserting on it would test a convention rather than the spec. Compare the remaining fields with a helper that reports a value neither side carries as a diagnostic instead of a failure, so that one unsupported field no longer hides the checks that follow it: the block IO and cpu validators used to return early and silently drop every later assertion. linux_cgroups_relative_cpus grows the same behaviour by using the shared CPU validator its siblings already use rather than its own copy of it. Signed-off-by: Daniel Golle <daniel@makrotopia.org>
linux.resources.network and linux.resources.devices have no counterpart in the unified hierarchy at all. net_cls and net_prio were never ported, and device access is enforced by an eBPF program that cannot be read back from the filesystem, so there is nothing for these tests to compare the configuration against. Skip the four tests where the host runs a unified hierarchy, the way the suite already skips tests that do not apply to the platform it runs on. Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Both delete tests build the cgroup directory out of a literal /sys/fs/cgroup/pids. On a unified hierarchy that directory never exists, so delete_only_create_resources fails when it tries to populate it and delete_resources passes without testing anything: the path it expects the runtime to have removed was never there. Ask the Cgroup implementation where the cgroup lives instead, which also picks up the real mount point rather than assuming one. Move a process with cgroup.procs, which both hierarchies provide, rather than with the cgroup v1 only tasks file. Fixes opencontainers#807 Signed-off-by: Daniel Golle <daniel@makrotopia.org>
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
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.
FindCgroup()has returned "cgroupv2 is not supported yet" ever since 9a56096 ("add cgroups package") introduced the TODO in November 2017, andCgroupV2was a stub whose seven methods were declared without receivers, so the type never satisfied theCgroupinterface and nothing would have compiled hadFindCgroup()tried to return it. On a host that mounts only the unified hierarchy, which is every current distribution, each validation test that reads a resource limit back out of the cgroup filesystem fails at its first assertion, and every assertion behind that point has never run there at all. This implements the reader, stops the bundles asking for fields the unified hierarchy cannot express, and fixes the two delete tests that build a cgroup v1 path by hand.Fixes #807
The reader takes
memory.max,memory.low,memory.swap.max,cpu.max,cpuset.cpus,cpuset.mems,pids.max,hugetlb.<size>.maxandio.max. Two of those need care:memory.swap.maxlimits swap alone whilelinux.resources.memory.swapis defined as memory plus swap, so the memory limit is added back, and thecpusetfiles are empty while a cgroup inherits, so the reader falls back to the.effectivevariants. Fields the hierarchy has no way to express are left unset rather than guessed, and the validators now report an unset field as a TAP diagnostic instead of a failed check. That also fixes an older defect: the block IO and CPU validators returned early at the first field they could not read, silently dropping every assertion after it.Some configuration is simply not applicable, and the suite should not ask for it.
memory.kernel,memory.kernelTCP,memory.swappiness,memory.disableOOMKiller,blockIO.leafWeightand the per-device leaf weights have no unified counterpart, and runtime.md tells a runtime that cannot apply a property to generate an error and not create the container, so a correct cgroup v2 runtime is obliged to refuse them. crun does exactly that, withcannot set kernel memory with cgroupv2, and the suite counts it as six failures inlinux_cgroups_memoryalone.linux.resources.networkis in the same position for a different reason, net_cls and net_prio having never been ported at all, so those two tests are skipped through the sameutil.Skipthe suite already uses for tests that do not apply to the platform they are run on.linux.resources.devicesis skipped too, but for a narrower reason that is worth stating precisely, because it is not that the restrictions stop working. The unified hierarchy has no devices controller with a filesystem interface, so the resulting rules cannot be read back the waydevices.listallowed on cgroup v1. The restrictions themselves are still applied and still enforced, by an attachedBPF_CGROUP_DEVICEprogram in the runtimes that implement it. What does not carry over to the unified hierarchy is this test's read-back approach, not the property the test is checking. That property is better checked from inside the container anyway, by attempting access to a device the bundle denies and one it allows, which would exercise the one requirement the spec actually states here, that "the runtime MUST apply entries in the listed order", and would work on both hierarchies and for any runtime whatever mechanism it enforces with. I would like to do that as a separate pull request rather than fold it into this one, since it changes what the test asserts rather than where it reads from.cpu.sharesandblockIO.weightare deliberately left out of the assertions on a unified hierarchy. Both runc and crun map shares ontocpu.weightwithceil(10^((log2(s)²+125·log2(s))/612 − 7/34)), which is lossy and is not described anywhere in the runtime-spec, andblockIO.weightgoes toio.bfq.weightunconverted or toio.weightconverted depending on the IO scheduler in use. Asserting either would test a convention between two implementations rather than the specification. I am happy to add them if the spec grows a normative mapping, and can open that issue against runtime-spec if you would like it.Measured with crun 1.29.1 on a cgroup v2 only host:
linux_cgroups_pidslinux_cgroups_relative_pidslinux_cgroups_memorylinux_cgroups_relative_memorylinux_cgroups_network,linux_cgroups_devicesdelete_resourcesdelete_only_create_resourceslinux_cgroups_pidsneeds one more fix to reach 3 ok, the pointer comparison in #814; without it the third assertion still fails. Thecpuset,ioandhugetlbcontrollers are not delegated to an unprivileged user, solinux_cgroups_cpus,linux_cgroups_blkioandlinux_cgroups_hugetlbwere exercised through the new unit tests rather than end to end.cgroups/cgroups_v2_test.gocovers the parsing against a fake hierarchy, including themaxsentinel, the memory plus swap reconstruction, thecpusetfallback andio.maxwith per-keymax. It opens withvar _ Cgroup = (*CgroupV2)(nil), the assertion whose absence let a type that could not satisfy the interface sit in the tree for eight years.One note on CI. The
lintjob fails here, and it fails for a reason that predates this branch and is unrelated to it: golangci-lint v2.9.0, which the workflow pins, cannot read the export data of the Go 1.27 toolchain thatgo-version: stablenow installs, and reportscould not load export data ... export data version 4 is greater than maximum supported version 2against files ingenerate/seccomp/that this branch does not touch. It is the same breakage that ce94ed3 fixed for go1.26 by bumping the pin, and it needs the same treatment again. I am happy to send that as its own pull request if it would help.