You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Joseph Wu <jo...@mesosphere.io> on 2017/05/02 02:01:03 UTC
Review Request 58899: Combined containerizer interface's launch
methods.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58899/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Bugs: MESOS-7449
https://issues.apache.org/jira/browse/MESOS-7449
Repository: mesos
Description
-------
When nested container support was added, we added a separate `launch`
path in the containerizer because nested containers do not need
an explicit TaskInfo/ExecutorInfo. Nested containers basically
only need the CommandInfo and ContainerInfo.
This commit combines the two launch methods by replacing most of the
"Infos" (Task, Executor, Command, Container) with a `ContainerConfig`
argument, which may contain multiple combinations of the "Infos".
The goal is to support three launch paths for containers:
1) When the `ContainerConfig` contains a TaskInfo/ExecutorInfo,
launch a task or executor.
2) When the `ContainerID` has a parent, launch a nested container.
3) (Not implemented yet) When there is no TaskInfo/ExecutorInfo or
parent container, launch a standalone container.
There are two other notable changes to the interface:
* The `SlaveID` field has been removed entirely. The code that
requires this (in the fetcher and Docker containerizer) will be
addressed in a separate commit.
* The `checkpoint` bool has been replaced by an Option<string>,
which contains the path that should be used for checkpointing.
This path includes the filename.
This is also one of the reasons why `SlaveID` was an argument.
Diffs
-----
src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
Diff: https://reviews.apache.org/r/58899/diff/1/
Testing
-------
See last patch in chain.
Thanks,
Joseph Wu
Re: Review Request 58899: Combined containerizer interface's launch
methods.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On May 11, 2017, 3:19 a.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 85-86 (original), 85-86 (patched)
> > <https://reviews.apache.org/r/58899/diff/1/?file=1705355#file1705355line85>
> >
> > Can we make the field in the protobuf optional and add this note there as well?
`ContainerConfig` is currently the public interface between containerizer and isolator. The isolator(s) definitely expect the `directory` field to be present, so I feel it is more appropriate to leave the `directory` as required.
I'll include some protobuf commenting additions in a subsequent patch (those comments will be applicable with or without this patch chain, for the most part).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58899/#review174626
-----------------------------------------------------------
On May 1, 2017, 7:01 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58899/
> -----------------------------------------------------------
>
> (Updated May 1, 2017, 7:01 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7449
> https://issues.apache.org/jira/browse/MESOS-7449
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When nested container support was added, we added a separate `launch`
> path in the containerizer because nested containers do not need
> an explicit TaskInfo/ExecutorInfo. Nested containers basically
> only need the CommandInfo and ContainerInfo.
>
> This commit combines the two launch methods by replacing most of the
> "Infos" (Task, Executor, Command, Container) with a `ContainerConfig`
> argument, which may contain multiple combinations of the "Infos".
>
> The goal is to support three launch paths for containers:
> 1) When the `ContainerConfig` contains a TaskInfo/ExecutorInfo,
> launch a task or executor.
> 2) When the `ContainerID` has a parent, launch a nested container.
> 3) (Not implemented yet) When there is no TaskInfo/ExecutorInfo or
> parent container, launch a standalone container.
>
> There are two other notable changes to the interface:
> * The `SlaveID` field has been removed entirely. The code that
> requires this (in the fetcher and Docker containerizer) will be
> addressed in a separate commit.
> * The `checkpoint` bool has been replaced by an Option<string>,
> which contains the path that should be used for checkpointing.
> This path includes the filename.
> This is also one of the reasons why `SlaveID` was an argument.
>
>
> Diffs
> -----
>
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
>
>
> Diff: https://reviews.apache.org/r/58899/diff/1/
>
>
> Testing
> -------
>
> See last patch in chain.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 58899: Combined containerizer interface's launch
methods.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58899/#review174626
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/containerizer.hpp
Lines 85-86 (original), 85-86 (patched)
<https://reviews.apache.org/r/58899/#comment247827>
Can we make the field in the protobuf optional and add this note there as well?
- Jie Yu
On May 2, 2017, 2:01 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58899/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 2:01 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-7449
> https://issues.apache.org/jira/browse/MESOS-7449
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When nested container support was added, we added a separate `launch`
> path in the containerizer because nested containers do not need
> an explicit TaskInfo/ExecutorInfo. Nested containers basically
> only need the CommandInfo and ContainerInfo.
>
> This commit combines the two launch methods by replacing most of the
> "Infos" (Task, Executor, Command, Container) with a `ContainerConfig`
> argument, which may contain multiple combinations of the "Infos".
>
> The goal is to support three launch paths for containers:
> 1) When the `ContainerConfig` contains a TaskInfo/ExecutorInfo,
> launch a task or executor.
> 2) When the `ContainerID` has a parent, launch a nested container.
> 3) (Not implemented yet) When there is no TaskInfo/ExecutorInfo or
> parent container, launch a standalone container.
>
> There are two other notable changes to the interface:
> * The `SlaveID` field has been removed entirely. The code that
> requires this (in the fetcher and Docker containerizer) will be
> addressed in a separate commit.
> * The `checkpoint` bool has been replaced by an Option<string>,
> which contains the path that should be used for checkpointing.
> This path includes the filename.
> This is also one of the reasons why `SlaveID` was an argument.
>
>
> Diffs
> -----
>
> src/slave/containerizer/containerizer.hpp 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41
>
>
> Diff: https://reviews.apache.org/r/58899/diff/1/
>
>
> Testing
> -------
>
> See last patch in chain.
>
>
> Thanks,
>
> Joseph Wu
>
>