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/21 23:23:07 UTC
Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/
-----------------------------------------------------------
Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
Bugs: MESOS-3483
https://issues.apache.org/jira/browse/MESOS-3483
Repository: mesos
Description
-------
Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
See ticket for motivation.
Diffs
-----
src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
Diff: https://reviews.apache.org/r/38569/diff/
Testing
-------
sudo make check
Thanks,
Jie Yu
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99927
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp (line 87)
<https://reviews.apache.org/r/38569/#comment156944>
This only checks if it is mounted, but not if it is bind mounted. Not sure if this is correct when work_dir itself is a non-bind mounted directory.
src/slave/containerizer/isolators/filesystem/linux.cpp (line 118)
<https://reviews.apache.org/r/38569/#comment156942>
Not sure if you need to undo the conditional previous bind mount.
src/slave/containerizer/isolators/filesystem/linux.cpp (line 129)
<https://reviews.apache.org/r/38569/#comment156943>
Ditto
src/tests/environment.cpp (line 47)
<https://reviews.apache.org/r/38569/#comment156941>
This one looks unnecessary.
- Cong Wang
On Sept. 21, 2015, 9:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 9:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99869
-----------------------------------------------------------
Ship it!
Ship It!
- Jiang Yan Xu
On Sept. 21, 2015, 2:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 2:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99895
-----------------------------------------------------------
Ship it!
Ship It!
- haosdent huang
On Sept. 21, 2015, 9:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 9:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99858
-----------------------------------------------------------
Patch looks great!
Reviews applied: [38569]
All tests passed.
- Mesos ReviewBot
On Sept. 21, 2015, 9:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 9:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 21, 2015, 10:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 73-74
> > <https://reviews.apache.org/r/38569/diff/1/?file=1078547#file1078547line73>
> >
> > After then sentense `using the bind backend)` add
> >
> > `because cleanup operations within the work_dir can be propagted to all container namespaces.`?
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99831
-----------------------------------------------------------
On Sept. 21, 2015, 9:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 9:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by Jie Yu <yu...@gmail.com>.
> On Sept. 21, 2015, 10:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 97-98
> > <https://reviews.apache.org/r/38569/diff/1/?file=1078547#file1078547line97>
> >
> > So this seems to work but oh my godness the the way `mount` command interacts with the syscall is very implicit.
> >
> > I can confirm this:
> >
> > ```
> > # /home/jyx/tmp/ is created by mount command and /home/jyx/tmp/target is mounted by syscall.
> > [root tmp]# mount --make-shared target
> > mount: can't find /home/jyx/tmp/target in /etc/fstab or /etc/mtab
> > [root tmp]# mount --make-rshared .
> > # OK.
> > ```
> >
> > So the rule seemed to be **as long as the arguments provided to the `mount` command themselves are mounted by the the command, we are OK**. (I guess because whatever is recurively done by the command is using syscall directly so it's fine)
> >
> > So I suspect that the `mount --make-rslave /` command inside the container **should work** because there is always a root `/` mount.
> >
> > I think we should seek consistency: use fs::mount() as long as it doesn't break `mount` commands. Or stick with `mount` command whenever possible?
OK, I think the comments above do not capture my intention. It's copied from the port mapping isolator. The motivation for using the command 'mount' to mount the work_dir is because: the mount will still be there after all containers and slave stopped. It's better to show this mount when operator types command 'mount' (so that it's not quite invisible). We did the same thing for /var/run/netns self bind mount.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99831
-----------------------------------------------------------
On Sept. 21, 2015, 9:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 9:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Sept. 21, 2015, 3:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 97-98
> > <https://reviews.apache.org/r/38569/diff/1/?file=1078547#file1078547line97>
> >
> > So this seems to work but oh my godness the the way `mount` command interacts with the syscall is very implicit.
> >
> > I can confirm this:
> >
> > ```
> > # /home/jyx/tmp/ is created by mount command and /home/jyx/tmp/target is mounted by syscall.
> > [root tmp]# mount --make-shared target
> > mount: can't find /home/jyx/tmp/target in /etc/fstab or /etc/mtab
> > [root tmp]# mount --make-rshared .
> > # OK.
> > ```
> >
> > So the rule seemed to be **as long as the arguments provided to the `mount` command themselves are mounted by the the command, we are OK**. (I guess because whatever is recurively done by the command is using syscall directly so it's fine)
> >
> > So I suspect that the `mount --make-rslave /` command inside the container **should work** because there is always a root `/` mount.
> >
> > I think we should seek consistency: use fs::mount() as long as it doesn't break `mount` commands. Or stick with `mount` command whenever possible?
>
> Jie Yu wrote:
> OK, I think the comments above do not capture my intention. It's copied from the port mapping isolator. The motivation for using the command 'mount' to mount the work_dir is because: the mount will still be there after all containers and slave stopped. It's better to show this mount when operator types command 'mount' (so that it's not quite invisible). We did the same thing for /var/run/netns self bind mount.
This SGTM!
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99831
-----------------------------------------------------------
On Sept. 21, 2015, 2:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 2:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by haosdent huang <ha...@gmail.com>.
> On Sept. 21, 2015, 10:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 97-98
> > <https://reviews.apache.org/r/38569/diff/1/?file=1078547#file1078547line97>
> >
> > So this seems to work but oh my godness the the way `mount` command interacts with the syscall is very implicit.
> >
> > I can confirm this:
> >
> > ```
> > # /home/jyx/tmp/ is created by mount command and /home/jyx/tmp/target is mounted by syscall.
> > [root tmp]# mount --make-shared target
> > mount: can't find /home/jyx/tmp/target in /etc/fstab or /etc/mtab
> > [root tmp]# mount --make-rshared .
> > # OK.
> > ```
> >
> > So the rule seemed to be **as long as the arguments provided to the `mount` command themselves are mounted by the the command, we are OK**. (I guess because whatever is recurively done by the command is using syscall directly so it's fine)
> >
> > So I suspect that the `mount --make-rslave /` command inside the container **should work** because there is always a root `/` mount.
> >
> > I think we should seek consistency: use fs::mount() as long as it doesn't break `mount` commands. Or stick with `mount` command whenever possible?
>
> Jie Yu wrote:
> OK, I think the comments above do not capture my intention. It's copied from the port mapping isolator. The motivation for using the command 'mount' to mount the work_dir is because: the mount will still be there after all containers and slave stopped. It's better to show this mount when operator types command 'mount' (so that it's not quite invisible). We did the same thing for /var/run/netns self bind mount.
>
> Jiang Yan Xu wrote:
> This SGTM!
Seems mount command would update mtab/fstab when necessary after call mount(2) https://github.com/karelzak/util-linux/blob/master/libmount/src/context_mount.c#L1090
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99831
-----------------------------------------------------------
On Sept. 21, 2015, 9:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 9:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 38569: Made slave's work_dir a shared mount in
LinuxFilesystemIsolator.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38569/#review99831
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 73 - 74)
<https://reviews.apache.org/r/38569/#comment156842>
After then sentense `using the bind backend)` add
`because cleanup operations within the work_dir can be propagted to all container namespaces.`?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 84)
<https://reviews.apache.org/r/38569/#comment156818>
Is `workDirMount` better?
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 97 - 98)
<https://reviews.apache.org/r/38569/#comment156811>
So this seems to work but oh my godness the the way `mount` command interacts with the syscall is very implicit.
I can confirm this:
```
# /home/jyx/tmp/ is created by mount command and /home/jyx/tmp/target is mounted by syscall.
[root tmp]# mount --make-shared target
mount: can't find /home/jyx/tmp/target in /etc/fstab or /etc/mtab
[root tmp]# mount --make-rshared .
# OK.
```
So the rule seemed to be **as long as the arguments provided to the `mount` command themselves are mounted by the the command, we are OK**. (I guess because whatever is recurively done by the command is using syscall directly so it's fine)
So I suspect that the `mount --make-rslave /` command inside the container **should work** because there is always a root `/` mount.
I think we should seek consistency: use fs::mount() as long as it doesn't break `mount` commands. Or stick with `mount` command whenever possible?
- Jiang Yan Xu
On Sept. 21, 2015, 2:23 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38569/
> -----------------------------------------------------------
>
> (Updated Sept. 21, 2015, 2:23 p.m.)
>
>
> Review request for mesos, Jojy Varghese, Timothy Chen, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3483
> https://issues.apache.org/jira/browse/MESOS-3483
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Made slave's work_dir a shared mount in LinuxFilesystemIsolator.
>
> See ticket for motivation.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/filesystem/linux.cpp f1e6f7519bdeeff7790fff63e7a9cb3075001758
> src/tests/environment.cpp e40cde23daba25b0b61b567ee73682c67e7acbdc
> src/tests/utils.cpp 498c9aa1442c4e937a74364073b2267dde6ffcb2
>
> Diff: https://reviews.apache.org/r/38569/diff/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Jie Yu
>
>