Re: Review Request 65382: Reaped Docker executor only when it can be connected.

[Qian Zhang]

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

[Qian Zhang]

> On Jan. 31, 2018, 6:25 a.m., Vinod Kone wrote:
> > src/slave/containerizer/docker.cpp
> > Lines 979 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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)
> > 
> >
> > 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.

[Qian Zhang]

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

[Vinod Kone]

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

[Vinod Kone]

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


s/in this case/in which case/



src/slave/containerizer/docker.cpp
Line 976 (original), 977 (patched)


s/not error/safe to skip/



src/slave/containerizer/docker.cpp
Lines 979 (patched)


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)


I moved this comment block to #1027



src/slave/containerizer/docker.cpp
Lines 1041 (patched)


s/, this/. This/



src/slave/containerizer/docker.cpp
Lines 1047 (patched)


s/reboots/reboots or restarts/ ?



src/slave/containerizer/docker.cpp
Lines 1052-1054 (patched)


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.

[Qian Zhang]

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

Re: Review Request 65382: Reaped Docker executor only when it can be connected.

[Qian Zhang]

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