mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Timothy Chen" <tnac...@apache.org>
Subject Re: Review Request 38137: Added Docker provisioner and local store
Date Fri, 11 Sep 2015 07:39:26 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message