[2/8] mesos git commit: Fixed a TODO in the MesosContainerizer.

[ji...@apache.org]

Fixed a TODO in the MesosContainerizer.

This patch fixed a bug in the destory path. It correctly handles a
race where the container is destroyed while it is being launched. The
idea is to make 'container->status' optional and don't set it until
the container is actually forked.

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


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/ee6b7495
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/ee6b7495
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/ee6b7495

Branch: refs/heads/master
Commit: ee6b7495d059f47f08735fb463c7cfd07bfc9877
Parents: 1bd974b
Author: Jie Yu <yu...@gmail.com>
Authored: Wed Aug 24 22:06:17 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Thu Aug 25 11:45:23 2016 -0700

----------------------------------------------------------------------
 src/slave/containerizer/mesos/containerizer.cpp | 29 ++++++++++++--------
 src/slave/containerizer/mesos/containerizer.hpp | 12 +++++---
 2 files changed, 26 insertions(+), 15 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/ee6b7495/src/slave/containerizer/mesos/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index f72d734..b225e06 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -1628,17 +1628,20 @@ void MesosContainerizerProcess::destroy(
 
     container->state = DESTROYING;
 
-    // TODO(jieyu): It's likely that the launcher already forked the
-    // container. However, since we change the state to 'DESTROYING',
-    // the 'isolate()' will fail, causing the control pipes being
-    // closed. The container will terminate itself. However, we should
-    // wait for the container to terminate before we start to cleanup
-    // isolators.
-
     // We need to wait for the isolators to finish preparing to
     // prevent a race that the destroy method calls the 'cleanup'
     // method of an isolator before the 'prepare' method is called.
-    container->launchInfos
+    //
+    // NOTE: It's likely that the launcher already forked the
+    // container. However, since we change the state to 'DESTROYING',
+    // the 'isolate()' will fail, causing the control pipes being
+    // closed. The container will terminate itself. Therefore, we
+    // should wait for the container to terminate before we start to
+    // cleanup isolators.
+    await(container->launchInfos,
+          container->status.isSome()
+            ? container->status.get()
+            : None())
       .onAny(defer(self(), &Self::___destroy, containerId));
 
     return;
@@ -1707,7 +1710,9 @@ void MesosContainerizerProcess::__destroy(
   // We've successfully killed all processes in the container so get
   // the exit status of the executor when it's ready (it may already
   // be) and continue the destroy.
-  container->status
+  CHECK_SOME(container->status);
+
+  container->status.get()
     .onAny(defer(self(), &Self::___destroy, containerId));
 }
 
@@ -1782,8 +1787,10 @@ void MesosContainerizerProcess::_____destroy(
 
   ContainerTermination termination;
 
-  if (container->status.isReady() && container->status->isSome()) {
-    termination.set_status(container->status->get());
+  if (container->status.isSome() &&
+      container->status->isReady() &&
+      container->status->get().isSome()) {
+    termination.set_status(container->status->get().get());
   }
 
   // NOTE: We may not see a limitation in time for it to be

http://git-wip-us.apache.org/repos/asf/mesos/blob/ee6b7495/src/slave/containerizer/mesos/containerizer.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.hpp b/src/slave/containerizer/mesos/containerizer.hpp
index 3e4fb97..5da561c 100644
--- a/src/slave/containerizer/mesos/containerizer.hpp
+++ b/src/slave/containerizer/mesos/containerizer.hpp
@@ -285,10 +285,14 @@ private:
     // Promise for futures returned from wait().
     process::Promise<mesos::slave::ContainerTermination> promise;
 
-    // We need to keep track of the future exit status for each
-    // executor because we'll only get a single notification when
-    // the executor exits.
-    process::Future<Option<int>> status;
+    // We keep track of the future exit status for the container if it
+    // has been launched. If the container has not been launched yet,
+    // 'status' will be set to None().
+    //
+    // NOTE: A container has an exit status does not mean that it has
+    // been properly destroyed. We need to perform cleanup on
+    // isolators and provisioner after that.
+    Option<process::Future<Option<int>>> status;
 
     // We keep track of the future that is waiting for the provisioner's
     // `ProvisionInfo`, so that destroy will only start calling