mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jiang Yan Xu" <>
Subject Re: Review Request 38137: Added Docker provisioner and local store
Date Thu, 10 Sep 2015 10:04:42 GMT

This is an automatically generated e-mail. To reply, visit:

Partial review, mostly high level.

src/messages/docker_provisioner.hpp (line 22)

    We typically have a comment above:

src/slave/containerizer/provisioners/docker.hpp (line 56)

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

src/slave/containerizer/provisioners/docker.hpp (line 59)

    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:
    Just an example.

src/slave/containerizer/provisioners/docker.hpp (line 66)

    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)

    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)

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

src/slave/containerizer/provisioners/docker.hpp (line 132)

    It's a parent namespace.

src/slave/containerizer/provisioners/docker.hpp (line 135)

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

src/slave/containerizer/provisioners/docker.cpp (line 100)

    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)

    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)

    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.

src/slave/containerizer/provisioners/docker/local_store.cpp (lines 158 - 160)

    Can we actually use fetcher here?

src/slave/containerizer/provisioners/docker/local_store.cpp (line 276)

    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)

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

    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

- Jiang Yan Xu

On Sept. 10, 2015, 12:31 a.m., Timothy Chen wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> -----------------------------------------------------------
> (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/ 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:
> Testing
> -------
> make check
> Thanks,
> Timothy Chen

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