-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51857/#review149726
-----------------------------------------------------------




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 604)
<https://reviews.apache.org/r/51857/#comment217406>

    No need for the else here.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 722 - 730)
<https://reviews.apache.org/r/51857/#comment217409>

    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.


- Jie Yu


On Sept. 20, 2016, 5:10 p.m., Avinash sridharan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> -----------------------------------------------------------
> 
> (Updated Sept. 20, 2016, 5:10 p.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.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
> 
>

Reply via email to