> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 41
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050965#file1050965line41>
> >
> >     No need for the process ID generation unless it's proven needed.

It's much harder to do this when debugging becauase you don't know which 
process to add the id to: https://issues.apache.org/jira/browse/MESOS-1457

But I understand that we are not doing this consistently. OK for dropping it. 
/sigh


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 45
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050965#file1050965line45>
> >
> >     I would sugguest you save the slave flags at least.
> >     
> >     ```
> >     public:
> >       BindBackendProcess(const Flags& _flags)
> >         : flags(_flags) {}
> >         
> >     private:
> >       const Flags flags;
> >     ```

Chatted offline. Not saving it for now because it's not needed for BindBackend. 
we can add it back when necessary of course.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 107
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050965#file1050965line107>
> >
> >     Can you try if MS_BIND | MS_RDONLY works here (so that you can save the 
> > remount below).
> >     
> >     Also, I think you might want to do a recursive bind mount in case the 
> > layer itself has some mounts underneath it.
> >     
> >     YOu can drop a TODO here.

MS_BIND | MS_RDONLY in one call doesn't work. Remount is necessary.

MS_REC: Not necessary for APPC but added the TODO. It's only necessary if 
mounts are used during image preparation for a single layer. Bind mount already 
limits the number of layers to be 1. Feels unlikely to me but a note here is 
definitely helpful.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 121
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050965#file1050965line121>
> >
> >     The read-only bind mount introduces a problem that the filesystem 
> > isolator cannot create the mount point for the sandbox anymore if it does 
> > not exist.
> >     
> >     Please add a NOTE states that all mount points needed must already be 
> > present in the rootfs.

Thanks for spotting this! This is quite a limitation for BindBackend's 
usability but luckily the user can work around this.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 143
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050965#file1050965line143>
> >
> >     Please add a TODO here saying that if recursive bind mount is used 
> > above, here you need to check `strings::contains(entry.target, rootfs)`.

`strings::startsWith(entry.target, rootfs)`. Added a note.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/backends/bind.cpp, line 144
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050965#file1050965line144>
> >
> >     Any reason use a detached UMOUNT here? The os::rmdir will fail if 
> > unmount hasn't finished yet.

Removed MNT_DETACH flag.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_backend_tests.cpp, line 45
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050968#file1050968line45>
> >
> >     Could you please call it ProvisionerBindBackendTest.

Named it `BindBackendTest`, I imagine `OverlayfsBackend` can also use this test 
but oh well, let's refactor later.


> On Aug. 25, 2015, 12:24 p.m., Jie Yu wrote:
> > src/tests/containerizer/provisioner_backend_tests.cpp, lines 74-76
> > <https://reviews.apache.org/r/37747/diff/1/?file=1050968#file1050968line74>
> >
> >     This is expensive. You don't need a working rootfs as far as I can 
> > tell, right?

Created a dummy fs.


- Jiang Yan


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


On Aug. 25, 2015, 3:59 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37747/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2015, 3:59 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Jie Yu, Timothy Chen, and Vinod Kone.
> 
> 
> Bugs: MESOS-3190
>     https://issues.apache.org/jira/browse/MESOS-3190
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced bind-mount based provisioner Backend.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
>   src/slave/containerizer/provisioners/backend.hpp 
> 46120e8420cc491a0decbd88301f89d6dfcff120 
>   src/slave/containerizer/provisioners/backend.cpp 
> 6190ce3eeff6ea22142c9eaa5a771ae1b767740c 
>   src/slave/containerizer/provisioners/backends/bind.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/bind.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner_backend_tests.cpp PRE-CREATION 
>   src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 
> 
> Diff: https://reviews.apache.org/r/37747/diff/
> 
> 
> Testing
> -------
> 
> sudo make check. Added one test.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Reply via email to