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

Ship it!


LGTM overall. Had some question about the bind mount in tests. Do you have a 
plan to test the OSX chroot path as well since it's in the code? Maybe add a 
TODO?


src/tests/launch_tests.cpp (line 79)
<https://reviews.apache.org/r/31444/#comment141159>

    Please use ```os::system(...) != 0``` consistently



src/tests/launch_tests.cpp (lines 96 - 101)
<https://reviews.apache.org/r/31444/#comment141162>

    Can you explain why this is needed? Maybe add a comment or something? If 
this is not strictly needed, I would remove it because a crash of the test will 
polute the host mount table.



src/tests/launch_tests.cpp (lines 113 - 114)
<https://reviews.apache.org/r/31444/#comment141163>

    +1
    
    Please fix the comments here.



src/tests/launch_tests.cpp (lines 122 - 123)
<https://reviews.apache.org/r/31444/#comment141164>

    Who is going to close these two fd?



src/tests/launch_tests.cpp (line 179)
<https://reviews.apache.org/r/31444/#comment141165>

    Who is using this?



src/tests/launch_tests.cpp (line 188)
<https://reviews.apache.org/r/31444/#comment141173>

    2 lines apart here.


- Jie Yu


On June 1, 2015, 9:50 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31444/
> -----------------------------------------------------------
> 
> (Updated June 1, 2015, 9:50 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-2350
>     https://issues.apache.org/jira/browse/MESOS-2350
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Optionally take a path that the launch helper should chroot to before 
> exec'ing the executor. It is assumed that the work directory is mounted to 
> the appropriate location under the chroot. In particular, the path to the 
> executor must be relative to the chroot.
> 
> Configuration that should be private to the chroot is done during the launch, 
> e.g. mounting proc and statically configuring basic devices. It is assumed 
> that other configuration, e.g., preparing the image, mounting in volumes or 
> persistent resources, is done by the caller.
> 
> Mounts can be made to the chroot (e.g., updating the volumes or persistent 
> resources) and they will propagate in to the container but mounts made inside 
> the container will not propagate out to the host.
> 
> It currently assumes that at least {{chroot}}/tmp is writeable and that mount 
> points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
> 
> This is specific to Linux.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a5a7306b1ef65ca2b643653779ab76c26dbb5c90 
>   src/linux/fs.cpp 1c9cf3f2ffead37148e4f6a81cefdbb97f679b09 
>   src/slave/containerizer/mesos/launch.hpp 
> 7c8b535746b5ce9add00afef86fdb6faefb5620e 
>   src/slave/containerizer/mesos/launch.cpp 
> 2f2d60e2011f60ec711d3b29fd2c157e30c83c34 
>   src/tests/launch_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31444/diff/
> 
> 
> Testing
> -------
> 
> Manual testing only so far. This is harder to automate because we need a 
> self-contained chroot to execute something in... Suggestions welcome.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Reply via email to