Re: Review Request 38137: Added Docker provisioner and local store

[Gilbert Song]

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


Read through all the codes. A discussion is needed to determine whether it is 
necessary to unify DOCKER and APPC together. Some parts of docker provisioner 
are almost copied from appc provisioner, but some of them are new for docker.

I am not pretty clear about why are Store and LocalStore implemented seperately?

Some improvement in Docker provisioner can be applied to appc provisioner.

- Gilbert Song


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

Re: Review Request 38137: Added Docker provisioner and local store

[Jie Yu]

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



src/slave/containerizer/provisioners/docker/paths.cpp (line 35)


Two lines apart please. Please do a sweep to fix those.



src/slave/containerizer/provisioners/docker/provisioner.cpp (lines 101 - 110)


Could you please follow the code in AppcProvisioner to get the realpath of 
'rootDir' here?

Eventually, I think we should unify the implementation of the Provisioner! 
Looking at the code here, more than 90% the code are copied! That's too bad!



src/slave/containerizer/provisioners/docker/provisioner.cpp (lines 247 - 273)


The recovery logic has been changed slightly. Please make sure the recovery 
logic here is the same as that in AppcProvisioner.

Honestly, I think it might be easier to just unify the Provisioner (instead 
of copying the code). I don't think there'll be too much additional work. I can 
help you with this.



src/slave/containerizer/provisioners/docker/provisioner.cpp (lines 376 - 379)


The logic here also changes in AppcProvisioner. Please make sure it's in 
sync. Or just unify the Provisioner.


- Jie Yu


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

Re: Review Request 38137: Added Docker provisioner and local store

[Jiang Yan Xu]

> On Sept. 10, 2015, 3:04 a.m., Jiang Yan Xu wrote:
> > 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.
> 
> Timothy Chen wrote:
> I think I'm trying to conform to the docker client experience, which is 
> what users perceive is the standard.
> Although docker supports sha256 syntax, users still all the time refers 
> to images as [registry_host:port]/repository:tag. When images are conflicting 
> between two registries, the user is usually impliciting defining which one to 
> pick based on the registry source. If user swithces the default registry and 
> we are now not in sync, this is at least identical to what docker client 
> behaves today and users should expect the same. We will provide comments in 
> the user guide.
> 
> The current docker client simpliy matches the full name like we do in the 
> code here, and caches that tag until a docker pull happens that attempts to 
> get the latest tag.
> 
> And yes Docker doesn't consistently name things correctly themselves :( 
> I'm going with the Docker registry API naming for now, which is repository 
> instead of name.
> 
> Jiang Yan Xu wrote:
> I am all for not diverging from the "standard" docker user experience 
> here but I think 
> 1) We have to support specifying image by ID.
> 
> Docker's tags are imprecise and mutable, as a large organization running 
> production services we have to have the option to be very precise about what 
> to deploy. If users don't want to be precise, they don't have to use it.
> 
> 2) From what I can tell, registry is not persisted in the "repositories" 
> file, so how does Docker engine know the locally cached image with the same 
> repo:tag matches a request for "myregistry/repo:tag" or not?
> 
> I am actually digging through docker code so feel free to share the link.

I take back my point 2). Registry is **part of** of the "repo".


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

Re: Review Request 38137: Added Docker provisioner and local store

[Jiang Yan Xu]

