Re: Review Request 38569: Made slave's work_dir a shared mount in LinuxFilesystemIsolator.

[Cong Wang]

---
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)


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)


Not sure if you need to undo the conditional previous bind mount.



src/slave/containerizer/isolators/filesystem/linux.cpp (line 129)


Ditto



src/tests/environment.cpp (line 47)


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.

[Jie Yu]

> On Sept. 21, 2015, 10:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 73-74
> > 
> >
> > 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.

[haosdent huang]

---
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.

[Jiang Yan Xu]

---
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.

[haosdent huang]

> On Sept. 21, 2015, 10:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 97-98
> > 
> >
> > 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.

[Jiang Yan Xu]

> On Sept. 21, 2015, 3:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 97-98
> > 
> >
> > 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.

[Mesos ReviewBot]

---
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.

[Jiang Yan Xu]

---
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)


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)


Is `workDirMount` better?



src/slave/containerizer/isolators/filesystem/linux.cpp (lines 97 - 98)


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
> 
>

Re: Review Request 38569: Made slave's work_dir a shared mount in LinuxFilesystemIsolator.

[Jie Yu]

> On Sept. 21, 2015, 10:35 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, lines 97-98
> > 
> >
> > 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
> 
>

Review Request 38569: Made slave's work_dir a shared mount in LinuxFilesystemIsolator.

[Jie Yu]

---
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