mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Timothy Chen" <tnac...@apache.org>
Subject Re: Review Request 29889: Recover Docker containers when mesos slave is in a container
Date Tue, 12 May 2015 08:08:17 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message