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/12 08:36:04 UTC
Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/
-----------------------------------------------------------
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
-------
Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
See the discussion thread in the ticket for motivation (also the NOTE in the code).
Diffs
-----
src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
Diff: https://reviews.apache.org/r/38329/diff/
Testing
-------
sudo make check on CentOS5 and CentOS6
Thanks,
Jie Yu
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98743
-----------------------------------------------------------
Ship it!
Ship It!
src/slave/containerizer/isolators/filesystem/linux.cpp (line 190)
<https://reviews.apache.org/r/38329/#comment155315>
"parent mount" should be "parent mount point"?
- haosdent huang
On Sept. 12, 2015, 7:26 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 7:26 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98745
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38328, 38329]
All tests passed.
- Mesos ReviewBot
On Sept. 12, 2015, 7:26 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 7:26 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/
-----------------------------------------------------------
(Updated Sept. 12, 2015, 7:26 a.m.)
Review request for mesos, haosdent huang, Timothy Chen, Vinod Kone, and Jiang Yan Xu.
Changes
-------
Review comments.
Moved the logics to recover because we want to make sure work_dir exists.
Bugs: MESOS-3349
https://issues.apache.org/jira/browse/MESOS-3349
Repository: mesos
Description
-------
Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
See the discussion thread in the ticket for motivation (also the NOTE in the code).
Diffs (updated)
-----
src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
Diff: https://reviews.apache.org/r/38329/diff/
Testing
-------
sudo make check on CentOS5 and CentOS6
Thanks,
Jie Yu
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by haosdent huang <ha...@gmail.com>.
> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > <https://reviews.apache.org/r/38329/diff/1/?file=1069300#file1069300line91>
> >
> > Should we check we did successfully found one that we can mark shared?
>
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
>
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.
In fact, we have another way. We could
```
fs::mount(target, target, None(), MS_BIND, NULL);
fs::mount(None(), target, None(), MS_SHARED, NULL);
```
to mount self. So that we don't need affect the exist system mount points.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 6:36 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > <https://reviews.apache.org/r/38329/diff/1/?file=1069300#file1069300line91>
> >
> > Should we check we did successfully found one that we can mark shared?
>
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
>
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.
>
> haosdent huang wrote:
> In fact, we have another way. We could
>
> ```
> fs::mount(target, target, None(), MS_BIND, NULL);
> fs::mount(None(), target, None(), MS_SHARED, NULL);
> ```
>
> to mount self. So that we don't need affect the exist system mount points.
I thought about that too. But that's more complicated to me because we need to do a recursive self bind mount of the work_dir (in case volumes have already been mounted, think about the recovery case), which sounds more invasive to me.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 6:36 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > <https://reviews.apache.org/r/38329/diff/1/?file=1069300#file1069300line91>
> >
> > Should we check we did successfully found one that we can mark shared?
Good catch! Yeah, let me at that check!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 6:36 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by haosdent huang <ha...@gmail.com>.
> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > <https://reviews.apache.org/r/38329/diff/1/?file=1069300#file1069300line91>
> >
> > Should we check we did successfully found one that we can mark shared?
>
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
>
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.
>
> haosdent huang wrote:
> In fact, we have another way. We could
>
> ```
> fs::mount(target, target, None(), MS_BIND, NULL);
> fs::mount(None(), target, None(), MS_SHARED, NULL);
> ```
>
> to mount self. So that we don't need affect the exist system mount points.
>
> Jie Yu wrote:
> I thought about that too. But that's more complicated to me because we need to do a recursive self bind mount of the work_dir (in case volumes have already been mounted, think about the recovery case), which sounds more invasive to me.
Oh, thank you for explanation.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 6:36 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > <https://reviews.apache.org/r/38329/diff/1/?file=1069300#file1069300line91>
> >
> > Should we check we did successfully found one that we can mark shared?
>
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
>
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.
>
> haosdent huang wrote:
> In fact, we have another way. We could
>
> ```
> fs::mount(target, target, None(), MS_BIND, NULL);
> fs::mount(None(), target, None(), MS_SHARED, NULL);
> ```
>
> to mount self. So that we don't need affect the exist system mount points.
>
> Jie Yu wrote:
> I thought about that too. But that's more complicated to me because we need to do a recursive self bind mount of the work_dir (in case volumes have already been mounted, think about the recovery case), which sounds more invasive to me.
>
> haosdent huang wrote:
> Oh, thank you for explanation.
>
> Jie Yu wrote:
> After some testing, I realized that marking '/' as shared mount is more invasive than I thought.
>
> I'll discard this patch and instead, self bind mount the sandbox directory and mark it as shared. Expect a patch soon.
Posted https://reviews.apache.org/r/38333/
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
On Sept. 12, 2015, 7:26 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 7:26 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > <https://reviews.apache.org/r/38329/diff/1/?file=1069300#file1069300line91>
> >
> > Should we check we did successfully found one that we can mark shared?
>
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
>
> Jie Yu wrote:
> In fact, that should be a CHECK because we should always find one.
>
> haosdent huang wrote:
> In fact, we have another way. We could
>
> ```
> fs::mount(target, target, None(), MS_BIND, NULL);
> fs::mount(None(), target, None(), MS_SHARED, NULL);
> ```
>
> to mount self. So that we don't need affect the exist system mount points.
>
> Jie Yu wrote:
> I thought about that too. But that's more complicated to me because we need to do a recursive self bind mount of the work_dir (in case volumes have already been mounted, think about the recovery case), which sounds more invasive to me.
>
> haosdent huang wrote:
> Oh, thank you for explanation.
After some testing, I realized that marking '/' as shared mount is more invasive than I thought.
I'll discard this patch and instead, self bind mount the sandbox directory and mark it as shared. Expect a patch soon.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
On Sept. 12, 2015, 7:26 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 7:26 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 12, 2015, 6:38 a.m., Timothy Chen wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 91
> > <https://reviews.apache.org/r/38329/diff/1/?file=1069300#file1069300line91>
> >
> > Should we check we did successfully found one that we can mark shared?
>
> Jie Yu wrote:
> Good catch! Yeah, let me at that check!
In fact, that should be a CHECK because we should always find one.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 6:36 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38329: Ensured that the device containing slave
work_dir is a shared mount when LinuxFilesystemIsolator is used.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38329/#review98735
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp (line 91)
<https://reviews.apache.org/r/38329/#comment155308>
Should we check we did successfully found one that we can mark shared?
- Timothy Chen
On Sept. 12, 2015, 6:36 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38329/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2015, 6:36 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
> -------
>
> Ensured that the device containing slave work_dir is a shared mount when LinuxFilesystemIsolator is used.
>
> See the discussion thread in the ticket for motivation (also the NOTE in the code).
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp 0970b3d48b13d5e9d2e0160df5cf14a3dcd0acc9
>
> Diff: https://reviews.apache.org/r/38329/diff/
>
>
> Testing
> -------
>
> sudo make check on CentOS5 and CentOS6
>
>
> Thanks,
>
> Jie Yu
>
>