mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 65518: Reaped the container process directly in Docker executor.
Date Mon, 12 Feb 2018 07:04:43 GMT


> On Feb. 10, 2018, 2:21 a.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 273-302 (patched)
> > <https://reviews.apache.org/r/65518/diff/2/?file=1953359#file1953359line273>
> >
> >     Could we validate this with a test? I think it would be possible to use a `MockDocker`
which intercepts the call to `run()`, so that we can prevent it from returning when the container
exits. I think something like the following should work:
> >     
> >     ```
> >       MockDocker* mockDocker =
> >         new MockDocker(tests::flags.docker, tests::flags.docker_socket);
> >     
> >       Promise<Option<int>> runPromise;
> >       Future<Option<int>> runFuture;
> >       auto interceptedRun = [mockDocker, &runPromise, &runFuture](
> >           const Docker::RunOptions& runOptions,
> >           const process::Subprocess::IO& _stdout,
> >           const process::Subprocess::IO& _stderr) {
> >         runFuture = mockDocker->_run(
> >             runOptions,
> >             _stdout,
> >             _stderr);
> >     
> >         return runPromise.future();
> >       };
> >     
> >       EXPECT_CALL(*mockDocker, run(_, _, _))
> >         .WillRepeatedly(Invoke(interceptedRun));
> >     ```
> >     Then we have control over when the `Future` associated with the 'docker run'
call is satisfied. See the Docker containerizer tests for examples of how we use the MockDocker
and MockDockerContainerizer.
> >     
> >     What do you think?
> 
> Qian Zhang wrote:
>     Thanks for your suggestion! But I think that `mockDocker` will be used by Docker
containerizer rather than Docker executor, right? What we want to control is the behavior
of the `docker run` in Docker executor.

Ah yea, of course :(
I wish we had a convenient to test this, but it would be difficult. We would need either a
separate Docker executor binary for testing purposes, or we could mock or intercept the calls
to the Docker binary itself.

I'll drop this issue for now, but since it seems like we'll be taking steps in the near future
to make the Docker containerizer more resilient to Docker daemon failures, it will probably
be worth adding such test infrastructure so that we can validate the fixes we put in place.
Let's give some thought to how we might actually test this stuff.


- Greg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65518/#review197203
-----------------------------------------------------------


On Feb. 12, 2018, 3:46 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2018, 3:46 a.m.)
> 
> 
> Review request for mesos, Gaston Kleiman, Gilbert Song, Greg Mann, and Vinod Kone.
> 
> 
> Bugs: MESOS-8488
>     https://issues.apache.org/jira/browse/MESOS-8488
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Due to a Docker issue (https://github.com/moby/moby/issues/33820),
> Docker daemon will fail to catch a container exit, i.e., the container
> process has already exited but the command `docker ps` shows the
> container still running, this will lead to the "docker run" command
> that we execute in Docker executor never returns, and it will also
> cause the `docker stop` command takes no effect, i.e., it will return
> without error but `docker ps` shows the container still running, so
> the task will stuck in `TASK_KILLING` state.
> 
> To workaround this Docker issue, in this patch we made Docker executor
> reaps the container process directly so Docker executor will be notified
> once the container process exits.
> 
> 
> Diffs
> -----
> 
>   src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 
> 
> 
> Diff: https://reviews.apache.org/r/65518/diff/3/
> 
> 
> Testing
> -------
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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