mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <...@jxu.me>
Subject Re: Review Request 38137: Added Docker provisioner and local store
Date Sat, 12 Sep 2015 09:39:11 GMT


> On Sept. 10, 2015, 3: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?
> 
> Timothy Chen wrote:
>     Not sure what you're referring to? We use the fetcher to fetch local images.

No. The fetcher pointer passed into the store is simply ignored in the code.


> On Sept. 10, 2015, 3: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?
> 
> Timothy Chen wrote:
>     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.
> 
> Jiang Yan Xu wrote:
>     I see. But this kind of sucks...
>     
>     I am all for stick

Sorry for the partial comment, I didn't finish it.

I meant to say that it's unfortunate because as a user I don't know when to do a force pull
unless I always force a pull. It's probably less of an issue if I am a casual user doing this
on a single host. (I can inspect the local cache with a sequence of docker commands.) If I
have O(10000) hosts and serve O(100) users it's going to be messy. I wouldn't know if `ubuntu-patched-for-my-company:vivid`
cached on any machine is really want I think it is.

So to reiterate my point, I think an ID is necessary (and Docker engine supports it to enable
preciseness). If an ID is used, here we need to rethink about the bookkeeping data structure
and get() semantics. FWICT Docker does two lookups: first look it up by using repo:tag against
the tag store and if not found, go look for it in the graph data structure which maintains
an index of IDs.


> On Sept. 10, 2015, 3: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".
> 
> Timothy Chen wrote:
>     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.

Fine about registry being part of the repo name then. 

Is the ID of the image the ID of its leaf layer then? Image ID is definitely used throughout
Docker's documentation, code, the "repositories" files and is what uniquely identify an image.

Docker code (here and in various other places)[https://github.com/docker/docker/blob/9d0954a83d6c7b52287a62f8ab7ad42d544322a0/pkg/parsers/parsers.go#L104]
kind of treats `sha256:<sha>` as a special case of tag, which I don't think is a good
practice. It also treats registry as **part of** the repo name.

So if we are separating the registry out as a field, we should separate out the ID as a separate
field.


- Jiang Yan


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


On Sept. 11, 2015, 12:42 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, 12:42 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