You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2018/03/26 18:04:59 UTC
Review Request 66283: Added support of max_duration in docker
executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66283/
-----------------------------------------------------------
Review request for mesos, Jason Lai and James Peach.
Bugs: MESOS-8725
https://issues.apache.org/jira/browse/MESOS-8725
Repository: mesos
Description
-------
Added support of max_duration in docker executor.
Diffs
-----
src/docker/executor.cpp 1d672112bae889cc9d19343a59e0ff66569785c4
Diff: https://reviews.apache.org/r/66283/diff/1/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66283: Added support of `max_completion_time` in
docker executor.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66283/#review201126
-----------------------------------------------------------
src/docker/executor.cpp
Lines 405 (patched)
<https://reviews.apache.org/r/66283/#comment282117>
Suggest:
```
Killing task $TASKID which exceeded its maximum completion time of $DURATION
```
src/docker/executor.cpp
Lines 412 (patched)
<https://reviews.apache.org/r/66283/#comment282113>
`shutdown()` runs `killTask()` with a grace period, so I think that you need to do more here.
Something like this would work:
```
killed = true;
killTask(driver, taskId, 0 /* grace */);
```
src/docker/executor.cpp
Lines 500 (patched)
<https://reviews.apache.org/r/66283/#comment282114>
Move this up into the `if`.
src/docker/executor.cpp
Lines 571 (patched)
<https://reviews.apache.org/r/66283/#comment282115>
You can omit this block since it can't happen because you only get here when `failReason` is `None`.
src/docker/executor.cpp
Lines 830 (patched)
<https://reviews.apache.org/r/66283/#comment282116>
Like the commant executor, I think you might as well be more explicit here with a `killedByCompletionTimeout` flag.
- James Peach
On April 8, 2018, 12:52 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66283/
> -----------------------------------------------------------
>
> (Updated April 8, 2018, 12:52 a.m.)
>
>
> Review request for mesos, Jason Lai and James Peach.
>
>
> Bugs: MESOS-8725
> https://issues.apache.org/jira/browse/MESOS-8725
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If `TaskInfo.max_completion_time` is set, docker executor will kill
> the task immediately. We reuse the `shutdown` method to achieve a
> forced kill ignoring any `KillPolicy`.
>
> Framework should only received a `TASK_FAILED` state with
> `REASON_MAX_COMPLETION_TIME_REACHED` reason.
>
>
> Diffs
> -----
>
> src/docker/executor.cpp 1d672112bae889cc9d19343a59e0ff66569785c4
>
>
> Diff: https://reviews.apache.org/r/66283/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66283: Added support of `max_completion_time` in
docker executor.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66283/#review202152
-----------------------------------------------------------
Fix it, then Ship it!
src/docker/executor.cpp
Lines 418 (patched)
<https://reviews.apache.org/r/66283/#comment283818>
As discussed on chat, we probably don't need to log here, just return if we already made the state transition.
src/docker/executor.cpp
Lines 420 (patched)
<https://reviews.apache.org/r/66283/#comment283790>
Two many newlines :)
src/docker/executor.cpp
Lines 422 (patched)
<https://reviews.apache.org/r/66283/#comment283784>
```
" which exceeded its ..."
```
src/docker/executor.cpp
Lines 427 (patched)
<https://reviews.apache.org/r/66283/#comment283788>
Nits: capitalize this sentence and add a blank line above it.
src/docker/executor.cpp
Lines 575 (patched)
<https://reviews.apache.org/r/66283/#comment283785>
This should be `!killedByTaskCompletionTimeout`, right?
- James Peach
On April 23, 2018, 10:25 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66283/
> -----------------------------------------------------------
>
> (Updated April 23, 2018, 10:25 p.m.)
>
>
> Review request for mesos, Jason Lai and James Peach.
>
>
> Bugs: MESOS-8725
> https://issues.apache.org/jira/browse/MESOS-8725
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If `TaskInfo.max_completion_time` is set, docker executor will kill
> the task immediately. We reuse the `shutdown` method to achieve a
> forced kill ignoring any `KillPolicy`.
>
> Framework should only received a `TASK_FAILED` state with
> `REASON_MAX_COMPLETION_TIME_REACHED` reason.
>
>
> Diffs
> -----
>
> src/docker/executor.cpp 1d672112bae889cc9d19343a59e0ff66569785c4
>
>
> Diff: https://reviews.apache.org/r/66283/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 66283: Added support of `max_completion_time` in
docker executor.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66283/
-----------------------------------------------------------
(Updated May 1, 2018, 9:42 a.m.)
Review request for mesos, Jason Lai and James Peach.
Changes
-------
Review comments.
Bugs: MESOS-8725
https://issues.apache.org/jira/browse/MESOS-8725
Repository: mesos
Description
-------
If `TaskInfo.max_completion_time` is set, docker executor will kill
the task immediately. We reuse the `shutdown` method to achieve a
forced kill ignoring any `KillPolicy`.
Framework should only received a `TASK_FAILED` state with
`REASON_MAX_COMPLETION_TIME_REACHED` reason.
Diffs (updated)
-----
src/docker/executor.cpp 1d672112bae889cc9d19343a59e0ff66569785c4
Diff: https://reviews.apache.org/r/66283/diff/5/
Changes: https://reviews.apache.org/r/66283/diff/4-5/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66283: Added support of `max_completion_time` in
docker executor.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66283/
-----------------------------------------------------------
(Updated April 23, 2018, 3:25 p.m.)
Review request for mesos, Jason Lai and James Peach.
Bugs: MESOS-8725
https://issues.apache.org/jira/browse/MESOS-8725
Repository: mesos
Description
-------
If `TaskInfo.max_completion_time` is set, docker executor will kill
the task immediately. We reuse the `shutdown` method to achieve a
forced kill ignoring any `KillPolicy`.
Framework should only received a `TASK_FAILED` state with
`REASON_MAX_COMPLETION_TIME_REACHED` reason.
Diffs (updated)
-----
src/docker/executor.cpp 1d672112bae889cc9d19343a59e0ff66569785c4
Diff: https://reviews.apache.org/r/66283/diff/4/
Changes: https://reviews.apache.org/r/66283/diff/3-4/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66283: Added support of `max_completion_time` in
docker executor.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66283/
-----------------------------------------------------------
(Updated April 23, 2018, 3:19 p.m.)
Review request for mesos, Jason Lai and James Peach.
Bugs: MESOS-8725
https://issues.apache.org/jira/browse/MESOS-8725
Repository: mesos
Description
-------
If `TaskInfo.max_completion_time` is set, docker executor will kill
the task immediately. We reuse the `shutdown` method to achieve a
forced kill ignoring any `KillPolicy`.
Framework should only received a `TASK_FAILED` state with
`REASON_MAX_COMPLETION_TIME_REACHED` reason.
Diffs (updated)
-----
src/docker/executor.cpp 1d672112bae889cc9d19343a59e0ff66569785c4
Diff: https://reviews.apache.org/r/66283/diff/3/
Changes: https://reviews.apache.org/r/66283/diff/2-3/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 66283: Added support of `max_completion_time` in
docker executor.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66283/
-----------------------------------------------------------
(Updated April 7, 2018, 5:52 p.m.)
Review request for mesos, Jason Lai and James Peach.
Changes
-------
Rename and proper state.
Summary (updated)
-----------------
Added support of `max_completion_time` in docker executor.
Bugs: MESOS-8725
https://issues.apache.org/jira/browse/MESOS-8725
Repository: mesos
Description (updated)
-------
If `TaskInfo.max_completion_time` is set, docker executor will kill
the task immediately. We reuse the `shutdown` method to achieve a
forced kill ignoring any `KillPolicy`.
Framework should only received a `TASK_FAILED` state with
`REASON_MAX_COMPLETION_TIME_REACHED` reason.
Diffs (updated)
-----
src/docker/executor.cpp 1d672112bae889cc9d19343a59e0ff66569785c4
Diff: https://reviews.apache.org/r/66283/diff/2/
Changes: https://reviews.apache.org/r/66283/diff/1-2/
Testing
-------
Thanks,
Zhitao Li