You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ji...@apache.org on 2016/08/25 05:23:45 UTC

[1/4] mesos git commit: Fixed a bug related to logger in MesosContainerizer.

Repository: mesos
Updated Branches:
  refs/heads/master 2f019d801 -> ae513df1e


Fixed a bug related to logger in MesosContainerizer.

It's possible that 'destroy' is called while logger 'prepare' is being
called. If that happens, when logger 'prepare' finishes, it'll trigger
an assertion failure.

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


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

Branch: refs/heads/master
Commit: ae513df1e64ecea1eb7bbaa0ca80abebde09dafa
Parents: a58adcf
Author: Jie Yu <yu...@gmail.com>
Authored: Wed Aug 24 14:03:33 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Wed Aug 24 22:23:38 2016 -0700

----------------------------------------------------------------------
 src/slave/containerizer/mesos/containerizer.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/ae513df1/src/slave/containerizer/mesos/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index 7a967dd..e87292f 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -1163,7 +1163,14 @@ Future<bool> MesosContainerizerProcess::_launch(
         self(),
         [=](const ContainerLogger::SubprocessInfo& subprocessInfo)
           -> Future<bool> {
-    CHECK(containers_.contains(containerId));
+    if (!containers_.contains(containerId)) {
+      return Failure("Container destroyed during preparing");
+    }
+
+    if (containers_[containerId]->state == DESTROYING) {
+      return Failure("Container is being destroyed during preparing");
+    }
+
     const Owned<Container>& container = containers_[containerId];
 
     // Use a pipe to block the child until it's been isolated.


[4/4] mesos git commit: Replaced raw pointers in MesosContainerizer with Owned pointers.

Posted by ji...@apache.org.
Replaced raw pointers in MesosContainerizer with Owned pointers.

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


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

Branch: refs/heads/master
Commit: 203a0803cf1fb24e45fe261922cd6cdbfe842e4c
Parents: 2f019d8
Author: Jie Yu <yu...@gmail.com>
Authored: Wed Aug 24 10:21:36 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Wed Aug 24 22:23:38 2016 -0700

----------------------------------------------------------------------
 src/slave/containerizer/mesos/containerizer.cpp | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/203a0803/src/slave/containerizer/mesos/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index 8a8985a..17657e9 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -691,7 +691,7 @@ Future<Nothing> MesosContainerizerProcess::__recover(
   foreach (const ContainerState& run, recovered) {
     const ContainerID& containerId = run.container_id();
 
-    Container* container = new Container();
+    Owned<Container> container(new Container());
 
     Future<Option<int>> status = process::reap(run.pid());
     status.onAny(defer(self(), &Self::reaped, containerId));
@@ -702,7 +702,7 @@ Future<Nothing> MesosContainerizerProcess::__recover(
     // containers should be running after recover.
     container->state = RUNNING;
 
-    containers_[containerId] = Owned<Container>(container);
+    containers_[containerId] = container;
 
     foreach (const Owned<Isolator>& isolator, isolators) {
       isolator->watch(containerId)
@@ -870,7 +870,7 @@ Future<bool> MesosContainerizerProcess::launch(
     const SlaveID& slaveId,
     bool checkpoint)
 {
-  Container* container = new Container();
+  Owned<Container> container(new Container());
   container->state = PROVISIONING;
   container->config = containerConfig;
   container->resources = containerConfig.resources();
@@ -881,7 +881,7 @@ Future<bool> MesosContainerizerProcess::launch(
   // be ready, which it never will. See MESOS-4878.
   container->launchInfos = list<Option<ContainerLaunchInfo>>();
 
-  containers_.put(containerId, Owned<Container>(container));
+  containers_.put(containerId, container);
 
   // We'll first provision the image for the container, and
   // then provision the images specified in `volumes` using
@@ -1581,7 +1581,7 @@ void MesosContainerizerProcess::destroy(
     return;
   }
 
-  Container* container = containers_[containerId].get();
+  const Owned<Container>& container = containers_[containerId];
 
   if (container->state == DESTROYING) {
     VLOG(1) << "Destroy has already been initiated for " << containerId;
@@ -1731,7 +1731,7 @@ void MesosContainerizerProcess::____destroy(
   CHECK_READY(cleanups);
   CHECK(containers_.contains(containerId));
 
-  Container* container = containers_[containerId].get();
+  const Owned<Container>& container = containers_[containerId];
 
   // Check cleanup succeeded for all isolators. If not, we'll fail the
   // container termination and remove the 'destroying' flag but leave
@@ -1769,7 +1769,7 @@ void MesosContainerizerProcess::_____destroy(
 {
   CHECK(containers_.contains(containerId));
 
-  Container* container = containers_[containerId].get();
+  const Owned<Container>& container = containers_[containerId];
 
   if (!destroy.isReady()) {
     container->promise.fail(


[3/4] mesos git commit: Added a TODO about a bug in MesosContainerizer.

Posted by ji...@apache.org.
Added a TODO about a bug in MesosContainerizer.

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


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

Branch: refs/heads/master
Commit: a58adcf7a6a810f811e482ed3916d4fb4c75a91d
Parents: e41d0f7
Author: Jie Yu <yu...@gmail.com>
Authored: Wed Aug 24 14:02:52 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Wed Aug 24 22:23:38 2016 -0700

----------------------------------------------------------------------
 src/slave/containerizer/mesos/containerizer.cpp | 7 +++++++
 1 file changed, 7 insertions(+)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/a58adcf7/src/slave/containerizer/mesos/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index e8a9d6e..7a967dd 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -1616,6 +1616,13 @@ 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.


[2/4] mesos git commit: A few style cleanups in MesosContainerizer.

Posted by ji...@apache.org.
A few style cleanups in MesosContainerizer.

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


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

Branch: refs/heads/master
Commit: e41d0f771f4be1ec081ac0620baf3a51f3d95521
Parents: 203a080
Author: Jie Yu <yu...@gmail.com>
Authored: Wed Aug 24 14:02:27 2016 -0700
Committer: Jie Yu <yu...@gmail.com>
Committed: Wed Aug 24 22:23:38 2016 -0700

----------------------------------------------------------------------
 src/slave/containerizer/mesos/containerizer.cpp | 30 ++++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/e41d0f77/src/slave/containerizer/mesos/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index 17657e9..e8a9d6e 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -1591,8 +1591,8 @@ void MesosContainerizerProcess::destroy(
   LOG(INFO) << "Destroying container " << containerId;
 
   if (container->state == PROVISIONING) {
-    VLOG(1) << "Waiting for the provisioner to complete for container '"
-            << containerId << "'";
+    VLOG(1) << "Waiting for the provisioner to complete provisioning "
+            << "before destroying container " << containerId;
 
     container->state = DESTROYING;
 
@@ -1611,29 +1611,28 @@ void MesosContainerizerProcess::destroy(
   }
 
   if (container->state == PREPARING) {
-    VLOG(1) << "Waiting for the isolators to complete preparing before "
-            << "destroying the container";
+    VLOG(1) << "Waiting for the isolators to complete preparing "
+            << "before destroying container " << containerId;
 
     container->state = DESTROYING;
 
-    Future<Option<int>> status = None();
-    // We need to wait for the isolators to finish preparing to prevent
-    // a race that the destroy method calls isolators' cleanup before
-    // it starts preparing.
+    // 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
       .onAny(defer(
           self(),
           &Self::___destroy,
           containerId,
-          status,
+          None(),
           "Container destroyed while preparing isolators"));
 
     return;
   }
 
   if (container->state == ISOLATING) {
-    VLOG(1) << "Waiting for the isolators to complete for container '"
-            << containerId << "'";
+    VLOG(1) << "Waiting for the isolators to complete isolation "
+            << "before destroying container " << containerId;
 
     container->state = DESTROYING;
 
@@ -1672,6 +1671,8 @@ void MesosContainerizerProcess::__destroy(
 {
   CHECK(containers_.contains(containerId));
 
+  const Owned<Container>& container = containers_[containerId];
+
   // Something has gone wrong and the launcher wasn't able to kill all
   // the processes in the container. We cannot clean up the isolators
   // because they may require that all processes have exited so just
@@ -1679,21 +1680,20 @@ void MesosContainerizerProcess::__destroy(
   // TODO(idownes): This is a pretty bad state to be in but we should
   // consider cleaning up here.
   if (!future.isReady()) {
-    containers_[containerId]->promise.fail(
-        "Failed to destroy container " + stringify(containerId) + ": " +
+    container->promise.fail(
+        "Failed to kill all processes in the container: " +
         (future.isFailed() ? future.failure() : "discarded future"));
 
     containers_.erase(containerId);
 
     ++metrics.container_destroy_errors;
-
     return;
   }
 
   // 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.
-  containers_[containerId]->status
+  container->status
     .onAny(defer(
         self(),
         &Self::___destroy,