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



src/slave/containerizer/provisioners/backend.cpp (lines 34 - 36)
<https://reviews.apache.org/r/37747/#comment151726>

    So bind backend only compiles on linux but backend.cpp compiles on osx as 
well. You need to use ifdef linux guard for "BindBackend::create".



src/slave/containerizer/provisioners/backends/bind.hpp (line 23)
<https://reviews.apache.org/r/37747/#comment151719>

    Add a blank line above.



src/slave/containerizer/provisioners/backends/bind.cpp (line 26)
<https://reviews.apache.org/r/37747/#comment151727>

    Add a blank line above.



src/slave/containerizer/provisioners/backends/bind.cpp (line 41)
<https://reviews.apache.org/r/37747/#comment151728>

    No need for the process ID generation unless it's proven needed.



src/slave/containerizer/provisioners/backends/bind.cpp (line 45)
<https://reviews.apache.org/r/37747/#comment151730>

    I would sugguest you save the slave flags at least.
    
    ```
    public:
      BindBackendProcess(const Flags& _flags)
        : flags(_flags) {}
        
    private:
      const Flags flags;
    ```



src/slave/containerizer/provisioners/backends/bind.cpp (line 51)
<https://reviews.apache.org/r/37747/#comment151718>

    Do you want to make sure the current user is root since 'mount' requires 
root permission.



src/slave/containerizer/provisioners/backends/bind.cpp (line 95)
<https://reviews.apache.org/r/37747/#comment151731>

    No layer specified.



src/slave/containerizer/provisioners/backends/bind.cpp (line 107)
<https://reviews.apache.org/r/37747/#comment151732>

    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.



src/slave/containerizer/provisioners/backends/bind.cpp (line 121)
<https://reviews.apache.org/r/37747/#comment151733>

    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.



src/slave/containerizer/provisioners/backends/bind.cpp (line 143)
<https://reviews.apache.org/r/37747/#comment151735>

    Please add a TODO here saying that if recursive bind mount is used above, 
here you need to check `strings::contains(entry.target, rootfs)`.



src/slave/containerizer/provisioners/backends/bind.cpp (line 144)
<https://reviews.apache.org/r/37747/#comment151734>

    Any reason use a detached UMOUNT here? The os::rmdir will fail if unmount 
hasn't finished yet.



src/tests/containerizer/provisioner_backend_tests.cpp (line 39)
<https://reviews.apache.org/r/37747/#comment151736>

    Please move this right after using namespace process;



src/tests/containerizer/provisioner_backend_tests.cpp (line 45)
<https://reviews.apache.org/r/37747/#comment151738>

    Could you please call it ProvisionerBindBackendTest.



src/tests/containerizer/provisioner_backend_tests.cpp (line 50)
<https://reviews.apache.org/r/37747/#comment151739>

    Please wrap the entire class under ifdef linux guard



src/tests/containerizer/provisioner_backend_tests.cpp (line 60)
<https://reviews.apache.org/r/37747/#comment151737>

    This is not needed.



src/tests/containerizer/provisioner_backend_tests.cpp (lines 74 - 76)
<https://reviews.apache.org/r/37747/#comment151740>

    This is expensive. You don't need a working rootfs as far as I can tell, 
right?


- Jie Yu


On Aug. 25, 2015, 5:22 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, 5:22 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.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