----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53354/#review155658 -----------------------------------------------------------
Fix it, then Ship it! src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (lines 326 - 346) <https://reviews.apache.org/r/53354/#comment225675> In fact, I'd prefer moving this up before we create the Info struct in 'infos' map. We only keep stuff in 'infos' map if cleanup is needed for the container. src/slave/containerizer/mesos/isolators/gpu/isolator.cpp (lines 279 - 282) <https://reviews.apache.org/r/53354/#comment225714> GPU isolator should depends on filesystem/linux isolator so that mount in the child do not propagate back to the host mount table. Once you have this dependency, no need to explicitly set 'enter_namespaces' here because it's implied by filesystem/linux isolator. Probably return None() here because no additional namespaces are needed. src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (line 92) <https://reviews.apache.org/r/53354/#comment225709> I'd remove CLONE_NEWNS here because this is implied by the dependency to filesystem/linux isolator. src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (line 102) <https://reviews.apache.org/r/53354/#comment225707> Given that this isolator depends on filesystem/linux isolator, let's remove the CLONE_NEWNS here. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 524 - 530) <https://reviews.apache.org/r/53354/#comment225746> This does not look right. network/cni isolator might be used with posix launcher or filesystem/posix isolator on linux. It'll be turn on by default on linux. The purpose of that is because we want to reject the cases where the user specifies a named network in NetworkInfo. Therefore, we need to consider the case where the host network is used and filesystem/posix isolator and posix launcher is used. To debug such a container, we should not specify any namespaces to enter or clone because it'll not be supported by posix launcher. So we should not shortcut here and bindly assume that we have namespace support. We need to do this down below where we know `!containerNetworks.empty()`. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 528) <https://reviews.apache.org/r/53354/#comment225735> Why PID namespace here? Also, where is UTS namesapce? src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 638 - 645) <https://reviews.apache.org/r/53354/#comment225751> If control reaches here, it's on linux and linux launcher was used. It's still possible that filesystem/posix is used. We don't want to create 'info' for DEBUG container, so you need skip this block. src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 686) <https://reviews.apache.org/r/53354/#comment225754> we need to distinguish between debug class or default class here. In both cases, we need to enter the network, uts and mnt namespace of the parent. For default nested container, we need to clone a mount namespace. src/slave/containerizer/mesos/isolators/volume/image.cpp (lines 87 - 100) <https://reviews.apache.org/r/53354/#comment225682> I'd suggest for this volume isolator, we put a dependency on filesystem/linux isolator (in 'create'). Since filesystem/linux isolator will properly set 'enter_namespaces', no need to set it here. I think the namesapces set in LaunchInfo should be the additional namesspaces needed by this isolator. src/slave/containerizer/mesos/isolators/volume/image.cpp (line 115) <https://reviews.apache.org/r/53354/#comment225684> I would add a check here. Image volume is not supported for DEBUG container. We should return an Error if container is DEBUG class. src/slave/containerizer/mesos/isolators/volume/image.cpp (line 195) <https://reviews.apache.org/r/53354/#comment225687> If we add the dependency of filesystem/linux isolator, we should probably just remove this because this is implied by the dependency. src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp (lines 132 - 138) <https://reviews.apache.org/r/53354/#comment225716> I would return None() here. This is consistnet with the code above in this function: ``` if (!containerConfig.has_container_info()) { return None(); } ``` If bindMountSupported is true, that implies the dependency on filesystem/linux isolator, which will properly set the 'enter_namespace'. Otherwise, the logic is a bit weird becasue why not set 'enter_namespace' if container_info is not set? src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp (line 152) <https://reviews.apache.org/r/53354/#comment225718> I would actually add a check here and return Error if this is a debug container to make sure we get a proper error message if a debug container tries to specify a sandbox_path volume. For this reason, I'd probably move the above ifdef block down below. src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 297 - 299) <https://reviews.apache.org/r/53354/#comment225760> This is a little flaky because `wc` process might or might not be forked while `ps` is executing. I'd suggest we get the inode number of the pid namespace handle from the host and inject that number into the shell command to make sure that in the target pid namespace is not the same. - Jie Yu On Nov. 9, 2016, 12:09 a.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53354/ > ----------------------------------------------------------- > > (Updated Nov. 9, 2016, 12:09 a.m.) > > > Review request for mesos and Jie Yu. > > > Bugs: MESOS-6464 > https://issues.apache.org/jira/browse/MESOS-6464 > > > Repository: mesos > > > Description > ------- > > The namespace-related isolators now do different things depending on > whether they are launching a "normal" nested container or a "debug" > nested container. Normal nested containers clone a new mount namespace > as well as a new pid namespace. Debug nested cotnainers do not -- they > simply inherit these namespaces from their parent. > > > Diffs > ----- > > src/slave/containerizer/mesos/containerizer.cpp > e57064c768937969ba4a071ae80165ccab2f1dff > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > df16b8fee6799a69c7d96f33a5049bd9787c48f5 > src/slave/containerizer/mesos/isolators/gpu/isolator.cpp > e3756c920081f2944bf4b640edf0a83f42784586 > src/slave/containerizer/mesos/isolators/namespaces/pid.cpp > 0d9ec57d9aa83bcc6cc2e5a8d75f2e2251179b1b > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > 50b43777b1e470898de139e2dae398ebb2c0d6bb > src/slave/containerizer/mesos/isolators/volume/image.cpp > 210e67ad0d84f52135e77184f21e574c9e31628d > src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp > 7b976d29226c3e0a4d52922e9d2f7e685de72297 > src/slave/containerizer/mesos/linux_launcher.cpp > 0305d14c1f791c93edcd3b32786b483b15f40a2d > src/tests/containerizer/nested_mesos_containerizer_tests.cpp > e6c690c411f57138207044f31b4816bd4090c1b7 > > Diff: https://reviews.apache.org/r/53354/diff/ > > > Testing > ------- > > make -j check > sudo src/mesos-tests > > > Thanks, > > Kevin Klues > >
