You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/05/13 02:47:46 UTC
Review Request 34135: Add filesystem/ isolators for persistent
volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/
-----------------------------------------------------------
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 14bc976a7b6a656fb58085484d25c3de3cf0f693
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 b9e22e3c70bed0c29e2ca8632411789d33f779a8
src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
Diff: https://reviews.apache.org/r/34135/diff/
Testing
-------
existing persistent volumes tests.
Thanks,
Ian Downes
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Ian Downes <ia...@gmail.com>.
> On May 20, 2015, 7:34 p.m., Paul Brett wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 69
> > <https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line69>
> >
> > We don't need to be root, we just need to have CAP_SYS_ADMIN, and we could pick that up through a helpful suid mount program.
We call mount(2) directly and Mesos is not expected to be setuid root. This is consistent with the rest of the code so if/when we change that we'll do everything.
> On May 20, 2015, 7:34 p.m., Paul Brett wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 178
> > <https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line178>
> >
> > This really tests if the container_path exists in the filesystem namespace, the actual location could be anywhere.
True, but is it a problem if it's elsewhere?
> On May 20, 2015, 7:34 p.m., Paul Brett wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 203
> > <https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line203>
> >
> > Should thie be comparing realpath(containerPath) with realpath(rootfs) in case the rootfs spec you are given contains symbolic links?
It's a precondition that rootfs is absolute, enforced elsewhere.
> On May 20, 2015, 7:34 p.m., Paul Brett wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 241
> > <https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line241>
> >
> > Don't we want the option of mounting read only?
Nope, this is the work directory which we state is always writable.
> On May 20, 2015, 7:34 p.m., Paul Brett wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, line 134
> > <https://reviews.apache.org/r/34135/diff/1/?file=957260#file957260line134>
> >
> > I'm sure there will be more than one linux filesystem isolator, should we call this filesystem/bind?
Maybe. But I'd expect to add to the linux isolator rather than having a multitude.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review84644
-----------------------------------------------------------
On May 12, 2015, 5:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:47 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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> 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 b9e22e3c70bed0c29e2ca8632411789d33f779a8
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review84644
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment135988>
We don't need to be root, we just need to have CAP_SYS_ADMIN, and we could pick that up through a helpful suid mount program.
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment135994>
This really tests if the container_path exists in the filesystem namespace, the actual location could be anywhere.
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment135996>
Should thie be comparing realpath(containerPath) with realpath(rootfs) in case the rootfs spec you are given contains symbolic links?
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment135999>
Don't we want the option of mounting read only?
src/slave/containerizer/mesos/containerizer.cpp
<https://reviews.apache.org/r/34135/#comment136000>
I'm sure there will be more than one linux filesystem isolator, should we call this filesystem/bind?
- Paul Brett
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> 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 b9e22e3c70bed0c29e2ca8632411789d33f779a8
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review89974
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp (line 216)
<https://reviews.apache.org/r/34135/#comment142881>
We also need to make sure the directory exists before mounting, and if not create it.
- Timothy Chen
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
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Ian Downes <ia...@gmail.com>.
> On June 25, 2015, 12:47 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 406-407
> > <https://reviews.apache.org/r/34135/diff/2/?file=989747#file989747line406>
> >
> > Do you need to umount persistent volumes as well?
As the comment states, it "notably this includes persistent volume mounts" :-)
> On June 25, 2015, 12:47 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 158-166
> > <https://reviews.apache.org/r/34135/diff/2/?file=989751#file989751line158>
> >
> > Hum... why this logic is under 'if (ModuleManager::contains<Isolator>(type)' ?
Good catch! No idea why it was there in the first place and why I perpetuated the erroneous code.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review84775
-----------------------------------------------------------
On June 22, 2015, 9:41 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 9:41 a.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
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review91401
-----------------------------------------------------------
Bad patch!
Reviews applied: [34908, 34136, 34137]
Failed command: ./support/apply-review.sh -n -r 34137
Error:
2015-07-12 04:46:07 URL:https://reviews.apache.org/r/34137/diff/raw/ [27315/27315] -> "34137.patch" [1]
error: patch failed: include/mesos/slave/isolator.hpp:30
error: include/mesos/slave/isolator.hpp: patch does not apply
error: patch failed: src/Makefile.am:525
error: src/Makefile.am: patch does not apply
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:548
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On July 12, 2015, 4:42 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:42 a.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 e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review92497
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/posix.cpp (lines 91 - 92)
<https://reviews.apache.org/r/34135/#comment146709>
This check does not seem to be necessary.
- Jie Yu
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.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)
> -
> The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified.
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review92512
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/posix.cpp (lines 198 - 204)
<https://reviews.apache.org/r/34135/#comment146722>
Rebase is needed to pickup the change that setup the ownership.
- Jie Yu
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.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)
> -
> The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified.
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review92516
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.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)
> -
> The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified.
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review92494
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp (line 52)
<https://reviews.apache.org/r/34135/#comment146705>
This shouldn't under #ifdef __linux__ guard
- Jie Yu
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.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)
> -
> The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified.
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
> On July 21, 2015, 9:41 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/filesystem/posix.cpp, lines 183-185
> > <https://reviews.apache.org/r/34135/diff/3/?file=1009133#file1009133line183>
> >
> > I just realized an issue here (not your fault).
> >
> > Since we don't checkpoint 'resources' in this isolator, when slave restarts and recovers, the 'resources' in Info will be empty, but there are symlinks exists in the sandbox.
> >
> > We'll end up with trying to create already exist symlinks (and fail). I think this is easy to resolve because we can just ignore EEXIST.
> >
> > Also, it's likely that we fail to remove some symlinks because they do not exist (e.g., a task finishes but the slave crashes after containerizer->update is called but before the status update is checkpointed).
Since this is not a issue introduced by this review. I created https://issues.apache.org/jira/browse/MESOS-3124 to track.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review92480
-----------------------------------------------------------
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.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)
> -
> The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified.
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review92480
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/posix.cpp (lines 183 - 185)
<https://reviews.apache.org/r/34135/#comment146681>
I just realized an issue here (not your fault).
Since we don't checkpoint 'resources' in this isolator, when slave restarts and recovers, the 'resources' in Info will be empty, but there are symlinks exists in the sandbox.
We'll end up with trying to create already exist symlinks (and fail). I think this is easy to resolve because we can just ignore EEXIST.
Also, it's likely that we fail to remove some symlinks because they do not exist (e.g., a task finishes but the slave crashes after containerizer->update is called but before the status update is checkpointed).
- Jie Yu
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.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)
> -
> The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified.
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/
-----------------------------------------------------------
(Updated July 11, 2015, 9:46 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Summary (updated)
-----------------
Add filesystem/posix isolator for persistent volumes.
Repository: mesos
Description (updated)
-------
Moved code from Mesos Containerizer to filesystem isolators
- filesystem/posix (symlinks, doesn't support container rootfs)
-
The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified.
Diffs
-----
src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
Diff: https://reviews.apache.org/r/34135/diff/
Testing
-------
existing persistent volumes tests.
Thanks,
Ian Downes
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/
-----------------------------------------------------------
(Updated July 11, 2015, 9:42 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Moved filesystem/linux to separate review. Addressed comments.
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 (updated)
-----
src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION
src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION
src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
Diff: https://reviews.apache.org/r/34135/diff/
Testing
-------
existing persistent volumes tests.
Thanks,
Ian Downes
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review91378
-----------------------------------------------------------
Bad patch!
Reviews applied: [34908, 34136, 34137]
Failed command: ./support/apply-review.sh -n -r 34137
Error:
2015-07-11 00:17:12 URL:https://reviews.apache.org/r/34137/diff/raw/ [27315/27315] -> "34137.patch" [1]
error: patch failed: include/mesos/slave/isolator.hpp:30
error: include/mesos/slave/isolator.hpp: patch does not apply
error: patch failed: src/Makefile.am:525
error: src/Makefile.am: patch does not apply
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:548
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On July 11, 2015, 12:05 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 12:05 a.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
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/
-----------------------------------------------------------
(Updated July 10, 2015, 5:05 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
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/
-----------------------------------------------------------
(Updated June 22, 2015, 9:41 a.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Removed rootfs recovery from isolator in favor of checkpointing the rootfs and recoverying as part of the RunState.
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 (updated)
-----
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
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 2, 2015, 2:45 p.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 95
> > <https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line95>
> >
> > actually I'm wrong, I was reading the old style guide. The newest style guide we do put a space, ignore my earler comment.
?
Doesn't https://github.com/apache/mesos/blob/master/docs/mesos-c%2B%2B-style-guide.md say: "Don't put a space between the capture list and the parameter list"?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review86293
-----------------------------------------------------------
On May 12, 2015, 5:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated May 12, 2015, 5:47 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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> 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 b9e22e3c70bed0c29e2ca8632411789d33f779a8
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review86293
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment138275>
actually I'm wrong, I was reading the old style guide. The newest style guide we do put a space, ignore my earler comment.
- Timothy Chen
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> 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 b9e22e3c70bed0c29e2ca8632411789d33f779a8
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review83558
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134561>
I'm still learning our lambda style rules, but seems like we don't put a space in between [] and ()
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134562>
Somehow this wrapping reads a bit funny to me, I don't think we usually line break like this?
Don't we either put them all on seperate lines, or as it fits here all the params in one line? Just a nit.
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134564>
s/directory//work directory/g
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134563>
What's the rationale here? Leave a comment perhaps?
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134565>
Same here
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134566>
if using the host's filesystem
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134567>
hmm, looks like the extra slash is intentinoal then?
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134568>
I think we don't put space between [] and ().
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134569>
ditto
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134570>
Log the umount error?
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment134572>
log the mount error?
- Timothy Chen
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> 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 b9e22e3c70bed0c29e2ca8632411789d33f779a8
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34135: Add filesystem/ isolators for persistent
volumes.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34135/#review84194
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment136749>
nit: comment can probably be killed.
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment136750>
move this comment down to above os::exist or reword it a bit?
I was thinking your were referring to the test above.
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment136754>
nit: comment can probably be killed.
src/slave/containerizer/isolators/filesystem/linux.cpp
<https://reviews.apache.org/r/34135/#comment135330>
flags.sandbox_diretory is introduced in r/34137
- Chi Zhang
On May 13, 2015, 12:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34135/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 12:47 a.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 14bc976a7b6a656fb58085484d25c3de3cf0f693
> 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 b9e22e3c70bed0c29e2ca8632411789d33f779a8
> src/slave/containerizer/mesos/containerizer.cpp b644b9c74bc23cf78c0a53284544be6cdaef2f8a
>
> Diff: https://reviews.apache.org/r/34135/diff/
>
>
> Testing
> -------
>
> existing persistent volumes tests.
>
>
> Thanks,
>
> Ian Downes
>
>