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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38137]

All tests passed.

- Mesos ReviewBot


On Sept. 24, 2015, 9:21 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 24, 2015, 9:21 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 776483b 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp 3f6601a 
>   src/slave/flags.cpp 69976d7 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 23, 2015, 5:34 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner/docker/puller.hpp, line 60
> > 
> >
> > I think timeout should be part of the interface. remote puller 
> > especially should be constrained by time.

I think for now I won't add this, since local shouldn't really need to capped 
by timeout. We already have a timeout around the whole containerizer launch 
which will guard this. For remote case we can but you can add that when you 
implement remote store.


> On Sept. 23, 2015, 5:34 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner/docker/puller.cpp, line 39
> > 
> >
> > "Failed to get puller for " ?

I changed it to the use the same wording as containerizer.cpp 
Containerizer::create


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 23, 2015, 5:34 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner/docker/store.cpp, line 257
> > 
> >
> > Dont we have to cleanup the created directory?

It's automatically cleaned up in the end.


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

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

(Updated Sept. 24, 2015, 9:21 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 776483b 
  src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/store.cpp 35d1199 
  src/slave/flags.hpp 3f6601a 
  src/slave/flags.cpp 69976d7 
  src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Timothy Chen]

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

(Updated Sept. 24, 2015, 8:02 p.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 776483b 
  src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/store.cpp 35d1199 
  src/slave/flags.hpp 3f6601a 
  src/slave/flags.cpp 8792162 
  src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Jie Yu]

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

Ship it!


LGTM! Let's get this committed!


src/slave/containerizer/provisioner/docker/local_puller.cpp (lines 126 - 127)


This is a little jagged, how about:
```
VLOG(1) << "Untarring image from '" << directory
<< "' to '" << tarPath << "'";
```



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 167)


This is a local helper, right? Please make it a 'static' function.



src/slave/containerizer/provisioner/docker/local_puller.cpp (lines 260 - 269)


This can be done in parallel as well, right?



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 287)


Insert a new line above.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 299)


Is this necessary?



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 342)


insert a new line above.



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 172)


No need for mesos::internal::slave?



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (lines 192 - 
193)


This fits in one line?



src/tests/containerizer/provisioner_docker_tests.cpp (line 700)


2 blank lines above.


- Jie Yu


On Sept. 24, 2015, 9:21 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 24, 2015, 9:21 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 776483b 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp 3f6601a 
>   src/slave/flags.cpp 69976d7 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jojy Varghese]

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

Ship it!


Ship It!

- Jojy Varghese


On Sept. 24, 2015, 9:21 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 24, 2015, 9:21 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 776483b 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp 3f6601a 
>   src/slave/flags.cpp 69976d7 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jiang Yan Xu]

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

Ship it!


Thanks for being patient. Please commit this!


src/slave/containerizer/provisioner/docker/local_puller.hpp (line 40)


s/docker_discovery_local_dir/docker_local_archives_dir/

I suggested the flag name based on the suggestion about ArchivesPuller, I 
see that you are keeping the name LocalPuller (because we are not supporting 
remote archives right now) but changed the flag.

Maybe s/LocalPuller/LocalArchivesPuller/ below for now to be consistent? 

We can revisit other archives locations after 
https://issues.apache.org/jira/browse/MESOS-3511.



src/slave/containerizer/provisioner/docker/local_puller.hpp (line 41)


s/images saved as tar with the name as the image name with tag/images saved 
as tars with file names in the form of `:.tar`.

We better be consistent with repo vs. name in our code and documentation so 
it's less confusing than docker docs. :)



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 279)


s/directory directory/directory/



src/slave/containerizer/provisioner/docker/paths.hpp (line 47)


Two blank lines here and elsewhere in this file beause they are outside a 
class.



src/slave/containerizer/provisioner/docker/paths.hpp (line 57)


s/staging/archivePath/ or s/staging/archiveDir/

This was suggested in my last review.

I think the easiest way to explain this bunch of methods is that: given an 
`archivePath` which is the base dir of the untarred archive, return me the path 
to the element inside the archive.



src/slave/containerizer/provisioner/docker/puller.hpp (line 42)


s/the directory the puller put its changeset to/the directory where the 
puller puts its changeset/



src/slave/containerizer/provisioner/docker/puller.hpp (line 57)


s/returns/return/ to be consistent with the use of first person verb form 
in this paragraph.



src/slave/containerizer/provisioner/docker/puller.hpp (line 74)


Kill line.


- Jiang Yan Xu


