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 53354: Updated namespace isolators to customize based on 'ContainerClass'.
Date Fri, 04 Nov 2016 23:08:58 GMT

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 267 - 282)
<https://reviews.apache.org/r/53354/#comment224727>

    This is not necessary for now because docker volume isolator is not nesting aware yet.
So this method won't be called for a debug container.
    
    I'll simply remove it for now. When we add support for nesting to Docker volume isolator,
we'll address this in an atomic patch.



src/slave/containerizer/mesos/isolators/filesystem/shared.cpp (lines 81 - 96)
<https://reviews.apache.org/r/53354/#comment224728>

    Ditto on removing this. Shared filesystem isolator is not nesting aware. We also plan
to just remove this isolator in favor of using linux filesystem isolator.



src/slave/containerizer/mesos/isolators/gpu/isolator.cpp (line 286)
<https://reviews.apache.org/r/53354/#comment224807>

    What if `!containerConfig.has_container_class` (meaning using the default), do you need
to short cut by `return _prepare(...);`?



src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (line 88)
<https://reviews.apache.org/r/53354/#comment224812>

    You want to enter the MNT namespace as well, right? Because otherwise, the /proc might
not be for the host.



src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (lines 90 - 98)
<https://reviews.apache.org/r/53354/#comment224815>

    I'd prefer the following logic for now. Let's add switch later once we have another type.
It's unknown to me if we'll follow the same pattern here.
    ```
    if (containerConfig.has_container_class() &&
        containerConfig.container_class() == ContainerClass::DEBUG) {
      return launchInfo;
    }
    ```



src/slave/containerizer/mesos/isolators/namespaces/pid.cpp (line 91)
<https://reviews.apache.org/r/53354/#comment224810>

    Want to remove this?



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 614 - 618)
<https://reviews.apache.org/r/53354/#comment224838>

    I'll also add a NOTE saying that if the parent container has a rootfs, the filesystem/linux
isolator will properly set the namespace to enter (MNT namespace). If the parent does not
have a rootfs, since it joins host network, no namespace needs to enter.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 620 - 627)
<https://reviews.apache.org/r/53354/#comment224833>

    For DEBUG containers, we don't need to create 'info' because we don't need to prepare
/etc/ network files for the container.
    
    The reason we create 'info' for normal nested container is because we need to setup network
files later in isolate().



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 664)
<https://reviews.apache.org/r/53354/#comment224837>

    I would add a NOTE saying that for debug class, we need to enter the mount namespace of
the parent container as well. We rely on filesystem/linux isolator to do that.
    
    Alternatively, we can make it explicit here.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 672 - 678)
<https://reviews.apache.org/r/53354/#comment224842>

    I prefer:
    ```
    if (containerConfig.has_container_class() &&
        containerConfig.container_class() == ContainerClass::DEBUG) {
      launchInfo.set_enter_namespaces(CLONE_NEWNS);
    } else {
      launchInfo.set_clone_namespaces(CLONE_NEWNS);
    }
    ```



src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 2517 - 2533)
<https://reviews.apache.org/r/53354/#comment224816>

    No need for this. port mapping isolator is not nesting aware.



src/slave/containerizer/mesos/isolators/volume/image.cpp (lines 91 - 101)
<https://reviews.apache.org/r/53354/#comment224844>

    I'd prefer:
    ```
    if (containerConfig.has_container_class() &&
        containerConfig.container_class() == ContainerClass::DEBUG) {
      ContainerLaunchInfo launchInfo;
      launchInfo.set_enter_namespaces(CLONE_NEWNS);
      return launchInfo;
    }
    ```



src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp (lines 130 - 140)
<https://reviews.apache.org/r/53354/#comment224845>

    I'd actually tweek the logics here. You want to short circut for the case where bindMount
is not supported as well as volume is not supported for DEBUG containers.
    
    ```
    if (containerId.has_parent() &&
        containerConfig.has_container_class() &&
        containerConfig.container_class() == ContainerClass::DEBUG) {
      if (bindMountSupported) {
        launchInfo.set_enter_namespaces(CLONE_NEWNS);
      }
      
      // No need to proceed because volumes are not supported
      // for DEBUG container currently.
      return launchInfo;
    }
      
    if (bindMountSupported) {
      launchInfo.set_clone_namespaces(CLONE_NEWNS);
    }
    ```


- Jie Yu


On Nov. 4, 2016, 9:34 p.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53354/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2016, 9:34 p.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 67cc595278f124cdf518d2f4fcfb257439f067e2

>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp af9f3736b487b595e8768e56ce60dc4823db28a1

>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp df16b8fee6799a69c7d96f33a5049bd9787c48f5

>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp a1283e5ee92c916baaf9fca8ce314d597e8421b3

>   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 939142e36b926d9e4201d35dedd25e32e9f8c63c

>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 48202fb5bf1ede71b80760844c6d8a36ca7c700c

>   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
> (Some tests are still fialing though -- need to debug)
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


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