----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25549/#review53126 -----------------------------------------------------------
src/common/parse.hpp <https://reviews.apache.org/r/25549/#comment92541> s/string/string or file/ src/slave/containerizer/isolators/filesystem/shared.hpp <https://reviews.apache.org/r/25549/#comment92549> Can you add a high level comment on what this isolator does? src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92550> You should print user.Error(). src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92552> s/container_path/container path/ s/host_path/host path/ here and everywhere else. either that or you need to say 'Volume.container_path' or 'Volume.host_path' which I'm assuming you don't want to do :) src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92551> s/shared/host/ ? src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92576> consider creating it if it's a relative path. src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92554> quote the paths here and everywhere else. src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92555> s/other/container_path/ ? src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92556> s/host_path/hostPath/ src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92559> hostPath = path::join( directory, strings::remove(...)); src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92560> include the error. src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92561> include chmod.error() src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92565> mind refactoring os::chown() to take uid and gid? src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25549/#comment92557> s/automatically/automatically by the kernel/ ? also, how is the mount namespace destroyed? src/slave/containerizer/linux_launcher.cpp <https://reviews.apache.org/r/25549/#comment92548> Kill this TODO? src/slave/containerizer/mesos/containerizer.cpp <https://reviews.apache.org/r/25549/#comment92577> I'm assuming we want to support TaskInfo.ContainerInfo.Type() to be MESOS? If yes, this check should be fixed too. src/slave/flags.hpp <https://reviews.apache.org/r/25549/#comment92543> I don't think you need this header in this file? src/slave/flags.hpp <https://reviews.apache.org/r/25549/#comment92546> What about TaskInfo that doesn't use a ExecutorInfo. Is the default injected into that as well? src/slave/flags.hpp <https://reviews.apache.org/r/25549/#comment92545> s/RO/RW/ ? :) src/slave/slave.cpp <https://reviews.apache.org/r/25549/#comment92547> How about doing this in Slave::getExecutorInfo() instead? Thats the place we do all sorts of manipulations on ExecutorInfo. src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25549/#comment92584> Can you add a comment here on what this test is doing? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25549/#comment92578> s/container_path/containerPath/ src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25549/#comment92580> Is this guaranteed to exist on all systems? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25549/#comment92579> s/host_path/hostPath/ src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25549/#comment92581> const? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25549/#comment92586> s/sHost/hostStat/ src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25549/#comment92587> s/sContainer/containerStat/ - Vinod Kone On Sept. 11, 2014, 6:46 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25549/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2014, 6:46 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 dea51f94d130c131421c43e7fd774ceb8941f501 > src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 > 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 > d5ef1d6aa762cf81a3e8384552d97fe95b9cbd95 > src/slave/containerizer/mesos/containerizer.cpp > 9d083294caa5c5a47ba3ceaa1b57346144cb795c > src/slave/flags.hpp 21e00212bc402674eaea73b44b3f91df477a7213 > src/slave/slave.cpp 1b3dc7370a2441e4159aa5ee552b64ca5e511e96 > src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 > > Diff: https://reviews.apache.org/r/25549/diff/ > > > Testing > ------- > > make check # added a test > > > Thanks, > > Ian Downes > >
