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


This looks great! Thanks Mei! I think the main thing left here is the test. You 
should be able to write some simple tests that test the overlay functionality 
(see the bind mount tests). Looking forward to the tests!


src/slave/containerizer/provisioners/backends/overlay.cpp (line 58)
<https://reviews.apache.org/r/37853/#comment152403>

    You also want to check if overlay fs is supported or not. Not every linux 
kernel supports overlay fs.



src/slave/containerizer/provisioners/backends/overlay.cpp (line 98)
<https://reviews.apache.org/r/37853/#comment152425>

    No need to use process:: prefix as you already have `using namespace 
process` above.
    
    Please all such occurances in this file.



src/slave/containerizer/provisioners/backends/overlay.cpp (lines 101 - 103)
<https://reviews.apache.org/r/37853/#comment152406>

    Hum, I'd like to understand why overlay backend cannot support 1 layer. Any 
reason?



src/slave/containerizer/provisioners/backends/overlay.cpp (lines 110 - 114)
<https://reviews.apache.org/r/37853/#comment152428>

    Could you please add a comment about the ordering the layers will be 
stacked.
    
    I think we should add a document at the backend interface as well stating 
that layers are stacking in such a way that the front layer in the vector will 
be the bottom most layer (i.e., applied first).
    
    The lowerdir here should actually reverse the order of the layers vector.
    
    ```
    The specified lower directories will be stacked beginning from the 
rightmost one and going left.  In the above example lower1 will be the top, 
lower2 the middle and lower3 the bottom layer.
    ```



src/slave/containerizer/provisioners/backends/overlay.cpp (line 111)
<https://reviews.apache.org/r/37853/#comment152424>

    I think you should be able to use strings::join here:
    
    ```
    strings::join(":", layers);
    ```


- Jie Yu


On Aug. 27, 2015, 9:11 p.m., Mei Wan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37853/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2015, 9:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Jie Yu.
> 
> 
> Bugs: MESOS-2971
>     https://issues.apache.org/jira/browse/MESOS-2971
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented the overlay filesystem backend by layering the images as a 
> read-only filesystem.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff 
>   src/slave/containerizer/provisioners/backend.cpp 2f7c335 
>   src/slave/containerizer/provisioners/backends/overlay.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/backends/overlay.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37853/diff/
> 
> 
> Testing
> -------
> 
> I haven't done any official testing. When I was working off Ian's branch, I 
> tested it manually and the provisioning works.
> 
> 
> Thanks,
> 
> Mei Wan
> 
>

Reply via email to