Skip to content

Conversation

@thaJeztah
Copy link
Member

fixes #2939
relates to moby/moby#42941 (comment)

This test was verifying that the container has the right options set (through
docker inspect), but also checks if the cgroup-rules are set within the
container by reading /sys/fs/cgroup/devices/devices.list

Unlike cgroups v1, on cgroups v2, there is no file interface, and rules are
handled through ebpf, which means that the test will fail because this file
is not present.

From the Linux documentation for cgroups v2:
https://github.com/torvalds/linux/blob/v5.16/Documentation/admin-guide/cgroup-v2.rst#device-controller

(...)
Device controller manages access to device files. It includes both creation of
new device files (using mknod), and access to the existing device files.

Cgroup v2 device controller has no interface files and is implemented on top
of cgroup BPF. To control access to device files, a user may create bpf programs
of type BPF_PROG_TYPE_CGROUP_DEVICE and attach them to cgroups with
BPF_CGROUP_DEVICE flag. (...)

Given that setting the right cgroups is not really a responsibility of this SDK,
it should be sufficient to verify that the right options were set in the container
configuration, so this patch is removing the part that checks the cgroup, to
allow this test to be run on a host with cgroups v2 enabled.

Signed-off-by: Sebastiaan van Stijn github@gone.nl

This test was verifying that the container has the right options set (through `docker inspect`), but also checks if the cgroup-rules are set within the container by reading `/sys/fs/cgroup/devices/devices.list` Unlike cgroups v1, on cgroups v2, there is no file interface, and rules are handled through ebpf, which means that the test will fail because this file is not present. From the Linux documentation for cgroups v2: https://github.com/torvalds/linux/blob/v5.16/Documentation/admin-guide/cgroup-v2.rst#device-controller > (...) > Device controller manages access to device files. It includes both creation of > new device files (using mknod), and access to the existing device files. > > Cgroup v2 device controller has no interface files and is implemented on top > of cgroup BPF. To control access to device files, a user may create bpf programs > of type BPF_PROG_TYPE_CGROUP_DEVICE and attach them to cgroups with > BPF_CGROUP_DEVICE flag. (...) Given that setting the right cgroups is not really a responsibility of this SDK, it should be sufficient to verify that the right options were set in the container configuration, so this patch is removing the part that checks the cgroup, to allow this test to be run on a host with cgroups v2 enabled. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@milas milas merged commit 7168e09 into docker:master Jul 26, 2022
@thaJeztah thaJeztah deleted the fix_devices_cgroupv2 branch July 26, 2022 16:32
@milas milas added this to the 6.0.0 milestone Jul 30, 2022
@thaJeztah
Copy link
Member Author

Hmm.. right, so I got confused a bit, because GitHub showed that this PR's commit was not part of the repository when I wanted to verify if this one was in the v6.0.0 tag; 55e3e4c

Screenshot 2022-10-03 at 23 17 18

Then I noticed that the merged commit was different than the PR commit (7168e09b1628b85a09e95cf8bae6bfd94b61a6c4);

Screenshot 2022-10-03 at 23 18 52

And.. lost my GPG signing

Screenshot 2022-10-03 at 23 16 51

.. I guess this was merged with the "rebase and merge" option on GitHub? 😬 (things like the above are why I don't like that feature 😂)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants