You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/07/07 21:43:14 UTC

Re: Review Request 34140: AppC image store

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

(Updated July 7, 2015, 12:43 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
-------

Updated dependencies.


Repository: mesos


Description
-------

Images are fetched into the store (after discovery). Stored images are
currently kept indefinitely.


Diffs
-----

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 

Diff: https://reviews.apache.org/r/34140/diff/


Testing
-------


Thanks,

Ian Downes

Re: Review Request 34140: AppC image store

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34140/#review90954
-----------------------------------------------------------



src/slave/flags.cpp (line 74)
<https://reviews.apache.org/r/34140/#comment144111>

    Since we're also adding docker, I think we should make this /tmp/mesos/store/[appc|docker]


- Timothy Chen


On July 7, 2015, 7:43 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34140/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 7:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Images are fetched into the store (after discovery). Stored images are
> currently kept indefinitely.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34140/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34140: AppC image store

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34140/#review90873
-----------------------------------------------------------



src/slave/containerizer/provisioners/appc/store.hpp (lines 22 - 33)
<https://reviews.apache.org/r/34140/#comment144684>

    Reorder includes.
    
    <std lib>
    
    <mesos/...>
    
    <process/...>
    
    <stout/...>



src/slave/containerizer/provisioners/appc/store.hpp (lines 68 - 69)
<https://reviews.apache.org/r/34140/#comment144036>

    I am sure you'll remove these but just putting on reminder here.



src/slave/containerizer/provisioners/appc/store.hpp (lines 70 - 71)
<https://reviews.apache.org/r/34140/#comment144038>

    Two empty lines used here. Kill one.



src/slave/containerizer/provisioners/appc/store.hpp (line 95)
<https://reviews.apache.org/r/34140/#comment144114>

    Move to the source file?



src/slave/containerizer/provisioners/appc/store.cpp (line 103)
<https://reviews.apache.org/r/34140/#comment144491>

    Suggestion:
    
    Somewhere in Store class if we draw a directory layout everything would be a lot clearer.



src/slave/containerizer/provisioners/appc/store.cpp (line 148)
<https://reviews.apache.org/r/34140/#comment144505>

    The code above captures the stage via `[=]()` so it's inconsistent here. Should be fine to keep using `[=]()`.



src/slave/containerizer/provisioners/appc/store.cpp (lines 154 - 170)
<https://reviews.apache.org/r/34140/#comment144123>

    TODO: This looks a bit hacky but that's what it takes to use the fetcher. That's some refactoring for the future.



src/slave/containerizer/provisioners/appc/store.cpp (lines 188 - 189)
<https://reviews.apache.org/r/34140/#comment144124>

    4 space indentation.



src/slave/containerizer/provisioners/appc/store.cpp (line 189)
<https://reviews.apache.org/r/34140/#comment144515>

    Make "image.ci" a const variable.



src/slave/containerizer/provisioners/appc/store.cpp (line 243)
<https://reviews.apache.org/r/34140/#comment144683>

    4 space indentation.



src/slave/containerizer/provisioners/appc/store.cpp (line 247)
<https://reviews.apache.org/r/34140/#comment144665>

    Indentation off by one space.



src/slave/containerizer/provisioners/appc/store.cpp (line 268)
<https://reviews.apache.org/r/34140/#comment144035>

    Just would like to mention the possibility of using `pigz` which speeds things up quite a bit but of course we need to check its availability.



src/slave/containerizer/provisioners/appc/store.cpp (line 274)
<https://reviews.apache.org/r/34140/#comment144136>

    Why different here? The other two formats have output being 'image'.
    
    FWIW I think image.aci.out looks clearer that it's a temp.



src/slave/containerizer/provisioners/appc/store.cpp (line 276)
<https://reviews.apache.org/r/34140/#comment144140>

    The availability of this binary is rearer than the previous two. And "xz not found" is treated the same as "file not compressed". 
    
    Maybe at least some logging to distinguish the two  cases. I understand that eventually the failure will occur in the untar step but it would be nice to see who the culprit is. (Maybe don't redirect stderr to /dev/null)?



src/slave/containerizer/provisioners/appc/store.cpp (line 349)
<https://reviews.apache.org/r/34140/#comment144192>

    Shift indentation left by one space.



src/slave/containerizer/provisioners/appc/store.cpp (line 392)
<https://reviews.apache.org/r/34140/#comment144196>

    "staging/XXXXXX to storage/hash"?



src/slave/containerizer/provisioners/appc/store.cpp (line 397)
<https://reviews.apache.org/r/34140/#comment144197>

    



src/slave/containerizer/provisioners/appc/store.cpp (line 408)
<https://reviews.apache.org/r/34140/#comment144198>

    What does this mean? TODO: rm store?
    
    We do need to remove the corrupted store to avoid accumulating too much space and at least avoid confusing.
    
    So how about: verify the image before renaming and do not move it into storage if it fails. The failed image is not immediately removed so the operator can log into the box to inspect it in the staging directoy but we know:
    1) Images in the staging directory are going to be cleaned up by slave restart.
    2) Images in the storage directory are all valid.



