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 50271: Created an isolator for Linux capabilities.
Date Tue, 20 Sep 2016 02:38:45 GMT

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


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