> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 59
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066156#file1066156line59>
> >
> >     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.

Should be addressed now by just putting the definitions in proto.


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 66
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066156#file1066156line66>
> >
> >     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".

I think registry should be part of the name since it's what uniquely identifies 
a "tag" of a image.
A name of a docker image, is merely a tag instead of a immutable id, and that's 
very different than how Appc works.
Only layers have uniquely identifable ids.


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 71
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066156#file1066156line71>
> >
> >     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.

Yep this is gone now!


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 122
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066156#file1066156line122>
> >
> >     Is the order of the list significant? We should document it.

Yes it is, I'll comment on it.


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 100
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066157#file1066157line100>
> >
> >     re my comment above. Something close to a spec.hpp|cpp to put it would 
> > be nice.

It's in proto now.


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 351
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066157#file1066157line351>
> >
> >     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?

I think this is done now with my latest changes, can you take a look?


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, lines 158-160
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066159#file1066159line158>
> >
> >     Can we actually use fetcher here?

Not sure what you're referring to? We use the fetcher to fetch local images.


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 276
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066159#file1066159line276>
> >
> >     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?

How do you model a JSON map of images in proto?


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker/metadata_manager.hpp, line 55
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066160#file1066160line55>
> >
> >     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)

I see, I think conceptually it probably fits nicer to have two pullers as you 
mentioned.
How about I can post another patch after this to restructure this as I like to 
get everything reviewed and merged so it's unblocking some other folks?


> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker/metadata_manager.cpp, lines 
> > 78-81
> > <https://reviews.apache.org/r/38137/diff/5/?file=1066161#file1066161line78>
> >
> >     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?

Yep, that's how docker client works for now. We will provide the option to pull 
again, or always pull latest which will attempt to update the metadata.


- Timothy


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


On Sept. 11, 2015, 7:22 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2015, 7:22 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 8963cea 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 2ac9008 
>   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/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/message.proto 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/provisioner.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/provisioner.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp 799c963 
>   src/slave/flags.cpp b676bac 
>   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