You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2018/09/19 04:51:37 UTC
Review Request 68759: Cleaned up the `ContainerDaemon` implementation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68759/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Bugs: MESOS-9228
https://issues.apache.org/jira/browse/MESOS-9228
Repository: mesos
Description
-------
This patch made the following changes to the container daemon:
1. Made it stores the launch parameters instead of an `agent::Call`.
2. Cleaned up the log messages.
3. Made the process managed by libprocess.
Diffs
-----
src/slave/container_daemon.hpp a58140dd787524b0438ba4719fa7e3365ab2fa17
src/slave/container_daemon.cpp e1b0812b8b467ee4061b23d39552e2417d65a14a
src/slave/container_daemon_process.hpp a5d19a04223f6f595e97ec83962558639fd51f7b
Diff: https://reviews.apache.org/r/68759/diff/1/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 68759: Cleaned up the `ContainerDaemon`
implementation.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Sept. 19, 2018, 1:38 p.m., Benjamin Bannier wrote:
> > src/slave/container_daemon.hpp
> > Lines 86 (patched)
> > <https://reviews.apache.org/r/68759/diff/1/?file=2090233#file2090233line87>
> >
> > Any reason we don't just use a `process::Promise` terminated here instead of sharing state?
The future comes from a promise inside `ContainerDaemonProcess`, who is responsible to set the promise. Not sure what you mean by "sharing state."
> On Sept. 19, 2018, 1:38 p.m., Benjamin Bannier wrote:
> > src/slave/container_daemon.cpp
> > Line 262 (original), 263 (patched)
> > <https://reviews.apache.org/r/68759/diff/1/?file=2090234#file2090234line294>
> >
> > That we don't `wait` for the wrapped process to terminate here seems significant. Could you explain why that is okay?
We had some offline discussions.
The major benefit of getting rid of `process::wait` is because we'll destruct `ContainerDaemon`s in a worker thread, and `process::wait` is a blocking call which should be avoided in any worker thread. BenM also mentioned that the long-term goal is to make all actors auto-managed and get rid of all `process::wait` calls in destructors (or the alternative is to allow `process::wait` but move the thread out from the worker thread pool).
In general, there is no restriction to create two `ContainerDaemon`s to monitor the same container, and doing a `process::wait` here won't prevent this from happening.
However in the original code, because we only create one single SLRP for a (type, name) pair at a time, the `process::wait` here essentially creates a synchronization between two SLRP instances of the same (type, name) pair to guarantee that there is at most one `ContainerDaemonProcess` to monitor the container at a time. To achieve the same, I'm proposing to remove `process::wait` here, but instead asking the caller to wait for `ContainerDaemon::wait` as the synchronization between two SLRP instances.
I could keep the `process::wait` here for now if you think it's better.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68759/#review208763
-----------------------------------------------------------
On Sept. 19, 2018, 4:51 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68759/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2018, 4:51 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-9228
> https://issues.apache.org/jira/browse/MESOS-9228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch made the following changes to the container daemon:
> 1. Made it stores the launch parameters instead of an `agent::Call`.
> 2. Cleaned up the log messages.
> 3. Made the process managed by libprocess.
>
>
> Diffs
> -----
>
> src/slave/container_daemon.hpp a58140dd787524b0438ba4719fa7e3365ab2fa17
> src/slave/container_daemon.cpp e1b0812b8b467ee4061b23d39552e2417d65a14a
> src/slave/container_daemon_process.hpp a5d19a04223f6f595e97ec83962558639fd51f7b
>
>
> Diff: https://reviews.apache.org/r/68759/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68759: Cleaned up the `ContainerDaemon`
implementation.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68759/#review208763
-----------------------------------------------------------
src/slave/container_daemon.hpp
Lines 86 (patched)
<https://reviews.apache.org/r/68759/#comment292916>
Any reason we don't just use a `process::Promise` terminated here instead of sharing state?
src/slave/container_daemon.cpp
Line 262 (original), 263 (patched)
<https://reviews.apache.org/r/68759/#comment292917>
That we don't `wait` for the wrapped process to terminate here seems significant. Could you explain why that is okay?
- Benjamin Bannier
On Sept. 19, 2018, 6:51 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68759/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2018, 6:51 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-9228
> https://issues.apache.org/jira/browse/MESOS-9228
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch made the following changes to the container daemon:
> 1. Made it stores the launch parameters instead of an `agent::Call`.
> 2. Cleaned up the log messages.
> 3. Made the process managed by libprocess.
>
>
> Diffs
> -----
>
> src/slave/container_daemon.hpp a58140dd787524b0438ba4719fa7e3365ab2fa17
> src/slave/container_daemon.cpp e1b0812b8b467ee4061b23d39552e2417d65a14a
> src/slave/container_daemon_process.hpp a5d19a04223f6f595e97ec83962558639fd51f7b
>
>
> Diff: https://reviews.apache.org/r/68759/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 68759: Cleaned up the `ContainerDaemon`
implementation.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68759/
-----------------------------------------------------------
(Updated Sept. 20, 2018, 3:16 a.m.)
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Changes
-------
Added the `process::wait` back in the destructor.
Bugs: MESOS-9228
https://issues.apache.org/jira/browse/MESOS-9228
Repository: mesos
Description
-------
This patch made the following changes to the container daemon:
1. Made it stores the launch parameters instead of an `agent::Call`.
2. Cleaned up the log messages.
3. Made the process managed by libprocess.
Diffs (updated)
-----
src/slave/container_daemon.hpp a58140dd787524b0438ba4719fa7e3365ab2fa17
src/slave/container_daemon.cpp e1b0812b8b467ee4061b23d39552e2417d65a14a
src/slave/container_daemon_process.hpp a5d19a04223f6f595e97ec83962558639fd51f7b
Diff: https://reviews.apache.org/r/68759/diff/2/
Changes: https://reviews.apache.org/r/68759/diff/1-2/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao