> On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote: > > 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.
This has been cleaned up some by removing the /sys mount (not needed by the port_mapping isolator) and moving the /proc mount to the pid isolator. > On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/filesystem/shared.cpp, lines 85-89 > > <https://reviews.apache.org/r/25865/diff/3/?file=711226#file711226line85> > > > > 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? (1) commented (2) Yes. Commented. (3) /sys mount removed. (4) Mostly yes. It turns out the remounting /sys is not required for a network namespace but it definitely is required for a pid namespace so that /proc lists the pids of the namespace, not the parent namespace. > On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/namespaces/pid.cpp, line 50 > > <https://reviews.apache.org/r/25865/diff/3/?file=711228#file711228line50> > > > > If it's a "pid namespace isolator", why not call this class > > 'PidNamespaceIsolator'? See Vinod's comment. I agree it reads better but this is inline with existing Cgroups{Cpu,Mem}Isolator. Happy to change though... > On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote: > > src/slave/containerizer/isolators/namespaces/pid.cpp, lines 112-119 > > <https://reviews.apache.org/r/25865/diff/3/?file=711228#file711228line112> > > > > Why is state.id an option in the first place? Historical reasons? It was kept as an Option when we moved from UUID to ContainerID. > On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote: > > src/slave/containerizer/linux_launcher.cpp, line 362 > > <https://reviews.apache.org/r/25865/diff/3/?file=711229#file711229line362> > > > > This line is unused...? It's used in the os::getns call below. I capture it so I can erase from the map in one place before any return code paths. > On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote: > > src/tests/isolator_tests.cpp, lines 976-978 > > <https://reviews.apache.org/r/25865/diff/3/?file=711231#file711231line976> > > > > 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? Comment on diffence added to pid.hpp. os::getns() returns any namespace of a pid, getNamespace() returns the pid namespace of *container*. No, linux specific. os::getns() follows from the existing os::setns() which I agree should be namespaced as linux:: > On Oct. 7, 2014, 2:23 p.m., Ben Mahler wrote: > > src/tests/isolator_tests.cpp, line 978 > > <https://reviews.apache.org/r/25865/diff/3/?file=711231#file711231line978> > > > > The value itself doesn't matter? No, it's None if it's in the caller's namespace (which would be a test failure), otherwise it is in its own namespace. - Ian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25865/#review55685 ----------------------------------------------------------- On Oct. 2, 2014, 11:23 a.m., Ian Downes wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25865/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2014, 11:23 a.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 > >
