Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Greg Mann]

> On Feb. 14, 2018, 5:49 p.m., Andrei Budnik wrote:
> > src/docker/executor.cpp
> > Lines 304 (patched)
> > 
> >
> > This case means `docker stop` is in progress (`killed == true`): 
> > https://github.com/apache/mesos/blob/e36e3b9989b1097dca9597097dd51d24aaba48fa/src/docker/executor.cpp#L473-L525.
> >  The container might still be running. If `docker stop` fails or hangs for 
> > more than 3 seconds, the executor will terminate, but the docker container 
> > might be running.

Could you elaborate here, I don't quite understand? In this case, we know that 
`container.pid.isNone() == true`, which means that the `Docker::inspect()` call 
returned successfully, but no running process was found for that container. The 
conditional above tests for `if (!killed)`, but we don't know anything about 
the value of `killed` here.


- Greg


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


On Feb. 13, 2018, 3:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 13, 2018, 3:20 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 can 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 returning, 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/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Andrei Budnik]

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




src/docker/executor.cpp
Lines 304 (patched)


This case means `docker stop` is in progress (`killed == true`): 
https://github.com/apache/mesos/blob/e36e3b9989b1097dca9597097dd51d24aaba48fa/src/docker/executor.cpp#L473-L525.
 The container might still be running. If `docker stop` fails or hangs for more 
than 3 seconds, the executor will terminate, but the docker container might be 
running.


- Andrei Budnik


On Feb. 13, 2018, 3:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 13, 2018, 3:20 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 can 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 returning, 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/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65518]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Feb. 13, 2018, 3:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 13, 2018, 3:20 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 can 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 returning, 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/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65518 was successfully built and tested.

Reviews applied: `['65518']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65518

- Mesos Reviewbot Windows


On Feb. 13, 2018, 3:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 13, 2018, 3:20 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 can 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 returning, 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/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Qian Zhang]

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

(Updated Feb. 13, 2018, 11:20 a.m.)


Review request for mesos, Gaston Kleiman, Gilbert Song, Greg Mann, and Vinod 
Kone.


Changes
---

Addressed review comments.


Bugs: MESOS-8488
https://issues.apache.org/jira/browse/MESOS-8488


Repository: mesos


Description (updated)
---

Due to a Docker issue (https://github.com/moby/moby/issues/33820),
Docker daemon can 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 returning, 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 (updated)
-

  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 


Diff: https://reviews.apache.org/r/65518/diff/4/

Changes: https://reviews.apache.org/r/65518/diff/3-4/


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Greg Mann]

> On Feb. 10, 2018, 2:21 a.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 273-302 (patched)
> > 
> >
> > 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 runPromise;
> >   Future runFuture;
> >   auto interceptedRun = [mockDocker, , ](
> >   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
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65518]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


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

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65518 was successfully built and tested.

Reviews applied: `['65518']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65518

- Mesos Reviewbot Windows


On Feb. 12, 2018, 11: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, 11: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
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Qian Zhang]

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

(Updated Feb. 12, 2018, 11:46 a.m.)


Review request for mesos, Gaston Kleiman, Gilbert Song, Greg Mann, and Vinod 
Kone.


Changes
---

Addressed review comments.


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 (updated)
-

  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 


Diff: https://reviews.apache.org/r/65518/diff/3/

Changes: https://reviews.apache.org/r/65518/diff/2-3/


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Qian Zhang]

> On Feb. 10, 2018, 8:55 a.m., Vinod Kone wrote:
> > src/docker/executor.cpp
> > Lines 277 (patched)
> > 
> >
> > s/never returns though/to never return although/

I see Greg suggests to change `returns` to `returning` in his comment below 
which I think is better.


> On Feb. 10, 2018, 8:55 a.m., Vinod Kone wrote:
> > src/docker/executor.cpp
> > Lines 280 (patched)
> > 
> >
> > when can this be None()?

According to this comment 
https://github.com/apache/mesos/blob/1.5.0/src/docker/docker.hpp#L104:L106, it 
will be `None()` when the container is not running.

But I am not sure if it will be `None()` when the container is not running AND 
the Docker issue (https://github.com/moby/moby/issues/33820) occurs. I mean if 
we launch a Docker container which exits immediately (e.g., execute the command 
`exit 0`), and due to that Docker issue Docker daemon does not catch the 
container's exit, will `container.pid` be `None()` or not? If it is not 
`None()` in this case, then we will reap the process in this lambda which is 
good, but if it is `None()`, then we will miss to reap the process which is not 
correct. My suspect is it will not be `None` in this case, but just to be safe, 
let's also do the below in the case that `container.pid` is `None()`, how do 
you think?
```
delay(
Seconds(3),
self(),
::reapedContainer,
container.pid.get());
```


> On Feb. 10, 2018, 8:55 a.m., Vinod Kone wrote:
> > src/docker/executor.cpp
> > Lines 288 (patched)
> > 
> >
> > Add a LOG line here?

If we add a LOG here, then we may need to add a LOG into `reaped` too? They are 
the two methods to catch the exit status of the container.


> On Feb. 10, 2018, 8:55 a.m., Vinod Kone wrote:
> > src/docker/executor.cpp
> > Lines 530 (patched)
> > 
> >
> > why not call `reaped` directly? sounds like `reaped` does a bunch of 
> > necessary cleanup that you are skipping by calling `_reaped`?

Agree, thanks for catching it!


> On Feb. 10, 2018, 8:55 a.m., Vinod Kone wrote:
> > src/docker/executor.cpp
> > Line 490 (original), 535 (patched)
> > 
> >
> > shouldn't we return here if `terminated` already is set in case the 
> > `run` returns after `reapedContainer` is called?

Yes, we should return here if we call `reaped` instead of `_reaped` in 
`reapedContainer` as you mentioned in the above comment.


- Qian


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


On Feb. 9, 2018, 9:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 9, 2018, 9:03 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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Qian Zhang]

