You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/07/12 06:46:24 UTC
Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/
-----------------------------------------------------------
Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
Repository: mesos
Description
-------
Moved filesystem/linux from review https://reviews.apache.org/r/34135/
Diffs
-----
src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
Diff: https://reviews.apache.org/r/36429/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review91404
-----------------------------------------------------------
Bad patch!
Reviews applied: [34136, 34908, 34137]
Failed command: ./support/apply-review.sh -n -r 34137
Error:
2015-07-12 05:56:06 URL:https://reviews.apache.org/r/34137/diff/raw/ [26600/26600] -> "34137.patch" [1]
error: patch failed: src/Makefile.am:522
error: src/Makefile.am: patch does not apply
error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:542
error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review93143
-----------------------------------------------------------
Please do a rebase first since quite a lot of the stuff have been changed.
Also, there's no test yet. Do you have tests in a subsequent patch? I think we might need a TestProvisioner that's based on copying the host file systems so that we can test the file system isolators. We can unify this with the `BasicLinuxChroot` in launch_tests.cpp. What do you think?
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 130 - 148)
<https://reviews.apache.org/r/36429/#comment147478>
Can you elaborate in the comment about why this is needed? I don't follow this part.
Also, I am wondering what if 'directory' itself contains symbolic links? For example, 'directory=/var/lib/mesos/...' and '/var' is a symbolic link to '/xxx/var'.
Is that a real issue observerd? Or we can drop a TODO instead for now?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 165)
<https://reviews.apache.org/r/36429/#comment147479>
s/!rootfs.isSome()/rootfs.isNone()
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 176 - 195)
<https://reviews.apache.org/r/36429/#comment147523>
Again, this part is not quite obvious to me. Can you explain why this is needed?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 185)
<https://reviews.apache.org/r/36429/#comment147524>
Failed to create 'container_path'?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 188)
<https://reviews.apache.org/r/36429/#comment147525>
What if 'rootfs' itself has symbolic links? Do we have code to guarantee that it won't be the case?
src/slave/containerizer/isolators/filesystem/linux.cpp (line 214)
<https://reviews.apache.org/r/36429/#comment147427>
Can you print the container type if this check fails:
```
CHECK(...)
<< "Unexpected container type " << ...
```
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 320 - 323)
<https://reviews.apache.org/r/36429/#comment147454>
You may want to check if some volumes already exist before mounting them because of the same reason you mentioned here.
src/slave/containerizer/isolators/filesystem/linux.cpp (lines 373 - 374)
<https://reviews.apache.org/r/36429/#comment147405>
The logic here needs to be adjusted. Please refer to
https://issues.apache.org/jira/browse/MESOS-3124
https://reviews.apache.org/r/36684
src/slave/containerizer/isolators/filesystem/linux.cpp (line 428)
<https://reviews.apache.org/r/36429/#comment147449>
I still don't get this part. Correct me if I'm wrong.
Say the sandbox in the host file system (i.e., `info->directory`) is `DIRECTORY`.
Persistent volumes are mounted at `$DIRECTORY/volume1`, `$DIRECTORY/volume2`, right?
Those volumes's target does not meet `strings::startsWith(entry->target, info->rootfs.get()`, right?
Also, if `info->rootfs.isNone()`, do you also need to cleanup persistent volume mounts?
- Jie Yu
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review91424
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
> On Aug. 11, 2015, 3:57 p.m., Marco Massenzio wrote:
> > Wouldn't it be good if we could have some comments as to how this class is supposed to be used, what does it encapsulate, etc.?
> >
> > At the very least a URL to a design doc or something?
> >
> > Also, all the methods are completely undocumented: this means that, whenever anyone will want in future to use it, they will have to go hunt for the cpp file and reverse engineer the code (with a large amount of guessing as to the intent for the ambiguous parts) trying to figure out what each of them does.
> >
> > (not to mention that it'll be anyone's guess what the methods' arguments are supposed to be).
> >
> > I'm sure that for those 2-3 people who have spent the last year or so thinking about FSIsolators this is all obvious; but not so for those who haven't, and even less so for those poor souls who may want to join the project in future...
FYI, this patch is being moved to as Ian is on vacation.
https://reviews.apache.org/r/37236/
https://reviews.apache.org/r/37330/
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review94920
-----------------------------------------------------------
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review94920
-----------------------------------------------------------
Wouldn't it be good if we could have some comments as to how this class is supposed to be used, what does it encapsulate, etc.?
At the very least a URL to a design doc or something?
Also, all the methods are completely undocumented: this means that, whenever anyone will want in future to use it, they will have to go hunt for the cpp file and reverse engineer the code (with a large amount of guessing as to the intent for the ambiguous parts) trying to figure out what each of them does.
(not to mention that it'll be anyone's guess what the methods' arguments are supposed to be).
I'm sure that for those 2-3 people who have spent the last year or so thinking about FSIsolators this is all obvious; but not so for those who haven't, and even less so for those poor souls who may want to join the project in future...
- Marco Massenzio
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by Jie Yu <yu...@gmail.com>.
> On July 29, 2015, 4:04 p.m., James DeFelice wrote:
> > src/slave/containerizer/isolators/filesystem/linux.cpp, line 238
> > <https://reviews.apache.org/r/36429/diff/1/?file=1009137#file1009137line238>
> >
> > why MS_SHARED (bidirectional) vs MS_SLAVE (one-way)?
> >
> > MS_SLAVE would probably give better isolation to the host mount-ns.
> >
> > MS_SHARED would probably be better for a use case that I have in mind (doc'd in MESOS-349), especially since cleanup() here does GC on mount points that are children of the sandbox.
SHARED mounts if mainly for propagating persistent volumes (imaging the executor has started and a new task with persistent volumes coming in).
The sandbox mount will be shared in host mnt namespace and slave in container mnt namespace.
FYI, this patch is being moved to as Ian is on vacation.
https://reviews.apache.org/r/37236/
https://reviews.apache.org/r/37330/
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review93457
-----------------------------------------------------------
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 36429: Add filesystem/linux isolator for persistent
volumes.
Posted by James DeFelice <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36429/#review93457
-----------------------------------------------------------
src/slave/containerizer/isolators/filesystem/linux.cpp (line 238)
<https://reviews.apache.org/r/36429/#comment147836>
why MS_SHARED (bidirectional) vs MS_SLAVE (one-way)?
MS_SLAVE would probably give better isolation to the host mount-ns.
MS_SHARED would probably be better for a use case that I have in mind (doc'd in MESOS-349), especially since cleanup() here does GC on mount points that are children of the sandbox.
- James DeFelice
On July 12, 2015, 4:46 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36429/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:46 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved filesystem/linux from review https://reviews.apache.org/r/34135/
>
>
> Diffs
> -----
>
> src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c
> src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION
> src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION
> src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5
>
> Diff: https://reviews.apache.org/r/36429/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>