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/07/12 06:46:44 UTC
Re: Review Request 34135: Add filesystem/posix isolator for persistent
volumes.
-----------------------------------------------------------
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/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
>
>