src/slave/containerizer/provisioners/appc/store.cpp (line 412)
<https://reviews.apache.org/r/34140/#comment144678>

    AppcImageManifest belongs to a later review /r/34142/.
    
    Maybe the manifest protobuf and its validation can be pulled out into a separate review that bears no depednency on store, discovery, etc and can be placed lower in the review depedency chain/tree?



src/slave/containerizer/provisioners/appc/store.cpp (line 417)
<https://reviews.apache.org/r/34140/#comment144199>

    Why have "sha512-" as id prefix but not on the path? It would be nice to have them be consistent.
    
    If id with sha512- prefix is clearer about the semantics of the id, perhaps using the full id (i.e. with the prefix) in the path also helps readability of the path.
    
    Can we can have a common helper method to convert hash to id, instead of currently there are two places that prepend the "sha512-" prefix.



src/slave/containerizer/provisioners/appc/store.cpp (line 466)
<https://reviews.apache.org/r/34140/#comment144485>

    No need for const.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34140/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Images are fetched into the store (after discovery). Stored images are
> currently kept indefinitely.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34140/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34140: AppC image store

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34140/#review91989
-----------------------------------------------------------


Not necessary to be done in this review but we should allow `putting` images that are already in the staging folder, this will support mechanisms that download stuff out-of-band.
Also we should rely on a `paths.hpp` when doing all the path manipulation.


src/slave/containerizer/provisioners/appc/store.hpp (line 50)
<https://reviews.apache.org/r/34140/#comment145928>

    It's worth noting that the id is computed from the sha512 hash here as this is actually the only place where the mechanism by which that this id is derived matters. Everywhere else it should be understood as an opaque string.



src/slave/containerizer/provisioners/appc/store.hpp (lines 80 - 83)
<https://reviews.apache.org/r/34140/#comment145766>

    Who uses this?



src/slave/containerizer/provisioners/appc/store.cpp (line 148)
<https://reviews.apache.org/r/34140/#comment146052>

    Here if the `stage` directory is moved into the store then `rmdir()` returns Error. Of course we don't check the result but maybe checking its existence before rmdir and log other rmdir Errors is better?



src/slave/containerizer/provisioners/appc/store.cpp (line 397)
<https://reviews.apache.org/r/34140/#comment145936>

    So at this point we have already downloaded the image, we might as well just go ahead and overwrite the existing folder. If not, we probably don't want to spend all the resources to download it in the first place (i.e. directly return in `put()`).
    
    But I think it would make sense to overwrite if we are going to open up some HTTP API to allow operators to resue some faulty images.



src/slave/containerizer/provisioners/appc/store.cpp (lines 406 - 415)
<https://reviews.apache.org/r/34140/#comment145933>

    Manifest validation is only one type of validation.
    We should also do other validation checks. e.g. whether rootfs exists; whether there are additional files outside of rootfs, etc.


- Jiang Yan Xu


On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34140/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 12:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Images are fetched into the store (after discovery). Stored images are
> currently kept indefinitely.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34140/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34140: AppC image store

Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34140/#review113545
-----------------------------------------------------------


Can we close this now? I don't think this is relevant anymore.

- Timothy Chen


On July 7, 2015, 7:43 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34140/
> -----------------------------------------------------------
> 
> (Updated July 7, 2015, 7:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Images are fetched into the store (after discovery). Stored images are
> currently kept indefinitely.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34140/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>