Re: Review Request 66283: Added support of `max_completion_time` in docker executor.

2018-05-01 Thread Zhitao Li 

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

2018-04-30 Thread James Peach 

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


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)


Two many newlines :)



src/docker/executor.cpp
Lines 422 (patched)


```
" which exceeded its ..."
```



src/docker/executor.cpp
Lines 427 (patched)


Nits: capitalize this sentence and add a blank line above it.



src/docker/executor.cpp
Lines 575 (patched)


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.

2018-04-23 Thread Zhitao Li 

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

2018-04-23 Thread Zhitao Li 

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

2018-04-13 Thread James Peach 

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




src/docker/executor.cpp
Lines 405 (patched)


Suggest:
```
Killing task $TASKID which exceeded its maximum completion time of $DURATION
```



src/docker/executor.cpp
Lines 412 (patched)


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


Move this up into the `if`.



src/docker/executor.cpp
Lines 571 (patched)


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)


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.

2018-04-07 Thread Zhitao Li 

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