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