mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 65987: Allow nested containers in pods to have separate namespaces(Ref: MESOS-8534).
Date Fri, 09 Mar 2018 23:40:13 GMT

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



Looking good! Is it possible to add some test?


src/slave/containerizer/mesos/isolators/network/cni/cni.hpp
Lines 114 (patched)
<https://reviews.apache.org/r/65987/#comment279262>

    nits: `joinsParentsNetwork`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 389 (original), 389-397 (patched)
<https://reviews.apache.org/r/65987/#comment279263>

    Instead of relying on an additional checkpoint file, I think we can infer if a nested
container joins its parent container's network or not by first listing all entries in `rootDir`,
and then recover those known containers, and then cleanup unknown orphans. Something like
the following:
    
    ```
    // Build ContainerID -> ContainerState mapping
    // List rootDir
    // For each entry, find the corresponding ContainerState state (state is optional)
    // Call _recover(containerId, state), if containerId is nested, set joinsParentsNetwork
to false.
    // Cleanup unknown orphans (not in container state mapping or orphans map)
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 576-579 (original), 585-588 (patched)
<https://reviews.apache.org/r/65987/#comment279257>

    These should be `const bool`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 577-578 (original), 586-587 (patched)
<https://reviews.apache.org/r/65987/#comment279246>

    style nits. We typically do the following
    
    ```
    bool isDebugContainer =
      containerConfig.container_class() == ContainerClass::DEBUG;
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 589-590 (patched)
<https://reviews.apache.org/r/65987/#comment279247>

    style nits: we typically align like this:
    ```
    bool joinParentsNetwork =
      !containerConfig.has_container_info() ||
      containerConfig.container_info().network_infos().empty();
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 613 (patched)
<https://reviews.apache.org/r/65987/#comment279248>

    I'd add a comment here:
    ```
    // This is either a top level container, or a nested container
    // joining separate network than its parent.
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 581 (original), 613-614 (patched)
<https://reviews.apache.org/r/65987/#comment279249>

    It's possible that `containerConfig.has_container_inf() == false` here (top level container
joining host network without container image).
    
    We typically avoid depending on the default value of `ContainerInfo`. So I'd suggest we
do:
    
    ```
    if (containerCOnfig.has_container_info()) {
      const ContainerInfo& containerInfo = containerConfig.container_info();
      
      if (containerInfo.type() != ContainerInfo::MESOS) {
        return Failure("Can only prepare CNI networks for a MESOS container");
      }
      ...
    }
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 683 (original), 687 (patched)
<https://reviews.apache.org/r/65987/#comment279255>

    Not yours, let's reduce nesting by remove this `else` here given that we already shortcircuit
above `return None()`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 696-698 (original), 700-702 (patched)
<https://reviews.apache.org/r/65987/#comment279254>

    Debug container is always nested. So I'd adjust the logic here:
    ```
    if (isDebugContainer) {
      CHECK(isNestedContainer);
      
      // Nested DEBUG containers never need a new MOUNT namespace, so
      // we don't maintain information about them in the `infos` map.
    }
    ```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 770 (patched)
<https://reviews.apache.org/r/65987/#comment279256>

    hum, this looks problematic. We don't create `Info` struct for debug containers.
    
    Also, should we also update that field for the case where `if (containerNetworks.empty())
{` above (just to be consistent)?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1269-1285 (patched)
<https://reviews.apache.org/r/65987/#comment279258>

    I don't think we need to checkpoint this additional information. You should be able to
infer if a nested container has its own network namespace or not by checking if there's a
container directory under `cni::paths::ROOT_DIR`. See src/slave/containerizer/mesos/isolators/network/cni/paths.hpp



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Line 1441 (original)
<https://reviews.apache.org/r/65987/#comment279259>

    Why this change?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1488 (patched)
<https://reviews.apache.org/r/65987/#comment279260>

    What about top level containers? I think `joinParentsNetwork` should be orthogonol to
whether it is nested or not. Top level container can join its parent's network (agent) too.
    
    If you think about that way, here we should do
    ```
    if (isNestedContainer && joinsParentsNetwork)
    ```


- Jie Yu


On March 8, 2018, 5:07 p.m., Sagar Patwardhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65987/
> -----------------------------------------------------------
> 
> (Updated March 8, 2018, 5:07 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-8534
>     https://issues.apache.org/jira/browse/MESOS-8534
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Continued from https://github.com/apache/mesos/pull/263
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp f0b86775b7919ba6aa4a73038edb78a0adca68f4 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 1d01915c2db66e54ed68a3dbaa12ea061ca5f6b2

>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 896656987012b3ffe5008ce6873c9a5249c058de

>   src/slave/containerizer/mesos/isolators/network/cni/paths.hpp 7678a7c81c3cdb27410c1f066021eb34bd02a83f

>   src/slave/containerizer/mesos/isolators/network/cni/paths.cpp f9056c90f1075cb19c4f554e7d5b629561d06035

> 
> 
> Diff: https://reviews.apache.org/r/65987/diff/1/
> 
> 
> Testing
> -------
> 
> Manually tested.
> 
> Working on unit tests.
> 
> 
> Thanks,
> 
> Sagar Patwardhan
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message