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



Haven't looked at the tests yet.


src/Makefile.am (line 833)
<https://reviews.apache.org/r/50271/#comment217232>

    This should belong linux files below?



src/Makefile.am (line 958)
<https://reviews.apache.org/r/50271/#comment217233>

    Ditto.



src/slave/containerizer/mesos/containerizer.cpp (line 73)
<https://reviews.apache.org/r/50271/#comment217234>

    Hum, the headers here is crazy (too many ifdef linux here). The order of 
this is not correct.
    
    I suggest we group all linux related headers together.



src/slave/containerizer/mesos/containerizer.cpp (line 99)
<https://reviews.apache.org/r/50271/#comment217235>

    Not yours, but why this is not wrapped with ifdef linux?



src/slave/containerizer/mesos/containerizer.cpp (lines 1242 - 1251)
<https://reviews.apache.org/r/50271/#comment217238>

    I understand why you want to do this check, but we usually do not do such 
checks in containerizer. We rely on operator to properly configure the agent.
    
    The correct solution I believe is to advertise agent isolation capabilities 
to the master and let master reject those tasks.
    
    I would simply remove this check here for now.



src/slave/containerizer/mesos/isolators/linux/capabilities.hpp (line 17)
<https://reviews.apache.org/r/50271/#comment217239>

    LINUX_CAPABILITIES



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (line 40)
<https://reviews.apache.org/r/50271/#comment217240>

    Given that we don't need to do the check in the agent, please just inline 
this method.



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (line 94)
<https://reviews.apache.org/r/50271/#comment217252>

    I don't follow the if/else here. Should we simply calculate "what will be 
the capabilities for the task/executor, depending on what is inside 
containerConfig.container_info() and flags.allowed_capabilities" first, and 
then, depending on if the container is a command task (w or w/ rootfs) or 
custom executor, setting launchInfo accordingly?



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (lines 103 - 104)
<https://reviews.apache.org/r/50271/#comment217251>

    Hum, if i specify `--allowed_capabilities` and the framework does not 
specify capabilities for TaskInfo.container. Will this work?
    
    I think you need to consider three cases here:
    1) For command task that has rootfs. In that case, we need to make sure 
when we launch the executor, we don't specify capabilities (keep it None() in 
launchInfo, indicating that we don't want to limit capabilities). This is 
because the executor will perform priviledged operations like pivot_root and 
setuid, we want to make sure it is not restricted.
    2) For command task that has no rootfs. In that case, simply set 
launchInfo.capabilities, meaning that when launching the executor, we limit the 
capabilities of it. So the task will inherit the capabilities when the executor 
fork-exec the task.
    3) For custom executor case, simply set launchInfo.capabilities.
    
    To check if it is a command task is by checking 
containerConfig.has_task_info().


- Jie Yu


On Sept. 19, 2016, 2:21 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2016, 2:21 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
>     https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This isolator evaluates agent allowed capabilities and passes
> net capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed from the isolator via a new
> capability field in `ContainerLaunchInfo`.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/containerizer.proto 
> 20db010ea158a813034b411111ce9cddac7d8317 
>   src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
>   src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
>   src/tests/containerizer/isolator_tests.cpp 
> 93ce75180520d382f63ce7323be844fe43c6594e 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> -------
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Reply via email to