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


Fix it, then Ship it!




Thanks! This is great!


src/slave/containerizer/mesos/provisioner/backend.cpp (line 50)
<https://reviews.apache.org/r/43932/#comment182221>

    Kill this line.



src/slave/containerizer/mesos/provisioner/backend.cpp (line 52)
<https://reviews.apache.org/r/43932/#comment182222>

    Add a new line above.



src/slave/containerizer/mesos/provisioner/backends/overlay.hpp (lines 30 - 31)
<https://reviews.apache.org/r/43932/#comment182223>

    can you wrap comments in 70 char width. It's less jagged IMO.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 28)
<https://reviews.apache.org/r/43932/#comment182224>

    Let's use 'using process::XXX' explicitly. We try to avoid using 'using 
namespace'.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 124 - 129)
<https://reviews.apache.org/r/43932/#comment182293>

    To be safe, can you do the same thing to mark the mount as slave+shared 
(like we did in the bind backend).
    
    So the goal of doing that is: we want to make sure when slave fork a 
subprocess with a new mount namespace, it does not create an extra reference to 
the mount so that rmdir might fail later.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 132 - 133)
<https://reviews.apache.org/r/43932/#comment182238>

    Can you align the error message:
    ```
    return Failure(
        "Failed to remount rootfs '" + rootfs +
        "' read-only: " + mount.error());
    ```



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 143)
<https://reviews.apache.org/r/43932/#comment182239>

    Kill this line.



src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (line 148)
<https://reviews.apache.org/r/43932/#comment182240>

    You can use mountTable->entries



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

    kill this line.



src/tests/containerizer/provisioner_backend_tests.cpp (line 51)
<https://reviews.apache.org/r/43932/#comment182306>

    We should add an TearDown method to unmount anything under sandbox. You can 
take a look at fs::unmountAll.



src/tests/containerizer/provisioner_backend_tests.cpp (lines 78 - 79)
<https://reviews.apache.org/r/43932/#comment182302>

    You can do EXPECT_SOME_EQ("1", read);



src/tests/containerizer/provisioner_backend_tests.cpp (lines 83 - 84)
<https://reviews.apache.org/r/43932/#comment182303>

    Ditto.



src/tests/containerizer/provisioner_backend_tests.cpp (lines 87 - 88)
<https://reviews.apache.org/r/43932/#comment182304>

    Ditto.



src/tests/environment.cpp (line 489)
<https://reviews.apache.org/r/43932/#comment182300>

    `s/_OVERLAYFS_/OVERLAYFS_/`


- Jie Yu


On Feb. 24, 2016, 4:38 p.m., Shuai Lin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43932/
> -----------------------------------------------------------
> 
> (Updated Feb. 24, 2016, 4:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2971
>     https://issues.apache.org/jira/browse/MESOS-2971
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added overlayfs provisioning backend.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt b13fb23219ebb23bcfd6db062e1c814ca2114aa4 
>   src/Makefile.am 2a26261b513bb7c03437ed8e850c3b36b93d82f5 
>   src/slave/containerizer/mesos/provisioner/backend.cpp 
> 01d06ebc67e259272ee57ea5c75bf7077ede65c4 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp PRE-CREATION 
>   src/slave/flags.cpp 1c6a87b670efde2deab4d6e3f24fd6eb3704a47d 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 25b28ef8fa5aae81e8dd0c9e33df4160dd912ce8 
>   src/tests/environment.cpp 6cd295f76496770774d090e0485ff87be378f74c 
> 
> Diff: https://reviews.apache.org/r/43932/diff/
> 
> 
> Testing
> -------
> 
> sudo modprobe overlayfs
> sudo make check -j4 
> GTEST_FILTER='OverlayBackendTest.ROOT_OVERLAYFS_OverlayFSBackend'
> 
> - OS: ubuntu 14.04 64bit vm
> - Kernel: 4.2.0-27-generic
> 
> 
> Thanks,
> 
> Shuai Lin
> 
>

Reply via email to