You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2017/04/11 20:28:49 UTC
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated April 11, 2017, 8:28 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Rebase.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp 29a99f33e646593127b9dc126f398f7bca88e21d
src/slave/containerizer/mesos/containerizer.cpp bc611a5e085de10e9953b5f942d98f2b5747fce6
src/slave/containerizer/mesos/paths.hpp d85fd34660faacd9c73de2ba7b87b3bbd4b6007b
src/slave/containerizer/mesos/paths.cpp ed4bbd2491e71ad1e4a41e0575b514377d02da9b
Diff: https://reviews.apache.org/r/55334/diff/4/
Changes: https://reviews.apache.org/r/55334/diff/3-4/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review186195
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
Line 1050 (original), 1088 (patched)
<https://reviews.apache.org/r/55334/#comment262653>
Added a check for `config.isSome()` and a `warning` log line.
- Zhitao Li
On Sept. 26, 2017, 6:09 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 6:09 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp cc23b4d91be16fc95a131c09d07378b801e34d6f
> src/slave/containerizer/mesos/containerizer.cpp 4d5dc13f363f5d8886983d7dd06a5cecc177c345
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/8/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review191124
-----------------------------------------------------------
Ship it!
Could you rebase this patch? Cannot apply.
- Gilbert Song
On Nov. 10, 2017, 11:30 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Nov. 10, 2017, 11:30 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/15/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review191282
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Nov. 15, 2017, 8:33 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Nov. 15, 2017, 8:33 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate
> whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp f5d5146de9abdd9804ede6a4b17a965a4b27c05a
> src/slave/containerizer/mesos/containerizer.cpp db5f044f7415386c94e4930f365dfc14d403414a
> src/slave/containerizer/mesos/paths.hpp 7b67ccf670d991cbc339d74ebf4fccf3ae9a98a8
> src/slave/containerizer/mesos/paths.cpp 23f1fee3667aa48e8c31c15e0b69201268e44d37
>
>
> Diff: https://reviews.apache.org/r/55334/diff/16/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Nov. 15, 2017, 8:33 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Fixed the comment message < 72 char.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description (updated)
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate
whether recovered.
Diffs
-----
src/slave/containerizer/mesos/containerizer.hpp f5d5146de9abdd9804ede6a4b17a965a4b27c05a
src/slave/containerizer/mesos/containerizer.cpp db5f044f7415386c94e4930f365dfc14d403414a
src/slave/containerizer/mesos/paths.hpp 7b67ccf670d991cbc339d74ebf4fccf3ae9a98a8
src/slave/containerizer/mesos/paths.cpp 23f1fee3667aa48e8c31c15e0b69201268e44d37
Diff: https://reviews.apache.org/r/55334/diff/16/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Nov. 16, 2017, 3:58 a.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Rebase after standalone container support is added.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp f5d5146de9abdd9804ede6a4b17a965a4b27c05a
src/slave/containerizer/mesos/containerizer.cpp db5f044f7415386c94e4930f365dfc14d403414a
src/slave/containerizer/mesos/paths.hpp 7b67ccf670d991cbc339d74ebf4fccf3ae9a98a8
src/slave/containerizer/mesos/paths.cpp 23f1fee3667aa48e8c31c15e0b69201268e44d37
Diff: https://reviews.apache.org/r/55334/diff/16/
Changes: https://reviews.apache.org/r/55334/diff/15-16/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Nov. 10, 2017, 7:30 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/15/
Changes: https://reviews.apache.org/r/55334/diff/14-15/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Nov. 10, 2017, 7:03 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description (updated)
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/14/
Changes: https://reviews.apache.org/r/55334/diff/13-14/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review190212
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/containerizer.cpp
Lines 777 (patched)
<https://reviews.apache.org/r/55334/#comment267521>
Let's use VLOG(1) instead since there might be a lot of containers.
src/slave/containerizer/mesos/containerizer.cpp
Line 807 (original), 839-842 (patched)
<https://reviews.apache.org/r/55334/#comment267520>
Seems like you forget to remove these.
src/slave/containerizer/mesos/containerizer.cpp
Lines 878 (patched)
<https://reviews.apache.org/r/55334/#comment267522>
ditto.
src/slave/containerizer/mesos/paths.hpp
Lines 48-63 (original), 48-63 (patched)
<https://reviews.apache.org/r/55334/#comment267503>
could you update this comment?
- Gilbert Song
On Oct. 27, 2017, 11 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2017, 11 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
> src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/13/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Oct. 27, 2017, 6 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/13/
Changes: https://reviews.apache.org/r/55334/diff/12-13/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Oct. 27, 2017, 5:57 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Review comments.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
src/slave/containerizer/mesos/containerizer.cpp 100e3bbda543d87808da9ff6bea42da5099ea8c5
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/12/
Changes: https://reviews.apache.org/r/55334/diff/11-12/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Gilbert Song <so...@gmail.com>.
> On Oct. 26, 2017, 12:33 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1113-1120 (original), 1152-1164 (patched)
> > <https://reviews.apache.org/r/55334/diff/11/?file=1863851#file1863851line1152>
> >
> > After looking at the slave code. it seems to me we can remove the whole part and the TODO.
> >
> > Not yours. this was from a tech debt when we combined the launch() method https://github.com/apache/mesos/commit/17ffb97ae7eda78edf85b204eba35bc59649a479
>
> Zhitao Li wrote:
> Are you suggesting that a nested container's config does not need `executor_info` anymore?
no, we already set it in slave.cpp before calling `containerizer::launch()`.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review188981
-----------------------------------------------------------
On Oct. 19, 2017, 9:41 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2017, 9:41 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
> src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/11/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
> On Oct. 26, 2017, 7:33 p.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 1113-1120 (original), 1152-1164 (patched)
> > <https://reviews.apache.org/r/55334/diff/11/?file=1863851#file1863851line1152>
> >
> > After looking at the slave code. it seems to me we can remove the whole part and the TODO.
> >
> > Not yours. this was from a tech debt when we combined the launch() method https://github.com/apache/mesos/commit/17ffb97ae7eda78edf85b204eba35bc59649a479
Are you suggesting that a nested container's config does not need `executor_info` anymore?
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review188981
-----------------------------------------------------------
On Oct. 20, 2017, 4:41 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2017, 4:41 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
> src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/11/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review188981
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/containerizer.cpp
Lines 757-758 (patched)
<https://reviews.apache.org/r/55334/#comment266344>
we don't quote the containerID since it was generated by the agent or executor, not by users.
src/slave/containerizer/mesos/containerizer.cpp
Lines 764-765 (patched)
<https://reviews.apache.org/r/55334/#comment266345>
ditto.
src/slave/containerizer/mesos/containerizer.cpp
Lines 764-765 (patched)
<https://reviews.apache.org/r/55334/#comment266348>
how about:
```
LOG(INFO) << "No container config is recovered for container " << containerId
<< ", image pruning will be disabled.";
```
src/slave/containerizer/mesos/containerizer.cpp
Lines 765 (patched)
<https://reviews.apache.org/r/55334/#comment266341>
we don't have a period at the end for `LOG(INFO)`.
src/slave/containerizer/mesos/containerizer.cpp
Line 793 (original), 811 (patched)
<https://reviews.apache.org/r/55334/#comment266350>
not yours. mind add the containerID here?
src/slave/containerizer/mesos/containerizer.cpp
Lines 819 (patched)
<https://reviews.apache.org/r/55334/#comment266349>
add containerID?
src/slave/containerizer/mesos/containerizer.cpp
Lines 823 (patched)
<https://reviews.apache.org/r/55334/#comment266352>
do we need to mention image pruning will be disabled?
src/slave/containerizer/mesos/containerizer.cpp
Lines 860 (patched)
<https://reviews.apache.org/r/55334/#comment266354>
s/config/container config/g
src/slave/containerizer/mesos/containerizer.cpp
Lines 861 (patched)
<https://reviews.apache.org/r/55334/#comment266353>
remove `this means`.
src/slave/containerizer/mesos/containerizer.cpp
Lines 862 (patched)
<https://reviews.apache.org/r/55334/#comment266342>
ditto.
src/slave/containerizer/mesos/containerizer.cpp
Lines 1113-1120 (original), 1152-1164 (patched)
<https://reviews.apache.org/r/55334/#comment266337>
After looking at the slave code. it seems to me we can remove the whole part and the TODO.
Not yours. this was from a tech debt when we combined the launch() method https://github.com/apache/mesos/commit/17ffb97ae7eda78edf85b204eba35bc59649a479
src/slave/containerizer/mesos/containerizer.cpp
Lines 1163 (patched)
<https://reviews.apache.org/r/55334/#comment266343>
ditto.
src/slave/containerizer/mesos/containerizer.cpp
Lines 1271 (patched)
<https://reviews.apache.org/r/55334/#comment266338>
could we move it below `CHECK_EQ(container->state, PROVISIONING);`?
src/slave/containerizer/mesos/containerizer.cpp
Lines 1321 (patched)
<https://reviews.apache.org/r/55334/#comment266339>
good catch/
src/slave/containerizer/mesos/paths.cpp
Lines 279-280 (original), 282-283 (patched)
<https://reviews.apache.org/r/55334/#comment266002>
mind add the containerID?
src/slave/containerizer/mesos/paths.cpp
Lines 310-311 (patched)
<https://reviews.apache.org/r/55334/#comment266003>
ditto.
src/slave/containerizer/mesos/paths.cpp
Lines 467-468 (patched)
<https://reviews.apache.org/r/55334/#comment265921>
extra newline?
- Gilbert Song
On Oct. 19, 2017, 9:41 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2017, 9:41 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
> src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/11/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Oct. 20, 2017, 4:41 a.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs
-----
src/slave/containerizer/mesos/containerizer.hpp 6d356ccf82f36df8c6f558fb0ace7d9f982a3d6b
src/slave/containerizer/mesos/containerizer.cpp 78fdd21f8b7ede4beedff31ba2b488ffebd4ea31
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/11/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Oct. 9, 2017, 11:13 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Move checkpointing after `provisioner::provision()` finishes.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp cc23b4d91be16fc95a131c09d07378b801e34d6f
src/slave/containerizer/mesos/containerizer.cpp 4d5dc13f363f5d8886983d7dd06a5cecc177c345
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/10/
Changes: https://reviews.apache.org/r/55334/diff/9-10/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review187225
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
Lines 1186-1202 (patched)
<https://reviews.apache.org/r/55334/#comment264161>
@gilbert, I realized one thing: should this be moved to or duplicated in `containerizer::prepare()`, after the `provisionInfo` is returned from `provisioner::provision()` finishes? In the chained callback there, `container->config` could be updated one more time if some rootfs is provisioned.
- Zhitao Li
On Sept. 26, 2017, 7:09 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 7:09 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp cc23b4d91be16fc95a131c09d07378b801e34d6f
> src/slave/containerizer/mesos/containerizer.cpp 4d5dc13f363f5d8886983d7dd06a5cecc177c345
> src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/9/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Sept. 26, 2017, 7:09 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Added `CHECK_SOME`
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp cc23b4d91be16fc95a131c09d07378b801e34d6f
src/slave/containerizer/mesos/containerizer.cpp 4d5dc13f363f5d8886983d7dd06a5cecc177c345
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/9/
Changes: https://reviews.apache.org/r/55334/diff/8-9/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Sept. 26, 2017, 6:09 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Rebase with Gilbert's comments addressed.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp cc23b4d91be16fc95a131c09d07378b801e34d6f
src/slave/containerizer/mesos/containerizer.cpp 4d5dc13f363f5d8886983d7dd06a5cecc177c345
src/slave/containerizer/mesos/paths.hpp a03f15e01e8eb8a1326baad8d7db96a79d785482
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/8/
Changes: https://reviews.apache.org/r/55334/diff/7-8/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/#review186159
-----------------------------------------------------------
could you rebase this chain? sorry for the delay, but thank you, Zhitao!
src/slave/containerizer/mesos/containerizer.cpp
Lines 689 (patched)
<https://reviews.apache.org/r/55334/#comment262616>
log the containerId?
src/slave/containerizer/mesos/containerizer.cpp
Lines 695 (patched)
<https://reviews.apache.org/r/55334/#comment262619>
should we use `WARNING` here? since everytime the containerizer recovers, it might log a bunch of warning if there are many legacy containers running.
src/slave/containerizer/mesos/containerizer.cpp
Lines 696 (patched)
<https://reviews.apache.org/r/55334/#comment262620>
do we still plan to back fill?
src/slave/containerizer/mesos/containerizer.cpp
Lines 755 (patched)
<https://reviews.apache.org/r/55334/#comment262622>
ditto.
src/slave/containerizer/mesos/containerizer.cpp
Lines 792-794 (patched)
<https://reviews.apache.org/r/55334/#comment262637>
identify this log for both executor container and nested container?
src/slave/containerizer/mesos/containerizer.cpp
Line 1050 (original), 1088 (patched)
<https://reviews.apache.org/r/55334/#comment262639>
this may cause segfault, right?
we should consider address this TODO in a separate patch in favor of checkpointing the `ContainerConfig`.
src/slave/containerizer/mesos/containerizer.cpp
Lines 1114-1117 (patched)
<https://reviews.apache.org/r/55334/#comment262612>
How about:
Checkpoint the `ContainerConfig` which includes all information to launch a container. Critical information (e.g., `ContainerInfo`) can be used for tracking container image usage.
src/slave/containerizer/mesos/containerizer.cpp
Lines 1130 (patched)
<https://reviews.apache.org/r/55334/#comment262617>
quote the path?
src/slave/containerizer/mesos/containerizer.cpp
Line 1191 (original), 1247 (patched)
<https://reviews.apache.org/r/55334/#comment262613>
add a `CHECK_SOME()` in the very beginning of `prepare()` funtion?
src/slave/containerizer/mesos/containerizer.cpp
Line 1486 (original), 1545 (patched)
<https://reviews.apache.org/r/55334/#comment262614>
fix the indentation (extra space)?
src/slave/containerizer/mesos/paths.cpp
Lines 438 (patched)
<https://reviews.apache.org/r/55334/#comment262609>
Move below `getContainerTermination()`?
src/slave/containerizer/mesos/paths.cpp
Lines 449 (patched)
<https://reviews.apache.org/r/55334/#comment262610>
quote the `path`?
and add `containerId`?
src/slave/containerizer/mesos/paths.cpp
Lines 458 (patched)
<https://reviews.apache.org/r/55334/#comment262611>
permutate the space to the line above?
mind fix getContainerTermination() as well?
- Gilbert Song
On Aug. 1, 2017, 10:10 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55334/
> -----------------------------------------------------------
>
> (Updated Aug. 1, 2017, 10:10 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-6894
> https://issues.apache.org/jira/browse/MESOS-6894
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch includes the following change:
> - Checkpointed `ContainerConfig` used to launch a container;
> - Added helper function to read checkpointed `ContainerConfig`;
> - Recovered `ContainerConfig`.
> - Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/containerizer.hpp fd586306f7a6d3d2cdb58481f0ef4906de8d0f88
> src/slave/containerizer/mesos/containerizer.cpp 6f100b516f2750732acaed693f7023b1e44b39d9
> src/slave/containerizer/mesos/paths.hpp 12ae7c7329f9e8d334cb32c30cc38465bddc046c
> src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
>
>
> Diff: https://reviews.apache.org/r/55334/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated Aug. 1, 2017, 5:10 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Rebase.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp fd586306f7a6d3d2cdb58481f0ef4906de8d0f88
src/slave/containerizer/mesos/containerizer.cpp 6f100b516f2750732acaed693f7023b1e44b39d9
src/slave/containerizer/mesos/paths.hpp 12ae7c7329f9e8d334cb32c30cc38465bddc046c
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/7/
Changes: https://reviews.apache.org/r/55334/diff/6-7/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 55334: Checkpoint and recover `ContainerConfig` in
Mesos containerizer.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55334/
-----------------------------------------------------------
(Updated May 31, 2017, 5:57 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Changes
-------
Rebase.
Bugs: MESOS-6894
https://issues.apache.org/jira/browse/MESOS-6894
Repository: mesos
Description
-------
This patch includes the following change:
- Checkpointed `ContainerConfig` used to launch a container;
- Added helper function to read checkpointed `ContainerConfig`;
- Recovered `ContainerConfig`.
- Make `ContainerConfig` Option in `Container` struct to indicate whether recovered.
Diffs (updated)
-----
src/slave/containerizer/mesos/containerizer.hpp ea0691945d3450fcc336df03d9cf7b48afbd8b3d
src/slave/containerizer/mesos/containerizer.cpp f3e6210eccd4a6b445ffd4447e69526d424ea36d
src/slave/containerizer/mesos/paths.hpp 12ae7c7329f9e8d334cb32c30cc38465bddc046c
src/slave/containerizer/mesos/paths.cpp 0c61c20c345a327ec469b382558aaeed0280e754
Diff: https://reviews.apache.org/r/55334/diff/5/
Changes: https://reviews.apache.org/r/55334/diff/4-5/
Testing
-------
Thanks,
Zhitao Li