----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49273/#review139601 -----------------------------------------------------------
src/Makefile.am (line 1375) <https://reviews.apache.org/r/49273/#comment204852> instead of adding new binary, can we leverage the existing binary (`mesos-containerizer`) and just add a subcommand there? src/slave/containerizer/mesos-chroot.cpp (lines 39 - 51) <https://reviews.apache.org/r/49273/#comment204918> Please take a look at `src/slave/containerizer/mesos/mount.hpp` see how to add a new subcommand for `mesos-containerizer` binary. src/slave/containerizer/mesos-chroot.cpp (line 52) <https://reviews.apache.org/r/49273/#comment204872> 2 lines apart. src/slave/containerizer/mesos-chroot.cpp (lines 57 - 61) <https://reviews.apache.org/r/49273/#comment204921> This is no longer needed if you use subcommand as I mentioned above. src/slave/containerizer/mesos-chroot.cpp (lines 107 - 110) <https://reviews.apache.org/r/49273/#comment204986> Hum, that reminds me that there's a bug in the current command executor impl. Since pivot_root will mutate the mount table, it actually has side effects (that's the reason there is a command called pivot_root, and you don't have to specify commands, unlike chroot or su)! For command tasks, we need to make sure the health check program is also accessible inside the chroot. We need to refactor that part to use a libprocess Process instead of a new binary. Ideally, command executor will use the same utility to create user task. However, to simply some of the tooling (e.g., mesos exec) for command tasks, we need to make sure executor and the actual tasks are in the same mount namespace (otherwise, how does the CLI find the namespace handle for the task). That makes me wonder that should we make 'unshare' optional here (via a flag)? Also, since binary does more than just chroot. It also set uid/gids, chroot, chdir. We need a better name for the subcommand? Ideally, if we can merge this impl. with `src/slave/containerizer/mesos/launch.cpp`, that'll be awesome (as you can see, most of the logics are the same). For now, if you feel there're too much work, we can just name it `LAUNCH_TASK`. src/slave/containerizer/mesos-chroot.cpp (line 127) <https://reviews.apache.org/r/49273/#comment204988> This won't work since it'll look into /etc/group? src/slave/containerizer/mesos-chroot.cpp (line 140) <https://reviews.apache.org/r/49273/#comment205010> Instead of using mutated argc and argv, i would prefer an explict flag `command` which is a JSON array. ``` --command="[\"task-binary\", \"args\"]" ``` - Jie Yu On June 27, 2016, 5:09 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49273/ > ----------------------------------------------------------- > > (Updated June 27, 2016, 5:09 p.m.) > > > Review request for mesos, Joshua Cohen and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Uses the same code as the agent uses for chroot'ing on Linux, i.e., > pivot_root and setting up /dev etc. Intention is that executors (like > Aurora's Thermos) can use it to chroot tasks. > > Currently, the root path is specified as a flag and the remaining arguments > are exec'ed. Joshua has also requested that the root path could be specified > as the first arg. @Jie, thoughts? > > > Diffs > ----- > > src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 > src/slave/containerizer/mesos-chroot.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/49273/diff/ > > > Testing > ------- > > Manual. > > > Thanks, > > Ian Downes > >
