Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Timothy Chen]

> On Sept. 7, 2015, 3:48 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/provisioner.cpp, line 45
> > 
> >
> > This should fit into 80 chars without a linebreak.

Also no longer valid :)


- Timothy


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


On Aug. 27, 2015, 11:40 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 27, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 4ff1c463efc527e239317ffa2d8a5607b11d2607 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner.hpp 
> a26b8138d8cc3086058b15a797dd15354a84019f 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Timothy Chen]

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



src/slave/containerizer/provisioners/docker.hpp (lines 19 - 20)


I'm going to follow Appc's provisioner style for now, which is 
__MESOS_DOCKER_HPP__

We can fix all later.



src/slave/containerizer/provisioners/docker.cpp (lines 182 - 183)


Also no longer valid :)



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


This is no longer valid.


- Timothy Chen


On Aug. 27, 2015, 11:40 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 27, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 4ff1c463efc527e239317ffa2d8a5607b11d2607 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner.hpp 
> a26b8138d8cc3086058b15a797dd15354a84019f 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Till Toenshoff]

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


I did not realize that we had a combined RR for this chain - please forgive my 
ignorance for now - I shall review the combined one once you fixed the locally 
mentioned issues.


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


This should fit into 80 chars without a linebreak.



src/slave/containerizer/provisioners/docker.hpp (lines 19 - 20)


I am not sure I like the way this include guard is named as it seems likely 
to conflict with other docker-related headers. Also we commonly include the 
extension in that guard (hpp).

I would suggest using `__SLAVE_CONTAINERIZER_PROVISIONER_DOCKER_HPP__` or 
`__MESOS_SLAVE_CONTAINERIZER_PROVISIONER_DOCKER_HPP__` which would strictly 
follow google's guide.

Too bad we never reached a consensus on MESOS-2211 but given that I see 
potential for a conflict, I would like to ask you to fix this risk no matter 
which scheme you chose.



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


The ordering for factory/methods and constructors/destructors is not 
matching google's styleguide here but given that we are rather inconsistent 
within the codebase already, I guess I should not start marking those things as 
issues -- please note but ignore for now :)



src/slave/containerizer/provisioners/docker.cpp (lines 182 - 183)


Fits into a single line.



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


Why 3 slashes?


- Till Toenshoff


On Aug. 27, 2015, 11:40 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 27, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> Timothy Chen, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> 4ff1c463efc527e239317ffa2d8a5607b11d2607 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner.hpp 
> a26b8138d8cc3086058b15a797dd15354a84019f 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 27, 2015, 11:40 p.m.)


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


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner.


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
4ff1c463efc527e239317ffa2d8a5607b11d2607 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
  src/tests/containerizer/provisioner.hpp 
a26b8138d8cc3086058b15a797dd15354a84019f 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 27, 2015, 9:51 p.m.)


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


Changes
---

Replaced docker-specific backend with shared backend and addressed style 
comments.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description (updated)
---

Add Docker image provisioner.


Diffs (updated)
-

  src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
4ff1c463efc527e239317ffa2d8a5607b11d2607 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
  src/tests/containerizer/provisioner.hpp 
a26b8138d8cc3086058b15a797dd15354a84019f 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Till Toenshoff]

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



src/Makefile.am (lines 747 - 748)


Formatting off, use hard-tabs for these files please.



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


Missing
```
#include 
```



src/slave/containerizer/provisioner.hpp (lines 60 - 61)


Should we adapt the comment to mention option to specify the sandbox 
location?



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


See my previous comment on "using namespace ...".



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


One space less for the indentation.



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


Missing:
```
#include 
```



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


Missing:
```
#include 
```



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


You are declaring virtual functions but your destructor is not virtual => 
aren't we reaching UB-land once this class gets dynamically created via its 
factory and later on destroyed via the smartpointer?



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


explicit?



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


Missing
```
#include 
```



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


This should be the top include.



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


Remove this line please.



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


You generally seem to prefer "initializing tightly followed by validation" 
but here you insert a blank line -- let's make this consistent.



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


Seems to be well below our 80 chars limit - could do without the wrapping.



src/slave/containerizer/provisioners/docker/backend.hpp (line 29)


Missing 
```
#include 
```



src/slave/containerizer/provisioners/docker/backend.hpp (line 39)


You should unify the "Docker" case -- make sure you always call it "Docker" 
or "docker" in your comments.



src/slave/containerizer/provisioners/docker/backend.hpp (line 58)


Kill this blank line please. We generally add a blank line after "closing a 
scope" and one before "openening a scope" -- it may be a bit vague as I call it 
out here, but that is how I remind myself :).



src/slave/containerizer/provisioners/docker/backend.cpp (line 21)


Missing
```
#include 
```



src/slave/containerizer/provisioners/docker/backend.cpp (line 27)


Top include!



src/slave/containerizer/provisioners/docker/backend.cpp (line 155)


