----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31444/#review76909 -----------------------------------------------------------
What's the test plan for this? Do you plan to test it with MesosContainerizer and the file system isolator? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124622> Make this a static function? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124638> The typedef is not needed to me. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124644> Can you also mention that the order of this list matters. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124646> Sounds like we should support Option<string> options for fs::mount. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124621> Make this a 'static' function? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124620> Could you please add a TODO here to make this list configurable (e.g., through flags). src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124624> Make this a 'static' function? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124626> The comment here is a little confusing to me. Are you saying that this is a best effort check? Also, if we are using pid namespace but not mount namespace, which of the 'later code' will fail? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124625> s/ourNS/self/ s/parentNS/parent/ src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124629> In fact, the chroot target needs to be 'recursively shared' (i.e., mount --make-rshared). Do we have a way to check that? If not, maybe leave a TODO here. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124628> Do you also want to check the case where the chroot target is not a mount point? The current code will output: ``` Chroot target is not a shared mount ``` which is fine but not as informative as ``` Chroot target needs to be a mount point ``` src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124636> I further tested it on fedora-20 and ubuntu trusty, and think this may not be necessary. Here are my findings. I ran strace on the command 'mount --make-rslave /' on both distributions: On ubuntu: ``` mount("/dev/sda1", "/", "none", MS_SLAVE, NULL) = 0 ``` On fedora: ``` mount("none", "/", NULL, MS_REC|MS_SLAVE, NULL) = 0 ``` Looks like the mount command on ubuntu does not pass the flag MS_REC to the system call. Maybe add a TODO to remove ? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment124637> Do you still need this? - Jie Yu On March 17, 2015, 10:44 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31444/ > ----------------------------------------------------------- > > (Updated March 17, 2015, 10:44 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/slave/containerizer/mesos/launch.hpp > 7c8b535746b5ce9add00afef86fdb6faefb5620e > src/slave/containerizer/mesos/launch.cpp > 2f2d60e2011f60ec711d3b29fd2c157e30c83c34 > > 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 > >
