Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_create_with_device_cgroup_rules: don't check devices.list #2940

Merged
merged 1 commit into from
Jul 26, 2022

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

@aiordache @ulyssessouza ptal

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CreateContainerTest::test_create_with_device_cgroup_rules fails on cgroups v2
3 participants