Re: Review Request 58898: Send task kill for non-Partition Aware frameworks.

[Megha Sharma]

> On May 10, 2017, 11:24 p.m., Neil Conway wrote:
> > My apologies for the delay in reviewing this.
> > 
> > High-level comments:
> > 
> > (a) Can we improve the description of the problem in the commit summary? It 
> > took me quite a while to figure out what is actually going on here. My 
> > understanding is:
> > 
> > (1) Agent re-registers
> > (2) Master sends `ShutdownFrameworkMessage` for non-PA frameworks on the 
> > agent
> > (3) Master offers agent resources to framework
> > (4) Framework launches new task on agent _before the agent has finished 
> > shutting down the framework_
> > (5) Agent ignores launch task because it believes the framework is still 
> > terminating.
> > 
> > This is a race condition, because if the agent finished shutting down the 
> > framework (in #4) before the launch task was received, there would not be a 
> > problem. Is my understanding correct?
> > 
> > (2) I'd prefer a new unit test that constructs exactly this scenario, 
> > rather than changing existing unit tests.
> > 
> > (3) The new behavior is that the framework will receive `TASK_KILLED` for 
> > any non-PA tasks running on a partitioned agent that re-registers. This 
> > does not seem ideal, because `TASK_KILLED` _normally_ corresponds to a 
> > framework-initiated `killTask` operation.
> > 
> > (4) Thinking out loud, what if a custom executor ignores the `killTask` 
> > request? When shutting down a framework, it seems we forcibly destroy the 
> > container (via `containerizer->destroy()`), if the executor doesn't exit 
> > promptly upon receiving the framework shutdown request. AFAIK we don't have 
> > similar logic for `killTask` requests.
> > 
> > 3 + 4 suggests that maybe we want a different way to terminate the task on 
> > the agent -- let's discuss.
> 
> Megha Sharma wrote:
> Summarizing the 2 approaches we talked about.
> 
> Approach 1: ShutdownFrameworkMessage
> 
> 1. Upon agent re-registration, master will add tasks even for non-PA 
> frameworks on this agent. This is needed by the master to do correct resource 
> accounting and not offer resources already in use on this agent. We need to 
> mutate the TaskState on the Task before adding them to the master's data 
> structures since the TaskState might be non-terminal when the agent sends 
> these tasks with ReregisterSlaveMessage. And the master has already sent 
> TASK_LOST for these tasks to the frameworks so we need to set the TaskState 
> to TASK_LOST so that any future reconciliations with the framework doesn't 
> have this task transitioning from TASK_LOST to TASK_RUNNNG/TASK_STARTING. 
> This is to avoid unnecessary confusion about task state as observed by the 
> framework but indeed this could have happened with non-strict registry as 
> well where the framework can actually receive a non terminal task state 
> update after receiving a TASK_LOST for the same task in the past.
> 
> 2. When the agent re-registers, the master will continue to send a 
> ShutdownFrameworkMessage to the agent to kill the tasks pertaining to non-PA 
> frameworks on the agent as it does today. An additional optional field will 
> be added to the ShutdownFrameworkMessage to indicate whether or not the 
> shutdown was initiated internally.
> 
> 3. During framework shutdown the state of the framework is set to 
> Framework::TERMINATING which prevents it from launching new tasks. Here, 
> since the framework is not really terminating so in order to allow it to 
> launch new tasks, the agent will not set the state to terminating if the 
> ShutdownFrameworkMessage is generated internally.
> 
> 4. The framework shutdown today doesn't generate any status updates which 
> needs to change. The status updates will be sent if the framework shutdown is 
> triggered internally, this is needed to remove the tasks of non-PA frameworks 
> that got added when the agent re-registered (1).
> 
> Approach 2: Do not shutdown non-PA framework when agent re-registers and 
> let the frameworks make the decision on what needs to be done when they 
> receive non-terminal status updates for tasks for which they have already 
> received a TASK_LOST. This hopefully won't break any frameworks since it 
> could have happened in the past with non-strict registry as well and 
> frameworks should be resilient enough to handle this scenario.
> 
> Let me know if I have missed anything. Personally, I am in favor of 
> approach 1 as it seems less catastrophic for the frameworks. What do you 
> think?

The proposed solution to fix the race between new task launches and shutdown 
framework on the agent, was to not kill the non-partition aware tasks when an 
unreachable agent re-registers with the master. So now as far as Mesos 
internals are concerned, the tasks from non-partition aware frameworks are to 
be treated the same way as partition aware tasks and one way to do it cleanly 
in Mesos was to transition such non-partition aware tasks to

Re: Review Request 58898: Send task kill for non-Partition Aware frameworks.

[Megha Sharma]

> On May 10, 2017, 11:24 p.m., Neil Conway wrote:
> > My apologies for the delay in reviewing this.
> > 
> > High-level comments:
> > 
> > (a) Can we improve the description of the problem in the commit summary? It 
> > took me quite a while to figure out what is actually going on here. My 
> > understanding is:
> > 
> > (1) Agent re-registers
> > (2) Master sends `ShutdownFrameworkMessage` for non-PA frameworks on the 
> > agent
> > (3) Master offers agent resources to framework
> > (4) Framework launches new task on agent _before the agent has finished 
> > shutting down the framework_
> > (5) Agent ignores launch task because it believes the framework is still 
> > terminating.
> > 
> > This is a race condition, because if the agent finished shutting down the 
> > framework (in #4) before the launch task was received, there would not be a 
> > problem. Is my understanding correct?
> > 
> > (2) I'd prefer a new unit test that constructs exactly this scenario, 
> > rather than changing existing unit tests.
> > 
> > (3) The new behavior is that the framework will receive `TASK_KILLED` for 
> > any non-PA tasks running on a partitioned agent that re-registers. This 
> > does not seem ideal, because `TASK_KILLED` _normally_ corresponds to a 
> > framework-initiated `killTask` operation.
> > 
> > (4) Thinking out loud, what if a custom executor ignores the `killTask` 
> > request? When shutting down a framework, it seems we forcibly destroy the 
> > container (via `containerizer->destroy()`), if the executor doesn't exit 
> > promptly upon receiving the framework shutdown request. AFAIK we don't have 
> > similar logic for `killTask` requests.
> > 
> > 3 + 4 suggests that maybe we want a different way to terminate the task on 
> > the agent -- let's discuss.

Summarizing the 2 approaches we talked about.

Approach 1: ShutdownFrameworkMessage

1. Upon agent re-registration, master will add tasks even for non-PA frameworks 
on this agent. This is needed by the master to do correct resource accounting 
and not offer resources already in use on this agent. We need to mutate the 
TaskState on the Task before adding them to the master's data structures since 
the TaskState might be non-terminal when the agent sends these tasks with 
ReregisterSlaveMessage. And the master has already sent TASK_LOST for these 
tasks to the frameworks so we need to set the TaskState to TASK_LOST so that 
any future reconciliations with the framework doesn't have this task 
transitioning from TASK_LOST to TASK_RUNNNG/TASK_STARTING. This is to avoid 
unnecessary confusion about task state as observed by the framework but indeed 
this could have happened with non-strict registry as well where the framework 
can actually receive a non terminal task state update after receiving a 
TASK_LOST for the same task in the past.

2. When the agent re-registers, the master will continue to send a 
ShutdownFrameworkMessage to the agent to kill the tasks pertaining to non-PA 
frameworks on the agent as it does today. An additional optional field will be 
added to the ShutdownFrameworkMessage to indicate whether or not the shutdown 
was initiated internally.

3. During framework shutdown the state of the framework is set to 
Framework::TERMINATING which prevents it from launching new tasks. Here, since 
the framework is not really terminating so in order to allow it to launch new 
tasks, the agent will not set the state to terminating if the 
ShutdownFrameworkMessage is generated internally.

4. The framework shutdown today doesn't generate any status updates which needs 
to change. The status updates will be sent if the framework shutdown is 
triggered internally, this is needed to remove the tasks of non-PA frameworks 
that got added when the agent re-registered (1).

Approach 2: Do not shutdown non-PA framework when agent re-registers and let 
the frameworks make the decision on what needs to be done when they receive 
non-terminal status updates for tasks for which they have already received a 
TASK_LOST. This hopefully won't break any frameworks since it could have 
happened in the past with non-strict registry as well and frameworks should be 
resilient enough to handle this scenario.

Let me know if I have missed anything. Personally, I am in favor of approach 1 
as it seems less catastrophic for the frameworks. What do you think?


- Megha


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


On May 1, 2017, 11:58 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58898/
> ---
> 
> (Updated May 1, 2017, 11:58 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 

Re: Review Request 58898: Send task kill for non-Partition Aware frameworks.

[Neil Conway]

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



My apologies for the delay in reviewing this.

High-level comments:

(a) Can we improve the description of the problem in the commit summary? It 
took me quite a while to figure out what is actually going on here. My 
understanding is:

(1) Agent re-registers
(2) Master sends `ShutdownFrameworkMessage` for non-PA frameworks on the agent
(3) Master offers agent resources to framework
(4) Framework launches new task on agent _before the agent has finished 
shutting down the framework_
(5) Agent ignores launch task because it believes the framework is still 
terminating.

This is a race condition, because if the agent finished shutting down the 
framework (in #4) before the launch task was received, there would not be a 
problem. Is my understanding correct?

(2) I'd prefer a new unit test that constructs exactly this scenario, rather 
than changing existing unit tests.

(3) The new behavior is that the framework will receive `TASK_KILLED` for any 
non-PA tasks running on a partitioned agent that re-registers. This does not 
seem ideal, because `TASK_KILLED` _normally_ corresponds to a 
framework-initiated `killTask` operation.

(4) Thinking out loud, what if a custom executor ignores the `killTask` 
request? When shutting down a framework, it seems we forcibly destroy the 
container (via `containerizer->destroy()`), if the executor doesn't exit 
promptly upon receiving the framework shutdown request. AFAIK we don't have 
similar logic for `killTask` requests.

3 + 4 suggests that maybe we want a different way to terminate the task on the 
agent -- let's discuss.


src/master/master.cpp
Lines 6034 (patched)


`tasksToKill`



src/master/master.cpp
Lines 6057 (patched)


"Keep"



src/master/master.cpp
Lines 6058 (patched)


"separate data structure"



src/master/master.cpp
Line 6097 (original), 6102 (patched)


"launched by"



src/tests/partition_tests.cpp
Lines 693 (patched)


Why did you move this to before the agent re-registers? The point of doing 
explicit reconciliation is to check the master's state after the agent 
re-registers.

There's no harm in _also_ reconciling before the agent re-registers, but if 
you want to add that, let's do it in a separate review.



src/tests/partition_tests.cpp
Line 754 (original), 786 (patched)


`EXPECT_FALSE`.



src/tests/partition_tests.cpp
Line 794 (original), 826 (patched)


The point of this part of the test is to check that the agent's metrics are 
correct after re-registration (and before starting any additional tasks). I'd 
prefer to check the metrics, then start another task on the agent afterward.



src/tests/partition_tests.cpp
Line 2049 (original)


Removing this results in two gmock warnings:

```
GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: killTask(0x7fbcf1c5fa20, @0x7fbcf1c54640 1)
```

```
GMOCK WARNING:
Uninteresting mock function call - returning directly.
Function call: shutdown(0x7fbcf1c5fa20)
```


- Neil Conway


On May 1, 2017, 11:58 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58898/
> ---
> 
> (Updated May 1, 2017, 11:58 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos is now sending ShutdownFrameworkMessages to the agent for all
> non-partition-aware frameworks (including the ones that are still
> registered). This is problematic. The offer from this agent can
> still go to the same framework which can then launch new tasks.
> The agent then receives tasks of the same framework and ignores
> them because it thinks the framework is shutting down. The framework
> is not shutting down of course, so from the master and the scheduler's
> perspective the task is pending in STAGING forever until the next agent
> reregistration, which could happen much later. This commit fixes
> the problem by sending a task kill instead of ShutdownFrameworkMessage,
> when the agent re-registers after being unreachable, for non-partition
> awar

Re: Review Request 58898: Send task kill for non-Partition Aware frameworks.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58898]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On May 1, 2017, 11:58 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58898/
> ---
> 
> (Updated May 1, 2017, 11:58 p.m.)
> 
> 
> Review request for mesos, Neil Conway and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7215
> https://issues.apache.org/jira/browse/MESOS-7215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos is now sending ShutdownFrameworkMessages to the agent for all
> non-partition-aware frameworks (including the ones that are still
> registered). This is problematic. The offer from this agent can
> still go to the same framework which can then launch new tasks.
> The agent then receives tasks of the same framework and ignores
> them because it thinks the framework is shutting down. The framework
> is not shutting down of course, so from the master and the scheduler's
> perspective the task is pending in STAGING forever until the next agent
> reregistration, which could happen much later. This commit fixes
> the problem by sending a task kill instead of ShutdownFrameworkMessage,
> when the agent re-registers after being unreachable, for non-partition
> aware framewworks.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
>   src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 
> 
> 
> Diff: https://reviews.apache.org/r/58898/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 58898: Send task kill for non-Partition Aware frameworks.

[Megha Sharma]

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

(Updated May 1, 2017, 11:58 p.m.)


Review request for mesos, Neil Conway and Jiang Yan Xu.


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


Repository: mesos


Description
---

Mesos is now sending ShutdownFrameworkMessages to the agent for all
non-partition-aware frameworks (including the ones that are still
registered). This is problematic. The offer from this agent can
still go to the same framework which can then launch new tasks.
The agent then receives tasks of the same framework and ignores
them because it thinks the framework is shutting down. The framework
is not shutting down of course, so from the master and the scheduler's
perspective the task is pending in STAGING forever until the next agent
reregistration, which could happen much later. This commit fixes
the problem by sending a task kill instead of ShutdownFrameworkMessage,
when the agent re-registers after being unreachable, for non-partition
aware framewworks.


Diffs (updated)
-

  src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
  src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 


Diff: https://reviews.apache.org/r/58898/diff/2/

Changes: https://reviews.apache.org/r/58898/diff/1-2/


Testing
---

make check


Thanks,

Megha Sharma

Review Request 58898: Send task kill for non-Partition Aware frameworks.

[Megha Sharma]

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

Review request for mesos, Neil Conway and Jiang Yan Xu.


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


Repository: mesos


Description
---

Mesos is now sending ShutdownFrameworkMessages to the agent for all
non-partition-aware frameworks (including the ones that are still
registered). This is problematic. The offer from this agent can
still go to the same framework which can then launch new tasks.
The agent then receives tasks of the same framework and ignores
them because it thinks the framework is shutting down. The framework
is not shutting down of course, so from the master and the scheduler's
perspective the task is pending in STAGING forever until the next agent
reregistration, which could happen much later. This commit fixes
the problem by sending a task kill instead of ShutdownFrameworkMessage,
when the agent re-registers after being unreachable, for non-partition
aware framewworks.


Diffs
-

  src/master/master.cpp 31a7a2fcf905c0c35e80692a69c290d4094deded 
  src/tests/partition_tests.cpp 4ff428564d1fa6cb96e6f8ec8edc331da88a3eb6 


Diff: https://reviews.apache.org/r/58898/diff/1/


Testing
---

make check


Thanks,

Megha Sharma