----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25549/#review55689 -----------------------------------------------------------
LGTM overall. I'll give a shipit once the following comments are addressed. Thanks! src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment96059> ``` if (!user.isSome()) { } ``` src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment96067> Should we check other error conditions as well? For example, `!executorInfo.has_container()`? src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment96091> Could you please expand a little bit in the comments explaining why this is necessary. One would ask why not just create the path if it does not exist. I guess the reason is because this is a shared filesystem isolator, you don't want to create an arbitratry directory outside a sandbox. src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment96096> Could you please add a NOTE here saying that you are assuming container_path is an absolute path in case we change the semantics in the future. src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment96097> Add a comment about why -n is used. src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/25549/#comment96098> Could you please expand why filesystem isolation must be the first one to use prepare()? src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/25549/#comment96100> This check is redundent, right? - Jie Yu On Oct. 2, 2014, 6:21 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25549/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2014, 6:21 p.m.) > > > Review request for mesos, Ben Mahler, Jie Yu, and Vinod Kone. > > > 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 shared host filesystem. > > This review replaces https://reviews.apache.org/r/24178/ because of some file > renaming. I addressed all comments from earlier reviews. > > > Diffs > ----- > > include/mesos/mesos.proto 735da535f136d1188d3c6cf47b2e11153dab6fc3 > src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b > src/common/parse.hpp e6153d8a1f25bc9ddbe1e391306beeacfc8d5ff6 > src/common/type_utils.hpp 480c0883fe6ed7f6a9daf77d83ebb077da2e66ee > src/slave/containerizer/isolators/filesystem/shared.hpp PRE-CREATION > src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION > src/slave/containerizer/linux_launcher.cpp > f7bc894830a7ca3f55465dacc7b653cdc2d7758b > src/slave/containerizer/mesos/containerizer.cpp > 9d083294caa5c5a47ba3ceaa1b57346144cb795c > src/slave/flags.hpp 16f0cc2ab5ba16a39499608174278b3082e0585d > src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 > src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 > src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 > > Diff: https://reviews.apache.org/r/25549/diff/ > > > Testing > ------- > > make check # added a test > > > Thanks, > > Ian Downes > >
