From commits-return-22660-archive-asf-public=cust-asf.ponee.io@mesos.apache.org Fri May 25 03:27:40 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 5A39A180636 for ; Fri, 25 May 2018 03:27:39 +0200 (CEST) Received: (qmail 79471 invoked by uid 500); 25 May 2018 01:27:38 -0000 Mailing-List: contact commits-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@mesos.apache.org Delivered-To: mailing list commits@mesos.apache.org Received: (qmail 79462 invoked by uid 99); 25 May 2018 01:27:38 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 25 May 2018 01:27:38 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 3C8C0E0BEF; Fri, 25 May 2018 01:27:38 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: qianzhang@apache.org To: commits@mesos.apache.org Date: Fri, 25 May 2018 01:27:38 -0000 Message-Id: <30e07ecdb482471088c03ad1385a00ce@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [1/8] mesos git commit: Removed `destroyed` from `Container` struct in composing containerizer. 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 Authored: Fri May 25 09:07:36 2018 +0800 Committer: Qian Zhang 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> destroyed; }; hashmap containers_; @@ -322,6 +321,16 @@ Future 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>&) { + if (containers_.contains(containerId)) { + delete containers_.at(containerId); + containers_.erase(containerId); + } + })); } return Nothing(); } @@ -358,7 +367,12 @@ Future 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>&) { + if (containers_.contains(containerId)) { + delete containers_.at(containerId); + containers_.erase(containerId); + } + })); } // Note that the return value is not impacted @@ -374,11 +388,6 @@ Future 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::none()); - // We destroy the container irrespective whether // a destroy is already in progress, for simplicity. containers_.erase(containerId); @@ -392,15 +401,7 @@ Future 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())); - - 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 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>&) { + if (containers_.contains(containerId)) { + delete containers_.at(containerId); + containers_.erase(containerId); + } + })); } // Note that the return value is not impacted @@ -516,11 +522,6 @@ Future 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::none()); - // We destroy the container irrespective whether // a destroy is already in progress, for simplicity. containers_.erase(containerId); @@ -609,64 +610,23 @@ Future> 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>& 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>& 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>&) { + if (containers_.contains(containerId)) { + delete containers_.at(containerId); + containers_.erase(containerId); + } + })); }