Re: 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/#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.
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.
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.
--- 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
Re: 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/#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
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
