You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2018/01/30 15:35:24 UTC
Re: Review Request 65382: Reaped Docker executor only when it can be
connected.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65382/
-----------------------------------------------------------
(Updated Jan. 30, 2018, 11:35 p.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Changed the solution.
Summary (updated)
-----------------
Reaped Docker executor only when it can be connected.
Bugs: MESOS-8125
https://issues.apache.org/jira/browse/MESOS-8125
Repository: mesos
Description (updated)
-------
Reaped Docker executor only when it can be connected.
Diffs (updated)
-----
src/slave/containerizer/docker.cpp f1d7d3e6afa119a6a24b054dcaa5ee68dbea965d
Diff: https://reviews.apache.org/r/65382/diff/2/
Changes: https://reviews.apache.org/r/65382/diff/1-2/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 65382: Reaped Docker executor only when it can be
connected.
Posted by Qian Zhang <zh...@gmail.com>.
> On Jan. 31, 2018, 6:26 a.m., Vinod Kone wrote:
> > The blocking review https://reviews.apache.org/r/65384/ seems non-existent?
Yeah, that seems a bug in review board. So I posted it again as another patch: https://reviews.apache.org/r/65420/.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65382/#review196538
-----------------------------------------------------------
On Jan. 31, 2018, 9:14 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65382/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2018, 9:14 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-8125
> https://issues.apache.org/jira/browse/MESOS-8125
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reaped Docker executor only when it can be connected.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.cpp f1d7d3e6afa119a6a24b054dcaa5ee68dbea965d
>
>
> Diff: https://reviews.apache.org/r/65382/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 65382: Reaped Docker executor only when it can be
connected.
Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65382/#review196538
-----------------------------------------------------------
The blocking review https://reviews.apache.org/r/65384/ seems non-existent?
- Vinod Kone
On Jan. 30, 2018, 3:42 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65382/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2018, 3:42 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-8125
> https://issues.apache.org/jira/browse/MESOS-8125
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reaped Docker executor only when it can be connected.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.cpp f1d7d3e6afa119a6a24b054dcaa5ee68dbea965d
>
>
> Diff: https://reviews.apache.org/r/65382/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 65382: Reaped Docker executor only when it can be
connected.
Posted by Qian Zhang <zh...@gmail.com>.
> On Jan. 31, 2018, 6:25 a.m., Vinod Kone wrote:
> > src/slave/containerizer/docker.cpp
> > Lines 979 (patched)
> > <https://reviews.apache.org/r/65382/diff/2/?file=1950458#file1950458line979>
> >
> > Do we return a "failed" container termination here? AFAICT, we return `None` on `wait` in this case?
Right, we return `None` in `DockerContainerizerProcess::wait`, I am not sure why it was a `failed 'ContainerTermination'`, let me correct it.
> On Jan. 31, 2018, 6:25 a.m., Vinod Kone wrote:
> > src/slave/containerizer/docker.cpp
> > Lines 1047 (patched)
> > <https://reviews.apache.org/r/65382/diff/2/?file=1950458#file1950458line1047>
> >
> > s/reboots/reboots or restarts/ ?
I think they are same, right? The point here is the agent host is rebooted rather than just agent process is restarted which will not cause the executor pid reused by an irrelevant process.
> On Jan. 31, 2018, 6:25 a.m., Vinod Kone wrote:
> > src/slave/containerizer/docker.cpp
> > Lines 1052-1054 (patched)
> > <https://reviews.apache.org/r/65382/diff/2/?file=1950458#file1950458line1052>
> >
> > LOG(WARNING) << "Failed to connect to executor '" << executor.id
> > << "' of framework " << framework.id
> >
> > Can we log the SocketError here as well?
Sure.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65382/#review196536
-----------------------------------------------------------
On Jan. 31, 2018, 9:14 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65382/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2018, 9:14 a.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-8125
> https://issues.apache.org/jira/browse/MESOS-8125
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reaped Docker executor only when it can be connected.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.cpp f1d7d3e6afa119a6a24b054dcaa5ee68dbea965d
>
>
> Diff: https://reviews.apache.org/r/65382/diff/3/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 65382: Reaped Docker executor only when it can be
connected.
Posted by Vinod Kone <vi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65382/#review196536
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/docker.cpp
Line 975 (original), 976 (patched)
<https://reviews.apache.org/r/65382/#comment276219>
s/in this case/in which case/
src/slave/containerizer/docker.cpp
Line 976 (original), 977 (patched)
<https://reviews.apache.org/r/65382/#comment276220>
s/not error/safe to skip/
src/slave/containerizer/docker.cpp
Lines 979 (patched)
<https://reviews.apache.org/r/65382/#comment276218>
Do we return a "failed" container termination here? AFAICT, we return `None` on `wait` in this case?
src/slave/containerizer/docker.cpp
Lines 1040-1048 (patched)
<https://reviews.apache.org/r/65382/#comment276221>
I moved this comment block to #1027
src/slave/containerizer/docker.cpp
Lines 1041 (patched)
<https://reviews.apache.org/r/65382/#comment276222>
s/, this/. This/
src/slave/containerizer/docker.cpp
Lines 1047 (patched)
<https://reviews.apache.org/r/65382/#comment276223>
s/reboots/reboots or restarts/ ?
src/slave/containerizer/docker.cpp
Lines 1052-1054 (patched)
<https://reviews.apache.org/r/65382/#comment276224>
LOG(WARNING) << "Failed to connect to executor '" << executor.id
<< "' of framework " << framework.id
Can we log the SocketError here as well?
- Vinod Kone
On Jan. 30, 2018, 3:42 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65382/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2018, 3:42 p.m.)
>
>
> Review request for mesos and Vinod Kone.
>
>
> Bugs: MESOS-8125
> https://issues.apache.org/jira/browse/MESOS-8125
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Reaped Docker executor only when it can be connected.
>
>
> Diffs
> -----
>
> src/slave/containerizer/docker.cpp f1d7d3e6afa119a6a24b054dcaa5ee68dbea965d
>
>
> Diff: https://reviews.apache.org/r/65382/diff/2/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 65382: Reaped Docker executor only when it can be
connected.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65382/
-----------------------------------------------------------
(Updated Jan. 31, 2018, 9:14 a.m.)
Review request for mesos and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-8125
https://issues.apache.org/jira/browse/MESOS-8125
Repository: mesos
Description
-------
Reaped Docker executor only when it can be connected.
Diffs (updated)
-----
src/slave/containerizer/docker.cpp f1d7d3e6afa119a6a24b054dcaa5ee68dbea965d
Diff: https://reviews.apache.org/r/65382/diff/3/
Changes: https://reviews.apache.org/r/65382/diff/2-3/
Testing
-------
sudo make check
Thanks,
Qian Zhang
Re: Review Request 65382: Reaped Docker executor only when it can be
connected.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65382/
-----------------------------------------------------------
(Updated Jan. 30, 2018, 11:42 p.m.)
Review request for mesos and Vinod Kone.
Bugs: MESOS-8125
https://issues.apache.org/jira/browse/MESOS-8125
Repository: mesos
Description
-------
Reaped Docker executor only when it can be connected.
Diffs
-----
src/slave/containerizer/docker.cpp f1d7d3e6afa119a6a24b054dcaa5ee68dbea965d
Diff: https://reviews.apache.org/r/65382/diff/2/
Testing (updated)
-------
sudo make check
Thanks,
Qian Zhang