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 Thu, 15 Mar 2018 21:08:10 GMT

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


Fix it, then Ship it!





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

    I'd make this `const` and not set a default value. This will force the caller to set a
value (reducing surprises)



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 409-410 (original), 398-399 (patched)
<https://reviews.apache.org/r/65987/#comment279589>

    This is not correct. The entries here might be nested container. We need to introduce
a parse method for ContainerID



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

    This should be:
    ```
    const bool joinsParentsNetwork =
      isDebugContainer ||
      !containerConfig.has_container_info() ||
      containerConfig.container_info().network_infos().empty();
    ```
    
    This is because for debug containers, it joins parent network, but does not specify container_info
or network_info



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

    Given above, this can be simplified to:
    ```
    if (isNestedContainer && joinsParentsNetwork) {
      ..
    }
    ```



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

    This comment is no longer accurate. We only checkpoint containers that join non-host networks.



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

    I'd move this up to where we created the Info struct.



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

    we also allow users to override the hostname. as a result, we should only do that if `info->hostname.isNone()`
    
    This can simply be:
    ```
    string hostname = info->hostname.isSome()
      ? info->hostname.get()
      : containerId.value();
    ```



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

    I'd rather let `getInterfaceDir` to take `ContainerID` as the parameter.
    
    Ditto else where



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

    typo `network`


- Jie Yu


On March 15, 2018, 4:30 a.m., Sagar Patwardhan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65987/
> -----------------------------------------------------------
> 
> (Updated March 15, 2018, 4:30 a.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 f0b86775b 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 1d01915c2 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 896656987 
> 
> 
> Diff: https://reviews.apache.org/r/65987/diff/5/
> 
> 
> Testing
> -------
> 
> Manually tested.
> 
> Working on unit tests.
> 
> 
> Thanks,
> 
> Sagar Patwardhan
> 
>


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