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 38900: Update command executor to support rootfs.
Date Wed, 14 Oct 2015 19:23:22 GMT

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


Only one issue (regarding user). See my detailed comments below.


src/launcher/executor.cpp (line 187)
<https://reviews.apache.org/r/38900/#comment160440>

    Could you please add some comments about what we are doing here:
    
    ```
    // If 'sandbox_diretory' is specified, that means the user
    // task specifies a root filesystem, and that root filesystem has
    // already been prepared at COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH.
    // The command executor is reponsible for mounting the sandbox
    // into the root filesystem, chrooting into it and changing the
    // user before exec-ing the user process.
    ```



src/launcher/executor.cpp (lines 203 - 212)
<https://reviews.apache.org/r/38900/#comment160433>

    This loop looks like unnecessary. Can you simply do:
    ```
    rootfs = path::join(os::getcwd(), COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH);
    ```



src/launcher/executor.cpp (line 218)
<https://reviews.apache.org/r/38900/#comment160441>

    Failed to create sandbox mount point at '...': <error>



src/launcher/executor.cpp (line 224)
<https://reviews.apache.org/r/38900/#comment160444>

    Please add a NOTE here saying that this is a non-recursive bind mount. You don't want
to recursively mount root filesystem into root filesystem since the root filesystem is under
sandbox as well.



src/launcher/executor.cpp (line 227)
<https://reviews.apache.org/r/38900/#comment160443>

    this should be 'sandbox'?



src/launcher/executor.cpp (line 306)
<https://reviews.apache.org/r/38900/#comment160446>

    Hum, looks like no one will use 'rootfs' variable on non linux platforms. WIll that trigger
compiler warning/error?
    
    So should you put #ifdef within the if block and abort if it's not linux?



src/slave/containerizer/mesos/containerizer.cpp (lines 592 - 597)
<https://reviews.apache.org/r/38900/#comment160448>

    WHy the change here? Can you do that in a separate patch?



src/slave/containerizer/mesos/containerizer.cpp 
<https://reviews.apache.org/r/38900/#comment160449>

    Ditto here.



src/slave/slave.cpp (lines 3390 - 3393)
<https://reviews.apache.org/r/38900/#comment160447>

    Hum... Does that mean that if the user task does not specify a user (e.g., rely on framework.user),
we are going to launch the task under root?


- Jie Yu


On Oct. 13, 2015, 3:39 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38900/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2015, 3:39 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3428
>     https://issues.apache.org/jira/browse/MESOS-3428
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Update command executor to support rootfs.
> 
> 
> Diffs
> -----
> 
>   src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 
>   src/slave/constants.hpp 1197268576ee2ec37601db75ea9536ec09882886 
>   src/slave/constants.cpp 96dadce6d28aa585410f50a2509a34445f8dcf82 
>   src/slave/containerizer/mesos/containerizer.hpp 4aad8a3be43b331efc6b8157b2fae090df16c1b4

>   src/slave/containerizer/mesos/containerizer.cpp d1fc5a460e7313828014eea999cf4e63dde01921

>   src/slave/slave.cpp 6b25b49458c163097a5292843134363c4d0f5e6f 
> 
> Diff: https://reviews.apache.org/r/38900/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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