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

validation: add cgroup devices validation #633

Merged
merged 1 commit into from
May 24, 2018

Conversation

zhouhao3
Copy link

@zhouhao3 zhouhao3 commented May 3, 2018

On the one hand in order to achieve devices validation, on the other hand in order to achieve the following specerror

DevicesApplyInOrder: The runtime MUST apply entries in the listed order.

But the test results are not the same as I expected, so I want to see what do you think.

➜  runtime-tools git:(devices-validation) ✗ sudo ./validation/linux_cgroups_devices.t
TAP version 13
ok 1 - find devices
ok 2 - get devices data
not ok 3 - devices c 10:229 rwm is set correctly
# expect: c 10:229 rwm, actual: a 0:0 rwm
not ok 4 - devices a 10:200 r is set correctly
# expect: a 10:200 r, actual: a 0:0 rwm
1..4

@wking @liangchenye @alban @dongsupark PTAL

Signed-off-by: Zhou Hao [email protected]

@dongsupark
Copy link
Contributor

When creating a cgroup like this,

# mkdir /sys/fs/cgroup/devices/cgrouptest
# cat /sys/fs/cgroup/devices/cgrouptest/devices.list 
a *:* rwm

all devices are allowed by default, so that the major, minor numbers are always interpreted to 0:0. We would not want it to happen.

How about doing initialization by denying all devices like this, before running tests?

# echo "a *:* rwm" > /sys/fs/cgroup/devices/cgrouptest/devices.deny
# cat /sys/fs/cgroup/devices/cgrouptest/devices.list
#

@zhouhao3
Copy link
Author

zhouhao3 commented May 7, 2018

How about doing initialization by denying all devices like this, before running tests?

I think it's hard to confirm this time, devices are built in runc, and initialization should be done after creation.

But when I use runc alone to start a container, It will implement devices correctly.

➜  ub sudo runc run ub
# 
➜  ~ cd /sys/fs/cgroup/devices/cgroupstest 
➜  cgroupstest cat devices.list 
c 133:229 rw
b 8:0 r
c *:* m
b *:* m
c 1:3 rwm
c 1:8 rwm
c 1:7 rwm
c 5:0 rwm
c 1:5 rwm
c 1:9 rwm
c 5:1 rwm
c 136:* rwm
c 5:2 rwm
c 10:200 rwm
➜  cgroupstest 

@zhouhao3 zhouhao3 force-pushed the devices-validation branch from fa93b60 to 762800c Compare May 11, 2018 06:00
@liangchenye
Copy link
Member

But when I use runc alone to start a container, It will implement devices correctly.

How do you start it, runc run -b bundlerdir ID ?

@zhouhao3
Copy link
Author

How do you start it, runc run -b bundlerdir ID ?

Just configured a runc can start the file, modify the config file, and then start directly.

➜  ls
config.json  rootfs
➜ sudo runc run ub

g.SetLinuxCgroupsPath(cgroups.AbsCgroupPath)
g.AddLinuxResourcesDevice(true, "c", &major1, &minor1, "rwm")
g.AddLinuxResourcesDevice(false, "b", &major2, &minor2, "rw")
g.AddLinuxResourcesDevice(true, "a", &major3, &minor3, "r")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@q384566678 I've just looked into this PR again.
True, as you said, when running manually runc run mycontainer, the device cgroups list shows the full result correctly.

Apart from that, I think the line above g.AddLinuxResourcesDevice(true, "a", &major3, &minor3, "r") causes the issue of the wrong device list.
Think about a sequence of the following command lines.

# echo "c 10:229 rwm" > /sys/fs/cgroup/devices/cgrouptest/devices.allow
# cat /sys/fs/cgroup/devices/cgrouptest/devices.list   # This shows a correct result
c 10:229 rwm

# echo "a 10:200 r" > /sys/fs/cgroup/devices/cgrouptest/devices.allow
# cat /sys/fs/cgroup/devices/cgrouptest/devices.list   # an unexpected result. Existing devices are now gone
a *:* rwm

According to the Kernel cgroup v1 document, doing echo a > /sys/fs/cgroup/1/devices.allow will add the 'a : rwm' entry to the whitelist. Apparently allowing all devices results in wiping out existing entries and adding a single wildcard entry, even when the input is given by a specific pair of major/minor number.

When testing without the line for a, it shows a lot better result, doesn't it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# echo "a 10:200 r" > /sys/fs/cgroup/devices/cgrouptest/devices.allow
# cat /sys/fs/cgroup/devices/cgrouptest/devices.list # an unexpected result. Existing devices are now gone
a *:* rwm

That's surprising to me. In the wild, I expect few users to care about major/minor but not block/char. Still, does anyone have time to file a kernel patch to either respect the passed value or error out when major or minor are passed with a?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, @dongsupark @liangchenye PTAL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@q384566678 Tested, and it works well. LGTM. 👍

@zhouhao3 zhouhao3 force-pushed the devices-validation branch from 762800c to 1794938 Compare May 21, 2018 08:56
@zhouhao3
Copy link
Author

@liangchenye PTAL

@liangchenye
Copy link
Member

liangchenye commented May 24, 2018

LGTM

Approved with PullApprove

@liangchenye
Copy link
Member

@q384566678 @dongsupark , as @wking mentioned, will you fire a bug to kernel?

@liangchenye liangchenye merged commit d9a8ce7 into opencontainers:master May 24, 2018
@zhouhao3
Copy link
Author

will you fire a bug to kernel?

I will try to do it when I have free time.

@zhouhao3 zhouhao3 deleted the devices-validation branch May 24, 2018 05:40
@zhouhao3 zhouhao3 mentioned this pull request May 24, 2018
44 tasks
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.

4 participants