On Sept. 24, 2015, 1:02 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 24, 2015, 1:02 p.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 776483b 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp 3f6601a 
>   src/slave/flags.cpp 8792162 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 24, 2015, 9 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/local_puller.hpp, line 40
> > 
> >
> > s/docker_discovery_local_dir/docker_local_archives_dir/
> > 
> > I suggested the flag name based on the suggestion about ArchivesPuller, 
> > I see that you are keeping the name LocalPuller (because we are not 
> > supporting remote archives right now) but changed the flag.
> > 
> > Maybe s/LocalPuller/LocalArchivesPuller/ below for now to be 
> > consistent? 
> > 
> > We can revisit other archives locations after 
> > https://issues.apache.org/jira/browse/MESOS-3511.

Hmm ok I'll rename to LocalArchivesPuller


- Timothy


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


On Sept. 24, 2015, 8:02 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 24, 2015, 8:02 p.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 776483b 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp 3f6601a 
>   src/slave/flags.cpp 8792162 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jiang Yan Xu]

> On Sept. 23, 2015, 12:50 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/puller.hpp, line 60
> > 
> >
> > If we use LinkedHashMap we don't need to create another struct right?
> 
> Timothy Chen wrote:
> True, but I actually like returning a list due to the fact we already 
> return a list when we collect all the futures, and thought converting it into 
> a LinkedHashMap in the end from a list was a bit unnecessary. What you think?

To me it's a bit contrived to explain this concept. Why is it LayerPath and not 
LayerInfo? Why is `id` a separate part of a "Path"? Why is the field inside be 
named `directory` instead of `path`? It's probably closer to a list of tuples 
of . I personally would spend a few more lines to do the conversion 
(which is an implementation detail) than exposing a concept that bears 
explanation to the caller.

That said, it's not a big deal. Feel free to drop this.


- Jiang Yan


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


On Sept. 24, 2015, 1:02 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 24, 2015, 1:02 p.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 776483b 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp 3f6601a 
>   src/slave/flags.cpp 8792162 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38137]

All tests passed.

- Mesos ReviewBot


On Sept. 24, 2015, 8:02 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 24, 2015, 8:02 p.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 776483b 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp 3f6601a 
>   src/slave/flags.cpp 8792162 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/local_puller.hpp, line 45
> > 
> >
> > Not a strong opinion but if you think this could potentially be "not 
> > local" (e.g. NFS or HDFS hosting these), you may name it ArchivesPuller 
> > because it's more about "what the image source format is" then "where it is 
> > from".
> > 
> > We don't, of course, need to implement all the remote cases in the 
> > first patch.

hmm, for now it's just local :(


> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/local_puller.cpp, line 197
> > 
> >
> > What's the problem with it and is it still a problem with recent JSON 
> > related commits?
> > 
> > (I have no idea, just would like to check.)

It's a problem since we find fields that's nested with in JSON with syntax 
"a.b.c", but if a tag itself has a period such as "14.04", we will try to find 
14 first in the object.


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jojy Varghese]

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



src/slave/containerizer/provisioner/docker/puller.hpp (line 40)


Shouldnt this be LayerInfo?



src/slave/containerizer/provisioner/docker/puller.hpp (line 55)


according to doxygen style guide, the param name and its description should 
be separated by 1 space (uniform separation between all sections).



src/slave/containerizer/provisioner/docker/puller.hpp (line 60)


I think timeout should be part of the interface. remote puller especially 
should be constrained by time.



src/slave/containerizer/provisioner/docker/puller.cpp (line 39)


"Failed to get puller for " ?



src/slave/containerizer/provisioner/docker/store.cpp (line 37)


this should be at the top of includes?



src/slave/containerizer/provisioner/docker/store.cpp (line 257)


Dont we have to cleanup the created directory?


- Jojy Varghese


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[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.
> 
> 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.
> 
> Jiang Yan Xu wrote:
> I take back my point 2). Registry is **part of** of the "repo".

Dropping this for now as it's going to be addressed later.


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   

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

[Jiang Yan Xu]

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



src/slave/containerizer/provisioner/docker/puller.hpp (line 60)


If we use LinkedHashMap we don't need to create another struct right?


- Jiang Yan Xu


On Sept. 22, 2015, 3:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 3:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jiang Yan Xu]

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


Much closer! Haven't looked at the tests yet but will do.


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






src/slave/containerizer/provisioner/docker/metadata_manager.hpp (line 57)






src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 229)






src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 243)






src/slave/containerizer/provisioner/docker/puller.hpp (line 34)






