mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 50271: Created an isolator for Linux capabilities.
Date Tue, 20 Sep 2016 11:12:29 GMT


> 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
> 
>


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