You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Till Toenshoff <to...@me.com> on 2014/04/07 15:19:37 UTC
Review Request 20080: Introducing ContainerInfo as part of CommandInfo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
Repository: mesos-git
Description
-------
Introduces the ContainerInfo protobuf as part of CommandInfo.
Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
This will be needed for the ExternalContainerizer and possibly other containerizers as well.
Diffs
-----
include/mesos/mesos.proto 37f8a7f
src/slave/containerizer/mesos_containerizer.cpp c819c97
Diff: https://reviews.apache.org/r/20080/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review40123
-----------------------------------------------------------
Bad patch!
Reviews applied: [19795]
Failed command: git apply --index 19795.patch
Error:
error: patch failed: src/slave/http.cpp:144
error: src/slave/http.cpp: patch does not apply
error: patch failed: src/slave/slave.cpp:1550
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review40244
-----------------------------------------------------------
Ship it!
While I think it's worth clarifying the plan for ContainerInfo.image, I don't think that's a blocker for this review right now.
- Benjamin Hindman
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/
-----------------------------------------------------------
(Updated April 24, 2014, 11:02 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
Repository: mesos-git
Description
-------
Introduces the ContainerInfo protobuf as part of CommandInfo.
Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
This will be needed for the ExternalContainerizer and possibly other containerizers as well.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/slave/containerizer/mesos_containerizer.cpp 4a5dfa7
Diff: https://reviews.apache.org/r/20080/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review41404
-----------------------------------------------------------
Ship it!
Looks great! I will get this committed in a few.
- Niklas Nielsen
On April 24, 2014, 2:45 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 2:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp 4a5dfa7
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/
-----------------------------------------------------------
(Updated April 24, 2014, 9:45 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Added newline for coherent styling reasons.
Repository: mesos-git
Description
-------
Introduces the ContainerInfo protobuf as part of CommandInfo.
Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
This will be needed for the ExternalContainerizer and possibly other containerizers as well.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/slave/containerizer/mesos_containerizer.cpp 4a5dfa7
Diff: https://reviews.apache.org/r/20080/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/
-----------------------------------------------------------
(Updated April 24, 2014, 9:12 p.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Updated according to offline discussion on comment-line-lengths. Also added an explicit TODO to make sure that we do not forget to describe the container-info image and parameters in more detail.
Repository: mesos-git
Description
-------
Introduces the ContainerInfo protobuf as part of CommandInfo.
Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
This will be needed for the ExternalContainerizer and possibly other containerizers as well.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/slave/containerizer/mesos_containerizer.cpp 4a5dfa7
Diff: https://reviews.apache.org/r/20080/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/
-----------------------------------------------------------
(Updated April 24, 2014, 1:11 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Rebased.
Repository: mesos-git
Description
-------
Introduces the ContainerInfo protobuf as part of CommandInfo.
Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
This will be needed for the ExternalContainerizer and possibly other containerizers as well.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/slave/containerizer/mesos_containerizer.cpp 722f3fe
Diff: https://reviews.apache.org/r/20080/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/
-----------------------------------------------------------
(Updated April 11, 2014, 2:57 a.m.)
Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
Changes
-------
Addressed all comments.
Repository: mesos-git
Description
-------
Introduces the ContainerInfo protobuf as part of CommandInfo.
Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
This will be needed for the ExternalContainerizer and possibly other containerizers as well.
Diffs (updated)
-----
include/mesos/mesos.proto 37f8a7f
src/slave/containerizer/mesos_containerizer.cpp c819c97
Diff: https://reviews.apache.org/r/20080/diff/
Testing
-------
make check
Thanks,
Till Toenshoff
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
> On April 7, 2014, 11:57 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 445
> > <https://reviews.apache.org/r/20080/diff/2/?file=550552#file550552line445>
> >
> > For posterity I'd like you to add a comment here that explains why it's OK to return a Failure. In particular, we should be asking ourselves the question: how does the slave know that it should be sending TASK_FAILED or TASK_LOST for the tasks that would have been launched? In this case any subsequent 'wait' will return a failed future as well which the slave knows to treat as a "terminated" container. This makes me think that 'wait' should have really returned an Option<Termination> to distinguish the case when it doesn't know about a container ID! But that's for another review.
>
> Till Toenshoff wrote:
> Added a verbose comment.
Leaving the issue open until a follow-up RR has been submitted that introduces Option<Termination> on wait return.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39749
-----------------------------------------------------------
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
> On April 7, 2014, 11:57 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 445
> > <https://reviews.apache.org/r/20080/diff/2/?file=550552#file550552line445>
> >
> > For posterity I'd like you to add a comment here that explains why it's OK to return a Failure. In particular, we should be asking ourselves the question: how does the slave know that it should be sending TASK_FAILED or TASK_LOST for the tasks that would have been launched? In this case any subsequent 'wait' will return a failed future as well which the slave knows to treat as a "terminated" container. This makes me think that 'wait' should have really returned an Option<Termination> to distinguish the case when it doesn't know about a container ID! But that's for another review.
>
> Till Toenshoff wrote:
> Added a verbose comment.
>
> Till Toenshoff wrote:
> Leaving the issue open until a follow-up RR has been submitted that introduces Option<Termination> on wait return.
>
> Benjamin Hindman wrote:
> Okay, did you have a plan for how the external containerizer program should return an Option<Termination>? Does that work cleanly with the ExternalContainerizer?
Have posted a JIRA (https://issues.apache.org/jira/browse/MESOS-1243) on this for reference purposes. Will close this issue here instead.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39749
-----------------------------------------------------------
On April 24, 2014, 9:45 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 24, 2014, 9:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp 4a5dfa7
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
> On April 7, 2014, 11:57 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 445
> > <https://reviews.apache.org/r/20080/diff/2/?file=550552#file550552line445>
> >
> > For posterity I'd like you to add a comment here that explains why it's OK to return a Failure. In particular, we should be asking ourselves the question: how does the slave know that it should be sending TASK_FAILED or TASK_LOST for the tasks that would have been launched? In this case any subsequent 'wait' will return a failed future as well which the slave knows to treat as a "terminated" container. This makes me think that 'wait' should have really returned an Option<Termination> to distinguish the case when it doesn't know about a container ID! But that's for another review.
Added a verbose comment.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39749
-----------------------------------------------------------
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On April 7, 2014, 11:57 p.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, line 445
> > <https://reviews.apache.org/r/20080/diff/2/?file=550552#file550552line445>
> >
> > For posterity I'd like you to add a comment here that explains why it's OK to return a Failure. In particular, we should be asking ourselves the question: how does the slave know that it should be sending TASK_FAILED or TASK_LOST for the tasks that would have been launched? In this case any subsequent 'wait' will return a failed future as well which the slave knows to treat as a "terminated" container. This makes me think that 'wait' should have really returned an Option<Termination> to distinguish the case when it doesn't know about a container ID! But that's for another review.
>
> Till Toenshoff wrote:
> Added a verbose comment.
>
> Till Toenshoff wrote:
> Leaving the issue open until a follow-up RR has been submitted that introduces Option<Termination> on wait return.
Okay, did you have a plan for how the external containerizer program should return an Option<Termination>? Does that work cleanly with the ExternalContainerizer?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39749
-----------------------------------------------------------
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39749
-----------------------------------------------------------
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/20080/#comment72362>
For posterity I'd like you to add a comment here that explains why it's OK to return a Failure. In particular, we should be asking ourselves the question: how does the slave know that it should be sending TASK_FAILED or TASK_LOST for the tasks that would have been launched? In this case any subsequent 'wait' will return a failed future as well which the slave knows to treat as a "terminated" container. This makes me think that 'wait' should have really returned an Option<Termination> to distinguish the case when it doesn't know about a container ID! But that's for another review.
- Benjamin Hindman
On April 7, 2014, 1:19 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
> On April 7, 2014, 5:15 p.m., Ian Downes wrote:
> > src/slave/containerizer/mesos_containerizer.cpp, lines 438-441
> > <https://reviews.apache.org/r/20080/diff/2/?file=550552#file550552line438>
> >
> > How about being very specific:
> > "The slave should expose which containerization mechanism(s) it supports to avoid scheduling tasks that it cannot run."
Yes, that is much better. Thanks!
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39696
-----------------------------------------------------------
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39696
-----------------------------------------------------------
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/20080/#comment72282>
How about being very specific:
"The slave should expose which containerization mechanism(s) it supports to avoid scheduling tasks that it cannot run."
- Ian Downes
On April 7, 2014, 1:19 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On April 7, 2014, 11:44 p.m., Benjamin Hindman wrote:
> > include/mesos/mesos.proto, line 140
> > <https://reviews.apache.org/r/20080/diff/2/?file=550551#file550551line140>
> >
> > Eventually this will need lots more documentation, here or someplace else but here is a good source of truth.
> >
> > In the mean time, how about a caveat that not all containerizers currently implement ContainerInfo so it's possible that you'll launch a task with a container but it will fail.
> >
> > Also, can we give at least some explanation of 'image' and 'options'? Must 'image' be a URI? Etc.
>
> Till Toenshoff wrote:
> 'image' and 'options' are not really restricted in their exact scheme or even semantics. I did however add a comment that should help explaining this.
> Deimos does e.g. use 'image' this way: "docker:///ubuntu" as a scheme for pinning docker use and selecting a registered image.
If 'image' and 'options' do not have some well know semantics than it'll end up being a total PITA for end users. For example, if one external containerizer program (e.g., Deimos) decides it wants 'docker:///ubuntu' to describe an image but another external containerizer program wants 'docker-image:ubuntu' then we'll have to some how expose to the user which external containerize program is running so that they can conditionally "configure" their ContainerInfo. Ugh, it sounds likes this was not considered in advance. :(
My hunch is that we'll be able to define a generic enough scheme for describing containers using 'image' so we can leave it as is for now. Although, I'm less convinced about 'options' ... how is/does Deimos try and use it now?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39746
-----------------------------------------------------------
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
> On April 7, 2014, 11:44 p.m., Benjamin Hindman wrote:
> > include/mesos/mesos.proto, line 140
> > <https://reviews.apache.org/r/20080/diff/2/?file=550551#file550551line140>
> >
> > Eventually this will need lots more documentation, here or someplace else but here is a good source of truth.
> >
> > In the mean time, how about a caveat that not all containerizers currently implement ContainerInfo so it's possible that you'll launch a task with a container but it will fail.
> >
> > Also, can we give at least some explanation of 'image' and 'options'? Must 'image' be a URI? Etc.
'image' and 'options' are not really restricted in their exact scheme or even semantics. I did however add a comment that should help explaining this.
Deimos does e.g. use 'image' this way: "docker:///ubuntu" as a scheme for pinning docker use and selecting a registered image.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39746
-----------------------------------------------------------
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Jason Dusek <ja...@gmail.com>.
> On April 7, 2014, 11:44 p.m., Benjamin Hindman wrote:
> > include/mesos/mesos.proto, line 140
> > <https://reviews.apache.org/r/20080/diff/2/?file=550551#file550551line140>
> >
> > Eventually this will need lots more documentation, here or someplace else but here is a good source of truth.
> >
> > In the mean time, how about a caveat that not all containerizers currently implement ContainerInfo so it's possible that you'll launch a task with a container but it will fail.
> >
> > Also, can we give at least some explanation of 'image' and 'options'? Must 'image' be a URI? Etc.
>
> Till Toenshoff wrote:
> 'image' and 'options' are not really restricted in their exact scheme or even semantics. I did however add a comment that should help explaining this.
> Deimos does e.g. use 'image' this way: "docker:///ubuntu" as a scheme for pinning docker use and selecting a registered image.
>
> Benjamin Hindman wrote:
> If 'image' and 'options' do not have some well know semantics than it'll end up being a total PITA for end users. For example, if one external containerizer program (e.g., Deimos) decides it wants 'docker:///ubuntu' to describe an image but another external containerizer program wants 'docker-image:ubuntu' then we'll have to some how expose to the user which external containerize program is running so that they can conditionally "configure" their ContainerInfo. Ugh, it sounds likes this was not considered in advance. :(
>
> My hunch is that we'll be able to define a generic enough scheme for describing containers using 'image' so we can leave it as is for now. Although, I'm less convinced about 'options' ... how is/does Deimos try and use it now?
It has always been my understanding that image is restricted to being a URI. The points Ben raises are exactly right on.
Options can be understood as image settings. Deimos passes these options to Docker `run`, so they change how the image is launched; but they aren't intended to change Docker's behaviour more broadly than that. Notably, global Docker settings -- the Docker host, debugging level, &al -- can not be passed this way. In one of our clusters, we have Marathon pass bind mounts in the options field (["-v", "/etc/secret/stuff"]) for containers that need access to certain special credentials; so the options field is important in practice.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39746
-----------------------------------------------------------
On April 11, 2014, 2:57 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 11, 2014, 2:57 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39746
-----------------------------------------------------------
include/mesos/mesos.proto
<https://reviews.apache.org/r/20080/#comment72360>
Eventually this will need lots more documentation, here or someplace else but here is a good source of truth.
In the mean time, how about a caveat that not all containerizers currently implement ContainerInfo so it's possible that you'll launch a task with a container but it will fail.
Also, can we give at least some explanation of 'image' and 'options'? Must 'image' be a URI? Etc.
- Benjamin Hindman
On April 7, 2014, 1:19 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Till Toenshoff <to...@me.com>.
> On April 7, 2014, 2:50 p.m., Niklas Nielsen wrote:
> > Is this RR really dependent on r19795, r18403? It seems to me that we could land this independently (while the others are in flight).
>
> Benjamin Hindman wrote:
> I agree that this does not depend on 19795 or 18403, so let's remove those and commit this independently.
Those two implement TaskInfo as a parameter within the Containerizer context. Without TaskInfo, we dont have a CommandInfo in that scope. If I did not have a CommandInfo all the way down within the Containerizer, I could not fail launching that task depending on if the Containerizer supports ContainerInfo or not.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39674
-----------------------------------------------------------
On April 7, 2014, 1:19 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On April 7, 2014, 2:50 p.m., Niklas Nielsen wrote:
> > Is this RR really dependent on r19795, r18403? It seems to me that we could land this independently (while the others are in flight).
I agree that this does not depend on 19795 or 18403, so let's remove those and commit this independently.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39674
-----------------------------------------------------------
On April 7, 2014, 1:19 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39674
-----------------------------------------------------------
Is this RR really dependent on r19795, r18403? It seems to me that we could land this independently (while the others are in flight).
include/mesos/mesos.proto
<https://reviews.apache.org/r/20080/#comment72243>
s/ that may be used// ?
src/slave/containerizer/mesos_containerizer.cpp
<https://reviews.apache.org/r/20080/#comment72244>
Can we condense this a bit?
For example, "We should expose available containerization mechanisms so frameworks can avoid attempts to run unsupported containers."
Don't know if that got any better, but had to read the current comment a couple of times :)
- Niklas Nielsen
On April 7, 2014, 6:19 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 6:19 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 20080: Introducing ContainerInfo as part of CommandInfo
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/20080/#review39671
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19795, 18403, 20080]
All tests passed.
- Mesos ReviewBot
On April 7, 2014, 1:19 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/20080/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 1:19 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ian Downes, Niklas Nielsen, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Introduces the ContainerInfo protobuf as part of CommandInfo.
> Right now, if present, the mesos containerizer fails the task launch to point out that we do not support it on that containerizer.
>
> This will be needed for the ExternalContainerizer and possibly other containerizers as well.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 37f8a7f
> src/slave/containerizer/mesos_containerizer.cpp c819c97
>
> Diff: https://reviews.apache.org/r/20080/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>