You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2014/08/05 23:29:44 UTC

Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()

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



src/slave/slave.hpp
<https://reviews.apache.org/r/23912/#comment86682>

    Can you add a comment here saying you made these virtual so that you can mock the Slave?



src/slave/slave.cpp
<https://reviews.apache.org/r/23912/#comment86683>

    thank you.



src/slave/slave.cpp
<https://reviews.apache.org/r/23912/#comment86882>

    I would recommend pulling this logic outside 'if (executor == NULL)' to #1326 to make it easy to reason about.
    
    also, task ids are globally unique. there shouldn't be multiple executors with the same task id. you could actually do a CHECK(executorIds.size() == 1) to confirm that.



src/slave/slave.cpp
<https://reviews.apache.org/r/23912/#comment86884>

    s/executor is running/it was launched/



src/slave/slave.cpp
<https://reviews.apache.org/r/23912/#comment86883>

    s/Task killed early/Task killed before it was launched/



src/tests/mesos.hpp
<https://reviews.apache.org/r/23912/#comment86873>

    



src/tests/mesos.hpp
<https://reviews.apache.org/r/23912/#comment86874>

    



src/tests/mesos.hpp
<https://reviews.apache.org/r/23912/#comment86875>

    



src/tests/mesos.cpp
<https://reviews.apache.org/r/23912/#comment86860>

    end with a period.



src/tests/mesos.cpp
<https://reviews.apache.org/r/23912/#comment86861>

    were you unable to directly call Slave::* methods here instead of having the redirection through unmocked_* methods?



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86863>

    unused?



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86864>

    To simplify this boiler plate you could use the LaunchTasks action. See TerminatingSlaveDoesNotReregister test in this file for an example.



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86871>

    Kill these expectations since an executor won't be launched in this test.



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86867>

    s/Slave::_runTask/'Slave::_runTask()'/



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86868>

    s/Slave::_runTask/'Slave::_runTask()'/



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86876>

    s/Slave::runTask/'Slave::runTask()'/



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86877>

    s/Slave::killTask/'Slave::killTask()'/



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86878>

    s/MockedSlave::unmocked__runTask/'MockedSlave::unmocked__runTask()'/



src/tests/slave_tests.cpp
<https://reviews.apache.org/r/23912/#comment86872>

    kill this.


- Vinod Kone


