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