mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <...@jxu.me>
Subject Re: Review Request 37747: Introduced bind-mount based provisioner Backend.
Date Tue, 25 Aug 2015 23:00:05 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message