On July 28, 2014, 8:24 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23912/
> -----------------------------------------------------------
> 
> (Updated July 28, 2014, 8:24 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-947
>     https://issues.apache.org/jira/browse/MESOS-947
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Fixes MESOS-947 "Slave should properly handle a killTask() that arrives between runTask() and _runTask()".
> 
> Slave::killTask() did not check for task in question combination to be "pending" (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked "LOST" instead of "KILLED". But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being "pending" is removed, and the task is marked "KILLED", and _runTask gets informed about this. It checks whether the task in question is currently "pending" and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp a896bb66db5d8cd27ef02b6498c9db93cb0d525f 
>   src/slave/slave.cpp 1d5691836822c8587e1aa8ed24860a8012c67a6e 
>   src/tests/mesos.hpp 75c66fda2485afa0d4541e710780d90b3411839a 
>   src/tests/mesos.cpp 35c94fa908ad728ea92a7d1bfcbe90d57b1b83d9 
>   src/tests/slave_tests.cpp e45255a6f699e51bf09397da95a5a11edbabe591 
> 
> Diff: https://reviews.apache.org/r/23912/diff/
> 
> 
> Testing
> -------
> 
> Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a "clean kill" now :-)
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>


Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()

Posted by Vinod Kone <vi...@gmail.com>.

> On Aug. 5, 2014, 9:29 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1337
> > <https://reviews.apache.org/r/23912/diff/3/?file=643812#file643812line1337>
> >
> >     s/executor is running/it was launched/

doesn't look like this was addressed.


> On Aug. 5, 2014, 9:29 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 1339
> > <https://reviews.apache.org/r/23912/diff/3/?file=643812#file643812line1339>
> >
> >     s/Task killed early/Task killed before it was launched/

doesn't look like this was addressed.


> On Aug. 5, 2014, 9:29 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 1330-1341
> > <https://reviews.apache.org/r/23912/diff/3/?file=643812#file643812line1330>
> >
> >     I would recommend pulling this logic outside 'if (executor == NULL)' to #1326 to make it easy to reason about.
> >     
> >     also, task ids are globally unique. there shouldn't be multiple executors with the same task id. you could actually do a CHECK(executorIds.size() == 1) to confirm that.
> 
> Vinod Kone wrote:
>     this wasn't addressed either. is there another diff coming?
> 
> Bernd Mathiske wrote:
>     Moving the code outside 'if (executor == NULL)' looks like more control flow to me, not less. There should not be any pending executor if there is an assigned one, which 'if (executor == NULL)' tests. We would then have to remember the result of the check for pending executors and take the same action in case there isn't an assigned one. What am I missing? I am dropping this part of the issue, I suppose.
>     
>     Adding CHECK(executorIds.size() == 1).

framework->getExecutor(taskId) returns NULL if the task is not assigned to the executor (i.e., _runTask() is not called yet) __or__ the executor is not running. That's what confused me the most. Also, the fact that your log line says "..before executor is running" threw me off, because thats not necessarily true.

For me, the following flow makes it easy to understand whats happening.

// If the task hasn't been launched yet send TASK_KIILED.
hashset<ExecutorID> executorIds = framework->pending.getKeys(taskId);
if (!executorIds.empty()) {
  CHECK_EQ(1, executorIds.size());
  framework->pending.remove(executorIds.front(), taskId);

  LOG(WARNING) << "Killing task " << taskId
               << " of framework " << frameworkId
               << " before executor is running";

  const StatusUpdate& update = protobuf::createStatusUpdate(
      frameworkId, info.id(), taskId, TASK_KILLED, "Task killed before it was launched");

  statusUpdate(update, UPID());
  return;
}

// If the executor doesn't exist, send TASK_LOST.
Executor* executor = framework->getExecutor(taskId);
.....same code as before.....


Let me know if I missed/misunderstood something.


- Vinod


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


On Aug. 6, 2014, 11:51 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23912/
> -----------------------------------------------------------
> 
> (Updated Aug. 6, 2014, 11:51 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-947
>     https://issues.apache.org/jira/browse/MESOS-947
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Fixes MESOS-947 "Slave should properly handle a killTask() that arrives between runTask() and _runTask()".
> 
> Slave::killTask() did not check for task in question combination to be "pending" (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked "LOST" instead of "KILLED". But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being "pending" is removed, and the task is marked "KILLED", and _runTask gets informed about this. It checks whether the task in question is currently "pending" and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp a896bb66db5d8cd27ef02b6498c9db93cb0d525f 
>   src/slave/slave.cpp 1d5691836822c8587e1aa8ed24860a8012c67a6e 
>   src/tests/mesos.hpp 75c66fda2485afa0d4541e710780d90b3411839a 
>   src/tests/mesos.cpp 35c94fa908ad728ea92a7d1bfcbe90d57b1b83d9 
>   src/tests/slave_tests.cpp e45255a6f699e51bf09397da95a5a11edbabe591 
> 
> Diff: https://reviews.apache.org/r/23912/diff/
> 
> 
> Testing
> -------
> 
> Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a "clean kill" now :-)
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>


Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()

Posted by Bernd Mathiske <be...@mesosphere.io>.

> On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 1330-1341
> > <https://reviews.apache.org/r/23912/diff/3/?file=643812#file643812line1330>
> >
> >     I would recommend pulling this logic outside 'if (executor == NULL)' to #1326 to make it easy to reason about.
> >     
> >     also, task ids are globally unique. there shouldn't be multiple executors with the same task id. you could actually do a CHECK(executorIds.size() == 1) to confirm that.
> 
> Vinod Kone wrote:
>     this wasn't addressed either. is there another diff coming?

Moving the code outside 'if (executor == NULL)' looks like more control flow to me, not less. There should not be any pending executor if there is an assigned one, which 'if (executor == NULL)' tests. We would then have to remember the result of the check for pending executors and take the same action in case there isn't an assigned one. What am I missing? I am dropping this part of the issue, I suppose.

Adding CHECK(executorIds.size() == 1).


> On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote:
> > src/tests/mesos.cpp, lines 330-335
> > <https://reviews.apache.org/r/23912/diff/3/?file=643814#file643814line330>
> >
> >     were you unable to directly call Slave::* methods here instead of having the redirection through unmocked_* methods?
> 
> Vinod Kone wrote:
>     we expect comments for dropped issues. mind adding a comment?

That is correct. Calling Slave::*() results in infinite recursion.


> On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, lines 938-957
> > <https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line938>
> >
> >     To simplify this boiler plate you could use the LaunchTasks action. See TerminatingSlaveDoesNotReregister test in this file for an example.
> 
> Vinod Kone wrote:
>     mind adding a comment?

I tried that. Cannot do that here. I need to have a variable that references the task in question.


> On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, lines 958-962
> > <https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line958>
> >
> >     Kill these expectations since an executor won't be launched in this test.
> 
> Vinod Kone wrote:
>     doesnt look like this was fixed.

I will take off the 'registered' expectation, but I prefer keeping the second one to make sure the test is tight. That includes that launchTask should not happen by accident.


- Bernd


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


On Aug. 5, 2014, 3:45 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23912/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 3:45 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-947
>     https://issues.apache.org/jira/browse/MESOS-947
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Fixes MESOS-947 "Slave should properly handle a killTask() that arrives between runTask() and _runTask()".
> 
> Slave::killTask() did not check for task in question combination to be "pending" (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked "LOST" instead of "KILLED". But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being "pending" is removed, and the task is marked "KILLED", and _runTask gets informed about this. It checks whether the task in question is currently "pending" and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp a896bb66db5d8cd27ef02b6498c9db93cb0d525f 
>   src/slave/slave.cpp 1d5691836822c8587e1aa8ed24860a8012c67a6e 
>   src/tests/mesos.hpp 75c66fda2485afa0d4541e710780d90b3411839a 
>   src/tests/mesos.cpp 35c94fa908ad728ea92a7d1bfcbe90d57b1b83d9 
>   src/tests/slave_tests.cpp e45255a6f699e51bf09397da95a5a11edbabe591 
> 
> Diff: https://reviews.apache.org/r/23912/diff/
> 
> 
> Testing
> -------
> 
> Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a "clean kill" now :-)
> 
> 
> File Attachments
> ----------------
> 
> stout patch
>   https://reviews.apache.org/media/uploaded/files/2014/08/05/5f4b6886-9a60-4ceb-ad99-6b5a2f69c870__MESOS-947-stout.patch2
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>


Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()

Posted by Vinod Kone <vi...@gmail.com>.

> On Aug. 5, 2014, 9:29 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 1330-1341
> > <https://reviews.apache.org/r/23912/diff/3/?file=643812#file643812line1330>
> >
> >     I would recommend pulling this logic outside 'if (executor == NULL)' to #1326 to make it easy to reason about.
> >     
> >     also, task ids are globally unique. there shouldn't be multiple executors with the same task id. you could actually do a CHECK(executorIds.size() == 1) to confirm that.

this wasn't addressed either. is there another diff coming?


- Vinod


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


On Aug. 5, 2014, 10:45 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23912/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 10:45 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-947
>     https://issues.apache.org/jira/browse/MESOS-947
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Fixes MESOS-947 "Slave should properly handle a killTask() that arrives between runTask() and _runTask()".
> 
> Slave::killTask() did not check for task in question combination to be "pending" (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked "LOST" instead of "KILLED". But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being "pending" is removed, and the task is marked "KILLED", and _runTask gets informed about this. It checks whether the task in question is currently "pending" and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp a896bb66db5d8cd27ef02b6498c9db93cb0d525f 
>   src/slave/slave.cpp 1d5691836822c8587e1aa8ed24860a8012c67a6e 
>   src/tests/mesos.hpp 75c66fda2485afa0d4541e710780d90b3411839a 
>   src/tests/mesos.cpp 35c94fa908ad728ea92a7d1bfcbe90d57b1b83d9 
>   src/tests/slave_tests.cpp e45255a6f699e51bf09397da95a5a11edbabe591 
> 
> Diff: https://reviews.apache.org/r/23912/diff/
> 
> 
> Testing
> -------
> 
> Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a "clean kill" now :-)
> 
> 
> File Attachments
> ----------------
> 
> stout patch
>   https://reviews.apache.org/media/uploaded/files/2014/08/05/5f4b6886-9a60-4ceb-ad99-6b5a2f69c870__MESOS-947-stout.patch2
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>


Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()

Posted by Vinod Kone <vi...@gmail.com>.

> On Aug. 5, 2014, 9:29 p.m., Vinod Kone wrote:
> > src/tests/mesos.cpp, lines 330-335
> > <https://reviews.apache.org/r/23912/diff/3/?file=643814#file643814line330>
> >
> >     were you unable to directly call Slave::* methods here instead of having the redirection through unmocked_* methods?

we expect comments for dropped issues. mind adding a comment?


> On Aug. 5, 2014, 9:29 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, lines 938-957
> > <https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line938>
> >
> >     To simplify this boiler plate you could use the LaunchTasks action. See TerminatingSlaveDoesNotReregister test in this file for an example.

mind adding a comment?


> On Aug. 5, 2014, 9:29 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, lines 958-962
> > <https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line958>
> >
> >     Kill these expectations since an executor won't be launched in this test.

doesnt look like this was fixed.


- Vinod


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


On Aug. 5, 2014, 10:45 p.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23912/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2014, 10:45 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-947
>     https://issues.apache.org/jira/browse/MESOS-947
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Fixes MESOS-947 "Slave should properly handle a killTask() that arrives between runTask() and _runTask()".
> 
> Slave::killTask() did not check for task in question combination to be "pending" (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked "LOST" instead of "KILLED". But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being "pending" is removed, and the task is marked "KILLED", and _runTask gets informed about this. It checks whether the task in question is currently "pending" and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp a896bb66db5d8cd27ef02b6498c9db93cb0d525f 
>   src/slave/slave.cpp 1d5691836822c8587e1aa8ed24860a8012c67a6e 
>   src/tests/mesos.hpp 75c66fda2485afa0d4541e710780d90b3411839a 
>   src/tests/mesos.cpp 35c94fa908ad728ea92a7d1bfcbe90d57b1b83d9 
>   src/tests/slave_tests.cpp e45255a6f699e51bf09397da95a5a11edbabe591 
> 
> Diff: https://reviews.apache.org/r/23912/diff/
> 
> 
> Testing
> -------
> 
> Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a "clean kill" now :-)
> 
> 
> File Attachments
> ----------------
> 
> stout patch
>   https://reviews.apache.org/media/uploaded/files/2014/08/05/5f4b6886-9a60-4ceb-ad99-6b5a2f69c870__MESOS-947-stout.patch2
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>


Re: Review Request 23912: Fix MESOS-947: Slave should properly handle a killTask() that arrives between runTask() and _runTask()

Posted by Bernd Mathiske <be...@mesosphere.io>.

> On Aug. 5, 2014, 2:29 p.m., Vinod Kone wrote:
> > src/tests/slave_tests.cpp, lines 958-962
> > <https://reviews.apache.org/r/23912/diff/3/?file=643815#file643815line958>
> >
> >     Kill these expectations since an executor won't be launched in this test.
> 
> Vinod Kone wrote:
>     doesnt look like this was fixed.
> 
> Bernd Mathiske wrote:
>     I will take off the 'registered' expectation, but I prefer keeping the second one to make sure the test is tight. That includes that launchTask should not happen by accident.
>

Turns out the executor should not even register. I have that expectation in there, too, now, and I believe it is for the best. The "AtMost(1)" above was wrong. Should be "0", and is now.


- Bernd


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


On Oct. 8, 2014, 3:57 a.m., Bernd Mathiske wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23912/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2014, 3:57 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-947
>     https://issues.apache.org/jira/browse/MESOS-947
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Fixes MESOS-947 "Slave should properly handle a killTask() that arrives between runTask() and _runTask()".
> 
> Slave::killTask() did not check for task in question combination to be "pending" (i.e. Slave::runTask had happened, but Slave::_runTask had not yet) and then erroneously assumed that Slave::runTask() had not been executed. The task was then marked "LOST" instead of "KILLED". But Slave::runTask had already scheduled Slave::_runTask to follow. Now the entry for being "pending" is removed, and the task is marked "KILLED", and _runTask gets informed about this. It checks whether the task in question is currently "pending" and if it is not, then it infers that the task has been killed and does not erroneously try to complete launching it.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0 
>   src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281 
>   src/tests/mesos.hpp 957e2233cc11c438fd80d3b6d1907a1223093104 
>   src/tests/mesos.cpp 3dcb2acd5ad4ab5e3a7b4fe524ee077558112773 
>   src/tests/slave_tests.cpp 69be28f6e82b99e23424bd2be8294f715d8040d4 
> 
> Diff: https://reviews.apache.org/r/23912/diff/
> 
> 
> Testing
> -------
> 
> Wrote a unit test that reliably created the situation described in the ticket. Observed that TASK_LOST and the listed log output occurred. This pointed directly to the lines in killTask() where the problem is rooted. Ran the test after fixing, it succeeded. Checked the log. It looks like a "clean kill" now :-)
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>