Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
On Aug. 12, 2015, 10:47 p.m., Jiang Yan Xu wrote: This thing is rather complex and it deals with Mesos managed persisetent volumes and other system volumes in different but related ways; rootfs may or may not be used; and we'll provision image volumes later. I hope we iterate on this later and look for ways (more documentation, refactorings) to simplify and clarify the code. Yeah, we'll continue to work that later for 'volumes in image'. The involves some refactor. I'll try to think about a way to make it more readable. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95154 --- On Aug. 12, 2015, 11:21 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 12, 2015, 11:21 p.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
On Aug. 12, 2015, 10:47 p.m., Jiang Yan Xu wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 485 https://reviews.apache.org/r/37330/diff/2/?file=1037046#file1037046line485 s/other/another/ 'other' sounds fine to me here. On Aug. 12, 2015, 10:47 p.m., Jiang Yan Xu wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 579 https://reviews.apache.org/r/37330/diff/2/?file=1037046#file1037046line579 What happens when persistent volumes are used without a new rootfs? Nice catch! That's a bug. I haven't tested the cases where a container does not have a new rootfs. I'll follow up with a test. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95154 --- On Aug. 11, 2015, 1:57 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 11, 2015, 1:57 a.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 12, 2015, 11:21 p.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Changes --- Addressed review comments. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs (updated) - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95154 --- This thing is rather complex and it deals with Mesos managed persisetent volumes and other system volumes in different but related ways; rootfs may or may not be used; and we'll provision image volumes later. I hope we iterate on this later and look for ways (more documentation, refactorings) to simplify and clarify the code. src/slave/containerizer/isolators/filesystem/linux.hpp (line 105) https://reviews.apache.org/r/37330/#comment14 Mention it's an absoluate path and always under the rootfs? src/slave/containerizer/isolators/filesystem/linux.cpp (lines 292 - 294) https://reviews.apache.org/r/37330/#comment150043 Add a TODO here to change this after we start to provsision images in volumes? src/slave/containerizer/isolators/filesystem/linux.cpp (line 485) https://reviews.apache.org/r/37330/#comment149989 s/other/another/ src/slave/containerizer/isolators/filesystem/linux.cpp (line 526) https://reviews.apache.org/r/37330/#comment150038 ``` Check that the source of the mount matches the entry with the same target in the mount table if one can be found. ``` src/slave/containerizer/isolators/filesystem/linux.cpp (line 553) https://reviews.apache.org/r/37330/#comment149990 Aha, OK I see that the assignment happens here. src/slave/containerizer/isolators/filesystem/linux.cpp (line 579) https://reviews.apache.org/r/37330/#comment150042 What happens when persistent volumes are used without a new rootfs? src/slave/containerizer/isolators/filesystem/linux.cpp (line 589) https://reviews.apache.org/r/37330/#comment150016 s/all/All/ - Jiang Yan Xu On Aug. 10, 2015, 6:57 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 10, 2015, 6:57 p.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95208 --- Ship it! Ship It! - Jiang Yan Xu On Aug. 12, 2015, 4:21 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 12, 2015, 4:21 p.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
On Aug. 12, 2015, 10:47 p.m., Jiang Yan Xu wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 579 https://reviews.apache.org/r/37330/diff/2/?file=1037046#file1037046line579 What happens when persistent volumes are used without a new rootfs? Jie Yu wrote: Nice catch! That's a bug. I haven't tested the cases where a container does not have a new rootfs. I'll follow up with a test. Added a test here https://reviews.apache.org/r/37422/ - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95154 --- On Aug. 12, 2015, 11:21 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 12, 2015, 11:21 p.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
On Aug. 12, 2015, 1:43 a.m., Timothy Chen wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 399 https://reviews.apache.org/r/37330/diff/2/?file=1037046#file1037046line399 Can we also log the container id and it's the linux filesystem isolator? It's much easier to debug instead of just seeing a message says Unknown container. The caller will print the container id in the error message. On Aug. 12, 2015, 1:43 a.m., Timothy Chen wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 412 https://reviews.apache.org/r/37330/diff/2/?file=1037046#file1037046line412 I don't think we hold reference to tempoaries? It returns a reference (not a temporary). - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95046 --- On Aug. 11, 2015, 1:57 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 11, 2015, 1:57 a.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/#review95046 --- src/slave/containerizer/isolators/filesystem/linux.cpp (line 399) https://reviews.apache.org/r/37330/#comment149834 Can we also log the container id and it's the linux filesystem isolator? It's much easier to debug instead of just seeing a message says Unknown container. src/slave/containerizer/isolators/filesystem/linux.cpp (line 412) https://reviews.apache.org/r/37330/#comment149835 I don't think we hold reference to tempoaries? - Timothy Chen On Aug. 11, 2015, 1:57 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 11, 2015, 1:57 a.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
Re: Review Request 37330: Added persistent volume support for linux filesystem isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37330/ --- (Updated Aug. 11, 2015, 1:57 a.m.) Review request for mesos, Lily Chen, Ian Downes, Jojy Varghese, Timothy Chen, Vinod Kone, and Jiang Yan Xu. Changes --- Create the mount points if needed. Bugs: MESOS-2794 https://issues.apache.org/jira/browse/MESOS-2794 Repository: mesos Description --- Added persistent volume support for linux filesystem isolator. Diffs (updated) - src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37330/diff/ Testing --- sudo make check Will follow up with more tests specificially for persistent volumes. Thanks, Jie Yu
