You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anand Mazumdar <an...@apache.org> on 2017/08/16 17:27:12 UTC
Review Request 61693: Fixed a bug around the `kill()` not removing
active containers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61693/
-----------------------------------------------------------
Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
Repository: mesos
Description
-------
The composing containerizer relies on users invoking `destroy()`
for removing the container from the list of active containers.
However, when using `kill()` the relevant containerizer just
reaps the container without notifying the composing containerizer
i.e., there is no `destroy()` invocation from the user. This change
chains on the `wait()` future to invoke `destroy()` that
eventually does the cleanup.
Diffs
-----
src/slave/containerizer/composing.cpp f1a9c3d08b408aa61198f4042b9274df88b789ea
Diff: https://reviews.apache.org/r/61693/diff/1/
Testing
-------
sudo make check (tests pass)
Thanks,
Anand Mazumdar
Re: Review Request 61693: Fixed a bug around the `kill()` not removing
active containers.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61693/#review183044
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61693]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 5:27 p.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.cpp f1a9c3d08b408aa61198f4042b9274df88b789ea
>
>
> Diff: https://reviews.apache.org/r/61693/diff/1/
>
>
> Testing
> -------
>
> sudo make check (tests pass)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 61693: Fixed a bug around the `kill()` not removing
active containers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61693/#review183101
-----------------------------------------------------------
src/slave/containerizer/composing.cpp
Lines 656-657 (patched)
<https://reviews.apache.org/r/61693/#comment259104>
Hum, this does not look like the right solution to me. How is that different from user manually sending a `SIGKILL` to the leading process of the container, triggering a reap? Don't you have the same problem in that case? Why we do a special case here?
A reap event should trigger a destroy of the container, leading to `wait` being returned. The component waiting for the container to terminate will then call destroy, causing the corresponding container in the composing containerizer being cleaned up.
- Jie Yu
On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 5:27 p.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.cpp f1a9c3d08b408aa61198f4042b9274df88b789ea
>
>
> Diff: https://reviews.apache.org/r/61693/diff/1/
>
>
> Testing
> -------
>
> sudo make check (tests pass)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 61693: Fixed a bug around the `kill()` not removing
active containers.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61693/#review183092
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/composing.cpp
Lines 654-655 (patched)
<https://reviews.apache.org/r/61693/#comment259097>
Could we add a `TODO` here later?
TODO: the `destroy` callback would result in extra unnecessary containerizer->destroy() call, which can be improved.
- Gilbert Song
On Aug. 16, 2017, 10:27 a.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 10:27 a.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.cpp f1a9c3d08b408aa61198f4042b9274df88b789ea
>
>
> Diff: https://reviews.apache.org/r/61693/diff/1/
>
>
> Testing
> -------
>
> sudo make check (tests pass)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 61693: Fixed a bug around the `kill()` not removing
active containers.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61693/#review183054
-----------------------------------------------------------
Patch looks great!
Reviews applied: [61693]
Logs available here: http://104.210.40.105/logs/master/61693
- Mesos Reviewbot Windows
On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 5:27 p.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.cpp f1a9c3d08b408aa61198f4042b9274df88b789ea
>
>
> Diff: https://reviews.apache.org/r/61693/diff/1/
>
>
> Testing
> -------
>
> sudo make check (tests pass)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 61693: Fixed a bug around the `kill()` not removing
active containers.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61693/#review183102
-----------------------------------------------------------
Also, could you please paste the commit sha and link the ticket?
- Jie Yu
On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 5:27 p.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.cpp f1a9c3d08b408aa61198f4042b9274df88b789ea
>
>
> Diff: https://reviews.apache.org/r/61693/diff/1/
>
>
> Testing
> -------
>
> sudo make check (tests pass)
>
>
> Thanks,
>
> Anand Mazumdar
>
>
Re: Review Request 61693: Fixed a bug around the `kill()` not removing
active containers.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61693/#review183091
-----------------------------------------------------------
Ship it!
LGTM assuming `destroy` is idempotent.
- Vinod Kone
On Aug. 16, 2017, 5:27 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61693/
> -----------------------------------------------------------
>
> (Updated Aug. 16, 2017, 5:27 p.m.)
>
>
> Review request for mesos, Gastón Kleiman, Gilbert Song, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The composing containerizer relies on users invoking `destroy()`
> for removing the container from the list of active containers.
> However, when using `kill()` the relevant containerizer just
> reaps the container without notifying the composing containerizer
> i.e., there is no `destroy()` invocation from the user. This change
> chains on the `wait()` future to invoke `destroy()` that
> eventually does the cleanup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/composing.cpp f1a9c3d08b408aa61198f4042b9274df88b789ea
>
>
> Diff: https://reviews.apache.org/r/61693/diff/1/
>
>
> Testing
> -------
>
> sudo make check (tests pass)
>
>
> Thanks,
>
> Anand Mazumdar
>
>