----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38900/#review102682 -----------------------------------------------------------
Only one issue (regarding user). See my detailed comments below. src/launcher/executor.cpp (line 187) <https://reviews.apache.org/r/38900/#comment160440> Could you please add some comments about what we are doing here: ``` // If 'sandbox_diretory' is specified, that means the user // task specifies a root filesystem, and that root filesystem has // already been prepared at COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH. // The command executor is reponsible for mounting the sandbox // into the root filesystem, chrooting into it and changing the // user before exec-ing the user process. ``` src/launcher/executor.cpp (lines 203 - 212) <https://reviews.apache.org/r/38900/#comment160433> This loop looks like unnecessary. Can you simply do: ``` rootfs = path::join(os::getcwd(), COMMAND_EXECUTOR_ROOTFS_CONTAINER_PATH); ``` src/launcher/executor.cpp (line 218) <https://reviews.apache.org/r/38900/#comment160441> Failed to create sandbox mount point at '...': <error> src/launcher/executor.cpp (line 224) <https://reviews.apache.org/r/38900/#comment160444> Please add a NOTE here saying that this is a non-recursive bind mount. You don't want to recursively mount root filesystem into root filesystem since the root filesystem is under sandbox as well. src/launcher/executor.cpp (line 227) <https://reviews.apache.org/r/38900/#comment160443> this should be 'sandbox'? src/launcher/executor.cpp (line 306) <https://reviews.apache.org/r/38900/#comment160446> Hum, looks like no one will use 'rootfs' variable on non linux platforms. WIll that trigger compiler warning/error? So should you put #ifdef within the if block and abort if it's not linux? src/slave/containerizer/mesos/containerizer.cpp (lines 592 - 597) <https://reviews.apache.org/r/38900/#comment160448> WHy the change here? Can you do that in a separate patch? src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/38900/#comment160449> Ditto here. src/slave/slave.cpp (lines 3390 - 3393) <https://reviews.apache.org/r/38900/#comment160447> Hum... Does that mean that if the user task does not specify a user (e.g., rely on framework.user), we are going to launch the task under root? - Jie Yu On Oct. 13, 2015, 3:39 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38900/ > ----------------------------------------------------------- > > (Updated Oct. 13, 2015, 3:39 a.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Bugs: MESOS-3428 > https://issues.apache.org/jira/browse/MESOS-3428 > > > Repository: mesos > > > Description > ------- > > Update command executor to support rootfs. > > > Diffs > ----- > > src/launcher/executor.cpp 50b3c6e319f4b1e08c8ebcdd9f161e19bb14d390 > src/slave/constants.hpp 1197268576ee2ec37601db75ea9536ec09882886 > src/slave/constants.cpp 96dadce6d28aa585410f50a2509a34445f8dcf82 > src/slave/containerizer/mesos/containerizer.hpp > 4aad8a3be43b331efc6b8157b2fae090df16c1b4 > src/slave/containerizer/mesos/containerizer.cpp > d1fc5a460e7313828014eea999cf4e63dde01921 > src/slave/slave.cpp 6b25b49458c163097a5292843134363c4d0f5e6f > > Diff: https://reviews.apache.org/r/38900/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