> On Sept. 10, 2015, 3:04 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, lines 158-160
> > 
> >
> > 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
> > 
> >
> > 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(1) 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
> > 
> >
> > 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:` 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 
>   

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 10, 2015, 12:21 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 19
> > 
> >
> > Please fix the include order please.

At one point I think our style guide has changed to put this in front, but I'm 
still a bit confused what's the correct style as of now.
I'll use the old order for now.


- Timothy


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


On Sept. 10, 2015, 7:31 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 10, 2015, 7: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/Makefile.am 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: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [38137]

Failed command: make -j3 distcheck

Error:
 make  dist-gzip am__post_remove_distdir='@:'
make[1]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot'
if test -d "mesos-0.25.0"; then find "mesos-0.25.0" -type d ! -perm -200 -exec 
chmod u+w {} ';' && rm -rf "mesos-0.25.0" || { sleep 5 && rm -rf 
"mesos-0.25.0"; }; else :; fi
test -d "mesos-0.25.0" || mkdir "mesos-0.25.0"
 (cd 3rdparty && make  top_distdir=../mesos-0.25.0 
distdir=../mesos-0.25.0/3rdparty \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
 (cd libprocess && make  top_distdir=../../mesos-0.25.0 
distdir=../../mesos-0.25.0/3rdparty/libprocess \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[3]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
:
test -d "../../mesos-0.25.0/3rdparty/libprocess" || mkdir 
"../../mesos-0.25.0/3rdparty/libprocess"
 (cd 3rdparty && make  top_distdir=../../../mesos-0.25.0 
distdir=../../../mesos-0.25.0/3rdparty/libprocess/3rdparty \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
 (cd stout && make  top_distdir=../../../../mesos-0.25.0 
distdir=../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[5]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
:
test -d "../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout" || mkdir 
"../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout"
 (cd include && make  top_distdir=../../../../../mesos-0.25.0 
distdir=../../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout/include \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[6]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
make[6]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/include'
test -n ":" \
|| find "../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout" 
-type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
  ! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -400 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -444 -exec /bin/bash 
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout/install-sh
 -c -m a+r {} {} \; \
|| chmod -R a+r 
"../../../../mesos-0.25.0/3rdparty/libprocess/3rdparty/stout"
make[5]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty/stout'
make[4]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/3rdparty'
 (cd include && make  top_distdir=../../../mesos-0.25.0 
distdir=../../../mesos-0.25.0/3rdparty/libprocess/include \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[4]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
make[4]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/include'
test -n ":" \
|| find "../../mesos-0.25.0/3rdparty/libprocess" -type d ! -perm -755 \
-exec chmod u+rwx,go+rx {} \; -o \
  ! -type d ! -perm -444 -links 1 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -400 -exec chmod a+r {} \; -o \
  ! -type d ! -perm -444 -exec /bin/bash 
/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess/install-sh
 -c -m a+r {} {} \; \
|| chmod -R a+r "../../mesos-0.25.0/3rdparty/libprocess"
make[3]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty/libprocess'
make[2]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/3rdparty'
 (cd src && make  top_distdir=../mesos-0.25.0 distdir=../mesos-0.25.0/src \
 am__remove_distdir=: am__skip_length_check=: am__skip_mode_fix=: distdir)
make[2]: Entering directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[2]: *** No rule to make target `messages/docker_provisioner.proto', needed 
by `distdir'.  Stop.
make[2]: Leaving directory 
`/home/jenkins/jenkins-slave/workspace/mesos-reviewbot/src'
make[1]: *** [distdir] Error 1
make[1]: Leaving directory 

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > 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.

I think I'm trying to conform to the docker client experience, which is what 
users perceive is the standard.
Although docker supports sha256 syntax, users still all the time refers to 
images as [registry_host:port]/repository:tag. When images are conflicting 
between two registries, the user is usually impliciting defining which one to 
pick based on the registry source. If user swithces the default registry and we 
are now not in sync, this is at least identical to what docker client behaves 
today and users should expect the same. We will provide comments in the user 
guide.

The current docker client simpliy matches the full name like we do in the code 
here, and caches that tag until a docker pull happens that attempts to get the 
latest tag.

And yes Docker doesn't consistently name things correctly themselves :( I'm 
going with the Docker registry API naming for now, which is repository instead 
of name.


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

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 10, 2015, 10:04 a.m., Jiang Yan Xu wrote:
> > 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: 
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

---
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 (updated)
-

  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

Re: Review Request 38137: Added Docker provisioner and local store

[Jiang Yan Xu]

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


Partial review, mostly high level.


src/messages/docker_provisioner.hpp (line 22)


We typically have a comment above:

// ONLY USEFUL AFTER RUNNING PROTOC.



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: 
https://github.com/docker/docker/blob/master/image/spec/v1.md

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)


s/mesos::internal::slave:://

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



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?

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 10, 2015, 7: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 (updated)
-

  src/Makefile.am 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: https://reviews.apache.org/r/38137/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38137: Added Docker provisioner and local store

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38137]

All tests passed.

- Mesos ReviewBot


On Sept. 10, 2015, 7:31 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 10, 2015, 7: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/Makefile.am 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: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 10, 2015, 12:21 a.m., Jie Yu wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 112
> > 
> >
> > Can you just call it Image given it's under docker namespace.

I think I'll still name it DockerImage just to differentiate from 
ContainerInfo::Image.


- Timothy


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


On Sept. 10, 2015, 7:31 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 10, 2015, 7: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/Makefile.am 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: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Jie Yu]

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


First pass. Haven't looked at LocalStore and MetadataManager in details yet.


src/messages/docker_provisioner.proto (line 28)


I would put this message under 
`src/slave/containerizer/provisioners/docker/message.proto`

since this message is docker provisioner specific.

ALso, why not unify this with DockerImage and ImageName? I.e.,

```
message DockerImage {
  message Name {
...
  }
  
  required Name name = 1;
  repeated string layer_ids = 2;
}

message DockerImages {
  repeated DockerImage images = 1;
};
```



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


```
__DOCKER_PROVISIONER_HPP__
```

I'll change the appc one as well.

