You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhongbo Tian <wi...@gmail.com> on 2017/04/17 03:51:34 UTC
Re: Review Request 58337: Allowed whitelist additional devices in
cgroups devices subsystem.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58337/
-----------------------------------------------------------
(Updated April 17, 2017, 3:51 a.m.)
Review request for mesos and haosdent huang.
Summary (updated)
-----------------
Allowed whitelist additional devices in cgroups devices subsystem.
Bugs: MESOS-6791
https://issues.apache.org/jira/browse/MESOS-6791
Repository: mesos
Description
-------
Add allowed devices whitelist for cgroups/devices isolator.
Diffs
-----
src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp ca2727142a9f257168f3cae0958f7b4665b63cf6
src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 9b5cf83093796b0c0cc5057b612f80bc8b8ba72f
src/slave/flags.hpp 171f67e44518e858049d002fcf037715021da265
src/slave/flags.cpp 9365da2c8462a4375a99a86210b9d6ec628510fe
Diff: https://reviews.apache.org/r/58337/diff/3/
Testing (updated)
-------
For simple test:
- Launch without additional devices:
1. Start agent with `sudo mesos-agent --master=127.0.0.1:5050 --work_dir=/tmp/mesos --isolation=cgroups/devices`
2. try open `/dev/rtc0` and failed with permission denied. `sudo mesos-execute --master=127.0.0.1:5050 --name=test --command="head -c 0 /dev/rtc0"`
- Launch with additional devices:
1. Start agent with `sudo mesos-agent --master=127.0.0.1:5050 --work_dir=/tmp/mesos --isolation=cgroups/devices --cgroups_whitelist_devices=[{'"path":"/dev/rtc0", "mknod_access":true, "ad_access":true, "write_access":true}]'`
2. open `/dev/rtc0` successfully. `sudo mesos-execute --master=127.0.0.1:5050 --name=test --command="head -c 0 /dev/rtc0"`
Thanks,
Zhongbo Tian
Re: Review Request 58337: Allowed whitelist additional devices in
cgroups devices subsystem.
Posted by haosdent huang <ha...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58337/#review172067
-----------------------------------------------------------
Fix it, then Ship it!
Need to update `docs/configuration.md` as well. I could fix these when commit it, please take a look.
src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp
Lines 150-152 (patched)
<https://reviews.apache.org/r/58337/#comment245140>
I think pass a `vector<cgroups::devices::Entry> whitelistDeviceEntries` contains default and additional entries would be more simple.
src/slave/flags.hpp
Lines 102 (patched)
<https://reviews.apache.org/r/58337/#comment245138>
Perfer `cgroups_whitelist_devices` to keep consistent with `DEFAULT_WHITELIST_ENTRIES`.
src/slave/flags.cpp
Lines 456 (patched)
<https://reviews.apache.org/r/58337/#comment245139>
`devcies` seems no necessary. Let's use array here directly.
```
add(&Flags::cgroups_whitelist_devices,
"cgroups_whitelist_devices",
"JSON array representing the devices that will be additionally\n"
"whitelisted by cgroups devices subsystem. This will take effect\n"
"only when `cgroups/devices` is set in `--isolation` flag.\n"
"Example:\n"
"[\n"
" {\n"
" \"path\": \"/path/to/device\",\n"
" \"read_access\": true,\n"
" \"write_access\": false,\n"
" \"mknod_access\": false\n"
" }\n"
"]\n"
);
```
- haosdent huang
On April 17, 2017, 3:51 a.m., Zhongbo Tian wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58337/
> -----------------------------------------------------------
>
> (Updated April 17, 2017, 3:51 a.m.)
>
>
> Review request for mesos and haosdent huang.
>
>
> Bugs: MESOS-6791
> https://issues.apache.org/jira/browse/MESOS-6791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add allowed devices whitelist for cgroups/devices isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp ca2727142a9f257168f3cae0958f7b4665b63cf6
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 9b5cf83093796b0c0cc5057b612f80bc8b8ba72f
> src/slave/flags.hpp 171f67e44518e858049d002fcf037715021da265
> src/slave/flags.cpp 9365da2c8462a4375a99a86210b9d6ec628510fe
>
>
> Diff: https://reviews.apache.org/r/58337/diff/3/
>
>
> Testing
> -------
>
> For simple test:
>
> - Launch without additional devices:
> 1. Start agent with `sudo mesos-agent --master=127.0.0.1:5050 --work_dir=/tmp/mesos --isolation=cgroups/devices`
> 2. try open `/dev/rtc0` and failed with permission denied. `sudo mesos-execute --master=127.0.0.1:5050 --name=test --command="head -c 0 /dev/rtc0"`
>
>
> - Launch with additional devices:
> 1. Start agent with `sudo mesos-agent --master=127.0.0.1:5050 --work_dir=/tmp/mesos --isolation=cgroups/devices --cgroups_whitelist_devices=[{'"path":"/dev/rtc0", "mknod_access":true, "ad_access":true, "write_access":true}]'`
> 2. open `/dev/rtc0` successfully. `sudo mesos-execute --master=127.0.0.1:5050 --name=test --command="head -c 0 /dev/rtc0"`
>
>
> Thanks,
>
> Zhongbo Tian
>
>
Re: Review Request 58337: Allowed whitelist additional devices in
cgroups devices subsystem.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58337/#review172159
-----------------------------------------------------------
Hey, thanks for the code and the review. I think this is some API question that needs to be resolved before we can allow people to use this. Also, we need to add test and update the doc.
@haosdent, I'd prefer we revert this patch for now and figure out what's the right semantics for this.
cc @vinodkone
src/slave/flags.cpp
Lines 457 (patched)
<https://reviews.apache.org/r/58337/#comment245265>
Usually, when you add a new flag, you should update the documentation as well (configuration.md)
src/slave/flags.cpp
Lines 462-471 (patched)
<https://reviews.apache.org/r/58337/#comment245264>
Instead of having this un-documented format. Let's model this in a protobuf so that people knows about the scheme.
I think this flags should also works for docker containerizer (i.e., docker run --device).
Looking at the OCI runtime spec, here is their way of specifying devices that need to be available in the container:
https://github.com/opencontainers/runtime-spec/blob/master/config-linux.md#devices
https://github.com/opencontainers/runtime-spec/blob/master/specs-go/config.go#L355
I don't think we should expose to the user the full cgroups control. For instance, most people won't try to mknod himself. Also, with user namespace in mind, mknod won't work in a child user namespace. We should also take a look at the discussion at the k8s community on this:
https://github.com/kubernetes/kubernetes/issues/5607
I was thinking about a simplified version:
```
message Device {
optional string path = 1;
}
message AllowdDevice {
required Device device = 1;
enum Mode {
RO = 1;
RW = 2;
}
optional Mode mode = 2;
}
message ContainerInfo {
...
repeated AllowedDevice allowed_devices;
}
```
Put the above protobuf definition into `mesos.proto`. We might want to allow frameworks to set that as well (in ContainerInfo) so that there will be per container device control (rather than on the agent level which is a global config for all containers on the agent).
- Jie Yu
On April 17, 2017, 3:51 a.m., Zhongbo Tian wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58337/
> -----------------------------------------------------------
>
> (Updated April 17, 2017, 3:51 a.m.)
>
>
> Review request for mesos and haosdent huang.
>
>
> Bugs: MESOS-6791
> https://issues.apache.org/jira/browse/MESOS-6791
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add allowed devices whitelist for cgroups/devices isolator.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp ca2727142a9f257168f3cae0958f7b4665b63cf6
> src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 9b5cf83093796b0c0cc5057b612f80bc8b8ba72f
> src/slave/flags.hpp 171f67e44518e858049d002fcf037715021da265
> src/slave/flags.cpp 9365da2c8462a4375a99a86210b9d6ec628510fe
>
>
> Diff: https://reviews.apache.org/r/58337/diff/3/
>
>
> Testing
> -------
>
> For simple test:
>
> - Launch without additional devices:
> 1. Start agent with `sudo mesos-agent --master=127.0.0.1:5050 --work_dir=/tmp/mesos --isolation=cgroups/devices`
> 2. try open `/dev/rtc0` and failed with permission denied. `sudo mesos-execute --master=127.0.0.1:5050 --name=test --command="head -c 0 /dev/rtc0"`
>
>
> - Launch with additional devices:
> 1. Start agent with `sudo mesos-agent --master=127.0.0.1:5050 --work_dir=/tmp/mesos --isolation=cgroups/devices --cgroups_whitelist_devices=[{'"path":"/dev/rtc0", "mknod_access":true, "ad_access":true, "write_access":true}]'`
> 2. open `/dev/rtc0` successfully. `sudo mesos-execute --master=127.0.0.1:5050 --name=test --command="head -c 0 /dev/rtc0"`
>
>
> Thanks,
>
> Zhongbo Tian
>
>