[2/7] mesos git commit: Fixed nested container agent flapping issue after reboot.

[al...@apache.org]

Fixed nested container agent flapping issue after reboot.

When recovering containers in provisioner, there is a particular case
that after the machine reboots the container runtime directory and
slave state is gone but the provisioner directory still exists since
it is under the agent work_dir(e.g., agent work_dir is /var/lib/mesos).
Then, all checkpointed containers will be cleaned up as unknown
containers in provisioner during recovery. However, the semantic that
a child container is always cleaned up before its parent container
cannot be guaranteed for this particular case. Ideally, we should
put the provisioner directory under the container runtime dir but this
is not backward compactible. It is an unfortunate that we have to
make the provisioner::destroy() to be recursive.

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


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

Branch: refs/heads/1.1.x
Commit: 52baba717e42907797b7cce65a51285593fd39ae
Parents: 645ad0f
Author: Gilbert Song <so...@gmail.com>
Authored: Wed Feb 22 09:10:58 2017 -0800
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Wed Apr 26 14:39:08 2017 +0200

----------------------------------------------------------------------
 .../mesos/provisioner/provisioner.cpp           | 78 +++++++++++++++-----
 .../mesos/provisioner/provisioner.hpp           | 11 ++-
 2 files changed, 70 insertions(+), 19 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/52baba71/src/slave/containerizer/mesos/provisioner/provisioner.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/provisioner.cpp b/src/slave/containerizer/mesos/provisioner/provisioner.cpp
index db2d267..3b041a7 100644
--- a/src/slave/containerizer/mesos/provisioner/provisioner.cpp
+++ b/src/slave/containerizer/mesos/provisioner/provisioner.cpp
@@ -326,32 +326,68 @@ Future<bool> ProvisionerProcess::destroy(const ContainerID& containerId)
     return false;
   }
 
+  if (infos[containerId]->destroying) {
+    return infos[containerId]->termination.future();
+  }
+
+  infos[containerId]->destroying = true;
+
   // Provisioner destroy can be invoked from:
   // 1. Provisioner `recover` to destroy all unknown orphans.
   // 2. Containerizer `recover` to destroy known orphans.
   // 3. Containerizer `destroy` on one specific container.
   //
-  // In the above cases, we assume that the container being destroyed
-  // has no corresponding child containers. We fail fast if this
-  // condition is not satisfied.
+  // NOTE: For (2) and (3), we expect the container being destory
+  // has no any child contain remain running. However, for case (1),
+  // if the container runtime directory does not survive after the
+  // machine reboots and the provisioner directory under the agent
+  // work dir still exists, all containers will be regarded as
+  // unkown containers and will be destroyed. In this case, a parent
+  // container may be destoryed before its child containers are
+  // cleaned up. So we have to make `destroy()` recursively for
+  // this particular case.
   //
-  // NOTE: This check is expensive since it traverses the entire
-  // `infos` hashmap. This is acceptable because we generally expect
-  // the number of containers on a single agent to be on the order of
-  // tens or hundreds of containers.
+  // TODO(gilbert): Move provisioner directory to the container
+  // runtime directory after a deprecation cycle to avoid
+  // making `provisioner::destroy()` being recursive.
+  list<Future<bool>> destroys;
+
   foreachkey (const ContainerID& entry, infos) {
-    if (entry.has_parent()) {
-      CHECK(entry.parent() != containerId)
-        << "Failed to destroy container "
-        << containerId << " since its nested container "
-        << entry << " has not been destroyed yet";
+    if (entry.has_parent() && entry.parent() == containerId) {
+      destroys.push_back(destroy(entry));
     }
   }
 
-  // Unregister the container first. If destroy() fails, we can rely
-  // on recover() to retry it later.
-  Owned<Info> info = infos[containerId];
-  infos.erase(containerId);
+  return await(destroys)
+    .then(defer(self(), &Self::_destroy, containerId, lambda::_1));
+}
+
+
+Future<bool> ProvisionerProcess::_destroy(
+    const ContainerID& containerId,
+    const list<Future<bool>>& destroys)
+{
+  CHECK(infos.contains(containerId));
+  CHECK(infos[containerId]->destroying);
+
+  vector<string> errors;
+  foreach (const Future<bool>& future, destroys) {
+    if (!future.isReady()) {
+      errors.push_back(future.isFailed()
+        ? future.failure()
+        : "discarded");
+    }
+  }
+
+  if (!errors.empty()) {
+    ++metrics.remove_container_errors;
+
+    return Failure(
+        "Failed to destory nested containers: " +
+        strings::join("; ", errors));
+  }
+
+  const Owned<Info>& info = infos[containerId];
 
   list<Future<bool>> futures;
   foreachkey (const string& backend, info->rootfses) {
@@ -381,12 +417,15 @@ Future<bool> ProvisionerProcess::destroy(const ContainerID& containerId)
 
   // TODO(xujyan): Revisit the usefulness of this return value.
   return collect(futures)
-    .then(defer(self(), &ProvisionerProcess::_destroy, containerId));
+    .then(defer(self(), &ProvisionerProcess::__destroy, containerId));
 }
 
 
-Future<bool> ProvisionerProcess::_destroy(const ContainerID& containerId)
+Future<bool> ProvisionerProcess::__destroy(const ContainerID& containerId)
 {
+  CHECK(infos.contains(containerId));
+  CHECK(infos[containerId]->destroying);
+
   // This should be fairly cheap as the directory should only
   // contain a few empty sub-directories at this point.
   //
@@ -406,6 +445,9 @@ Future<bool> ProvisionerProcess::_destroy(const ContainerID& containerId)
     ++metrics.remove_container_errors;
   }
 
+  infos[containerId]->termination.set(true);
+  infos.erase(containerId);
+
   return true;
 }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/52baba71/src/slave/containerizer/mesos/provisioner/provisioner.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/provisioner/provisioner.hpp b/src/slave/containerizer/mesos/provisioner/provisioner.hpp
index 0a48617..a988025 100644
--- a/src/slave/containerizer/mesos/provisioner/provisioner.hpp
+++ b/src/slave/containerizer/mesos/provisioner/provisioner.hpp
@@ -134,7 +134,11 @@ private:
       const Image& image,
       const ImageInfo& imageInfo);
 
-  process::Future<bool> _destroy(const ContainerID& containerId);
+  process::Future<bool> _destroy(
+      const ContainerID& containerId,
+      const std::list<process::Future<bool>>& destroys);
+
+  process::Future<bool> __destroy(const ContainerID& containerId);
 
   const Flags flags;
 
@@ -152,6 +156,11 @@ private:
   {
     // Mappings: backend -> {rootfsId, ...}
     hashmap<std::string, hashset<std::string>> rootfses;
+
+    process::Promise<bool> termination;
+
+    // The container status in provisioner.
+    bool destroying = false;
   };
 
   hashmap<ContainerID, process::Owned<Info>> infos;