Also, I think this should be put into
```
src/slave/containerizer/provisioners/docker/provisioner.hpp
```

I'll change the appc one accordingly as well.



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


Two lines apart.



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


Can you put ImageName and DockerImage into a separate file under 
src/slave/containerizer/provisioners/docker

I feels strange when provisioner header depends on store header and store 
header depends on provisioner header.

Also, a second thought.

Why not make 'ImageName' and 'DockerImage' a protobuf as well?



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


s/create/parse/?

s/name/value/



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


Any reason those fields are not const?



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


Can you just call it Image given it's under docker namespace.



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


Two lines apart.



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


Two lines apart.



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


Kill one blank line here.



src/slave/containerizer/provisioners/docker.cpp (lines 59 - 61)


Why do you need this function. Can you move all the logics in this function 
to DockerProvisioner::create?

Feels free to make DockerProvisionerProcess constructor public because it's 
in a source file (i.e., hidden).



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


Please move this function to a separate source file.



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


Two lines apart. Please do a sweep to fix all such occurances.



src/slave/containerizer/provisioners/docker.cpp (lines 243 - 248)


OK, this is problematic if default container info is used. What if the 
framework launches a task that does not have container info specified, but 
mesos slave provides a default container info. The container will be treated as 
unknown orphans and thus destroyed.

FYI, we checkpoint the ExecutorInfo/TaskInfo before filling the default 
container info.

What you need to do here is to remove the 'if' check here.



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


s/VLOG/LOG(INFO)/

"Destroying rootfs '" << rootfs << "' from an unknown orphan container " << 
containerId;



src/slave/containerizer/provisioners/docker/local_store.hpp (lines 48 - 50)


Please reorder these two methods.



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


Please fix the include order please.



src/slave/containerizer/provisioners/docker/local_store.cpp (lines 60 - 62)


Similar to provisioner, you can get rid of this 'create' function, instead 
you can expose LocalStoreProcess constructor.



src/slave/containerizer/provisioners/docker/local_store.cpp (lines 103 - 116)

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 9, 2015, 4:57 p.m., Jiang Yan Xu wrote:
> > Could you take out the paths.hpp|cpp changes that have already been 
> > committed?

done


- Timothy


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


On Sept. 9, 2015, 5:03 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 9, 2015, 5:03 p.m.)
> 
> 
> Review request for mesos, 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 0a8ef6d 
>   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: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 9, 2015, 5:03 p.m.)


Review request for mesos, 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 (updated)
-

  src/Makefile.am 0a8ef6d 
  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: https://reviews.apache.org/r/38137/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38137: Added Docker provisioner and local store

[Jiang Yan Xu]

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


Could you take out the paths.hpp|cpp changes that have already been committed?

- Jiang Yan Xu


On Sept. 8, 2015, 12:52 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 8, 2015, 12:52 p.m.)
> 
> 
> Review request for mesos, 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 5fdca0f 
>   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/appc.cpp fc5ee19 
>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
>   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/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [38137]

Failed command: ./support/apply-review.sh -n -r 38137

Error:
 2015-09-09 21:07:50 URL:https://reviews.apache.org/r/38137/diff/raw/ 
[68721/68721] -> "38137.patch" [1]
error: patch failed: src/Makefile.am:489
error: src/Makefile.am: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Sept. 9, 2015, 5:03 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 9, 2015, 5:03 p.m.)
> 
> 
> Review request for mesos, 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 0a8ef6d 
>   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: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Jojy Varghese]

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



src/slave/containerizer/provisioner.hpp (line 54)


doxygen style?



src/slave/containerizer/provisioner.cpp (line 36)


I think we should guard against multiple "create" calls. Call to "create" 
multiple times here would repeat the whole logic of creating the creator map 
and provsioners.



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


doxygen comments? This applies for other places too.



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


s/repo/repositorypath



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


I believe the default value of an option is None.So need not explicitly 
declare.



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


Is it layers or just layer ids? If its just layer ids, then the struct 
should be DockerImageInfo (and not DockerImage).



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


According to google style guide, this should be declared last. 

Also, why not use the "delete" keyword (c++11)?



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


maybe explicit namespace names?



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


I strongly believe that we should have strong/explicit ownership semantics 
for pointers (instead of raw pointers).



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


Would be better if we follow the factory patttern (create method that 
returns a Try)?



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


should we create a guard here so that multiple calls to "create" wont call 
multiple "process" create?



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


should we check/validate for the docker_rootfs_dir?



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


const?



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


since destroy can be called from multiple thread contexts, should we 
proetct it?



src/slave/containerizer/provisioners/docker/local_store.hpp (line 34)


destructor declaration should be after constructor(create)?



src/slave/containerizer/provisioners/docker/local_store.hpp (line 53)


This should be  declared last. Applies everywhere.



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