See my earlier comments on the status-code/exit-code mixup.



src/slave/containerizer/provisioners/docker/backend.cpp (lines 166 - 174)


Why using rm via subprocess instead of os::rmdir?


- Till Toenshoff


On Aug. 25, 2015, 8:58 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 25, 2015, 8:58 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> f36424e94c380870cfde49d55af397fa3dc4a612 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 25, 2015, 8:58 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, and 
Jiang Yan Xu.


Changes
---

Rebased on master.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am 571e1ac0f96b2452797a478680b540f2aab63aab 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
f36424e94c380870cfde49d55af397fa3dc4a612 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
  src/tests/containerizer/provisioner.hpp 
c4ba46794fe5d7875fda11155367f521c34ea339 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 25, 2015, 6:48 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, and 
Jiang Yan Xu.


Changes
---

Rebased on master.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
f36424e94c380870cfde49d55af397fa3dc4a612 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
  src/tests/containerizer/provisioner.hpp 
c4ba46794fe5d7875fda11155367f521c34ea339 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 25, 2015, 1:32 a.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, and 
Jiang Yan Xu.


Changes
---

Moved validation of images to process.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am 9fd71d1ddf442712977596e7a13969ff5c1d68db 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
f36424e94c380870cfde49d55af397fa3dc4a612 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
  src/tests/containerizer/provisioner.hpp 
c4ba46794fe5d7875fda11155367f521c34ea339 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

> On Aug. 19, 2015, 6:21 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 81
> > 
> >
> > Did we introduce DockerImageName later?
> > A pair of strings is pretty confusing, how about pulling it earlier to 
> > here?

The ImageName struct is introduced in 37200, but it also has other changes to 
the store implementation that wouldn't be relevant in this commit.


> On Aug. 19, 2015, 6:21 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 84
> > 
> >
> > Actually it's valid to have a registry name  with a custom port in the 
> > image name:
> > 
> > localhost:5050/ubuntu:14.04
> > 
> > You need to first split on "/", then do this check.
> > 
> > And we should actually include a optional registry name in the 
> > DockerImageName struct too.

If there are more than 2 path components, what would constitute as the registry 
name? Simply the first path component?


- Lily


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


On Aug. 19, 2015, 6:42 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 19, 2015, 6:42 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> f36424e94c380870cfde49d55af397fa3dc4a612 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
>   src/tests/containerizer/provisioner.hpp 
> c4ba46794fe5d7875fda11155367f521c34ea339 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 19, 2015, 6:42 p.m.)


Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, and 
Jiang Yan Xu.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
f36424e94c380870cfde49d55af397fa3dc4a612 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
  src/tests/containerizer/provisioner.hpp 
c4ba46794fe5d7875fda11155367f521c34ea339 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Timothy Chen]

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



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


Did we introduce DockerImageName later?
A pair of strings is pretty confusing, how about pulling it earlier to here?



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


Actually it's valid to have a registry name  with a custom port in the 
image name:

localhost:5050/ubuntu:14.04

You need to first split on "/", then do this check.

And we should actually include a optional registry name in the 
DockerImageName struct too.



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


I think we usually put all the validation logic inside of the Process so 
it's all one place, rather than checking here, which makes this object very 
simple.



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


Seems like a useless comment :)



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


"Unable to join discovery local path: " + error()


- Timothy Chen


On Aug. 17, 2015, 9:11 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 17, 2015, 9:11 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Timothy Chen, 
> and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
>   src/slave/containerizer/isolators/filesystem/linux.cpp 
> f36424e94c380870cfde49d55af397fa3dc4a612 
>   src/slave/containerizer/provisioner.hpp 
> 541dd4e0b2f0c92a45c00cab6132a2be69654838 
>   src/slave/containerizer/provisioner.cpp 
> efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp e43dd1c13dd4263dc326842233808ddb7a9bb74c 
>   src/tests/containerizer/provisioner.hpp 
> c4ba46794fe5d7875fda11155367f521c34ea339 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 16, 2015, 8:31 a.m.)


Review request for mesos and Timothy Chen.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am 457ad26ee55bd7a2aedf27f45db58a9a4a6a5dc5 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
f36424e94c380870cfde49d55af397fa3dc4a612 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp e43dd1c13dd4263dc326842233808ddb7a9bb74c 
  src/tests/containerizer/provisioner.hpp 
c4ba46794fe5d7875fda11155367f521c34ea339 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 15, 2015, 12:39 a.m.)


Review request for mesos and Timothy Chen.


Changes
---

Rebased on master.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am e990369139e7ac3b86f8b04cfd5bef559e16dd24 
  src/slave/containerizer/isolators/filesystem/linux.cpp 
f36424e94c380870cfde49d55af397fa3dc4a612 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
  src/slave/flags.cpp e43dd1c13dd4263dc326842233808ddb7a9bb74c 
  src/tests/containerizer/provisioner.hpp 
