> On Sept. 20, 2016, 9:10 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 725-733 > > <https://reviews.apache.org/r/51857/diff/5/?file=1505752#file1505752line725> > > > > This is problematic. > > > > If both rootDir and pluginDir is not set, meaning that containers will > > only join host network. Say the parent container joins the host network. > > The child container will join host network as well, but it has a rootfs so > > we need to setup network files for it. > > > > First of all, the CHECK_SOME above will fail. Then the > > `CHECK(!infos.contains(...))` will fail as well. > > > > That's the reason I highly suggested previously that you combine the > > logic here with the if block in line 701. > > > > For child containers, do not add 'containerNetworks' to its info > > because itself does not create/join a new CNI network. You'll need to get > > the Info for its parent container anyway, so that's redundant information. > > > > In that way, the 'if' condition at line 701 still applies to child > > containers: > > ``` > > if (infos[containerId]->containerNetworks.empty() && > > infos[containerId]->rootfs.isSome()) { > > ``` > > > > However, instead of using the host file directly, you may want to check > > if the parent container has network files or not. It's likely that the > > parent container joins host network without rootfs. In that case, it does > > not have network files. > > Avinash sridharan wrote: > Why is this is an issue for a child container with a rootfs ? > The child container with a rootfs is no different than the parent > container. For these containers, the code would never reach line `727`. The > execution would be that of code block `707` to `725`. This shouldn't any > different than the way it is working today. > > Avinash sridharan wrote: > By the way my above comment is valid only for child containers with > rootfs joining the host network.
oops. my bad. I forgot that the if block starting from line 701 will short circuit. So info.containerNetworks for nested container is the networks of its parent. Let's document this in hpp. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51857/#review149726 ----------------------------------------------------------- On Sept. 21, 2016, 12:06 a.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51857/ > ----------------------------------------------------------- > > (Updated Sept. 21, 2016, 12:06 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang. > > > Bugs: MESOS-6156 > https://issues.apache.org/jira/browse/MESOS-6156 > > > Repository: mesos > > > Description > ------- > > The network file setup in the `network/cni` isolator is now nesting > aware. Since the children share the network and UTS namespace with the > parent, the network files need to be created only for the parent > container. For the child containers, the network files will be simply > a bind mount of the parents network files. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp > 949da8f70fb1cd13d6359780b032cb170693ea3e > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp > 359479083894e887647a694a1a133dce44817073 > > Diff: https://reviews.apache.org/r/51857/diff/ > > > Testing > ------- > > make > make check > and > sudo ./bin/mesos-tests.sh > > The only tests that failed were the SUDO make check tests: > [ FAILED ] 3 tests, listed below: > [ FAILED ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen > [ FAILED ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS > [ FAILED ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume > > > Thanks, > > Avinash sridharan > >
