mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 58603: Allowed whitelist additional devices in cgroups devices subsystem.
Date Fri, 21 Apr 2017 19:08:50 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58603/#review172699
-----------------------------------------------------------



At a high level, I'd suggest breaking this patch into several smaller patches:
1) add protobuf definitions (both unversioned and v1)
2) Add necessary helper functions for the protobuf (e.g., streaming, etc.)
3) Add new flags to agent
4) update isolator to use the new flag.


docs/configuration.md
Lines 1156 (patched)
<https://reviews.apache.org/r/58603/#comment245759>

    I'd remove the `cgroups` prefix.
    
    This flag expresed a default list of devices that will be exposed to a container by default.
Framework can overwrite it in the future by specifying `ContainerInfo.device_whitelist`.
    
    So, I'd rename this to:
    `--device_whitelist`



include/mesos/mesos.proto
Lines 2711 (patched)
<https://reviews.apache.org/r/58603/#comment245752>

    2 lines apart. See our style guide. 2 lines apart for top level definitions



include/mesos/mesos.proto
Lines 2713 (patched)
<https://reviews.apache.org/r/58603/#comment245755>

    I wouldn't mention 'by path' here. In the future, we might want to describe a device by
major/minor number.



include/mesos/mesos.proto
Lines 2715 (patched)
<https://reviews.apache.org/r/58603/#comment245760>

    Please update v1 proto as well!



include/mesos/mesos.proto
Lines 2718 (patched)
<https://reviews.apache.org/r/58603/#comment245753>

    2 lines apart



include/mesos/mesos.proto
Lines 2731 (patched)
<https://reviews.apache.org/r/58603/#comment245754>

    Ditto



include/mesos/mesos.proto
Lines 2732 (patched)
<https://reviews.apache.org/r/58603/#comment245756>

    s/WhiteList/Whitelist/
    
    Whitelist is a single english word



include/mesos/mesos.proto
Lines 2733 (patched)
<https://reviews.apache.org/r/58603/#comment245757>

    s/devices/allowed_devices/



include/mesos/type_utils.hpp
Line 372 (original), 377 (patched)
<https://reviews.apache.org/r/58603/#comment245758>

    2 lines apart


- Jie Yu


On April 21, 2017, 7:02 a.m., Zhongbo Tian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58603/
> -----------------------------------------------------------
> 
> (Updated April 21, 2017, 7:02 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Jie Yu.
> 
> 
> Bugs: MESOS-6791
>     https://issues.apache.org/jira/browse/MESOS-6791
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Allowed whitelist additional devices in cgroups devices subsystem.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 159f946216299fc52171e0a58c7eb7c888c1eec8 
>   include/mesos/mesos.proto eaa2d2ac697cfc4f5aa56db0fb37363339608f43 
>   include/mesos/type_utils.hpp 5f771aaf2f4e76ac06bfd8f77b0b744ed2854b27 
>   src/common/parse.hpp e90738a91161e26a48a6e381765e631492294641 
>   src/common/type_utils.cpp dc0dd71f52581e2067fed279677bda8c82aa7298 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.hpp ca2727142a9f257168f3cae0958f7b4665b63cf6

>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 9b5cf83093796b0c0cc5057b612f80bc8b8ba72f

>   src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94 
>   src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173 
> 
> 
> Diff: https://reviews.apache.org/r/58603/diff/1/
> 
> 
> Testing
> -------
> 
> For 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='{"devices":[{"device":{"path":"/dev/rtc0"},
"access":{"mknod":true, "read":true, "write":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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message