----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31444/#review79948 -----------------------------------------------------------
Nice test! src/linux/fs.cpp <https://reviews.apache.org/r/31444/#comment129633> s/typede/types/ src/linux/fs.cpp <https://reviews.apache.org/r/31444/#comment129634> Remember to move '{' to the next line. src/linux/fs.cpp <https://reviews.apache.org/r/31444/#comment129635> Ditto. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment129636> Why do you need this include? Seems that list is not used. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment129637> Ditto. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment129639> Could you change the flag help string for --directory stating that if --rootfs is used, the 'directory' should be relative to the new root? src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129661> Maybe add a comment about this method? What does it do? src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129642> ``` if (os::system("...") != 0) { return ErrnoError("..."); } ``` src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129643> Ditto. Also, do you also wanna copy /lib as well? Looking at my ubuntu trusty 64: ``` vagrant@vagrant-ubuntu-trusty-64:~$ uname -a Linux vagrant-ubuntu-trusty-64 3.13.0-32-generic #57-Ubuntu SMP Tue Jul 15 03:51:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux vagrant@vagrant-ubuntu-trusty-64:~$ ls / bin boot dev etc home initrd.img lib lib64 lost+found media mnt opt proc root run sbin srv sys tmp usr vagrant var vmlinuz workspace vagrant@vagrant-ubuntu-trusty-64:~$ ls /lib64 ld-linux-x86-64.so.2 vagrant@vagrant-ubuntu-trusty-64:~$ ldd /bin/sh linux-vdso.so.1 => (0x00007fff23331000) libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f8c8e240000) /lib64/ld-linux-x86-64.so.2 (0x00007f8c8e831000) ``` src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129644> Why slave mount? Shouldn't this be a SHARED mount? src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129651> Please refine the comments here;) src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129652> Why `// XXX` at the end of the line? src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129658> Do you need stdin? Or you can just use PATH("/dev/null")? src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129659> Any reason you want to do this? src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129655> s/stack/Stack/ src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129660> Do you still need this? src/tests/launch_tests.cpp <https://reviews.apache.org/r/31444/#comment129657> Add one more blank line above. - Jie Yu On April 8, 2015, 3:24 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31444/ > ----------------------------------------------------------- > > (Updated April 8, 2015, 3:24 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 9c01f5d6c692f835100e7cade928748cc4763cc8 > 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 > >
