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


Partial review, mostly high level.


src/messages/docker_provisioner.hpp (line 22)
<https://reviews.apache.org/r/38137/#comment154515>

    We typically have a comment above:
    
    // ONLY USEFUL AFTER RUNNING PROTOC.



src/slave/containerizer/provisioners/docker.hpp (line 56)
<https://reviews.apache.org/r/38137/#comment154518>

    s/Image Name/image name/
    
    s/composes/consists/ or s/composes/is composed/?



src/slave/containerizer/provisioners/docker.hpp (line 59)
<https://reviews.apache.org/r/38137/#comment154808>

    re Jie's comment, Appc uses a spec.hpp|cpp to break the circular dependency 
and consolidates definitions and validations of concepts specific to the spec. 
For docker it's something similar: 
https://github.com/docker/docker/blob/master/image/spec/v1.md
    
    Just an example.



src/slave/containerizer/provisioners/docker.hpp (line 66)
<https://reviews.apache.org/r/38137/#comment154520>

    Should registry be part of the name?
    
    I know it has significance in discovery but it doesn't appear to be part of 
the name.
    
    I would suggest we model our data structure close to the spec so when 
there's confusion it's easy to refer to something that's definitive.
    
    I have another comment below about the "registry" in "name".



src/slave/containerizer/provisioners/docker.hpp (line 71)
<https://reviews.apache.org/r/38137/#comment154811>

    I see that this is for `ImageName::create()` but if we get rid of 
`registry` and use the other constructor while parsing the value we don't need 
this and we can make the fileds const.



src/slave/containerizer/provisioners/docker.hpp (line 122)
<https://reviews.apache.org/r/38137/#comment154827>

    Is the order of the list significant? We should document it.



src/slave/containerizer/provisioners/docker.hpp (line 132)
<https://reviews.apache.org/r/38137/#comment154814>

    s/mesos::internal::slave:://
    
    It's a parent namespace.



src/slave/containerizer/provisioners/docker.hpp (line 135)
<https://reviews.apache.org/r/38137/#comment154815>

    Ditto about `mesos::internal::slave::`.



src/slave/containerizer/provisioners/docker.cpp (line 100)
<https://reviews.apache.org/r/38137/#comment154820>

    re my comment above. Something close to a spec.hpp|cpp to put it would be 
nice.



src/slave/containerizer/provisioners/docker.cpp (lines 104 - 107)
<https://reviews.apache.org/r/38137/#comment154826>

    I guess you are modeling this after the `docker pull` syntax here: 
`[REGISTRY_HOST[:REGISTRY_PORT]/]NAME[:TAG]`
    
    I found that some docker help pages have conflicting use of repository vs. 
name and we should probably refer to the spec.
    
    I think the following questions need to be answered:
    
    1) If we use image name as the indentifier, it defines "what the image is". 
The registry specifies "where the image comes from". If an identical image is 
hosted on two registries, should they have the same name?
    
    2) ID. `docker pull` supports @sha256 syntax and `repositories` file (which 
our docker metadata file is modeled after) maps repo:tag to an image ID. 
Shouldn't the ID be part of the name? If ubuntu:latest changes when use 
releases come out, I don't think repo:tag uniquely identifies the image.



src/slave/containerizer/provisioners/docker.cpp (line 351)
<https://reviews.apache.org/r/38137/#comment154830>

    The necessity of this conversion makes me think that we should probably 
take push the structured definition of the `name` to `Image::Docker`.
    
    It's probably the frameowrk's reponsibility to ensure the name is "valid" 
(i.e. can be parsed into a structured name)
    
    The Store::get() could just be:
    
    ```
    process::Future<std::vector<std::string>> get(const Image::Docker& image);
    ```
    
    If we push towards a common provisioner, we need a common store API and 
this could be a first step. 
    
    But most importantly, I think we should push up the conversion from a 
unstrutured string and use a structured name throughout the provisioner 
components (metadata manger, store, etc.) The structured name can always be 
stringified when we need to encode it into the diretory names and such.
    
    Thoughts?



src/slave/containerizer/provisioners/docker/local_store.cpp (lines 158 - 160)
<https://reviews.apache.org/r/38137/#comment154856>

    Can we actually use fetcher here?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 276)
<https://reviews.apache.org/r/38137/#comment154841>

    So the schema of the "repositories" file seems to be straightfoward here. 
Worth defining a proto schema so it's clearer what we are parsing and where to 
look for which fields here?



src/slave/containerizer/provisioners/docker/metadata_manager.hpp (line 55)
<https://reviews.apache.org/r/38137/#comment154849>

    We can iterate on this but I feel strongly that the metadata here is an 
inherent part of the store (it ties to the structure of the local cache) and we 
don't need another "actor".
    
    I understand that this is partially for sharing code between local and 
remote stores and depending on the source of the images (local vs. remote store 
in the curent terminology), the source and intermediate artifacts are different 
and it requires different actions to process them but ultimately they both 
convert and store things into the same local cache and register them with the 
same metadata file. It's then the save logic to provide an locally cached image 
to the provisioner. These all suggest that we should have "one store".
    
    What we need are not two discovery mechanisms but rather two types of 
"pullers" (the term used by [docker 
code|https://github.com/docker/docker/blob/634a848b8e3bdd8aed834559f3b2e0dfc7f5ae3a/graph/pull.go#L28]
 as well) that handle two types of sources. ("Local store" can entirely be on 
HDFS with no added complexity, i.e. same mesos fetcher)



src/slave/containerizer/provisioners/docker/metadata_manager.cpp (lines 78 - 81)
<https://reviews.apache.org/r/38137/#comment154846>

    How is the "latest" tag handled in this case? If I have downloaded an 
ubuntu:latest and 1 month later I want another ubuntu:latest, the store/slave 
is always using to use the old ubuntu:latest?


- Jiang Yan Xu


On Sept. 10, 2015, 12:31 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2015, 12:31 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, Till Toenshoff, and Jiang 
> Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am cea470e 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 95894c0 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Reply via email to