This is an automatically generated e-mail. To reply, visit:

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 

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

src/slave/containerizer/isolators/filesystem/linux.hpp (line 72)

    You don't need `explicit` here.

src/slave/containerizer/isolators/filesystem/linux.cpp (lines 48 - 51)


src/slave/containerizer/isolators/filesystem/linux.cpp (line 85)

    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)

    static Try<string> prepareHostPath

src/slave/containerizer/isolators/filesystem/linux.cpp (lines 207 - 210)

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

src/slave/containerizer/isolators/filesystem/linux.cpp (line 220)

    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)

    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)

    Do you need to umount persistent volumes as well?

src/slave/containerizer/isolators/filesystem/posix.cpp (lines 42 - 45)

    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)

    Please remove the space between [] and ()

src/slave/containerizer/linux_launcher.cpp (lines 120 - 121)

    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)

    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)

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

src/slave/containerizer/mesos/containerizer.cpp (lines 175 - 182)

    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

Reply via email to