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 34135: Add filesystem/ isolators for persistent volumes.
Date Thu, 25 Jun 2015 19:47:16 GMT

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


I wish you could have split the patch. I think the Posix filesystem isolator looks good and
we can commit that first (that matches the existing semantics). We can add the linux filesystem
isolator in the following patch. What do you think?

I'll do a second pass on the linux isolator once you resovle the existing issues.


src/slave/containerizer/isolators/filesystem/linux.hpp (line 72)
<https://reviews.apache.org/r/34135/#comment136127>

    You don't need `explicit` here.



src/slave/containerizer/isolators/filesystem/linux.cpp (lines 48 - 51)
<https://reviews.apache.org/r/34135/#comment142007>

    Ditto.



src/slave/containerizer/isolators/filesystem/linux.cpp (line 85)
<https://reviews.apache.org/r/34135/#comment142009>

    Hum, does this patch depend on some other patches? I don't see 'rootfs' is a field in
ExecutorRunState?



src/slave/containerizer/isolators/filesystem/linux.cpp (line 92)
<https://reviews.apache.org/r/34135/#comment142014>

    static Try<string> prepareHostPath



src/slave/containerizer/isolators/filesystem/linux.cpp (lines 207 - 210)
<https://reviews.apache.org/r/34135/#comment142012>

    Should this be a CHECK instead since the MesosContainerizer should have already checked
that?



src/slave/containerizer/isolators/filesystem/linux.cpp (line 220)
<https://reviews.apache.org/r/34135/#comment142013>

    Can you add a comment explaining why MS_SHARED is used? Also, can you also explain why
this has to be done in the host mount namespace.



src/slave/containerizer/isolators/filesystem/linux.cpp (line 234)
<https://reviews.apache.org/r/34135/#comment142015>

    Can you add a comment about why you want to do the mount of volumes in the isolation script
(e.g., do not want to polute the host mount table)?



src/slave/containerizer/isolators/filesystem/linux.cpp (lines 406 - 407)
<https://reviews.apache.org/r/34135/#comment142016>

    Do you need to umount persistent volumes as well?



src/slave/containerizer/isolators/filesystem/posix.cpp (lines 42 - 45)
<https://reviews.apache.org/r/34135/#comment141981>

    We typically put using clauses outside the namespace scope. Please move these declarations
up right below 'using namespace process'. Here and everywhere else.



src/slave/containerizer/isolators/filesystem/posix.cpp (line 98)
<https://reviews.apache.org/r/34135/#comment141993>

    Please remove the space between [] and ()



src/slave/containerizer/linux_launcher.cpp (lines 120 - 121)
<https://reviews.apache.org/r/34135/#comment141967>

    I think you need to do a rebase. Kapil recently added a method to Isolators to return
the namespaces required. You don't need to do this anymore. Yuu can just overload that method
in your isolator and return the required namespaces.



src/slave/containerizer/mesos/containerizer.cpp (lines 111 - 114)
<https://reviews.apache.org/r/34135/#comment142005>

    Since you are using strings::tokenize below, I think you can simply do the following here.
    ```
    isolation += ",filesystem/posix";
    ```



src/slave/containerizer/mesos/containerizer.cpp (lines 158 - 166)
<https://reviews.apache.org/r/34135/#comment142002>

    Hum... why this logic is under 'if (ModuleManager::contains<Isolator>(type)' ?



src/slave/containerizer/mesos/containerizer.cpp (lines 175 - 182)
<https://reviews.apache.org/r/34135/#comment142004>

    I think you need a rebase. This logic has been changed.


- Jie Yu


On June 22, 2015, 4:41 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
> 
> (Updated June 22, 2015, 4:41 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Moved code from Mesos Containerizer to filesystem isolators
>  - filesystem/posix (symlinks, doesn't support container rootfs)
>  - filesystem/linux (bind mounts, does support container rootfs)
> 
> The filesystem/posix isolator will be automatically included if no filesystem/ isolator
is specified.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025

>   src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712

> 
> Diff: https://reviews.apache.org/r/34135/diff/
> 
> 
> Testing
> -------
> 
> existing persistent volumes tests.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>


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