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 Sun, 02 Oct 2016 00:22:09 GMT

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




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

    this is linux only as we agreed, right?



src/slave/containerizer/mesos/launch.cpp (lines 114 - 116)
<https://reviews.apache.org/r/50271/#comment218113>

    You need to move this out as well?



src/slave/flags.hpp (line 104)
<https://reviews.apache.org/r/50271/#comment218111>

    I thought we agree that this is a linux only feature as the isolator is only available
on linux?



src/tests/containerizer/isolator_tests.cpp (lines 43 - 47)
<https://reviews.apache.org/r/50271/#comment218114>

    Please create a separate test files for this: `src/tests/containerizer/capabilities_isolator_tests.cpp`



src/tests/containerizer/isolator_tests.cpp (line 375)
<https://reviews.apache.org/r/50271/#comment219263>

    Rename this to `TestConfig` or `TestSetup`?



src/tests/containerizer/isolator_tests.cpp (line 378)
<https://reviews.apache.org/r/50271/#comment219258>

    I suggest we take `Option<vector<Capability>>` here so that the caller can
do:
    ```
    TestConfig(
        {NET_RAW, SYS_ADMIN},
        {...},
        ...
    )
    ```



src/tests/containerizer/isolator_tests.cpp (line 380)
<https://reviews.apache.org/r/50271/#comment219261>

    I would remove this and leave a TODO.



src/tests/containerizer/isolator_tests.cpp (line 381)
<https://reviews.apache.org/r/50271/#comment219262>

    I would simply this to be a bool: if the task should finish normally or not.
    
    In the test, I would watch for terminal status update and see if it's TASK_FINISHED or
not.



src/tests/containerizer/isolator_tests.cpp (line 395)
<https://reviews.apache.org/r/50271/#comment219264>

    s/os/stream/



src/tests/containerizer/isolator_tests.cpp (lines 573 - 578)
<https://reviews.apache.org/r/50271/#comment219260>

    This can be
    ```
    TestConfig({}, None(), false)
    ```



src/tests/containerizer/isolator_tests.cpp (line 576)
<https://reviews.apache.org/r/50271/#comment219259>

    I would use 'using capabilities::xxx' in the begining of this file to reduce verbosity
here.



src/tests/containerizer/isolator_tests.cpp (lines 657 - 659)
<https://reviews.apache.org/r/50271/#comment219265>

    Should we fix those?



src/tests/containerizer/isolator_tests.cpp (lines 665 - 682)
<https://reviews.apache.org/r/50271/#comment219266>

    I am wondering if we can parameterize this as well?


- Jie Yu


On Sept. 29, 2016, 4:20 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> -----------------------------------------------------------
> 
> (Updated Sept. 29, 2016, 4:20 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 86d1859854b44f237ac2ca1ef74982b543c08d25 
>   src/CMakeLists.txt 35eb63fe9a8e47d97512e9904bf5a714c63722a7 
>   src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a 
>   src/slave/containerizer/mesos/containerizer.cpp 31064aefa6053eb65fbb2855929118e798b131ad

>   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 a9b6ee2f0da2c1c84c6a0642adbce20a9f0218dc 
>   src/slave/containerizer/mesos/launch.cpp 7cc0c3f46fe1a5883b7ccd474595b8be412b355c 
>   src/slave/flags.hpp 3952d04f6a00ac1dca1adf2bea7cc6e415620ce5 
>   src/tests/containerizer/isolator_tests.cpp 9458e7ef205c1e7ed496b53cd28ffc23e1dc1401

> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> -------
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)
> `make` (OS X, clang-4.0 w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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