----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25865/#review55685 -----------------------------------------------------------
Vinod and Jie asked me to take a look at this review. It looks like there are dependent changes that are not linked in? (filesystem islotor, ContainerInfo::MESOS, os::namespaces os::getns). My main comment is that it seems really unfortuante that the isolator boundary is broken here between the filesystem isolator and the pid namespace isolator. I'm sure there will be folks out there that want pid namespace isolation without having to use the shared filesystem isolator. src/Makefile.am <https://reviews.apache.org/r/25865/#comment96053> Can you line up the \'s here? src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25865/#comment96057> Why is the filesystem isolator doing the pid namespace related isolation? If it's necessary, please add a comment as to why we had to break the isolation boundary here!! Seems really unfortunate that only using the pid namespace isolator (without fs isolation) does not get you the /proc isolation. Is there any way for this to be the responsibility of the pid namespace isolator instead? src/slave/containerizer/isolators/filesystem/shared.cpp <https://reviews.apache.org/r/25865/#comment96056> A few questions: (1) Why are you saying lazily here? What aspect of this is done lazily? (2) So.. with pid namespaces you have to manually remount /proc to hide the changes? Can you make the comment more explicit about how important it is to do this? (3) Why is /sys remounted? An explanation in the comment would be great for when us dummies are reading this code later and scratching our heads. :) (4) Curious, is the remounting only specific to pid namespacing, as opposed to other namespaces? src/slave/containerizer/isolators/namespaces/pid.hpp <https://reviews.apache.org/r/25865/#comment96058> Typically, we include what we use, rather than relying on transitive includes. src/slave/containerizer/isolators/namespaces/pid.hpp <https://reviews.apache.org/r/25865/#comment96060> Why does this need to be exposed, who will rely on it? src/slave/containerizer/isolators/namespaces/pid.hpp <https://reviews.apache.org/r/25865/#comment96061> It looks like these can be moved into the .cpp file, instead of being static class methods? Also, what do they do? ;) src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96063> Try to include what you're using, instead of relying on transitive includes. The latter are really difficult to manage and reason about. src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96086> If it's a "pid namespace isolator", why not call this class 'PidNamespaceIsolator'? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96088> This is unfortunate, because I'm sure many will want pid namespace isolation **without** having to use the shared filesystem isolator. Why can't the mounting be done in this isolator? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96090> Bad indentation? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96081> s/buf/s/ src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96084> Why is state.id an option in the first place? src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96083> Do you want to include the BIND_MOUNT_ROOT in this message to aid debugging? os::ls seems to include it in some cases.. unfortunately =/ src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96069> Whoops! src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96068> I'm still confused after reading this comment. :) src/slave/containerizer/isolators/namespaces/pid.cpp <https://reviews.apache.org/r/25865/#comment96065> When are we going to encounter the case where the mount is not removed yet? Can you provie more comments here, as to why we don't force or expire? src/slave/containerizer/linux_launcher.cpp <https://reviews.apache.org/r/25865/#comment96054> No need to say "Failed to destroy" within the Failure returned by `destroy`, that is the callers responsibility: ``` Future<Nothing> destroy = launcher->destroy(c); ... if (destroy.isFailed()) { LOG(ERROR) << "Failed to destroy container " << c << ": " << destroy.failure(); } ``` This type of message leads us to have redundant log messages. In general we would just include a reason, and since the caller knows (1) what was called, and (2) what arguments were provided, it's up to the caller to log the relevant information: ``` return Failure("Unknown container"); ``` src/slave/containerizer/linux_launcher.cpp <https://reviews.apache.org/r/25865/#comment96055> This line is unused...? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25865/#comment96094> s/>/> / src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25865/#comment96093> What is the difference between os::getns() here and NamespacesPidIsolatorProcess::getNamespace? Where is the review that created os::getns, could we use a better name for that method? Why isn't is linux::namespaces and linux::getns, are namespaces applicable to other systems? src/tests/isolator_tests.cpp <https://reviews.apache.org/r/25865/#comment96095> The value itself doesn't matter? - Ben Mahler On Oct. 2, 2014, 6:23 p.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25865/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2014, 6:23 p.m.) > > > Review request for mesos, Jie Yu and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Add namespaces/pid to --isolation slave flag. Places executor into a pid > namespace so it and all descendants will be contained in the namespace. > Requires the filesystem/shared isolator so /proc and /sys are remounted to > reflect the different namespace. > > > Diffs > ----- > > src/Makefile.am 27c42dfde45a449750132e416b4eaf776f8c5e3b > src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION > src/slave/containerizer/isolators/namespaces/pid.hpp PRE-CREATION > src/slave/containerizer/isolators/namespaces/pid.cpp PRE-CREATION > src/slave/containerizer/linux_launcher.cpp > f7bc894830a7ca3f55465dacc7b653cdc2d7758b > src/slave/containerizer/mesos/containerizer.cpp > 9d083294caa5c5a47ba3ceaa1b57346144cb795c > src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 > > Diff: https://reviews.apache.org/r/25865/diff/ > > > Testing > ------- > > Added test that command in pid namespaced container is in a different > namespace and that the command is 'init' (verifies remount of /proc). > > > Thanks, > > Ian Downes > >
