Re: Review Request 24178: Basic filesystem isolator for Linux.

2014-09-11 Thread Ian Downes 


> On Aug. 19, 2014, 5:21 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/filesystem.cpp, lines 44-46
> > 
> >
> > Check for kernel version? Check for root permission? For example, when 
> > bind mount is introduced?

bind mounts were introduced in 2.4.0 so safe to assume. Added test for root.


> On Aug. 19, 2014, 5:21 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/filesystem.cpp, lines 87-91
> > 
> >
> > Him, this reveals a more interesting problem. Say in the future, we 
> > want to implement a full file system isolator. The order in which we bind 
> > mount those volumns needs to be carefully calculated. For example, say we 
> > have two volumns: / and /lib. Can we actually support that?

Added checks to ensure masking doesn't occur. Added a TODO to improve this to 
support reordering mounts if possible.


> On Aug. 19, 2014, 5:21 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/filesystem.cpp, lines 102-126
> > 
> >
> > Can you explain why we need to create those directories? Is is 
> > necessary?
> > 
> > My feeling is that it's not necessary. If you want those dir 
> > structures, you should specify more Volumns, right?
> > 
> > Well, again, if we have multiple volumns, the question is how do we 
> > ensure the correct mount order.

I've kept the isolator simple by not supporting mounting to ancestors.

The directory creation code is necessary for something like a private mount of 
/var/tmp which is present under the container work directory as something like 
.private/var/tmp


> On Aug. 19, 2014, 5:21 p.m., Jie Yu wrote:
> > src/slave/flags.hpp, lines 245-252
> > 
> >
> > This needs some discussion. I am in favor of using ContainerInfo to 
> > specify private dirs so that we can be more flexible in the future when we 
> > have full file system isolation.
> > 
> > For the first step (for /tmp), you can introduce a default 
> > ContainerInfo (through slave flags) for mesos containerizer (if not 
> > specified) which contains information about default private mounts. Sine 
> > it's a protobuf, it's very straightforward to pass it in as a json object.
> > 
> > Also, have you considered using Volumn to specify private dirs? We need 
> > some discussion here. For example, one way is to introduce a new Mode.

Changed to using a MESOS ContainerInfo and using Volumes to specify the mounts.


- Ian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24178/#review51046
---


