You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Timothy Chen <tn...@apache.org> on 2015/05/01 23:43:10 UTC
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 1, 2015, 9:43 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 93c7c8a
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/containerizer/docker.hpp b25ec55
src/slave/containerizer/docker.cpp f9fc89a
src/slave/flags.hpp d3b1ce1
src/slave/flags.cpp d0932b0
src/tests/docker_containerizer_tests.cpp c9d66b3
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review83952
-----------------------------------------------------------
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment135009>
Let's use Flags here so that we don't have to manually construct the command string.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment135012>
then(defer(self(), [=]() { return launchDockerRun(containerId, containerName); }))
then(defer(self(), &Self::launchDockerRun, containerId, containerName))
then(defer(self(), lambda::bind(&Self::launchDockerRun, containerId, containerName)))
launchDockerRun(...)
{
...
return docker->run();
}
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment135018>
const&
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment135017>
ASSERT_SOME(read);
vector<string> lines = strings::split(read.get());
EXPECT_TRUE(Collection(lines).exists([=](const string& line) { return line == "err" + uuid; });
EXPECT_TRUE(Collection(lines).exists(line));
EXPECT_TRUE(containsLine(lines, "err" + uuid));
Collection(strings::split(read.get()))
.exists([](const string& line) {
return line == ("err" + uuid);
});
- Benjamin Hindman
On May 14, 2015, 9:39 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 9:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp ca7cc13
> src/slave/flags.cpp f35c76a
> src/tests/docker_containerizer_tests.cpp 154bf98
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review83956
-----------------------------------------------------------
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment135013>
change container to containerName
- Timothy Chen
On May 14, 2015, 9:39 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 9:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp ca7cc13
> src/slave/flags.cpp f35c76a
> src/tests/docker_containerizer_tests.cpp 154bf98
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review84892
-----------------------------------------------------------
Bad patch!
Reviews applied: [29327]
Failed command: ./support/apply-review.sh -n -r 29327
Error:
2015-05-22 08:39:00 URL:https://reviews.apache.org/r/29327/diff/raw/ [2935/2935] -> "29327.patch" [1]
error: patch failed: src/slave/containerizer/docker.cpp:499
error: src/slave/containerizer/docker.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 22, 2015, 8:31 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 22, 2015, 8:31 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.hpp PRE-CREATION
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp 5c57478
> src/slave/flags.cpp b5e2518
> src/tests/docker_containerizer_tests.cpp 154bf98
> src/tests/docker_tests.cpp 5520c58
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
> On May 24, 2015, 6:07 p.m., Timothy Chen wrote:
> > src/slave/containerizer/docker.cpp, lines 845-848
> > <https://reviews.apache.org/r/29889/diff/13/?file=970849#file970849line845>
> >
> > Ah, I realize why now I want a promise.
> > So the semantics I want to have is that I want to able to propagate the failure from run back out to the caller, so that it shows up in the TaskStatus update, but otherwise return the inspect future when it succeed. So I do want to use the promise associate to handle both cases. I'll leave a comment.
I'm keeping the current version since I cannot just use associate, as I no longer can override the failure once associated. I'm leaving comments on why.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review85096
-----------------------------------------------------------
On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 6:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.hpp PRE-CREATION
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp 5c57478
> src/slave/flags.cpp b5e2518
> src/tests/docker_containerizer_tests.cpp 154bf98
> src/tests/docker_tests.cpp 5520c58
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review85096
-----------------------------------------------------------
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136568>
Ah, I realize why now I want a promise.
So the semantics I want to have is that I want to able to propagate the failure from run back out to the caller, so that it shows up in the TaskStatus update, but otherwise return the inspect future when it succeed. So I do want to use the promise associate to handle both cases. I'll leave a comment.
- Timothy Chen
On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 6:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.hpp PRE-CREATION
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp 5c57478
> src/slave/flags.cpp b5e2518
> src/tests/docker_containerizer_tests.cpp 154bf98
> src/tests/docker_tests.cpp 5520c58
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/docker/docker.hpp, line 64
> > <https://reviews.apache.org/r/29889/diff/13/?file=970842#file970842line64>
> >
> > Can you add a small comment explaining when 'started' would be 'false'? I'm guessing you want to use 'started' here to help figure out when a container has "started" when you do an attached 'docker run', but I don't know what 'started' implies, and how it is different than checking when 'pid.isSome()'?
Good point, I cannot use pid because at the time when I inspect again the container might already finished and there is no pid available.
So the easiest way to know if the container has actually started is by examining the startedAt
> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.hpp, lines 462-464
> > <https://reviews.apache.org/r/29889/diff/13/?file=970848#file970848line462>
> >
> > Any reason not to make these Option as well? It seems a little wierd to potentially read these and get garbage (or missing) data in the case that they were actually passed into the constructor as 'None'.
Since we always set it to something in the constructor, I thought there is no point to set them to Option. If you look in the constructor code you'll see that.
> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.cpp, lines 845-848
> > <https://reviews.apache.org/r/29889/diff/13/?file=970849#file970849line845>
> >
> > Why do you need to associate the promise within the 'onAny' callback? You should just be able to do:
> >
> >
> > Future<Docker::Container> inspect =
> > docker->inspect(containerName, slave::DOCKER_INSPECT_DELAY);
> >
> > promise->associate(inspect);
> >
> >
> > The other thing that is a bit mysterious to me here is why you actually want/need an extra Promise? Seems like you can just do:
> >
> > run.onFailed([=](const string& failure) {
> > inspect.discard();
> > });
> >
> > return inspect;
> >
> >
> > Maybe it's because you don't want a 'discarded' future to propagate out from 'launchExecutorContainer'? In which case, I'd comment as much, i.e., that you explicitly want a failed future rather than let a discarded future propagate and clearly explain why (hopefully there isn't other code relying on it someplace because that's definitely a global invariant that we should avoid!).
I think you're right the simplified version is all I need, in the beginning I made a promise because I was thinking I might need to call inspect again later as I was thinking to keep inpsect simple and made the retries outside. But now with retries happening in inspect, this is a lot easier.
> On May 24, 2015, 1:55 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/docker.cpp, line 912
> > <https://reviews.apache.org/r/29889/diff/13/?file=970849#file970849line912>
> >
> > So this version is correctly working? Just wanted to call this out explicitly because the other one is not and that makes me wonder if maybe this code path isn't being tested?
No this actually works if I just pass the flags straight without slicing, but in the other path where I originally store the flags in a Option<flags::FlagsBase> and then pass that into subprocess actually gives me bad access from time to time. I didn't want to hard code the type in dockerRun to be docker::Flags because I thought docker::run should take a generic flagbase in case it wants to pass any arbitrary flags in.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review85080
-----------------------------------------------------------
On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 6:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.hpp PRE-CREATION
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp 5c57478
> src/slave/flags.cpp b5e2518
> src/tests/docker_containerizer_tests.cpp 154bf98
> src/tests/docker_tests.cpp 5520c58
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review85080
-----------------------------------------------------------
Ship it!
docs/configuration.md
<https://reviews.apache.org/r/29889/#comment136541>
s/slave recovers/slave restarts and recovers/
src/docker/docker.hpp
<https://reviews.apache.org/r/29889/#comment136542>
Can you add a small comment explaining when 'started' would be 'false'? I'm guessing you want to use 'started' here to help figure out when a container has "started" when you do an attached 'docker run', but I don't know what 'started' implies, and how it is different than checking when 'pid.isSome()'?
src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment136543>
FYI, there have been additions to the JSON library that should make this (and the code above) much easier now. In particular, you should now be able to do:
Result<JSON::String> startedAt = stateValue.as<JSON::Object>().find("StartedAt");
if (startedAt.isError()) {
return Error("Unable to find 'StartedAt' in State: " + startedAt.error());
} else if (startedAt.isError()) {
return Error("No 'StartedAt' found in State");
}
bool started = startedAt.value != "0001-01-01T00:00:00Z";
This cleanup needs to happen across this entire file so it might make sense to just put a TODO here for now if you don't want to take this on just yet.
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29889/#comment136547>
Any reason not to make these Option as well? It seems a little wierd to potentially read these and get garbage (or missing) data in the case that they were actually passed into the constructor as 'None'.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136546>
Not used? Looks shadowed below.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136548>
Super duper clean Tim!
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136549>
Yes Yes Yes YES YES!
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136550>
Why do you need to associate the promise within the 'onAny' callback? You should just be able to do:
Future<Docker::Container> inspect =
docker->inspect(containerName, slave::DOCKER_INSPECT_DELAY);
promise->associate(inspect);
The other thing that is a bit mysterious to me here is why you actually want/need an extra Promise? Seems like you can just do:
run.onFailed([=](const string& failure) {
inspect.discard();
});
return inspect;
Maybe it's because you don't want a 'discarded' future to propagate out from 'launchExecutorContainer'? In which case, I'd comment as much, i.e., that you explicitly want a failed future rather than let a discarded future propagate and clearly explain why (hopefully there isn't other code relying on it someplace because that's definitely a global invariant that we should avoid!).
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136551>
So this version is correctly working? Just wanted to call this out explicitly because the other one is not and that makes me wonder if maybe this code path isn't being tested?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136553>
Wait, are we doing a 'docker rm -f' here or just a 'docker stop'?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment136554>
I don't fully grok this, does this mean that we always have two containers? I would have expected a version of remove that maybe takes an 'Option<string> executorContainerName' and only does an two 'docker rm' if there are two containers. Were you just assuming that the extra 'docker rm' would be a no-op? If so, you should definitely document that, but I'm also not convinced that it's not hard for us to know whether or not we need to do two 'docker rm' calls, or am I missing something?
src/slave/flags.cpp
<https://reviews.apache.org/r/29889/#comment136545>
See comment above!
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment136555>
This looks like we're introducing a broken window. Maybe you were looking for:
AWAIT_READY_TRUE(docker.get()->rm(container.id, true), Seconds(30));
And that didn't exist? We should create it! In the mean time, I'd rather see:
AWAIT_READY_EQ(true, docker.get()->rm(container.id, true), Seconds(30));
Then let's get an AWAIT_READY_TRUE implemented later! I can help, just let me know!
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment136556>
Looks like we need another helper in libprocess! Are you looking for something along the lines of:
AWAIT_COMPLETED(usage);
Can you follow up with me on all of these kinds of helpers that you're missing? Adding in await everywhere is a pattern I'd like to avoid.
- Benjamin Hindman
On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 6:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.hpp PRE-CREATION
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp 5c57478
> src/slave/flags.cpp b5e2518
> src/tests/docker_containerizer_tests.cpp 154bf98
> src/tests/docker_tests.cpp 5520c58
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review85058
-----------------------------------------------------------
Bad patch!
Reviews applied: [29327]
Failed command: ./support/apply-review.sh -n -r 29327
Error:
2015-05-23 06:40:39 URL:https://reviews.apache.org/r/29327/diff/raw/ [2935/2935] -> "29327.patch" [1]
error: patch failed: src/slave/containerizer/docker.cpp:499
error: src/slave/containerizer/docker.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 23, 2015, 6:14 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 23, 2015, 6:14 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.hpp PRE-CREATION
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp 5c57478
> src/slave/flags.cpp b5e2518
> src/tests/docker_containerizer_tests.cpp 154bf98
> src/tests/docker_tests.cpp 5520c58
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 25, 2015, 5:25 a.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 34755cf
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.hpp PRE-CREATION
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/containerizer/docker.hpp ed4ee19
src/slave/containerizer/docker.cpp 408a443
src/slave/flags.hpp 5c57478
src/slave/flags.cpp b5e2518
src/tests/docker_containerizer_tests.cpp 154bf98
src/tests/docker_tests.cpp 5520c58
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 24, 2015, 6:26 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 34755cf
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.hpp PRE-CREATION
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/containerizer/docker.hpp ed4ee19
src/slave/containerizer/docker.cpp 408a443
src/slave/flags.hpp 5c57478
src/slave/flags.cpp b5e2518
src/tests/docker_containerizer_tests.cpp 154bf98
src/tests/docker_tests.cpp 5520c58
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 23, 2015, 6:14 a.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 34755cf
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.hpp PRE-CREATION
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/containerizer/docker.hpp ed4ee19
src/slave/containerizer/docker.cpp 408a443
src/slave/flags.hpp 5c57478
src/slave/flags.cpp b5e2518
src/tests/docker_containerizer_tests.cpp 154bf98
src/tests/docker_tests.cpp 5520c58
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 22, 2015, 8:31 a.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 34755cf
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.hpp PRE-CREATION
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/containerizer/docker.hpp ed4ee19
src/slave/containerizer/docker.cpp 408a443
src/slave/flags.hpp 5c57478
src/slave/flags.cpp b5e2518
src/tests/docker_containerizer_tests.cpp 154bf98
src/tests/docker_tests.cpp 5520c58
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review83850
-----------------------------------------------------------
Bad patch!
Reviews applied: [29327]
Failed command: ./support/apply-review.sh -n -r 29327
Error:
2015-05-14 21:55:18 URL:https://reviews.apache.org/r/29327/diff/raw/ [2935/2935] -> "29327.patch" [1]
error: patch failed: src/slave/containerizer/docker.cpp:499
error: src/slave/containerizer/docker.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 14, 2015, 9:39 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 9:39 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 34755cf
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/containerizer/docker.hpp ed4ee19
> src/slave/containerizer/docker.cpp 408a443
> src/slave/flags.hpp ca7cc13
> src/slave/flags.cpp f35c76a
> src/tests/docker_containerizer_tests.cpp 154bf98
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 14, 2015, 9:39 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 34755cf
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp df02043
src/slave/constants.cpp 07f699a
src/slave/containerizer/docker.hpp ed4ee19
src/slave/containerizer/docker.cpp 408a443
src/slave/flags.hpp ca7cc13
src/slave/flags.cpp f35c76a
src/tests/docker_containerizer_tests.cpp 154bf98
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/docker/docker.cpp, line 305
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948190#file948190line305>
> >
> > 1. There is no declaration in the hpp file with a comment and there is no explanation here. I would suggest using "static" here so nobody needs to browse the hpp file in search for a potential comment that does not exist.
> >
> > 2. OK, I can guess what this does, but future readers may want to skip reading the function body to find out. The name suggests running something.
> >
> > 3. I can't find where this function is used.
> >
> > It seems to me that you wanted to break out part of Docker::run() below, but have not removed this code there and replaced it with a call yet.
Sorry I originally made a refactor change and then abandoned it since I don't need to anymore, but thought I removed all the helper methods I added.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/docker/executor.cpp, line 139
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line139>
> >
> > Why can't we call 'stop.onAny(...)' directly here?
Because we need to handle two cases: 1) When the docker container exits on its own 2) When the container was asked to be killed.
I added a stop future because if a container was asked to be killed, the stop future holds the docker-stop future and I want to make sure I wait on that future to finish before I send back the last TASK_STATUS as this will start causing more problems in the tests where I don't know when the container will exactly stop.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/docker/executor.cpp, line 170
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line170>
> >
> > Could we use a LOCAL future 'stop' instead and discard 'run' once it is satisfied? This way 'stop' does not have to be a global and one can see here locally what is going on.
> >
> > You could then amend _reaped() for checking for run having been discarded.
I can't, since I need to discard the docker run future first so I stop the container to be started if it wasn't launched yet. And if the container is already launched, then it will be stopped by docker-stop.
If I call docker-stop first, there might be a race where we asked to the stop the container but it hasn't been launched yet, yet the docker->run future might eventually go thourgh and start the container.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/docker/executor.cpp, line 76
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line76>
> >
> > This is a bit tricky. You have to know that 'stop' receives a non-Nothing value once 'run' gets one. Suggestion: reduce this to only one future - 'run'. See below in line 170.
See comment on line 170 comment.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/docker/executor.cpp, line 419
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948191#file948191line419>
> >
> > No default? Why?
It should always be passed in, doesn't make sense to have a default in the slave flags and another default here.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.hpp, line 206
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948194#file948194line206>
> >
> > "only"? What other cases are there?
Where do you think it makes sense to specifiy all other cases? In every launch method?
There are 3 paths basically, when slave is contained we launch 1) task, slave is not contained we launch 2) task or 3) custom executor.
This call back is applicable for 1 and 3.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/tests/docker_containerizer_tests.cpp, line 2596
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948198#file948198line2596>
> >
> > What changed so that we don't need this test any more?
We shouldn't need this test anymore, but thinking about it more I'll just leave it in.
> On May 12, 2015, 1:58 a.m., Bernd Mathiske wrote:
> > src/tests/docker_containerizer_tests.cpp, line 175
> > <https://reviews.apache.org/r/29889/diff/9-10/?file=948198#file948198line175>
> >
> > We should generally avoid bool args, because it is hard to see at the call site what they mean.
> >
> > Suggestions:
> > - Use an enum.
> > - Keep the exists() and running() methods, factor out what's common between them, pass a closure as an arg to the subroutine for what's different.
I'll just use an enum.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review83305
-----------------------------------------------------------
On May 8, 2015, 9:40 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 9:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 54271f7
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review83305
-----------------------------------------------------------
src/docker/docker.cpp
<https://reviews.apache.org/r/29889/#comment134286>
1. There is no declaration in the hpp file with a comment and there is no explanation here. I would suggest using "static" here so nobody needs to browse the hpp file in search for a potential comment that does not exist.
2. OK, I can guess what this does, but future readers may want to skip reading the function body to find out. The name suggests running something.
3. I can't find where this function is used.
It seems to me that you wanted to break out part of Docker::run() below, but have not removed this code there and replaced it with a call yet.
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134292>
This is a bit tricky. You have to know that 'stop' receives a non-Nothing value once 'run' gets one. Suggestion: reduce this to only one future - 'run'. See below in line 170.
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134245>
Why can't we call 'stop.onAny(...)' directly here?
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134241>
s/might still/might still be
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134242>
Suggestions:
Break this out as a separate statement/comment pair.
s/copy to remove const/mutable copy of the future
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134294>
Could we use a LOCAL future 'stop' instead and discard 'run' once it is satisfied? This way 'stop' does not have to be a global and one can see here locally what is going on.
You could then amend _reaped() for checking for run having been discarded.
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134247>
See line 136, it seems that we do not need this function.
If you were to keep it, it would be better placed above _reaped, so that one could read "upper half / lower half" contiguously.
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134248>
Aren't we going to inspect 'stop'? What if it failed? Could we at least log something then?
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134295>
s/after stopping/after attempting to stop
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment134296>
No default? Why?
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29889/#comment134298>
Which docker image?
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29889/#comment134305>
"only"? What other cases are there?
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment134303>
Which of these arguments signifies the executor?
'container'? Could we rename it accordingly?
Before you updated the comment, I overlooked that this might be executor-specific code!
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment134310>
We should generally avoid bool args, because it is hard to see at the call site what they mean.
Suggestions:
- Use an enum.
- Keep the exists() and running() methods, factor out what's common between them, pass a closure as an arg to the subroutine for what's different.
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment134311>
Insert blank line above, please.
In other places below, too.
Maybe better: move the comment above the declaration.
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment134314>
What changed so that we don't need this test any more?
- Bernd Mathiske
On May 8, 2015, 2:40 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 2:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 54271f7
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review83108
-----------------------------------------------------------
Bad patch!
Reviews applied: [29327]
Failed command: ./support/apply-review.sh -n -r 29327
Error:
2015-05-08 21:55:17 URL:https://reviews.apache.org/r/29327/diff/raw/ [2935/2935] -> "29327.patch" [1]
error: patch failed: src/slave/containerizer/docker.cpp:499
error: src/slave/containerizer/docker.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 8, 2015, 9:40 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 8, 2015, 9:40 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 54271f7
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 8, 2015, 9:40 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 54271f7
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/containerizer/docker.hpp b25ec55
src/slave/containerizer/docker.cpp f9fc89a
src/slave/flags.hpp d3b1ce1
src/slave/flags.cpp d0932b0
src/tests/docker_containerizer_tests.cpp c9d66b3
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
> On May 4, 2015, 8:52 p.m., Bernd Mathiske wrote:
> > src/tests/docker_containerizer_tests.cpp, line 1260
> > <https://reviews.apache.org/r/29889/diff/9/?file=948198#file948198line1260>
> >
> > Same magical 30 as above. global constant?
We have a lot of these in the test code base, I think ideally we'll remove them in the future.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review82426
-----------------------------------------------------------
On May 2, 2015, 8:22 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 8:22 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 93c7c8a
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review82426
-----------------------------------------------------------
General comment: the _launch, __launch, ___launch, etc. cascade is still a bit hard to follow through. I wonder if we can find another style for this eventually. But this does not have to be part of this review.
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29889/#comment133154>
Can you please put comments before every *{_}launch method explining roughly what it does? That would help since none of these have descriptive names.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133153>
Is the comment stale? This seems to start the Docker executor.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133155>
Can't use trailing underscore for a local.
Which container is this now?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133157>
s/container/inspect
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133159>
Can we make this wait loop a subroutine for reuse?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133158>
s/container/inspect
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133160>
30 seconds - magic number? global constant?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133162>
TODO: come up with a plan to not depend on external dependencies?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133169>
s/container/inspect
Or maybe call this without temp var?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133170>
Same magical 30 as above. global constant?
src/tests/docker_containerizer_tests.cpp
<https://reviews.apache.org/r/29889/#comment133173>
Is it possible to not rely on dependencies outside the Mesos repo? If so, can we have a TODO here?
- Bernd Mathiske
On May 2, 2015, 1:22 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 1:22 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 93c7c8a
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review82325
-----------------------------------------------------------
Bad patch!
Reviews applied: [29327]
Failed command: ./support/apply-review.sh -n -r 29327
Error:
2015-05-02 20:34:47 URL:https://reviews.apache.org/r/29327/diff/raw/ [2935/2935] -> "29327.patch" [1]
error: patch failed: src/slave/containerizer/docker.cpp:499
error: src/slave/containerizer/docker.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 2, 2015, 8:22 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 2, 2015, 8:22 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 93c7c8a
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/
-----------------------------------------------------------
(Updated May 2, 2015, 8:22 p.m.)
Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
Bugs: MESOS-2115
https://issues.apache.org/jira/browse/MESOS-2115
Repository: mesos
Description
-------
This is a one mega patch and contains many reviews that's already on rb.
This review is not meant to be merged, only provided for easier review.
Diffs (updated)
-----
Dockerfile 35abf25
docs/configuration.md 54c4e31
docs/docker-containerizer.md a5438b7
src/Makefile.am 93c7c8a
src/docker/docker.hpp 3ebbc1f
src/docker/docker.cpp 3a485a2
src/docker/executor.cpp PRE-CREATION
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/containerizer/docker.hpp b25ec55
src/slave/containerizer/docker.cpp f9fc89a
src/slave/flags.hpp d3b1ce1
src/slave/flags.cpp d0932b0
src/tests/docker_containerizer_tests.cpp c9d66b3
Diff: https://reviews.apache.org/r/29889/diff/
Testing
-------
make check
Thanks,
Timothy Chen
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Timothy Chen <tn...@apache.org>.
> On May 1, 2015, 11:14 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.hpp, line 454
> > <https://reviews.apache.org/r/29889/diff/8/?file=946143#file946143line454>
> >
> > This is a good place to explain the whole picture what each override<...> is, what it's purpose is and how it gets used. However, if this is deemed a generic facility, then some of the use sites need to be upgraded with docker-specific scenario descriptions.
I got rid of override completely now.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review82287
-----------------------------------------------------------
On May 1, 2015, 9:43 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 9:43 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 93c7c8a
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 29889: Recover Docker containers when mesos slave
is in a container
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29889/#review82287
-----------------------------------------------------------
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133009>
Can we add some output here, too, indicating that the reason for shutting down is killing?
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133010>
Emit some output here?
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133011>
This is copied from CommandExecutor. Please add a TODO to fix both.
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133012>
Owned<...>
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133013>
cli -> executable
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133014>
Suggestion:
"The path to the container sandbox holding stdout and stderr files into which docker container logs will be redirected."
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133016>
This pertains to the boolean arg. Please indicate this.
src/docker/executor.cpp
<https://reviews.apache.org/r/29889/#comment133017>
Suggestion, add something like: "..., since this has already been validated in <which step> before."
src/slave/containerizer/docker.hpp
<https://reviews.apache.org/r/29889/#comment133026>
This is a good place to explain the whole picture what each override<...> is, what it's purpose is and how it gets used. However, if this is deemed a generic facility, then some of the use sites need to be upgraded with docker-specific scenario descriptions.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133027>
zap the TABs
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133028>
You have a constant declared for 5.
src/slave/containerizer/docker.cpp
<https://reviews.apache.org/r/29889/#comment133029>
Would it be possible to pull up the conditional branch to here and then call two different constructors depending on the condition, each of which fills out the fields differently. We would then no longer need the override<...> fields, would we?
src/slave/flags.cpp
<https://reviews.apache.org/r/29889/#comment133021>
There is extra text on this in the docs. Please determine how much of it pull up here as well.
- Bernd Mathiske
On May 1, 2015, 2:43 p.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29889/
> -----------------------------------------------------------
>
> (Updated May 1, 2015, 2:43 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff.
>
>
> Bugs: MESOS-2115
> https://issues.apache.org/jira/browse/MESOS-2115
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This is a one mega patch and contains many reviews that's already on rb.
>
> This review is not meant to be merged, only provided for easier review.
>
>
> Diffs
> -----
>
> Dockerfile 35abf25
> docs/configuration.md 54c4e31
> docs/docker-containerizer.md a5438b7
> src/Makefile.am 93c7c8a
> src/docker/docker.hpp 3ebbc1f
> src/docker/docker.cpp 3a485a2
> src/docker/executor.cpp PRE-CREATION
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/containerizer/docker.hpp b25ec55
> src/slave/containerizer/docker.cpp f9fc89a
> src/slave/flags.hpp d3b1ce1
> src/slave/flags.cpp d0932b0
> src/tests/docker_containerizer_tests.cpp c9d66b3
>
> Diff: https://reviews.apache.org/r/29889/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>