mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From qianzh...@apache.org
Subject [1/8] mesos git commit: Removed `destroyed` from `Container` struct in composing containerizer.
Date Fri, 25 May 2018 01:27:38 GMT
Repository: mesos
Updated Branches:
  refs/heads/master 24359e643 -> d2ab700cd


Removed `destroyed` from `Container` struct in composing containerizer.

Previously, we stored `destroyed` promise for each container and used
it to guarantee that `destroy()` returns a non-empty value when the
destroy-in-progress stops an launch-in-progress using the next
containerizer. Since `wait()` and `destroy()` return the same
`ContainerTermination` value when called with the same ContainerID
argument, we can remove `destroyed` promise and add callbacks to
clean up `containers_` map instead.

Moreover, we added a clean up for terminated containers that have
been recovered after agent's restart.

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


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

Branch: refs/heads/master
Commit: 896c593c7918dd14d44740af22d63f82c0d4813b
Parents: 24359e6
Author: Andrei Budnik <abudnik@mesosphere.com>
Authored: Fri May 25 09:07:36 2018 +0800
Committer: Qian Zhang <zhq527725@gmail.com>
Committed: Fri May 25 09:07:36 2018 +0800

----------------------------------------------------------------------
 src/slave/containerizer/composing.cpp | 116 ++++++++++-------------------
 1 file changed, 38 insertions(+), 78 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/896c593c/src/slave/containerizer/composing.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/composing.cpp b/src/slave/containerizer/composing.cpp
index 1fb79f5..3c03d66 100644
--- a/src/slave/containerizer/composing.cpp
+++ b/src/slave/containerizer/composing.cpp
@@ -136,7 +136,6 @@ private:
   {
     State state;
     Containerizer* containerizer;
-    Promise<Option<ContainerTermination>> destroyed;
   };
 
   hashmap<ContainerID, Container*> containers_;
@@ -322,6 +321,16 @@ Future<Nothing> ComposingContainerizerProcess::__recover(
     container->state = LAUNCHED;
     container->containerizer = containerizer;
     containers_[containerId] = container;
+
+    // This is needed for eventually removing the given container from
+    // the list of active containers.
+    containerizer->wait(containerId)
+      .onAny(defer(self(), [=](const Future<Option<ContainerTermination>>&)
{
+        if (containers_.contains(containerId)) {
+          delete containers_.at(containerId);
+          containers_.erase(containerId);
+        }
+      }));
   }
   return Nothing();
 }
@@ -358,7 +367,12 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch(
       // This is needed for eventually removing the given container from
       // the list of active containers.
       container->containerizer->wait(containerId)
-        .onAny(defer(self(), &Self::destroy, containerId));
+        .onAny(defer(self(), [=](const Future<Option<ContainerTermination>>&)
{
+          if (containers_.contains(containerId)) {
+            delete containers_.at(containerId);
+            containers_.erase(containerId);
+          }
+        }));
     }
 
     // Note that the return value is not impacted
@@ -374,11 +388,6 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch(
   if (containerizer == containerizers_.end()) {
     // If we are here none of the containerizers support the launch.
 
-    // We set this to `None` because the container has no chance of
-    // getting launched by any containerizer. This is similar to what
-    // would happen if the destroy "started" after launch returned false.
-    container->destroyed.set(Option<ContainerTermination>::none());
-
     // We destroy the container irrespective whether
     // a destroy is already in progress, for simplicity.
     containers_.erase(containerId);
@@ -392,15 +401,7 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch(
     // If we are here there is at least one more containerizer that could
     // potentially launch this container. But since a destroy is in progress
     // we do not try any other containerizers.
-
-    // We set this because the destroy-in-progress stopped an
-    // launch-in-progress (using the next containerizer).
-    container->destroyed.set(
-        Option<ContainerTermination>(ContainerTermination()));
-
-    containers_.erase(containerId);
-    delete container;
-
+    //
     // We return failure here because there is a chance some other
     // containerizer might be able to launch this container but
     // we are not trying it because a destroy is in progress.
@@ -506,7 +507,12 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch(
       // This is needed for eventually removing the given container from
       // the list of active containers.
       container->containerizer->wait(containerId)
-        .onAny(defer(self(), &Self::destroy, containerId));
+        .onAny(defer(self(), [=](const Future<Option<ContainerTermination>>&)
{
+          if (containers_.contains(containerId)) {
+            delete containers_.at(containerId);
+            containers_.erase(containerId);
+          }
+        }));
     }
 
     // Note that the return value is not impacted
@@ -516,11 +522,6 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch(
 
   // If we are here, the launch is not supported by the containerizer.
 
-  // We set this to `None` because the container has no chance of
-  // getting launched. This is similar to what would happen if the
-  // destroy "started" after launch returned false.
-  container->destroyed.set(Option<ContainerTermination>::none());
-
   // We destroy the container irrespective whether
   // a destroy is already in progress, for simplicity.
   containers_.erase(containerId);
@@ -609,64 +610,23 @@ Future<Option<ContainerTermination>> ComposingContainerizerProcess::destroy(
 
   Container* container = containers_.at(containerId);
 
-  switch (container->state) {
-    case DESTROYING:
-      break; // No-op.
-
-    case LAUNCHING:
-      container->state = DESTROYING;
-
-      // Forward the destroy request to the containerizer. Note that
-      // a containerizer is expected to handle a destroy while
-      // `launch()` is in progress. If the containerizer could not
-      // handle launching the container (`launch()` returns false),
-      // then the containerizer may no longer know about this
-      // container. If the launch returns false, we will stop trying
-      // to launch the container on other containerizers.
-      container->containerizer->destroy(containerId)
-        .onAny(defer(
-            self(),
-            [=](const Future<Option<ContainerTermination>>& destroy) {
-              // We defer the association of the promise in order to
-              // surface a successful destroy (by setting
-              // `Container.destroyed` to true in `_launch()`) when
-              // the containerizer cannot handle this type of container
-              // (`launch()` returns false). If we do not defer here and
-              // instead associate the future right away, the setting of
-              // `Container.destroy` in `_launch()` will be a no-op;
-              // this might result in users waiting on the future
-              // incorrectly thinking that the destroy failed when in
-              // fact the destroy is implicitly successful because the
-              // launch failed.
-              if (containers_.contains(containerId)) {
-                containers_.at(containerId)->destroyed.associate(destroy);
-                delete containers_.at(containerId);
-                containers_.erase(containerId);
-              }
-            }));
-
-      break;
-
-    case LAUNCHED:
-      container->state = DESTROYING;
-
-      container->destroyed.associate(
-          container->containerizer->destroy(containerId));
-
-      container->destroyed.future()
-        .onAny(defer(
-            self(),
-            [=](const Future<Option<ContainerTermination>>& destroy) {
-              if (containers_.contains(containerId)) {
-                delete containers_.at(containerId);
-                containers_.erase(containerId);
-              }
-            }));
-
-      break;
+  if (container->state == LAUNCHING || container->state == LAUNCHED) {
+    // Note that this method might be called between two successive attempts to
+    // launch a container using different containerizers. In this case, we will
+    // return `None`, because there is no underlying containerizer that is
+    // actually aware of launching a container.
+    container->state = DESTROYING;
   }
 
-  return container->destroyed.future();
+  CHECK_EQ(container->state, DESTROYING);
+
+  return container->containerizer->destroy(containerId)
+    .onAny(defer(self(), [=](const Future<Option<ContainerTermination>>&)
{
+      if (containers_.contains(containerId)) {
+        delete containers_.at(containerId);
+        containers_.erase(containerId);
+      }
+    }));
 }
 
 


Mime
View raw message