Re: Review Request 29329: Add executor for docker containerizer

2015-04-06 Thread Timothy Chen

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

(Updated April 7, 2015, 12:39 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


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


Repository: mesos


Description
---

Add executor for docker containerizer, replaces the usage of command executor


Diffs
-

  src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
  src/docker/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen



Re: Review Request 29329: Add executor for docker containerizer

2015-04-06 Thread Timothy Chen

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

(Updated April 7, 2015, 12:34 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


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


Repository: mesos


Description
---

Add executor for docker containerizer, replaces the usage of command executor


Diffs (updated)
-

  src/Makefile.am 9c01f5d6c692f835100e7cade928748cc4763cc8 
  src/docker/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen



Re: Review Request 29329: Add executor for docker containerizer

2015-03-20 Thread Vinod Kone

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


Hey Tim. Made some comments on the doc. Mainly, I'm interested in understanding 
how the shortcomings you discussed in the doc will be addressed for custom 
executor scenarios. Also, can you create a JIRA ticket and attach?

- Vinod Kone


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-20 Thread Timothy Chen
Sorry about that! I just relaxed the perms so can anyone can edit/comment.

Tim

On Thu, Feb 19, 2015 at 7:58 PM, Vinod Kone vinodk...@gmail.com wrote:
 Thanks Tim for the doc. Can you open it up for comments (it's currently view
 only)?

 On Thu, Feb 19, 2015 at 5:15 PM, Timothy Chen tnac...@apache.org wrote:



  On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
   How are we going to manage the duplication across the command executor
   and the docker executor?
 
  Timothy Chen wrote:
  I think I'm going to leave them seperate as they most likely will
  grow independent in tangent.
  The docker executor is solely responsible for launching docker
  containers, and have different assumptions.
 
  Ben Mahler wrote:
  Is there a design doc about this docker architecture decision? It's
  a bit hard to see the context for this, and how this will play out for 
  other
  executors that frameworks want to run inside docker, are you planning to
  support that, or do users have to bake-in their executors..?
 
  Steve Niemitz wrote:
  I'm curious about this too.  I'm having trouble understanding how
  other executors will run inside containers as they did previously after 
  this
  change.  I'd love to see a design doc about how this proposed system would
  work.

 I've updated the Docker containerizer update google doc to hold a high
 level diagram with the new Docker executor.

 https://docs.google.com/a/mesosphere.io/document/d/1_1oLHXg_aHj_fYCzsjYwox9xvIYNAKIeVjO5BFxsUGI/edit


 - Timothy


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


 On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
  ---
  This is an automatically generated e-mail. To reply, visit:
  https://reviews.apache.org/r/29329/
  ---
 
  (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
  Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
  Repository: mesos
 
 
  Description
  ---
 
  Add executor for docker containerizer, replaces the usage of command
  executor
 
 
  Diffs
  -
 
src/Makefile.am 07bea1f
src/docker/executor.cpp PRE-CREATION
 
  Diff: https://reviews.apache.org/r/29329/diff/
 
 
  Testing
  ---
 
  make check
 
 
  Thanks,
 
  Timothy Chen
 
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-19 Thread Vinod Kone
Thanks Tim for the doc. Can you open it up for comments (it's currently
view only)?

On Thu, Feb 19, 2015 at 5:15 PM, Timothy Chen tnac...@apache.org wrote:



  On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
   How are we going to manage the duplication across the command executor
 and the docker executor?
 
  Timothy Chen wrote:
  I think I'm going to leave them seperate as they most likely will
 grow independent in tangent.
  The docker executor is solely responsible for launching docker
 containers, and have different assumptions.
 
  Ben Mahler wrote:
  Is there a design doc about this docker architecture decision? It's
 a bit hard to see the context for this, and how this will play out for
 other executors that frameworks want to run inside docker, are you planning
 to support that, or do users have to bake-in their executors..?
 
  Steve Niemitz wrote:
  I'm curious about this too.  I'm having trouble understanding how
 other executors will run inside containers as they did previously after
 this change.  I'd love to see a design doc about how this proposed system
 would work.

 I've updated the Docker containerizer update google doc to hold a high
 level diagram with the new Docker executor.

 https://docs.google.com/a/mesosphere.io/document/d/1_1oLHXg_aHj_fYCzsjYwox9xvIYNAKIeVjO5BFxsUGI/edit


 - Timothy


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


 On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
  ---
  This is an automatically generated e-mail. To reply, visit:
  https://reviews.apache.org/r/29329/
  ---
 
  (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
  Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
  Repository: mesos
 
 
  Description
  ---
 
  Add executor for docker containerizer, replaces the usage of command
 executor
 
 
  Diffs
  -
 
src/Makefile.am 07bea1f
src/docker/executor.cpp PRE-CREATION
 
  Diff: https://reviews.apache.org/r/29329/diff/
 
 
  Testing
  ---
 
  make check
 
 
  Thanks,
 
  Timothy Chen
 
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-19 Thread Timothy Chen


 On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
  How are we going to manage the duplication across the command executor and 
  the docker executor?
 
 Timothy Chen wrote:
 I think I'm going to leave them seperate as they most likely will grow 
 independent in tangent.
 The docker executor is solely responsible for launching docker 
 containers, and have different assumptions.
 
 Ben Mahler wrote:
 Is there a design doc about this docker architecture decision? It's a bit 
 hard to see the context for this, and how this will play out for other 
 executors that frameworks want to run inside docker, are you planning to 
 support that, or do users have to bake-in their executors..?
 
 Steve Niemitz wrote:
 I'm curious about this too.  I'm having trouble understanding how other 
 executors will run inside containers as they did previously after this 
 change.  I'd love to see a design doc about how this proposed system would 
 work.

I've updated the Docker containerizer update google doc to hold a high level 
diagram with the new Docker executor.
https://docs.google.com/a/mesosphere.io/document/d/1_1oLHXg_aHj_fYCzsjYwox9xvIYNAKIeVjO5BFxsUGI/edit


- Timothy


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


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-13 Thread Bill Farner


 On Feb. 3, 2015, 11:23 p.m., Bill Farner wrote:
  Is there a shortcoming in the executor API that prevents this from being an 
  independent binary?
 
 Timothy Chen wrote:
 Hi Bill, this is going to be a binary on itself just like command 
 exexutor. Do you have something else in mind?

Sorry for my late reply, gmail filter mishap on my end.

It's my understanding that this will not be a standalone binary, but a part of 
the slave binary (which, IIUC, is true of the command executor as well).  If 
this is incorrect, please excuse my ignorance and read no further.

The distinction worrying me is that this is creating a first-class executor 
implementation within mesos itself.  I agree that in some respects this is not 
dissimilar from the command executor, though i think coupling to a shell is not 
as extreme as a third-party container system.  This sets a risky precedent for 
mesos, as it allows the project to make short-sighted decisions coupling to the 
first-class executor, and fail to generalize its APIs in ways that limits other 
executors when compared to those built in to the slave.


- Bill


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


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-13 Thread Timothy Chen


 On Feb. 3, 2015, 11:23 p.m., Bill Farner wrote:
  Is there a shortcoming in the executor API that prevents this from being an 
  independent binary?
 
 Timothy Chen wrote:
 Hi Bill, this is going to be a binary on itself just like command 
 exexutor. Do you have something else in mind?
 
 Bill Farner wrote:
 Sorry for my late reply, gmail filter mishap on my end.
 
 It's my understanding that this will not be a standalone binary, but a 
 part of the slave binary (which, IIUC, is true of the command executor as 
 well).  If this is incorrect, please excuse my ignorance and read no further.
 
 The distinction worrying me is that this is creating a first-class 
 executor implementation within mesos itself.  I agree that in some respects 
 this is not dissimilar from the command executor, though i think coupling to 
 a shell is not as extreme as a third-party container system.  This sets a 
 risky precedent for mesos, as it allows the project to make short-sighted 
 decisions coupling to the first-class executor, and fail to generalize its 
 APIs in ways that limits other executors when compared to those built in to 
 the slave.

Hi Bill, in my mind this isn't changing the executor API, it's implementing the 
API that works the containerizer system we're integrating with. Unfortunately a 
container system (or any) has limitations that we will have to overcome, and 
Mesos being a pluggable containerizer system will have to integrate with the 
limitations in mind.

This isn't really a short-sighted decision as we're not changing the API to 
cope with Docker, we need to implement a specific implementation same as the 
Docker containerizer itself which is a implementation of the containerizer 
interface.

And really thinking about the nature of the executor in Mesos, we're only 
creating the necessary ones that we manage ourselves, which is command executor 
for the Mesos containerizer, and a docker executor for the Docker 
containerizer, and we still allow custom executors via the API.


- Timothy


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


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-05 Thread Steve Niemitz


 On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
  How are we going to manage the duplication across the command executor and 
  the docker executor?
 
 Timothy Chen wrote:
 I think I'm going to leave them seperate as they most likely will grow 
 independent in tangent.
 The docker executor is solely responsible for launching docker 
 containers, and have different assumptions.
 
 Ben Mahler wrote:
 Is there a design doc about this docker architecture decision? It's a bit 
 hard to see the context for this, and how this will play out for other 
 executors that frameworks want to run inside docker, are you planning to 
 support that, or do users have to bake-in their executors..?

I'm curious about this too.  I'm having trouble understanding how other 
executors will run inside containers as they did previously after this change.  
I'd love to see a design doc about how this proposed system would work.


- Steve


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


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-03 Thread Bill Farner

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


Is there a shortcoming in the executor API that prevents this from being an 
independent binary?

- Bill Farner


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-03 Thread Timothy Chen


 On Feb. 3, 2015, 11:23 p.m., Bill Farner wrote:
  Is there a shortcoming in the executor API that prevents this from being an 
  independent binary?

Hi Bill, this is going to be a binary on itself just like command exexutor. Do 
you have something else in mind?


- Timothy


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


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-02-03 Thread Ben Mahler


 On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
  How are we going to manage the duplication across the command executor and 
  the docker executor?
 
 Timothy Chen wrote:
 I think I'm going to leave them seperate as they most likely will grow 
 independent in tangent.
 The docker executor is solely responsible for launching docker 
 containers, and have different assumptions.

Is there a design doc about this docker architecture decision? It's a bit hard 
to see the context for this, and how this will play out for other executors 
that frameworks want to run inside docker, are you planning to support that, or 
do users have to bake-in their executors..?


- Ben


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


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-16 Thread Timothy Chen

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

(Updated Jan. 17, 2015, 1:26 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Changes
---

rebased


Repository: mesos-git


Description
---

Add executor for docker containerizer, replaces the usage of command executor


Diffs (updated)
-

  src/Makefile.am 07bea1f 
  src/docker/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen



Re: Review Request 29329: Add executor for docker containerizer

2015-01-16 Thread Ben Mahler

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


How are we going to manage the duplication across the command executor and the 
docker executor?


src/docker/executor.cpp
https://reviews.apache.org/r/29329/#comment112834

Just passing by, this says the executor launches the container, but below 
it just looks like it waits for it to terminate via `docker wait`, or am I 
missing something?



src/docker/executor.cpp
https://reviews.apache.org/r/29329/#comment112833

Probably warrants a comment, what does `exit `...` do?`


- Ben Mahler


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-16 Thread Timothy Chen


 On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
  How are we going to manage the duplication across the command executor and 
  the docker executor?

I think I'm going to leave them seperate as they most likely will grow 
independent in tangent.
The docker executor is solely responsible for launching docker containers, and 
have different assumptions.


 On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
  src/docker/executor.cpp, lines 50-53
  https://reviews.apache.org/r/29329/diff/3/?file=824290#file824290line50
 
  Just passing by, this says the executor launches the container, but 
  below it just looks like it waits for it to terminate via `docker wait`, or 
  am I missing something?

Ah sorry, I think it's confusing that I later changed the executor to actually 
launch containers. Somehow I fixed the comment in this commit.


 On Jan. 17, 2015, 1:39 a.m., Ben Mahler wrote:
  src/docker/executor.cpp, line 103
  https://reviews.apache.org/r/29329/diff/3/?file=824290#file824290line103
 
  Probably warrants a comment, what does `exit `...` do?`

This is actually going to removed in a later commit. I posted my whole commit 
chain.


- Timothy


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


On Jan. 17, 2015, 1:26 a.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 17, 2015, 1:26 a.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 07bea1f 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-09 Thread Bernd Mathiske

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

Ship it!


Ship It!

- Bernd Mathiske


On Jan. 8, 2015, 5:20 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 8, 2015, 5:20 p.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am fc0c322 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-08 Thread Timothy Chen

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

(Updated Jan. 9, 2015, 1:20 a.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Repository: mesos-git


Description
---

Add executor for docker containerizer, replaces the usage of command executor


Diffs (updated)
-

  src/Makefile.am fc0c322 
  src/docker/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen



Re: Review Request 29329: Add executor for docker containerizer

2015-01-08 Thread Timothy Chen


 On Jan. 8, 2015, 11:27 a.m., Bernd Mathiske wrote:
  src/docker/executor.cpp, line 165
  https://reviews.apache.org/r/29329/diff/1/?file=798951#file798951line165
 
  This cryptic statement is worth a comment (maybe mention man waitpid? 
  Or mention higher up in this code that the status code is based on waitpid, 
  which is not immediately evident in this scope.). 
  
  Also maybe add some more output to increase human parsing ease. For 
  example: CHECK(WIFEXITED(s) || WIFSIGNALED(s))  status code:   s;
  
  (I know you just copied this particular stretch of code (from 
  executor.cpp). Is there a general rule that code can or should remain 
  suboptimal if copied? Please feel free to drop this issue if this is the 
  case.)

It is a general assumption across everywhere using subprocess, I can put it 
here again.


- Timothy


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


On Jan. 6, 2015, 10:16 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 6, 2015, 10:16 p.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-08 Thread Bernd Mathiske

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


What bug ID does this refer to?

Any tests? Does this work already?


src/docker/executor.cpp
https://reviews.apache.org/r/29329/#comment110858

The _ is unnecessary.



src/docker/executor.cpp
https://reviews.apache.org/r/29329/#comment111055

This is a major constraint that should be mentioned at the top of the file.



src/docker/executor.cpp
https://reviews.apache.org/r/29329/#comment111053

An important constraint/assumption like this should be mentioned at the top 
of the file.



src/docker/executor.cpp
https://reviews.apache.org/r/29329/#comment111049

This cryptic statement is worth a comment (maybe mention man waitpid? Or 
mention higher up in this code that the status code is based on waitpid, which 
is not immediately evident in this scope.). 

Also maybe add some more output to increase human parsing ease. For 
example: CHECK(WIFEXITED(s) || WIFSIGNALED(s))  status code:   s;

(I know you just copied this particular stretch of code (from 
executor.cpp). Is there a general rule that code can or should remain 
suboptimal if copied? Please feel free to drop this issue if this is the case.)



src/docker/executor.cpp
https://reviews.apache.org/r/29329/#comment111056

This stems from copied code. Any ideas how to do this properly?


- Bernd Mathiske


On Jan. 6, 2015, 2:16 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 6, 2015, 2:16 p.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-08 Thread Timothy Chen


 On Jan. 8, 2015, 11:27 a.m., Bernd Mathiske wrote:
  src/docker/executor.cpp, line 189
  https://reviews.apache.org/r/29329/diff/1/?file=798951#file798951line189
 
  This stems from copied code. Any ideas how to do this properly?

We probably need to add some visibility under libprocess, or perhaps a send 
that returns a future that we want wait on.
Either way, it's a TODO that should be fixed altogether with other executors in 
another review.


- Timothy


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


On Jan. 6, 2015, 10:16 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 6, 2015, 10:16 p.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-08 Thread Timothy Chen


 On Jan. 8, 2015, 11:27 a.m., Bernd Mathiske wrote:
  What bug ID does this refer to?
  
  Any tests? Does this work already?

This is a refactor and able to easier recover when the slave is running under a 
docker container.
All the docker tests inheriently test this executor since it's part of 
launching a docker containter.


- Timothy


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


On Jan. 6, 2015, 10:16 p.m., Timothy Chen wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29329/
 ---
 
 (Updated Jan. 6, 2015, 10:16 p.m.)
 
 
 Review request for mesos, Benjamin Hindman and Bernd Mathiske.
 
 
 Repository: mesos-git
 
 
 Description
 ---
 
 Add executor for docker containerizer, replaces the usage of command executor
 
 
 Diffs
 -
 
   src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
   src/docker/executor.cpp PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29329/diff/
 
 
 Testing
 ---
 
 make check
 
 
 Thanks,
 
 Timothy Chen
 




Re: Review Request 29329: Add executor for docker containerizer

2015-01-06 Thread Timothy Chen

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

(Updated Jan. 6, 2015, 10:16 p.m.)


Review request for mesos, Benjamin Hindman and Bernd Mathiske.


Repository: mesos-git


Description
---

Add executor for docker containerizer, replaces the usage of command executor


Diffs
-

  src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
  src/docker/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen



Review Request 29329: Add executor for docker containerizer

2014-12-22 Thread Timothy Chen

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

Review request for mesos and Benjamin Hindman.


Repository: mesos-git


Description
---

Add executor for docker containerizer, replaces the usage of command executor


Diffs
-

  src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
  src/docker/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen



Re: Review Request 29329: Add executor for docker containerizer

2014-12-22 Thread Timothy Chen

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

(Updated Dec. 22, 2014, 11:47 p.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos-git


Description
---

Add executor for docker containerizer, replaces the usage of command executor


Diffs
-

  src/Makefile.am 6f132b5a66e117c2a907763217bfafe1fce1b7a0 
  src/docker/executor.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Timothy Chen