src/slave/containerizer/provisioner/docker/puller.hpp (line 43)






src/slave/containerizer/provisioner/docker/puller.hpp (line 59)






src/slave/containerizer/provisioner/docker/store.cpp (line 182)






src/slave/flags.hpp (line 54)






src/slave/containerizer/provisioner/docker/local_puller.hpp (line 29)


Kill line.



src/slave/containerizer/provisioner/docker/local_puller.hpp (line 45)


Not a strong opinion but if you think this could potentially be "not local" 
(e.g. NFS or HDFS hosting these), you may name it ArchivesPuller because it's 
more about "what the image source format is" then "where it is from".

We don't, of course, need to implement all the remote cases in the first 
patch.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 83)


This is not used at all here.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 119)


No longer a "local store" if we now use 'store' to refer to the internal 
object and directory structure right? 

What should we call this? `local archieves`? (`archieves` being a singluar 
word describing the place that these tars are kept).



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 121)


Add the image name in the error message?

I am not sure if you need the additional log line above but I guess I don't 
have major objection either.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 131)


About the name `staging` here and elsewhere in the file.

First of all `staging` could be the name of the base staging directory 
under the store root and here it is referring to the temp dir.

Furthermore, I like the fact that your Puller interface intentionally 
avoids the need to figure out the directory hierarchy but instead takes a 
generic `directory`. 

Therefore within the puller things should be centered around the 
`directory` and only the caller knows it's a staging temp dir.

So I suggest s/staging/directory.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 197)


What's the problem with it and is it still a problem with recent JSON 
related commits?

(I have no idea, just would like to check.)



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 237)


Does it need to be a member method. If not better be written as an private 
free-standing function.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 288)






src/slave/containerizer/provisioner/docker/local_puller.cpp (line 309)


I see that this is leaving behind the layer.tar file after untarring, which 
is fine but maybe a comment that it will be cleaned up when the entire staging 
dir is deleted?



src/slave/containerizer/provisioner/docker/metadata_manager.hpp (line 57)


With the pullers refactored out of the store I don't see the need for a 
separate metadata manager. The metadata is literally "the metadata of the 
store" and it mirrors the set of operations of the store. e.g. Store::recover() 
just calls MetadataManager::recover().

