> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/Makefile.am, line 834
> > <https://reviews.apache.org/r/50271/diff/11/?file=1502609#file1502609line834>
> >
> >     This should belong linux files below?

This isolator has no hard dependency on Linux so we can build it on all 
platforms. I think it would be great to have as much code as possible not 
behind `ifdef`s. What do you think?


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/Makefile.am, line 959
> > <https://reviews.apache.org/r/50271/diff/11/?file=1502609#file1502609line959>
> >
> >     Ditto.

See above.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 99
> > <https://reviews.apache.org/r/50271/diff/11/?file=1502610#file1502610line99>
> >
> >     Not yours, but why this is not wrapped with ifdef linux?

This isolator builds fine under e.g., macos, so we can build it everywhere. I 
believe this is A Good Thing.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 73
> > <https://reviews.apache.org/r/50271/diff/11/?file=1502610#file1502610line73>
> >
> >     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.

I moved this to l.59 outside of any conditionally included code.

I also added https://reviews.apache.org/r/52081 up the chain to reorganize the 
includes here, could you shepherd that one?


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1242-1251
> > <https://reviews.apache.org/r/50271/diff/11/?file=1502610#file1502610line1242>
> >
> >     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.

Like you probably know I added this to satisfy a requirement from the design 
doc where we specified for the case when `--isolation=capabilities` is absent:

a) ContainerInfo::CapabilityInfo not provided:
    All capabilities for the user. (no isolation)
b) ContainerInfo::CapabilityInfo provided, ContainerInfo::CapabilityInfo is 
empty:
    Not supported. Agent will fail the task with appropriate reasoning.
c) ContainerInfo::CapabilityInfo is NOT empty:
    Not supported. Agent will fail the task with appropriate reasoning.    

Do you mean the design doc is wrong when it says this work should be performed 
in the agent (instead the master should be fail the task)? In that case I 
wonder if we wouldn't still need to check in the agent, e.g., to allow 
restarting agents with changed capabilities.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp, lines 
> > 103-104
> > <https://reviews.apache.org/r/50271/diff/11/?file=1502612#file1502612line103>
> >
> >     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().

You are right, there are two issues here (improper handling of the custom 
executor case, and incorrect setup for command task with rootfs of in case only 
agent `--allowed_capabilities` are give.

I simplified and fixed the logic here like you suggested above.


> On Sept. 20, 2016, 4:38 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp, line 94
> > <https://reviews.apache.org/r/50271/diff/11/?file=1502612#file1502612line94>
> >
> >     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?

Done.


- Benjamin


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


On Sept. 20, 2016, 1:12 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2016, 1:12 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 via a new field in
> `ContainerLaunchInfo`.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/containerizer.proto 
> 20db010ea158a813034b411111ce9cddac7d8317 
>   src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
>   src/Makefile.am 478fb5ae01dbdf1a735680edf7c5f30867726e07 
>   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.hpp 
> 859990cb85e9e8c06400397256cfc512f0811800 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
>   src/slave/flags.hpp 3952d04f6a00ac1dca1adf2bea7cc6e415620ce5 
>   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