> On Feb. 10, 2018, 10:21 a.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 273-302 (patched)
> > 
> >
> > 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 runPromise;
> >   Future runFuture;
> >   auto interceptedRun = [mockDocker, , ](
> >   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?

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.


> On Feb. 10, 2018, 10:21 a.m., Greg Mann wrote:
> > src/docker/executor.cpp
> > Lines 280 (patched)
> > 
> >
> > Should we log an error in the case that `container.pid.isNone()`? AFAIK 
> > this means that the Docker daemon was not able to locate a process 
> > associated with the container.
> > 
> > This makes me wonder if we should actually do 
> > CHECK_SOME(container.pid)? It looks like the only place where we use 
> > `containerPid` is in the health checker, where the PID is used to enter the 
> > appropriate namespaces.
> > 
> > For now, I would suggest that we log an error to help in debugging.

Vinod also has a comment for this code, I have replied to him under his 
comment, let's discuss there :-)


- Qian


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


On Feb. 9, 2018, 9:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 9, 2018, 9:03 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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Greg Mann]

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




src/docker/executor.cpp
Lines 273-302 (patched)


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 runPromise;
  Future runFuture;
  auto interceptedRun = [mockDocker, , ](
  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?



src/docker/executor.cpp
Lines 275 (patched)


s/will/can/



src/docker/executor.cpp
Lines 276 (patched)


Nit:
s/"docker run"/'docker run'/



src/docker/executor.cpp
Lines 277 (patched)


s/returns/returning/



src/docker/executor.cpp
Lines 280 (patched)


Should we log an error in the case that `container.pid.isNone()`? AFAIK 
this means that the Docker daemon was not able to locate a process associated 
with the container.

This makes me wonder if we should actually do CHECK_SOME(container.pid)? It 
looks like the only place where we use `containerPid` is in the health checker, 
where the PID is used to enter the appropriate namespaces.

For now, I would suggest that we log an error to help in debugging.



src/docker/executor.cpp
Lines 283 (patched)


s/can not/cannot/


- Greg Mann


On Feb. 9, 2018, 1:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 9, 2018, 1:03 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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Vinod Kone]

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




src/docker/executor.cpp
Lines 277 (patched)


s/never returns though/to never return although/



src/docker/executor.cpp
Lines 280 (patched)


when can this be None()?



src/docker/executor.cpp
Lines 288 (patched)


Add a LOG line here?



src/docker/executor.cpp
Lines 289 (patched)


s/lambda/lambda;/



src/docker/executor.cpp
Lines 530 (patched)


why not call `reaped` directly? sounds like `reaped` does a bunch of 
necessary cleanup that you are skipping by calling `_reaped`?



src/docker/executor.cpp
Line 490 (original), 535 (patched)


shouldn't we return here if `terminated` already is set in case the `run` 
returns after `reapedContainer` is called?


- Vinod Kone


On Feb. 9, 2018, 1:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 9, 2018, 1:03 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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65518]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On Feb. 9, 2018, 1:03 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 9, 2018, 1:03 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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Gaston Kleiman]

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


Ship it!




Ship It!

- Gaston Kleiman


On Feb. 8, 2018, 5:03 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 8, 2018, 5:03 p.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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65518 was successfully built and tested.

Reviews applied: `['65518']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65518

- Mesos Reviewbot Windows


On Feb. 6, 2018, 1:45 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 6, 2018, 1:45 p.m.)
> 
> 
> Review request for mesos, 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/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Qian Zhang]

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

(Updated Feb. 6, 2018, 9:45 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

Replaced `onAny` with `then`.


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 (updated)
-

  src/docker/executor.cpp e4c53d558e414e50b1c429fba8e31e504c63744a 


Diff: https://reviews.apache.org/r/65518/diff/2/

Changes: https://reviews.apache.org/r/65518/diff/1-2/


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 65518: Reaped the container process directly in Docker executor.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 65518 was successfully built and tested.

Reviews applied: `['65518']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/65518

- Mesos Reviewbot Windows


On Feb. 5, 2018, 6:54 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65518/
> ---
> 
> (Updated Feb. 5, 2018, 6:54 p.m.)
> 
> 
> Review request for mesos, 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/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>