This is an automatically generated e-mail. To reply, visit:

src/slave/containerizer/provisioners/backend.cpp (lines 34 - 36)

    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)

    Add a blank line above.

src/slave/containerizer/provisioners/backends/bind.cpp (line 26)

    Add a blank line above.

src/slave/containerizer/provisioners/backends/bind.cpp (line 41)

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

src/slave/containerizer/provisioners/backends/bind.cpp (line 45)

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

src/slave/containerizer/provisioners/backends/bind.cpp (line 51)

    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)

    No layer specified.

src/slave/containerizer/provisioners/backends/bind.cpp (line 107)

    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)

    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)

    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)

    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)

    Please move this right after using namespace process;

src/tests/containerizer/provisioner_backend_tests.cpp (line 45)

    Could you please call it ProvisionerBindBackendTest.

src/tests/containerizer/provisioner_backend_tests.cpp (line 50)

    Please wrap the entire class under ifdef linux guard

src/tests/containerizer/provisioner_backend_tests.cpp (line 60)

    This is not needed.

src/tests/containerizer/provisioner_backend_tests.cpp (lines 74 - 76)

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

- 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