On July 31, 2014, 11:08 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24178/
> ---
> 
> (Updated July 31, 2014, 11:08 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-1586
> https://issues.apache.org/jira/browse/MESOS-1586
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Does not report usage or enforce quota but can create 'private' directories 
> for each container which mask parts of the root filesystem.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b660d912c92594ef679b71caf508134f20511dae 
>   src/slave/containerizer/isolators/filesystem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 
> f3cc813c64e9298f43c9691de9d51ba29c3a8e91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 2c394e2c8702166266f5d20ff005abb218da8a6c 
>   src/slave/flags.hpp 146c4011eb6a64dbb03555cece1c5303338ae240 
>   src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 
> 
> Diff: https://reviews.apache.org/r/24178/diff/
> 
> 
> Testing
> ---
> 
> # added two tests for the FilesystemIsolator
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 24178: Basic filesystem isolator for Linux.

2014-08-19 Thread Timothy Chen 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24178/#review51049
---



src/slave/containerizer/isolators/filesystem.hpp


__FILESYSTEM_ISOLATOR_HPP__



src/slave/containerizer/isolators/filesystem.cpp


Should also append the error



src/slave/containerizer/isolators/filesystem.cpp


I think the message should probably reflect that you're also not allowed to 
mount a parent of the work directory.



src/slave/containerizer/isolators/filesystem.cpp


I believe if it passes realpath it should exist in the file system.


- Timothy Chen


On Aug. 1, 2014, 6:08 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24178/
> ---
> 
> (Updated Aug. 1, 2014, 6:08 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-1586
> https://issues.apache.org/jira/browse/MESOS-1586
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Does not report usage or enforce quota but can create 'private' directories 
> for each container which mask parts of the root filesystem.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b660d912c92594ef679b71caf508134f20511dae 
>   src/slave/containerizer/isolators/filesystem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 
> f3cc813c64e9298f43c9691de9d51ba29c3a8e91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 2c394e2c8702166266f5d20ff005abb218da8a6c 
>   src/slave/flags.hpp 146c4011eb6a64dbb03555cece1c5303338ae240 
>   src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 
> 
> Diff: https://reviews.apache.org/r/24178/diff/
> 
> 
> Testing
> ---
> 
> # added two tests for the FilesystemIsolator
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 24178: Basic filesystem isolator for Linux.

2014-08-19 Thread Jie Yu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24178/#review51046
---


Nice to see this! Added some high level comments.


src/Makefile.am


Why are you adding these two headers? (duplicated?)



src/slave/containerizer/isolators/filesystem.cpp


Check for kernel version? Check for root permission? For example, when bind 
mount is introduced?



src/slave/containerizer/isolators/filesystem.cpp


Here, you can just check the ContainerInfo inside executorInfo. If it does 
not exist, use the default one specified by the slave.



src/slave/containerizer/isolators/filesystem.cpp


Alignment.



src/slave/containerizer/isolators/filesystem.cpp


Him, this reveals a more interesting problem. Say in the future, we want to 
implement a full file system isolator. The order in which we bind mount those 
volumns needs to be carefully calculated. For example, say we have two volumns: 
/ and /lib. Can we actually support that?



src/slave/containerizer/isolators/filesystem.cpp


Can you explain why we need to create those directories? Is is necessary?

My feeling is that it's not necessary. If you want those dir structures, 
you should specify more Volumns, right?

Well, again, if we have multiple volumns, the question is how do we ensure 
the correct mount order.



src/slave/containerizer/isolators/filesystem.cpp


please use set -x to print those mount commands.



src/slave/flags.hpp


This needs some discussion. I am in favor of using ContainerInfo to specify 
private dirs so that we can be more flexible in the future when we have full 
file system isolation.

For the first step (for /tmp), you can introduce a default ContainerInfo 
(through slave flags) for mesos containerizer (if not specified) which contains 
information about default private mounts. Sine it's a protobuf, it's very 
straightforward to pass it in as a json object.

Also, have you considered using Volumn to specify private dirs? We need 
some discussion here. For example, one way is to introduce a new Mode.


- Jie Yu


On Aug. 1, 2014, 6:08 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24178/
> ---
> 
> (Updated Aug. 1, 2014, 6:08 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-1586
> https://issues.apache.org/jira/browse/MESOS-1586
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Does not report usage or enforce quota but can create 'private' directories 
> for each container which mask parts of the root filesystem.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b660d912c92594ef679b71caf508134f20511dae 
>   src/slave/containerizer/isolators/filesystem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 
> f3cc813c64e9298f43c9691de9d51ba29c3a8e91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 2c394e2c8702166266f5d20ff005abb218da8a6c 
>   src/slave/flags.hpp 146c4011eb6a64dbb03555cece1c5303338ae240 
>   src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 
> 
> Diff: https://reviews.apache.org/r/24178/diff/
> 
> 
> Testing
> ---
> 
> # added two tests for the FilesystemIsolator
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 24178: Basic filesystem isolator for Linux.

2014-08-01 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24178/#review49383
---


Patch looks great!

Reviews applied: [24177, 24178]

All tests passed.

- Mesos ReviewBot


On Aug. 1, 2014, 6:08 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24178/
> ---
> 
> (Updated Aug. 1, 2014, 6:08 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-1586
> https://issues.apache.org/jira/browse/MESOS-1586
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Does not report usage or enforce quota but can create 'private' directories 
> for each container which mask parts of the root filesystem.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am b660d912c92594ef679b71caf508134f20511dae 
>   src/slave/containerizer/isolators/filesystem.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/filesystem.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp 
> f3cc813c64e9298f43c9691de9d51ba29c3a8e91 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 2c394e2c8702166266f5d20ff005abb218da8a6c 
>   src/slave/flags.hpp 146c4011eb6a64dbb03555cece1c5303338ae240 
>   src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 
> 
> Diff: https://reviews.apache.org/r/24178/diff/
> 
> 
> Testing
> ---
> 
> # added two tests for the FilesystemIsolator
> make check
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 24178: Basic filesystem isolator for Linux.

2014-07-31 Thread Ian Downes 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24178/
---

Review request for mesos, Ben Mahler and Jie Yu.


Bugs: MESOS-1586
https://issues.apache.org/jira/browse/MESOS-1586


Repository: mesos-git


Description
---

Does not report usage or enforce quota but can create 'private' directories for 
each container which mask parts of the root filesystem.


Diffs
-

  src/Makefile.am b660d912c92594ef679b71caf508134f20511dae 
  src/slave/containerizer/isolators/filesystem.hpp PRE-CREATION 
  src/slave/containerizer/isolators/filesystem.cpp PRE-CREATION 
  src/slave/containerizer/linux_launcher.cpp 
f3cc813c64e9298f43c9691de9d51ba29c3a8e91 
  src/slave/containerizer/mesos/containerizer.cpp 
2c394e2c8702166266f5d20ff005abb218da8a6c 
  src/slave/flags.hpp 146c4011eb6a64dbb03555cece1c5303338ae240 
  src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 

Diff: https://reviews.apache.org/r/24178/diff/


Testing
---

# added two tests for the FilesystemIsolator
make check


Thanks,

Ian Downes