commit 6be17200b8084ad3524e7d450c411765b3214c0f
author Benjamin Mahler <bmahler@apache.org> Tue Sep 01 14:58:28 2020 -0400
committer Benjamin Mahler <bmahler@apache.org> Thu Sep 10 13:09:42 2020 -0400
tree afdf927f70931da6e92107f429c8dfe44e225a2e
parent b0ee625ce1c07177e3e12ffa56ce3d76969dfdbe

Fixed a CHECK failure in master during agent removal.

Per MESOS-9609, it's possible for the master to encounter a CHECK
failure during agent removal in the following situation:

  1. Given a framework with checkpoint == false, with only
     executor(s) (no tasks) running on an agent:
  2. When this agent disconects from the master,
     Master::removeFramework(Slave*, Framework*) removes the
     tasks and executors. However, when there are no tasks, this
     function will accidentally insert an entry into
     Master::Slave::tasks! (Due to the [] operator usage)
  3. Now if the framework is removed, we have an entry in
     Slave::tasks, for which there is no corresponding framework.
  4. When the agent is removed, we have a CHECK failure given
     we can't find the framework.

This fixes the issue by avoiding the accidental insertion.

Review: https://reviews.apache.org/r/72831

src/master/master.cpp

diff --git a/src/master/master.cpp b/src/master/master.cpp
index e99cc6b..97654b5 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -10550,30 +10550,32 @@
 
   // Remove pointers to framework's tasks in slaves, and send status
   // updates.
-  // NOTE: A copy is needed because removeTask modifies slave->tasks.
-  foreachvalue (Task* task, utils::copy(slave->tasks[framework->id()])) {
-    // Remove tasks that belong to this framework.
-    if (task->framework_id() == framework->id()) {
-      // A framework might not actually exist because the master failed
-      // over and the framework hasn't reconnected yet. For more info
-      // please see the comments in 'removeFramework(Framework*)'.
-      const StatusUpdate& update = protobuf::createStatusUpdate(
-        task->framework_id(),
-        task->slave_id(),
-        task->task_id(),
-        TASK_LOST,
-        TaskStatus::SOURCE_MASTER,
-        None(),
-        "Agent " + slave->info.hostname() + " disconnected",
-        TaskStatus::REASON_SLAVE_DISCONNECTED,
-        (task->has_executor_id()
-            ? Option<ExecutorID>(task->executor_id()) : None()));
+  if (slave->tasks.contains(framework->id())) {
+    // NOTE: A copy is needed because removeTask modifies slave->tasks.
+    foreachvalue (Task* task, utils::copy(slave->tasks.at(framework->id()))) {
+      // Remove tasks that belong to this framework.
+      if (task->framework_id() == framework->id()) {
+        // A framework might not actually exist because the master failed
+        // over and the framework hasn't reconnected yet. For more info
+        // please see the comments in 'removeFramework(Framework*)'.
+        const StatusUpdate& update = protobuf::createStatusUpdate(
+          task->framework_id(),
+          task->slave_id(),
+          task->task_id(),
+          TASK_LOST,
+          TaskStatus::SOURCE_MASTER,
+          None(),
+          "Agent " + slave->info.hostname() + " disconnected",
+          TaskStatus::REASON_SLAVE_DISCONNECTED,
+          (task->has_executor_id()
+              ? Option<ExecutorID>(task->executor_id()) : None()));
 
-      updateTask(task, update);
-      removeTask(task);
+        updateTask(task, update);
+        removeTask(task);
 
-      if (framework->connected()) {
-        forward(update, UPID(), framework);
+        if (framework->connected()) {
+          forward(update, UPID(), framework);
+        }
       }
     }
   }