Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 285-290
> > 
> >
> > No need for the note here. Can you also move this down into the 
> > conditional that gates it below?
> > 
> > ```
> > // Issue the kill signal if the container is running
> > // and this is the first time we've received the kill.
> > if (run.isSome() && !terminated && !killing) {
> > ```
> > 
> > Why are we issuing a kill to the health pid every time? It seems like 
> > it also needs to be guarded by this condition :(
> 
> Alexander Rukletsov wrote:
> The reason I've added this note (and also the note above) is to provide 
> some context and thought for a reader, especially for someone who is going to 
> implement kill policy override in the docker executor. The note below tries 
> to explain that there is actually one more state (being terminated but not 
> yet terminated) that we ignore for now but may (and should!) add in the 
> future.
> 
> Or is your proposal to move this comment into a separate patch?

Filed https://issues.apache.org/jira/browse/MESOS-5355


- Alexander


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


On May 10, 2016, 2:15 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 10, 2016, 2:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

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

(Updated May 10, 2016, 2:15 p.m.)


Review request for mesos, Benjamin Bannier and Ben Mahler.


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


Repository: mesos


Description
---

Capture the state when a task has terminated (i.e., reaped) in a flag,
and use it to prevent calls to `killTask()` and `escalated()` when
they are executed after the task has terminated.


Diffs
-

  src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
  src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
  src/launcher/http_command_executor.cpp 
d2f15b0447d91f3a4cd92f07114cb366647cc7d3 

Diff: https://reviews.apache.org/r/46491/diff/


Testing
---

`make check` on Mac OS 10.10.4


Thanks,

Alexander Rukletsov

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 292-294
> > 
> >
> > This is confusing to me, it just means a retry as far as this patch is 
> > concerned. Can you remove this TODO?
> 
> Alexander Rukletsov wrote:
> See above.

The comment is food for thought . There is actually one more state, which we 
ignore for now, but I would like to make readers aware about it. People look at 
this code when they implement their own executors. I have rephrased it for 
clarity.


- Alexander


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


On May 7, 2016, 1:02 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 1:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

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

(Updated May 10, 2016, 1:37 p.m.)


Review request for mesos, Benjamin Bannier and Ben Mahler.


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


Repository: mesos


Description
---

Capture the state when a task has terminated (i.e., reaped) in a flag,
and use it to prevent calls to `killTask()` and `escalated()` when
they are executed after the task has terminated.


Diffs (updated)
-

  src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
  src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
  src/launcher/http_command_executor.cpp 
d2f15b0447d91f3a4cd92f07114cb366647cc7d3 

Diff: https://reviews.apache.org/r/46491/diff/


Testing
---

`make check` on Mac OS 10.10.4


Thanks,

Alexander Rukletsov

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Ben Mahler]

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


Ship it!




Modulo comments.

- Ben Mahler


On May 7, 2016, 1:02 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 1:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

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




src/docker/executor.cpp (lines 396 - 397)


Consider starting health checks even if we have already been killed to 
ensure that tasks are health checked while in their kill grace period.



src/launcher/executor.cpp (lines 516 - 518)


If a kill is in progress, consider adjusting the grace period if a new one 
is provided.


- Alexander Rukletsov


On May 7, 2016, 1:02 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 1:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[haosdent huang]

> On May 7, 2016, 1:22 p.m., haosdent huang wrote:
> > src/docker/executor.cpp, line 398
> > 
> >
> > Should it change to `if (killed || terminated ||..)`?
> 
> Alexander Rukletsov wrote:
> Currently, if `terminated == true` then `killed == true` as well. There 
> is no need to introduce an extra condition.

Got it, thx!


- haosdent


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


On May 7, 2016, 1:02 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 1:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

> On May 7, 2016, 1:22 p.m., haosdent huang wrote:
> > src/docker/executor.cpp, line 398
> > 
> >
> > Should it change to `if (killed || terminated ||..)`?

Currently, if `terminated == true` then `killed == true` as well. There is no 
need to introduce an extra condition.


- Alexander


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


On May 7, 2016, 1:02 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 1:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[haosdent huang]

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




src/docker/executor.cpp (line 398)


Should it change to `if (killed || terminated ||..)`?


- haosdent huang


On May 7, 2016, 1:02 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 1:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

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

(Updated May 7, 2016, 1:02 p.m.)


Review request for mesos, Benjamin Bannier and Ben Mahler.


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


Repository: mesos


Description
---

Capture the state when a task has terminated (i.e., reaped) in a flag,
and use it to prevent calls to `killTask()` and `escalated()` when
they are executed after the task has terminated.


Diffs (updated)
-

  src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
  src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
  src/launcher/http_command_executor.cpp 
d2f15b0447d91f3a4cd92f07114cb366647cc7d3 

Diff: https://reviews.apache.org/r/46491/diff/


Testing
---

`make check` on Mac OS 10.10.4


Thanks,

Alexander Rukletsov

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 285-290
> > 
> >
> > No need for the note here. Can you also move this down into the 
> > conditional that gates it below?
> > 
> > ```
> > // Issue the kill signal if the container is running
> > // and this is the first time we've received the kill.
> > if (run.isSome() && !terminated && !killing) {
> > ```
> > 
> > Why are we issuing a kill to the health pid every time? It seems like 
> > it also needs to be guarded by this condition :(

The reason I've added this note (and also the note above) is to provide some 
context and thought for a reader, especially for someone who is going to 
implement kill policy override in the docker executor. The note below tries to 
explain that there is actually one more state (being terminated but not yet 
terminated) that we ignore for now but may (and should!) add in the future.

Or is your proposal to move this comment into a separate patch?


> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/docker/executor.cpp, lines 292-294
> > 
> >
> > This is confusing to me, it just means a retry as far as this patch is 
> > concerned. Can you remove this TODO?

See above.


> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/launcher/executor.cpp, lines 509-521
> > 
> >
> > Ditto from above: can you move the terminated check into the condition 
> > and update the comment?

See above.


> On May 5, 2016, 6:08 p.m., Ben Mahler wrote:
> > src/launcher/http_command_executor.cpp, lines 597-609
> > 
> >
> > Ditto from above.

See above.


- Alexander


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


On May 7, 2016, 12:50 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 12:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

[Alexander Rukletsov]

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

(Updated May 7, 2016, 12:50 p.m.)


Review request for mesos, Benjamin Bannier and Ben Mahler.


Summary (updated)
-

Ensured subsequent kills are ignored after the task is reaped.


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


Repository: mesos


Description (updated)
---

Capture the state when a task has terminated (i.e., reaped) in a flag,
and use it to prevent calls to `killTask()` and `escalated()` when
they are executed after the task has terminated.


Diffs (updated)
-

  src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
  src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
  src/launcher/http_command_executor.cpp 
d2f15b0447d91f3a4cd92f07114cb366647cc7d3 

Diff: https://reviews.apache.org/r/46491/diff/


Testing
---

`make check` on Mac OS 10.10.4


Thanks,

Alexander Rukletsov