----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24178/#review51046 -----------------------------------------------------------
Nice to see this! Added some high level comments. src/Makefile.am <https://reviews.apache.org/r/24178/#comment88950> Why are you adding these two headers? (duplicated?) src/slave/containerizer/isolators/filesystem.cpp <https://reviews.apache.org/r/24178/#comment88957> Check for kernel version? Check for root permission? For example, when bind mount is introduced? src/slave/containerizer/isolators/filesystem.cpp <https://reviews.apache.org/r/24178/#comment88956> Here, you can just check the ContainerInfo inside executorInfo. If it does not exist, use the default one specified by the slave. src/slave/containerizer/isolators/filesystem.cpp <https://reviews.apache.org/r/24178/#comment88958> Alignment. src/slave/containerizer/isolators/filesystem.cpp <https://reviews.apache.org/r/24178/#comment88959> Him, this reveals a more interesting problem. Say in the future, we want to implement a full file system isolator. The order in which we bind mount those volumns needs to be carefully calculated. For example, say we have two volumns: / and /lib. Can we actually support that? src/slave/containerizer/isolators/filesystem.cpp <https://reviews.apache.org/r/24178/#comment88961> Can you explain why we need to create those directories? Is is necessary? My feeling is that it's not necessary. If you want those dir structures, you should specify more Volumns, right? Well, again, if we have multiple volumns, the question is how do we ensure the correct mount order. src/slave/containerizer/isolators/filesystem.cpp <https://reviews.apache.org/r/24178/#comment88960> please use set -x to print those mount commands. src/slave/flags.hpp <https://reviews.apache.org/r/24178/#comment88955> This needs some discussion. I am in favor of using ContainerInfo to specify private dirs so that we can be more flexible in the future when we have full file system isolation. For the first step (for /tmp), you can introduce a default ContainerInfo (through slave flags) for mesos containerizer (if not specified) which contains information about default private mounts. Sine it's a protobuf, it's very straightforward to pass it in as a json object. Also, have you considered using Volumn to specify private dirs? We need some discussion here. For example, one way is to introduce a new Mode. - Jie Yu On Aug. 1, 2014, 6:08 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24178/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2014, 6:08 a.m.) > > > Review request for mesos, Ben Mahler and Jie Yu. > > > Bugs: MESOS-1586 > https://issues.apache.org/jira/browse/MESOS-1586 > > > Repository: mesos-git > > > Description > ------- > > Does not report usage or enforce quota but can create 'private' directories > for each container which mask parts of the root filesystem. > > > Diffs > ----- > > src/Makefile.am b660d912c92594ef679b71caf508134f20511dae > src/slave/containerizer/isolators/filesystem.hpp PRE-CREATION > src/slave/containerizer/isolators/filesystem.cpp PRE-CREATION > src/slave/containerizer/linux_launcher.cpp > f3cc813c64e9298f43c9691de9d51ba29c3a8e91 > src/slave/containerizer/mesos/containerizer.cpp > 2c394e2c8702166266f5d20ff005abb218da8a6c > src/slave/flags.hpp 146c4011eb6a64dbb03555cece1c5303338ae240 > src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 > > Diff: https://reviews.apache.org/r/24178/diff/ > > > Testing > ------- > > # added two tests for the FilesystemIsolator > make check > > > Thanks, > > Ian Downes > >