c4ba46794fe5d7875fda11155367f521c34ea339 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 11, 2015, 11:23 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

Rebased on master to be consistent with provisioner interface.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 
  src/slave/containerizer/provisioner.hpp 
541dd4e0b2f0c92a45c00cab6132a2be69654838 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
  src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 10, 2015, 10:47 p.m.)


Review request for mesos and Timothy Chen.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
  src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

(Updated Aug. 8, 2015, 1:31 a.m.)


Review request for mesos and Timothy Chen.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs (updated)
-

  src/Makefile.am c213ac779e7acc3235312ca9524b3959417b8c33 
  src/slave/containerizer/provisioner.cpp 
efc7e6996ff6663bebaf61989a7e040bd2ad7a5e 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
  src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 

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


Testing
---

make check


Thanks,

Lily Chen

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Jiang Yan Xu]

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


Could you consider my comment on MESOS-2968 w.r.t the Backend API: 
https://issues.apache.org/jira/browse/MESOS-2968?focusedCommentId=14652859&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14652859

Does it make sense?

Thanks!

- Jiang Yan Xu


On Aug. 6, 2015, 1:37 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 6, 2015, 1:37 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 
>   src/slave/containerizer/provisioner.hpp 
> cb4d511e8189b65df9b9803f23812dd98edc44ac 
>   src/slave/containerizer/provisioner.cpp 
> df52e36b23ad3cd28f50e96865d0b163cc245cb2 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

> On Aug. 7, 2015, 1:45 a.m., Timothy Chen wrote:
> > src/slave/flags.cpp, line 81
> > 
> >
> > Is this going to be the same with appc? Should we just have one config?

Changed to /tmp/mesos/containers/docker, to match the structure of other 
default configured directories in Docker provisioner


> On Aug. 7, 2015, 1:45 a.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 202
> > 
> >
> > Remove extra space between [] and (
> > 
> > And also seems like we're doing duplicate functionality here with appc? 
> > Should be consolidate?

This part is pretty much the same functionality as with appc. How might we 
consolidate? With a common provision function?


- Lily


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


On Aug. 6, 2015, 8:37 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 6, 2015, 8:37 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 
>   src/slave/containerizer/provisioner.hpp 
> cb4d511e8189b65df9b9803f23812dd98edc44ac 
>   src/slave/containerizer/provisioner.cpp 
> df52e36b23ad3cd28f50e96865d0b163cc245cb2 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Guangya Liu]

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

Ship it!


LGTM

- Guangya Liu


On 八月 6, 2015, 8:37 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated 八月 6, 2015, 8:37 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 
>   src/slave/containerizer/provisioner.hpp 
> cb4d511e8189b65df9b9803f23812dd98edc44ac 
>   src/slave/containerizer/provisioner.cpp 
> df52e36b23ad3cd28f50e96865d0b163cc245cb2 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37198: Add Docker image provisioner and copy backend.

[Timothy Chen]

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



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


Let's use a struct instead of a pair here.



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


Doesn't seem like this TODO comment means much :) Remove it.



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


Remove extra space between [] and (

And also seems like we're doing duplicate functionality here with appc? 
Should be consolidate?



src/slave/flags.cpp (line 81)


Is this going to be the same with appc? Should we just have one config?


- Timothy Chen


On Aug. 6, 2015, 8:37 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37198/
> ---
> 
> (Updated Aug. 6, 2015, 8:37 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-2850
> https://issues.apache.org/jira/browse/MESOS-2850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Docker image provisioner and copy backend.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 
>   src/slave/containerizer/provisioner.hpp 
> cb4d511e8189b65df9b9803f23812dd98edc44ac 
>   src/slave/containerizer/provisioner.cpp 
> df52e36b23ad3cd28f50e96865d0b163cc245cb2 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
>   src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 
> 
> Diff: https://reviews.apache.org/r/37198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Review Request 37198: Add Docker image provisioner and copy backend.

[Lily Chen]

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

Review request for mesos and Timothy Chen.


Bugs: MESOS-2850
https://issues.apache.org/jira/browse/MESOS-2850


Repository: mesos


Description
---

Add Docker image provisioner and copy backend.


Diffs
-

  src/Makefile.am 35ebbbd0bd9c9dd059c02ce3dc22c780b929be81 
  src/slave/containerizer/mesos/containerizer.cpp 
6d07ff151770bac4eeeb7cd8c9d03f54f2e78ec1 
  src/slave/containerizer/provisioner.hpp 
cb4d511e8189b65df9b9803f23812dd98edc44ac 
  src/slave/containerizer/provisioner.cpp 
df52e36b23ad3cd28f50e96865d0b163cc245cb2 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/backend.cpp PRE-CREATION 
  src/slave/flags.hpp 881d494c06fea5c382d27b357d65c1baf201ae46 
  src/slave/flags.cpp 82b6cf47af26f0533ff603a67240777e9a9b986e 

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


Testing
---

make check


Thanks,

Lily Chen