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

2015-09-07 Thread 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.

2015-09-07 Thread 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.

2015-09-07 Thread 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.

2015-08-27 Thread 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.

2015-08-26 Thread 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)
https://reviews.apache.org/r/37198/#comment151949

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



src/slave/containerizer/provisioner.hpp (line 23)
https://reviews.apache.org/r/37198/#comment151952

Missing
```
#include string
```



src/slave/containerizer/provisioner.hpp (lines 60 - 61)
https://reviews.apache.org/r/37198/#comment151951

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



src/slave/containerizer/provisioner.cpp (line 23)
https://reviews.apache.org/r/37198/#comment151950

See my previous comment on using namespace 



src/slave/containerizer/provisioner.cpp (line 46)
https://reviews.apache.org/r/37198/#comment151953

One space less for the indentation.



src/slave/containerizer/provisioners/docker.hpp (line 29)
https://reviews.apache.org/r/37198/#comment151956

Missing:
```
#include stout/nothing.hpp
```



src/slave/containerizer/provisioners/docker.hpp (line 31)
https://reviews.apache.org/r/37198/#comment151954

Missing:
```
#include stout/option.hpp
```



src/slave/containerizer/provisioners/docker.hpp (line 107)
https://reviews.apache.org/r/37198/#comment151955

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)
https://reviews.apache.org/r/37198/#comment151958

explicit?



src/slave/containerizer/provisioners/docker.cpp (line 18)
https://reviews.apache.org/r/37198/#comment152032

Missing
```
#include glog/logging.hpp
```



src/slave/containerizer/provisioners/docker.cpp (line 31)
https://reviews.apache.org/r/37198/#comment151967

This should be the top include.



src/slave/containerizer/provisioners/docker.cpp (line 49)
https://reviews.apache.org/r/37198/#comment151957

Remove this line please.



src/slave/containerizer/provisioners/docker.cpp (line 206)
https://reviews.apache.org/r/37198/#comment151960

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)
https://reviews.apache.org/r/37198/#comment151962

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



src/slave/containerizer/provisioners/docker/backend.hpp (line 29)
https://reviews.apache.org/r/37198/#comment151963

Missing 
```
#include process/owned.hpp
```



src/slave/containerizer/provisioners/docker/backend.hpp (line 39)
https://reviews.apache.org/r/37198/#comment151990

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)
https://reviews.apache.org/r/37198/#comment151965

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)
https://reviews.apache.org/r/37198/#comment152033

Missing
```
#include glog/logging.h
```



src/slave/containerizer/provisioners/docker/backend.cpp (line 27)
https://reviews.apache.org/r/37198/#comment151969

Top include!



src/slave/containerizer/provisioners/docker/backend.cpp (line 155)
https://reviews.apache.org/r/37198/#comment151975

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



src/slave/containerizer/provisioners/docker/backend.cpp (lines 166 - 174)
https://reviews.apache.org/r/37198/#comment151979

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

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

2015-08-25 Thread 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.

2015-08-25 Thread 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.

2015-08-24 Thread 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.

2015-08-24 Thread Lily Chen 


 On Aug. 19, 2015, 6:21 a.m., Timothy Chen wrote:
  src/slave/containerizer/provisioners/docker.hpp, line 81
  https://reviews.apache.org/r/37198/diff/6/?file=1041387#file1041387line81
 
  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
  https://reviews.apache.org/r/37198/diff/6/?file=1041387#file1041387line84
 
  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.

2015-08-19 Thread 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.

2015-08-19 Thread 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)
https://reviews.apache.org/r/37198/#comment150975

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)
https://reviews.apache.org/r/37198/#comment150976

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)
https://reviews.apache.org/r/37198/#comment150977

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)
https://reviews.apache.org/r/37198/#comment150978

Seems like a useless comment :)



src/slave/containerizer/provisioners/docker.cpp (line 234)
https://reviews.apache.org/r/37198/#comment150979

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.

2015-08-16 Thread 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.

2015-08-14 Thread 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.

2015-08-11 Thread 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.

2015-08-10 Thread 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.

2015-08-07 Thread 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=14652859page=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.

2015-08-07 Thread Lily Chen 


 On Aug. 7, 2015, 1:45 a.m., Timothy Chen wrote:
  src/slave/flags.cpp, line 81
  https://reviews.apache.org/r/37198/diff/1/?file=1033693#file1033693line81
 
  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
  https://reviews.apache.org/r/37198/diff/1/?file=1033689#file1033689line202
 
  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.

2015-08-07 Thread 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

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

2015-08-06 Thread 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

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

2015-08-06 Thread 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