I am not adamant about making it happen in this review though.



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (lines 210 - 

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

[Jie Yu]

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



src/slave/containerizer/provisioner/docker/local_puller.hpp (line 20)


Please adjust the naming of those macro guards (see how we name appc 
headers). Please be consistent.

We don't need the prefix MESOS here because it's not a public header.

Please change other headers as well.



src/slave/containerizer/provisioner/docker/store.cpp (lines 209 - 216)


moveLayer can be executed in parallel, right?


- Jie Yu


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[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.
> 
> Jiang Yan Xu wrote:
> I take back my point 2). Registry is **part of** of the "repo".
> 
> Timothy Chen wrote:
> Dropping this for now as it's going to be addressed later.

That's fine. Mind if I create a ticket to track? Another chance for me to 
articulate the need for it.


- Jiang Yan


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


On Sept. 22, 2015, 3:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 3:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   

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

[Jiang Yan Xu]

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



src/slave/containerizer/provisioner/docker/store.cpp (lines 210 - 213)


This can be parallelized right?



src/tests/containerizer/provisioner_docker_tests.cpp (lines 737 - 741)


4-space indentation.



src/tests/containerizer/provisioner_docker_tests.cpp (lines 747 - 749)


4-space indentation.



src/tests/containerizer/provisioner_docker_tests.cpp (lines 765 - 767)


4 space indentation.



src/tests/containerizer/provisioner_docker_tests.cpp (line 802)


s/mesos::internal:://

Just because it's way to long and hard on the eyes. :)



src/tests/containerizer/provisioner_docker_tests.cpp (line 818)


s/reference store/metadata manager/



src/tests/containerizer/provisioner_docker_tests.cpp (line 828)


s/mesos::internal:://



src/tests/containerizer/provisioner_docker_tests.cpp (line 841)


s/reference store/metadata manager/



src/tests/containerizer/provisioner_docker_tests.cpp (line 844)


s/mesos::internal:://


- Jiang Yan Xu


On Sept. 22, 2015, 3:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 3:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/metadata_manager.hpp, line 57
> > 
> >
> > With the pullers refactored out of the store I don't see the need for a 
> > separate metadata manager. The metadata is literally "the metadata of the 
> > store" and it mirrors the set of operations of the store. e.g. 
> > Store::recover() just calls MetadataManager::recover().
> > 
> > I am not adamant about making it happen in this review though.

Ok, we'll change this next iteration.


> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/paths.hpp, line 36
> > 
> >
> > This diagram doesn't explain some of the methods in this file involving 
> > stuff under the "staging" dir below.
> > 
> > I imagine with a registry puller the temp_dir looks a bit different.
> > 
> > ```
> > staging
> > |-- 
> > |-- 
> > |-- VERSION
> > |-- json
> > |-- layer.tar
> > ```

Ya the registry looks different, so I'll just extend it to the point where it 
has the rootfs


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 23, 2015, 7:45 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/store.cpp, line 191
> > 
> >
> > How large could this folder potentially be? To remove large directories 
> > I think a subprocess is more suitable as doing it this way could keep 
> > threads busy.

Good point, it's hard to say what's in a layer.


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 23, 2015, 5:34 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner/docker/puller.hpp, line 40
> > 
> >
> > Shouldnt this be LayerInfo?

I don't think we need to call it Info as it only holds a path and id.


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 23, 2015, 7:50 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/provisioner/docker/puller.hpp, line 60
> > 
> >
> > If we use LinkedHashMap we don't need to create another struct right?

True, but I actually like returning a list due to the fact we already return a 
list when we collect all the futures, and thought converting it into a 
LinkedHashMap in the end from a list was a bit unnecessary. What you think?


- Timothy


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


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote:
> > src/Makefile.am, line 491
> > 
> >
> > Similar to master/resgistry.proto, we should put this proto file under
> > 
> > `slave/containerizer/provisioner/docker.proto`
> > or
> > `slave/containerizer/provisioner/docker/message.proto`
> > ?

This is actually gone now.


> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner/docker/message.proto, line 21
> > 
> >
> > Remove .docker?

I added the docker namespace since it's pretty specific to just docker and is 
in the same namespace of the docker provisioner. 
Any reason why we should move it up?


- Timothy


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


On Sept. 21, 2015, 5:50 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 21, 2015, 5:50 p.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 834f69a 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner/docker/paths.hpp, line 40
> > 
> >
> > What's this?

No longer needed with the shared provisioner change, thanks!


> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner/docker/store.cpp, line 228
> > 
> >
> > Why do you need the 'Nothing()' here?

Just so we can chain callbacks.


- Timothy


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


On Sept. 21, 2015, 5:50 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 21, 2015, 5:50 p.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 834f69a 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner/docker/store.cpp, line 256
> > 
> >
> > According to your current design, Store is supposed to be agnostic to 
> > which puller is being used, right? Therefore, there should be local puller 
> > specific code in Store.
> > 
> > Shouldn't the puller interface returns a map: imageId -> 
> > path_to_fs_layer and the Store just looks at the paths and move them to 
> > getImageLayerRootfsPath?

I thought about this, but I also want to know the dependencies among layers. So 
decided that the simplest is to return the vector of layer IDs, but just expect 
the layers to be prepared under the staging directory with the right structure. 
I've commented on that in the puller interface.


- Timothy


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


On Sept. 21, 2015, 5:50 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 21, 2015, 5:50 p.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 834f69a 
>   src/slave/containerizer/provisioner.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jie Yu]

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner/docker/store.cpp, line 256
> > 
> >
> > According to your current design, Store is supposed to be agnostic to 
> > which puller is being used, right? Therefore, there should be local puller 
> > specific code in Store.
> > 
> > Shouldn't the puller interface returns a map: imageId -> 
> > path_to_fs_layer and the Store just looks at the paths and move them to 
> > getImageLayerRootfsPath?
> 
> Timothy Chen wrote:
> I thought about this, but I also want to know the dependencies among 
> layers. So decided that the simplest is to return the vector of layer IDs, 
> but just expect the layers to be prepared under the staging directory with 
> the right structure. I've commented on that in the puller interface.

Have you looked at LinkedHashMap?


- Jie


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


On Sept. 22, 2015, 8:22 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 8: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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Jie Yu]

> On Sept. 21, 2015, 11:22 p.m., Jie Yu wrote:
> > src/slave/containerizer/provisioner/docker/message.proto, line 21
> > 
> >
> > Remove .docker?
> 
> Timothy Chen wrote:
> I added the docker namespace since it's pretty specific to just docker 
> and is in the same namespace of the docker provisioner. 
> Any reason why we should move it up?

You already have 'Docker' in DockerImage, docker::DockerImage does not read 
well. Either remove .docker or remove Docker.


