----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31444/#review74382 -----------------------------------------------------------
Let's add some comments to document all the subtleness. And I do think we need unit test for that. Can you add a mesos_containerizer_tests.cpp and introduced a MesosContainerizerLaunchTest. In the test, maybe manually open a pipe and launch the subprocess? You can maybe verify that at least you can read /proc from the new root. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment120987> Can you pull the chroot impl. to a separate helper function. Looks like it's quite long and hurts the readability of `execute()`. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment120950> Please add spaces before and after `|`. Style sugguestion: ``` Try<Nothing> mount = fs::mount( None(), "/", None(), MS_SLAVE | MS_REC, NULL); if (mount.isError()) { ... } ``` src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121101> So here you assume `newroot/proc` exists. Could you please add a comment here? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121100> Ditto. Add spaces before and after `|`. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121102> Add a new line above. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121103> Ditto. You assume newroot/sys exists. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121104> new line above src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121105> Maybe the following so it's more extensible? ``` vector<string> devices = { "full", "null", "random", "tty", "urandom", "zero" }; ``` src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121108> Can you move all the symlinking steps together (i.e., move these down after /dev/pts is mounted)? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121106> No need for the :: prefix before fs? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment120982> Per our discussion offline, this does not seem to be necessary. Instead, you can just do a private bind mount of `flags.chroot.get()` src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment120984> Let's document the order which is very important here. src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121112> Can you callout in the comments why we need to chdir to the new root before doing pivot_root? It's not quite obvious and in fact quite subtle. Also, can you make the naming consistent? I suggest always using 'new root' and 'old root' (instead of 'target' here). src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment120986> Some of my findings regarding pivot_root. It's quite subtle:) This works: ``` [vagrant@localhost ~]$ sudo unshare -m [root@localhost vagrant]# ls busybox [root@localhost vagrant]# mount --make-rslave / [root@localhost vagrant]# mount --bind busybox/ busybox/ [root@localhost vagrant]# mount --make-private busybox/ [root@localhost vagrant]# cd busybox/ [root@localhost busybox]# pivot_root . mnt ``` This does not work: ``` [root@localhost vagrant]# unshare -m [root@localhost vagrant]# mount --make-rslave / [root@localhost vagrant]# cd busybox/ [root@localhost busybox]# mount --bind . . [root@localhost busybox]# mount --make-private . [root@localhost busybox]# pivot_root . mnt/ pivot_root: failed to change root from `.' to `mnt/': Device or resource busy ``` src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121114> Again, please callout in the comments the purpose of this step and why it's necessary! src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121115> "Failed to read mount table: " << xxx.error src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121119> s/old/oldRootRelative/? Also, what if the user uses `/xxx/xxx/newroot/` or a relative path? Will that fail the following 'strings::startsWith' check? src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment121118> ``` foreach (const fs::MountTable::Entry& entry, mountTable.get().entries) { xxx } `` src/slave/containerizer/mesos/launch.cpp <https://reviews.apache.org/r/31444/#comment120988> Is the `directory` here the path in the host file system or the path in the chroot. Please add some comments here. - Jie Yu On Feb. 25, 2015, 10:48 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31444/ > ----------------------------------------------------------- > > (Updated Feb. 25, 2015, 10:48 p.m.) > > > Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, and Jie > Yu. > > > 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 > >
