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/10/16 23:25:33 UTC
Re: Review Request 60888: Added recovery logic for standalone
containers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/
-----------------------------------------------------------
(Updated Oct. 16, 2017, 4:25 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Rebased and corrected two things:
1) Nothing created the standalone marker file.
2) During recovery, the containerizer needs to rebuild the standalone container's sandbox directory.
Summary (updated)
-----------------
Added recovery logic for standalone containers.
Bugs: MESOS-7305
https://issues.apache.org/jira/browse/MESOS-7305
Repository: mesos
Description (updated)
-------
Although there is no way to launch standalone containers yet,
this commit outlines the expected layout of container metadata
which should be populated when launching standalone containers.
The layout is fairly simple, as standalone containers have no
framework, executor, or tasks to worry about. The sandbox directory
will live under a new top-level directory `containers` and there is
no metadata to checkpoint at the moment.
The containerizer will checkpoint a marker file (in the runtime
directory) so that it knows to recover all standalone containers.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
Diff: https://reviews.apache.org/r/60888/diff/3/
Changes: https://reviews.apache.org/r/60888/diff/2-3/
Testing
-------
See next patch and later ones too.
Thanks,
Joseph Wu
Re: Review Request 60888: Added recovery logic for standalone
containers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/#review190388
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Nov. 2, 2017, 3:43 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2017, 3:43 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
>
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about. The sandbox directory
> will live under a new top-level directory `containers` and there is
> no metadata to checkpoint at the moment.
>
> The containerizer will checkpoint a marker file (in the runtime
> directory) so that it knows to recover all standalone containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
> src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
> src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
>
>
> Diff: https://reviews.apache.org/r/60888/diff/4/
>
>
> Testing
> -------
>
> See next patch and later ones too.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 60888: Added recovery logic for standalone
containers.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/
-----------------------------------------------------------
(Updated Nov. 2, 2017, 8:43 a.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Fixed recovery for nested containers launched underneath standalone containers. These nested containers were not being marked as `isRecoverableNestedContainer = true` because the set of "alive" containers is determined from the `SlaveState`, which does not include standalone containers. To fix this, the `alive` hashset was removed, because, for the purpose of finding the top-level container, the existing `containers_` hashmap is sufficient.
Bugs: MESOS-7305
https://issues.apache.org/jira/browse/MESOS-7305
Repository: mesos
Description
-------
Although there is no way to launch standalone containers yet,
this commit outlines the expected layout of container metadata
which should be populated when launching standalone containers.
The layout is fairly simple, as standalone containers have no
framework, executor, or tasks to worry about. The sandbox directory
will live under a new top-level directory `containers` and there is
no metadata to checkpoint at the moment.
The containerizer will checkpoint a marker file (in the runtime
directory) so that it knows to recover all standalone containers.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
Diff: https://reviews.apache.org/r/60888/diff/4/
Changes: https://reviews.apache.org/r/60888/diff/3-4/
Testing
-------
See next patch and later ones too.
Thanks,
Joseph Wu
Re: Review Request 60888: Added recovery logic for standalone
containers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Oct. 17, 2017, 10:42 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 850 (patched)
> > <https://reviews.apache.org/r/60888/diff/3/?file=1858652#file1858652line852>
> >
> > Some isolator might be surprised by standalone containers. Is there a patch in this chain that checks if an isolator is standalone container capable?
Not in the current chain. But this is one of the next things for me to do.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/#review188345
-----------------------------------------------------------
On Oct. 16, 2017, 4:25 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 4:25 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
>
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about. The sandbox directory
> will live under a new top-level directory `containers` and there is
> no metadata to checkpoint at the moment.
>
> The containerizer will checkpoint a marker file (in the runtime
> directory) so that it knows to recover all standalone containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
> src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
> src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
>
>
> Diff: https://reviews.apache.org/r/60888/diff/3/
>
>
> Testing
> -------
>
> See next patch and later ones too.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 60888: Added recovery logic for standalone
containers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/#review188345
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
Lines 850 (patched)
<https://reviews.apache.org/r/60888/#comment265327>
Some isolator might be surprised by standalone containers. Is there a patch in this chain that checks if an isolator is standalone container capable?
- Jie Yu
On Oct. 16, 2017, 11:25 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 11:25 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
>
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about. The sandbox directory
> will live under a new top-level directory `containers` and there is
> no metadata to checkpoint at the moment.
>
> The containerizer will checkpoint a marker file (in the runtime
> directory) so that it knows to recover all standalone containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
> src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
> src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
>
>
> Diff: https://reviews.apache.org/r/60888/diff/3/
>
>
> Testing
> -------
>
> See next patch and later ones too.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 60888: Added recovery logic for standalone
containers.
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 17, 2017, 5:39 p.m., Jie Yu wrote:
> > src/slave/paths.hpp
> > Lines 53-54 (patched)
> > <https://reviews.apache.org/r/60888/diff/3/?file=1858655#file1858655line53>
> >
> > Putting this path here is a bit wierd because this directory is kind of containerizer specific.
> >
> > I'd suggest we create a `src/slave/containerizer/paths.hpp` to put those path helper functions.
> >
> > `src/slave/paths.hpp` should probably only have paths that are related to slave, but not containers. In the future, the sandbox for executors can be a symlink to the actual container sandbox.
>
> Joseph Wu wrote:
> Leaving this open as I didn't change the directory structure...
>
> Sandbox directories (for normal and standalone containers) are currently specified by the agent, so it makes sense to have this directory in the agent's helper. The containerizer only specifies sandboxes for nested containers by appending onto an existing sandbox (which itself is directly or indirectly (for multi-level nested containers) specified by the agent).
ok, fair enough. I think we can revisit this once we want to support just running containerizer alone without the agent.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/#review188334
-----------------------------------------------------------
On Nov. 2, 2017, 3:43 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2017, 3:43 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
>
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about. The sandbox directory
> will live under a new top-level directory `containers` and there is
> no metadata to checkpoint at the moment.
>
> The containerizer will checkpoint a marker file (in the runtime
> directory) so that it knows to recover all standalone containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
> src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
> src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
>
>
> Diff: https://reviews.apache.org/r/60888/diff/4/
>
>
> Testing
> -------
>
> See next patch and later ones too.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 60888: Added recovery logic for standalone
containers.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Oct. 17, 2017, 10:39 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Line 757 (original), 758 (patched)
> > <https://reviews.apache.org/r/60888/diff/3/?file=1858652#file1858652line758>
> >
> > not yours, can you add a comment here saying that parent containerid will be guaranteed to appear first in the vector. This is important for the following code to always be able to find the parent container information for a nested container.
The ordering did not appear to matter for nested containers in the past, but now it does. The note I added explains why.
> On Oct. 17, 2017, 10:39 a.m., Jie Yu wrote:
> > src/slave/paths.hpp
> > Lines 53-54 (patched)
> > <https://reviews.apache.org/r/60888/diff/3/?file=1858655#file1858655line53>
> >
> > Putting this path here is a bit wierd because this directory is kind of containerizer specific.
> >
> > I'd suggest we create a `src/slave/containerizer/paths.hpp` to put those path helper functions.
> >
> > `src/slave/paths.hpp` should probably only have paths that are related to slave, but not containers. In the future, the sandbox for executors can be a symlink to the actual container sandbox.
Leaving this open as I didn't change the directory structure...
Sandbox directories (for normal and standalone containers) are currently specified by the agent, so it makes sense to have this directory in the agent's helper. The containerizer only specifies sandboxes for nested containers by appending onto an existing sandbox (which itself is directly or indirectly (for multi-level nested containers) specified by the agent).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/#review188334
-----------------------------------------------------------
On Nov. 2, 2017, 8:43 a.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> -----------------------------------------------------------
>
> (Updated Nov. 2, 2017, 8:43 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
>
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about. The sandbox directory
> will live under a new top-level directory `containers` and there is
> no metadata to checkpoint at the moment.
>
> The containerizer will checkpoint a marker file (in the runtime
> directory) so that it knows to recover all standalone containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
> src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
> src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
>
>
> Diff: https://reviews.apache.org/r/60888/diff/4/
>
>
> Testing
> -------
>
> See next patch and later ones too.
>
>
> Thanks,
>
> Joseph Wu
>
>
Re: Review Request 60888: Added recovery logic for standalone
containers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60888/#review188334
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/containerizer.cpp
Line 757 (original), 758 (patched)
<https://reviews.apache.org/r/60888/#comment265324>
not yours, can you add a comment here saying that parent containerid will be guaranteed to appear first in the vector. This is important for the following code to always be able to find the parent container information for a nested container.
src/slave/paths.hpp
Lines 53-54 (patched)
<https://reviews.apache.org/r/60888/#comment265321>
Putting this path here is a bit wierd because this directory is kind of containerizer specific.
I'd suggest we create a `src/slave/containerizer/paths.hpp` to put those path helper functions.
`src/slave/paths.hpp` should probably only have paths that are related to slave, but not containers. In the future, the sandbox for executors can be a symlink to the actual container sandbox.
- Jie Yu
On Oct. 16, 2017, 11:25 p.m., Joseph Wu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60888/
> -----------------------------------------------------------
>
> (Updated Oct. 16, 2017, 11:25 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-7305
> https://issues.apache.org/jira/browse/MESOS-7305
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Although there is no way to launch standalone containers yet,
> this commit outlines the expected layout of container metadata
> which should be populated when launching standalone containers.
>
> The layout is fairly simple, as standalone containers have no
> framework, executor, or tasks to worry about. The sandbox directory
> will live under a new top-level directory `containers` and there is
> no metadata to checkpoint at the moment.
>
> The containerizer will checkpoint a marker file (in the runtime
> directory) so that it knows to recover all standalone containers.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
> src/slave/paths.hpp f000508d414daf9f943561f89c7105503a8a98b3
> src/slave/paths.cpp fd546525b900cb6524fb9196d19616ef18de0f30
>
>
> Diff: https://reviews.apache.org/r/60888/diff/3/
>
>
> Testing
> -------
>
> See next patch and later ones too.
>
>
> Thanks,
>
> Joseph Wu
>
>