You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/09/13 08:33:17 UTC
Review Request 38333: Made container sandbox a shared mount to address
MESOS-3349.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38333/
-----------------------------------------------------------
Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
Bugs: MESOS-3349
https://issues.apache.org/jira/browse/MESOS-3349
Repository: mesos
Description
-------
Made container sandbox a shared mount to address MESOS-3349.
See the discussion in https://reviews.apache.org/r/38329/ for more context.
The idea is to mark container sandbox a shared mount (do a self bind mount first) so that persistent volume mounts can be propagated.
This is less invasive than marking '/' as a shared mount.
One followup for this patch is to set the default filesystem isolator to posix as the linux isolator will manipulate host mount table.
We also need to address the TODO in `LinuxFilesystemIsolator::_recover` so that tests do not leak mounts in the host mount table.
Diffs
-----
src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
src/slave/containerizer/provisioners/appc/provisioner.cpp cd29a00fa0db8af294c10bb7a2e0cb4252bd2993
src/slave/containerizer/provisioners/backends/bind.cpp 1cdae61786790dc6a475ae5f73c8cc92d2bbf739
Diff: https://reviews.apache.org/r/38333/diff/
Testing
-------
sudo make check on Centos5 and Centos6
Thanks,
Jie Yu
Re: Review Request 38333: Made container sandbox a shared mount to
address MESOS-3349.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38333/#review98764
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38333]
All tests passed.
- Mesos ReviewBot
On Sept. 13, 2015, 6:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38333/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2015, 6:33 a.m.)
>
>
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made container sandbox a shared mount to address MESOS-3349.
>
> See the discussion in https://reviews.apache.org/r/38329/ for more context.
>
> The idea is to mark container sandbox a shared mount (do a self bind mount first) so that persistent volume mounts can be propagated.
>
> This is less invasive than marking '/' as a shared mount.
>
> One followup for this patch is to set the default filesystem isolator to posix as the linux isolator will manipulate host mount table.
>
> We also need to address the TODO in `LinuxFilesystemIsolator::_recover` so that tests do not leak mounts in the host mount table.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
> src/slave/containerizer/provisioners/appc/provisioner.cpp cd29a00fa0db8af294c10bb7a2e0cb4252bd2993
> src/slave/containerizer/provisioners/backends/bind.cpp 1cdae61786790dc6a475ae5f73c8cc92d2bbf739
>
> Diff: https://reviews.apache.org/r/38333/diff/
>
>
> Testing
> -------
>
> sudo make check on Centos5 and Centos6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38333: Made container sandbox a shared mount to
address MESOS-3349.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38333/
-----------------------------------------------------------
(Updated Sept. 14, 2015, 10:29 p.m.)
Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Addressed comments. NNFR.
Bugs: MESOS-3349
https://issues.apache.org/jira/browse/MESOS-3349
Repository: mesos
Description
-------
Made container sandbox a shared mount to address MESOS-3349.
See the discussion in https://reviews.apache.org/r/38329/ for more context.
The idea is to mark container sandbox a shared mount (do a self bind mount first) so that persistent volume mounts can be propagated.
This is less invasive than marking '/' as a shared mount.
One followup for this patch is to set the default filesystem isolator to posix as the linux isolator will manipulate host mount table.
We also need to address the TODO in `LinuxFilesystemIsolator::_recover` so that tests do not leak mounts in the host mount table.
Diffs (updated)
-----
src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
src/slave/containerizer/provisioners/appc/provisioner.cpp cd29a00fa0db8af294c10bb7a2e0cb4252bd2993
src/slave/containerizer/provisioners/backends/bind.cpp 1cdae61786790dc6a475ae5f73c8cc92d2bbf739
Diff: https://reviews.apache.org/r/38333/diff/
Testing
-------
sudo make check on Centos5 and Centos6
Thanks,
Jie Yu
Re: Review Request 38333: Made container sandbox a shared mount to
address MESOS-3349.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 14, 2015, 6 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/appc/provisioner.cpp, lines 368-369
> > <https://reviews.apache.org/r/38333/diff/1/?file=1069474#file1069474line368>
> >
> > Oh...
> >
> > I thought `root` was being copied but I guess the compilers says otherwise?
Yeah, C++11 lambda only binds variables in the current scope. 'root' is a field member, not part of the current scope.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38333/#review98863
-----------------------------------------------------------
On Sept. 13, 2015, 6:33 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38333/
> -----------------------------------------------------------
>
> (Updated Sept. 13, 2015, 6:33 a.m.)
>
>
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made container sandbox a shared mount to address MESOS-3349.
>
> See the discussion in https://reviews.apache.org/r/38329/ for more context.
>
> The idea is to mark container sandbox a shared mount (do a self bind mount first) so that persistent volume mounts can be propagated.
>
> This is less invasive than marking '/' as a shared mount.
>
> One followup for this patch is to set the default filesystem isolator to posix as the linux isolator will manipulate host mount table.
>
> We also need to address the TODO in `LinuxFilesystemIsolator::_recover` so that tests do not leak mounts in the host mount table.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
> src/slave/containerizer/provisioners/appc/provisioner.cpp cd29a00fa0db8af294c10bb7a2e0cb4252bd2993
> src/slave/containerizer/provisioners/backends/bind.cpp 1cdae61786790dc6a475ae5f73c8cc92d2bbf739
>
> Diff: https://reviews.apache.org/r/38333/diff/
>
>
> Testing
> -------
>
> sudo make check on Centos5 and Centos6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38333: Made container sandbox a shared mount to
address MESOS-3349.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38333/#review98863
-----------------------------------------------------------
Ship it!
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 721 - 723)
<https://reviews.apache.org/r/38333/#comment155489>
You are logging the sandbox as "working directory" and "sandbox" respectively for the two (w/ new rootfs and w/o) cases in `prepare()`, should we aim for symmetry here when in `cleanup()`?
Or we can say `<< "Unmounting sandbox/work directory '" <<`?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 728)
<https://reviews.apache.org/r/38333/#comment155491>
Ditto about the name "sandbox" here.
src/slave/containerizer/provisioners/appc/provisioner.cpp (lines 368 - 369)
<https://reviews.apache.org/r/38333/#comment155494>
Oh...
I thought `root` was being copied but I guess the compilers says otherwise?
src/slave/containerizer/provisioners/appc/provisioner.cpp (line 379)
<https://reviews.apache.org/r/38333/#comment155495>
Adjust the expected likelyhood a bit by s/likely/possible/?
src/slave/containerizer/provisioners/appc/provisioner.cpp (line 380)
<https://reviews.apache.org/r/38333/#comment155529>
Expand to explain the reason for EBUSY?
```
EBUSY because of the race between cleaning up this container and new containers copying the host mount table
```
It's not immediately obvious without some comments.
src/slave/containerizer/provisioners/backends/bind.cpp (line 169)
<https://reviews.apache.org/r/38333/#comment155496>
s/returns/return/.
- Jiang Yan Xu
On Sept. 12, 2015, 11:33 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38333/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 11:33 p.m.)
>
>
> Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3349
> https://issues.apache.org/jira/browse/MESOS-3349
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made container sandbox a shared mount to address MESOS-3349.
>
> See the discussion in https://reviews.apache.org/r/38329/ for more context.
>
> The idea is to mark container sandbox a shared mount (do a self bind mount first) so that persistent volume mounts can be propagated.
>
> This is less invasive than marking '/' as a shared mount.
>
> One followup for this patch is to set the default filesystem isolator to posix as the linux isolator will manipulate host mount table.
>
> We also need to address the TODO in `LinuxFilesystemIsolator::_recover` so that tests do not leak mounts in the host mount table.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
> src/slave/containerizer/provisioners/appc/provisioner.cpp cd29a00fa0db8af294c10bb7a2e0cb4252bd2993
> src/slave/containerizer/provisioners/backends/bind.cpp 1cdae61786790dc6a475ae5f73c8cc92d2bbf739
>
> Diff: https://reviews.apache.org/r/38333/diff/
>
>
> Testing
> -------
>
> sudo make check on Centos5 and Centos6
>
>
> Thanks,
>
> Jie Yu
>
>