- Jie


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


On Sept. 22, 2015, 8:22 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 8: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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38137]

All tests passed.

- Mesos ReviewBot


On Sept. 22, 2015, 10:15 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 22, 2015, 10:15 p.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 e224060 
>   src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/metadata_manager.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioner/store.cpp 35d1199 
>   src/slave/flags.hpp e31a418 
>   src/slave/flags.cpp add4196 
>   src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

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

[Timothy Chen]

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

(Updated Sept. 22, 2015, 10:15 p.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 e224060 
  src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/store.cpp 35d1199 
  src/slave/flags.hpp e31a418 
  src/slave/flags.cpp add4196 
  src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Timothy Chen]

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

(Updated Sept. 22, 2015, 8: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 e224060 
  src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/store.cpp 35d1199 
  src/slave/flags.hpp e31a418 
  src/slave/flags.cpp add4196 
  src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Jie Yu]

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


Overall, looks good. The main issue is that 'Store' should not have 
dependencies on 'Local' puller (see my detailed comments below).


src/Makefile.am (line 256)


Kill one blank line here.



src/Makefile.am (line 491)


Similar to master/resgistry.proto, we should put this proto file under

`slave/containerizer/provisioner/docker.proto`
or
`slave/containerizer/provisioner/docker/message.proto`
?



src/slave/containerizer/provisioner.cpp (lines 49 - 51)


You need to do a rebase. This file has been moved to 
src/containerizer/provisioner/provisioner.cpp.

Or you just forgot to remove this file from the patch?



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 61)


I don't see the implementation of this method?



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 122)


Insert a blank line above.



src/slave/containerizer/provisioner/docker/local_puller.cpp (lines 146 - 151)


Do you want to kill the 'tar' process when slave exits? You may want to 
take a look at how we setup death signal for 'perf' (src/linuc/perf.cpp).

It's ok you don't want to address that in this patch. Please add a TODO 
somewhere.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 158)


Can you just capture 'tarPath' here to be more explicit?



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 293)


Insert a blank line above.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 321)


Insert a blank line above.



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 326)


Why do you need to capture '='?



src/slave/containerizer/provisioner/docker/local_puller.cpp (line 338)


Kill a blank line here.



src/slave/containerizer/provisioner/docker/message.proto (line 21)


Remove .docker?



src/slave/containerizer/provisioner/docker/message.proto (line 40)


2 lines apart.



src/slave/containerizer/provisioner/docker/metadata_manager.hpp (lines 86 - 89)


Please move 'recover()' above 'put'.



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 51)


Kill a blank line here.



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (lines 57 - 58)


As we discussed before, please kill this method. Instead, expose the 
constructor.



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 89)


Insert a blank line above.



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 92)


Insert a blank line above.



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (lines 120 - 
121)


Mind adjust the arguments like the following. It's less jagged:)

```
return dispatch(
process.get(),
::put,
name,
layerIds);
```



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 203)


Why do you need this?



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 206)


Please quote the path (what if path contains spaces?)

Also, remove the period in the end (please make sure to fix all occurances 
in this patch).



src/slave/containerizer/provisioner/docker/metadata_manager.cpp (line 207)


Insert a blank line above.



src/slave/containerizer/provisioner/docker/paths.hpp (line 40)


What's this?



src/slave/containerizer/provisioner/docker/store.hpp (lines 22 - 37)


Please cleanup the header includes here. Remove those that 

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

[Timothy Chen]

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

(Updated Sept. 21, 2015, 5:06 p.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 834f69a 
  src/slave/containerizer/provisioner.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e31a418 
  src/slave/flags.cpp add4196 
  src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Timothy Chen]

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

(Updated Sept. 21, 2015, 5:33 p.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 834f69a 
  src/slave/containerizer/provisioner.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e31a418 
  src/slave/flags.cpp add4196 
  src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Timothy Chen]

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

(Updated Sept. 21, 2015, 5:50 p.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 834f69a 
  src/slave/containerizer/provisioner.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/metadata_manager.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION 
  src/slave/flags.hpp e31a418 
  src/slave/flags.cpp add4196 
  src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 

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


Testing
---

make check


Thanks,

Timothy Chen

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

[Mesos ReviewBot]

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


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 
`slave/containerizer/provisioner/docker.hpp', 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, store and local puller

[Mesos ReviewBot]

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


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, store and local puller

[Timothy Chen]

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

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


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


Summary (updated)
-

Added Docker provisioner, store and local puller


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