s/refStore/referenceStore?



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


guard for multiple calls?



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


here it is possible that refStore could be not created and we still have a 
LocalStoreProcess object. Maybe we should move the refStore creation to before 
line 152.



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


const?



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


Can this return an error?



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


Can this return an error?



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


s/layers/layerIds ?



src/slave/flags.hpp (line 56)


Adding more fields to base Flag class might not scale? Should we adopt 
feature/subsystem specific flags as subclass of base Flags?


- Jojy Varghese


On Sept. 7, 2015, 11:31 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> 

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 8, 2015, 7:52 p.m.)


Review request for mesos, 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 (updated)
-

  src/Makefile.am 5fdca0f 
  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/appc.cpp fc5ee19 
  src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
  src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
  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/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38137/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38137: Added Docker provisioner and local store

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38137]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 7:52 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 8, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos, 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 5fdca0f 
>   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/appc.cpp fc5ee19 
>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
>   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/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner.hpp, line 54
> > 
> >
> > doxygen style?

I'll fix this later with another patch, this isn't related to docker.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner.cpp, line 36
> > 
> >
> > I think we should guard against multiple "create" calls. Call to 
> > "create" multiple times here would repeat the whole logic of creating the 
> > creator map and provsioners.

Ditto.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 67
> > 
> >
> > I believe the default value of an option is None.So need not explicitly 
> > declare.

It's provided so users can skip providing a value all together.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 94
> > 
> >
> > Is it layers or just layer ids? If its just layer ids, then the struct 
> > should be DockerImageInfo (and not DockerImage).

Not sure what the difference between DockerImageInfo and DockerImage is. But 
it's just layer ids.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 56
> > 
> >
> > I strongly believe that we should have strong/explicit ownership 
> > semantics for pointers (instead of raw pointers).

Sure, perhaps you can have a new patch that fixes from the interface to all the 
implementations.
I rather not do this in this patch.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 111
> > 
> >
> > should we create a guard here so that multiple calls to "create" wont 
> > call multiple "process" create?

We assume each create is a seperate new process.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 172
> > 
> >
> > should we check/validate for the docker_rootfs_dir?

This is removed.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 279
> > 
> >
> > since destroy can be called from multiple thread contexts, should we 
> > proetct it?

It's libprocess, so no.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 94
> > 
> >
> > s/refStore/referenceStore?

Renamed to metadataManager.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 157
> > 
> >
> > here it is possible that refStore could be not created and we still 
> > have a LocalStoreProcess object. Maybe we should move the refStore creation 
> > to before line 152.

Thanks this is already fixed in latest revision.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 174
> > 
> >
> > Can this return an error?

This is already fixed in latest revision


- Timothy


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


On Sept. 8, 2015, 7:52 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 8, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos, 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 5fdca0f 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   

Re: Review Request 38137: Added Docker provisioner and local store

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [38141, 38137]

Failed command: ./support/apply-review.sh -n -r 38137

Error:
 2015-09-07 22:18:35 URL:https://reviews.apache.org/r/38137/diff/raw/ 
[86583/86583] -> "38137.patch" [1]
error: patch failed: src/Makefile.am:483
error: src/Makefile.am: patch does not apply
error: patch failed: src/slave/containerizer/provisioners/appc.cpp:33
error: src/slave/containerizer/provisioners/appc.cpp: patch does not apply
error: patch failed: src/slave/containerizer/provisioners/appc/paths.hpp:47
error: src/slave/containerizer/provisioners/appc/paths.hpp: patch does not apply
error: patch failed: src/slave/containerizer/provisioners/appc/paths.cpp:85
error: src/slave/containerizer/provisioners/appc/paths.cpp: patch does not apply
error: src/slave/containerizer/provisioners/paths.hpp: already exists in index
error: src/slave/containerizer/provisioners/paths.cpp: already exists in index
Failed to apply patch

- Mesos ReviewBot


On Sept. 7, 2015, 9:58 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 7, 2015, 9:58 p.m.)
> 
> 
> Review request for mesos, 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 5fdca0f 
>   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/appc.cpp fc5ee19 
>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
>   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/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 7, 2015, 11:31 p.m.)


Review request for mesos, 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 5fdca0f 
  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/appc.cpp fc5ee19 
  src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
  src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
  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/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38137/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 7, 2015, 9:58 p.m.)


Review request for mesos, 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 (updated)
-

  src/Makefile.am 5fdca0f 
  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/appc.cpp fc5ee19 
  src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
  src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
  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/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38137/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 7, 2015, 9:32 p.m.)


Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.


Summary (updated)
-

Added Docker provisioner and local store


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 5fdca0f 
  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/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.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: https://reviews.apache.org/r/38137/diff/


Testing
---

make check


Thanks,

Timothy Chen