Unified return type of `wait` and `destroy` containerizer methods. This patch changes the return type of the `destroy()` method for all containerizers. As a result, `destroy()` and `wait()` methods now have the same signature and return type. In fact, all these methods depend on the same container termination promise, so this unification makes the containerizer implementations more consistent.
Review: https://reviews.apache.org/r/66510/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/ff8999b6 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/ff8999b6 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/ff8999b6 Branch: refs/heads/master Commit: ff8999b6dae9b8450e698ca8efafba7122e2bf83 Parents: 8a6e912 Author: Andrei Budnik <abud...@mesosphere.com> Authored: Wed Apr 11 14:29:15 2018 -0700 Committer: Greg Mann <gregorywm...@gmail.com> Committed: Wed Apr 11 15:33:12 2018 -0700 ---------------------------------------------------------------------- src/slave/containerizer/composing.cpp | 77 +++++++++++--------- src/slave/containerizer/composing.hpp | 3 +- src/slave/containerizer/containerizer.hpp | 5 +- src/slave/containerizer/docker.cpp | 23 +++--- src/slave/containerizer/docker.hpp | 5 +- src/slave/containerizer/mesos/containerizer.cpp | 21 +++--- src/slave/containerizer/mesos/containerizer.hpp | 7 +- src/slave/http.cpp | 2 +- 8 files changed, 79 insertions(+), 64 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/containerizer/composing.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/composing.cpp b/src/slave/containerizer/composing.cpp index cd840a5..186102c 100644 --- a/src/slave/containerizer/composing.cpp +++ b/src/slave/containerizer/composing.cpp @@ -87,7 +87,8 @@ public: Future<Option<ContainerTermination>> wait( const ContainerID& containerId); - Future<bool> destroy(const ContainerID& containerId); + Future<Option<ContainerTermination>> destroy( + const ContainerID& containerId); Future<bool> kill(const ContainerID& containerId, int signal); @@ -135,7 +136,7 @@ private: { State state; Containerizer* containerizer; - Promise<bool> destroyed; + Promise<Option<ContainerTermination>> destroyed; }; hashmap<ContainerID, Container*> containers_; @@ -228,7 +229,8 @@ Future<Option<ContainerTermination>> ComposingContainerizer::wait( } -Future<bool> ComposingContainerizer::destroy(const ContainerID& containerId) +Future<Option<ContainerTermination>> ComposingContainerizer::destroy( + const ContainerID& containerId) { return dispatch(process, &ComposingContainerizerProcess::destroy, @@ -370,10 +372,10 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch( if (containerizer == containerizers_.end()) { // If we are here none of the containerizers support the launch. - // We set this to `false` because the container has no chance of + // 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(false); + container->destroyed.set(Option<ContainerTermination>::none()); // We destroy the container irrespective whether // a destroy is already in progress, for simplicity. @@ -389,9 +391,10 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch( // potentially launch this container. But since a destroy is in progress // we do not try any other containerizers. - // We set this to `true` because the destroy-in-progress stopped an + // We set this because the destroy-in-progress stopped an // launch-in-progress (using the next containerizer). - container->destroyed.set(true); + container->destroyed.set( + Option<ContainerTermination>(ContainerTermination())); containers_.erase(containerId); delete container; @@ -511,10 +514,10 @@ Future<Containerizer::LaunchResult> ComposingContainerizerProcess::_launch( // If we are here, the launch is not supported by the containerizer. - // We set this to `false` because the container has no chance of + // 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(false); + container->destroyed.set(Option<ContainerTermination>::none()); // We destroy the container irrespective whether // a destroy is already in progress, for simplicity. @@ -590,7 +593,7 @@ Future<Option<ContainerTermination>> ComposingContainerizerProcess::wait( } -Future<bool> ComposingContainerizerProcess::destroy( +Future<Option<ContainerTermination>> ComposingContainerizerProcess::destroy( const ContainerID& containerId) { if (!containers_.contains(containerId)) { @@ -599,7 +602,7 @@ Future<bool> ComposingContainerizerProcess::destroy( // Move this logging into the callers. LOG(WARNING) << "Attempted to destroy unknown container " << containerId; - return false; + return None(); } Container* container = containers_.at(containerId); @@ -619,24 +622,26 @@ Future<bool> ComposingContainerizerProcess::destroy( // 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<bool>& 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); - } - })); + .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; @@ -647,12 +652,14 @@ Future<bool> ComposingContainerizerProcess::destroy( container->containerizer->destroy(containerId)); container->destroyed.future() - .onAny(defer(self(), [=](const Future<bool>& destroy) { - if (containers_.contains(containerId)) { - delete containers_.at(containerId); - containers_.erase(containerId); - } - })); + .onAny(defer( + self(), + [=](const Future<Option<ContainerTermination>>& destroy) { + if (containers_.contains(containerId)) { + delete containers_.at(containerId); + containers_.erase(containerId); + } + })); break; } http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/containerizer/composing.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/composing.hpp b/src/slave/containerizer/composing.hpp index 1221d9b..15b78a9 100644 --- a/src/slave/containerizer/composing.hpp +++ b/src/slave/containerizer/composing.hpp @@ -76,7 +76,8 @@ public: virtual process::Future<Option<mesos::slave::ContainerTermination>> wait( const ContainerID& containerId); - virtual process::Future<bool> destroy(const ContainerID& containerId); + virtual process::Future<Option<mesos::slave::ContainerTermination>> destroy( + const ContainerID& containerId); virtual process::Future<bool> kill( const ContainerID& containerId, http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/containerizer/containerizer.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/containerizer.hpp b/src/slave/containerizer/containerizer.hpp index 836283a..5817b1d 100644 --- a/src/slave/containerizer/containerizer.hpp +++ b/src/slave/containerizer/containerizer.hpp @@ -135,12 +135,13 @@ public: const ContainerID& containerId) = 0; // Destroy a running container, killing all processes and releasing - // all resources. Returns false when the container cannot be found, + // all resources. Returns None when the container cannot be found, // or a failure if something went wrong. // // NOTE: You cannot wait() on containers that have been destroyed, // so you should always call wait() before destroy(). - virtual process::Future<bool> destroy(const ContainerID& containerId) = 0; + virtual process::Future<Option<mesos::slave::ContainerTermination>> destroy( + const ContainerID& containerId) = 0; // Sends a signal to a running container. Returns false when the container // cannot be found. The future may be failed if an error occurs in sending http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/containerizer/docker.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp index 31d64a7..a4c9c10 100644 --- a/src/slave/containerizer/docker.cpp +++ b/src/slave/containerizer/docker.cpp @@ -870,7 +870,8 @@ Future<Option<ContainerTermination>> DockerContainerizer::wait( } -Future<bool> DockerContainerizer::destroy(const ContainerID& containerId) +Future<Option<ContainerTermination>> DockerContainerizer::destroy( + const ContainerID& containerId) { return dispatch( process.get(), @@ -2197,7 +2198,7 @@ Future<Option<ContainerTermination>> DockerContainerizerProcess::wait( } -Future<bool> DockerContainerizerProcess::destroy( +Future<Option<ContainerTermination>> DockerContainerizerProcess::destroy( const ContainerID& containerId, bool killed) { @@ -2207,7 +2208,7 @@ Future<bool> DockerContainerizerProcess::destroy( // Move this logging into the callers. LOG(WARNING) << "Attempted to destroy unknown container " << containerId; - return false; + return None(); } // TODO(klueska): Ideally, we would do this check as the first thing @@ -2230,19 +2231,21 @@ Future<bool> DockerContainerizerProcess::destroy( // cleanup. CHECK_PENDING(container->status.future()); + ContainerTermination termination; + // NOTE: The launch error message will be retrieved by the slave // and properly set in the corresponding status update. - container->termination.set(ContainerTermination()); + container->termination.set(termination); containers_.erase(containerId); delete container; - return true; + return termination; } if (container->state == Container::DESTROYING) { return container->termination.future() - .then([]() { return true; }); + .then(Option<ContainerTermination>::some); } // It's possible that destroy is getting called before @@ -2281,7 +2284,7 @@ Future<bool> DockerContainerizerProcess::destroy( containers_.erase(containerId); delete container; - return true; + return termination; } if (container->state == Container::PULLING) { @@ -2296,7 +2299,7 @@ Future<bool> DockerContainerizerProcess::destroy( containers_.erase(containerId); delete container; - return true; + return termination; } if (container->state == Container::MOUNTING) { @@ -2317,7 +2320,7 @@ Future<bool> DockerContainerizerProcess::destroy( containers_.erase(containerId); delete container; - return true; + return termination; } CHECK(container->state == Container::RUNNING); @@ -2352,7 +2355,7 @@ Future<bool> DockerContainerizerProcess::destroy( .onAny(defer(self(), &Self::_destroy, containerId, killed)); return container->termination.future() - .then([]() { return true; }); + .then(Option<ContainerTermination>::some); } http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/containerizer/docker.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.hpp b/src/slave/containerizer/docker.hpp index f1b56c8..6bdf4c7 100644 --- a/src/slave/containerizer/docker.hpp +++ b/src/slave/containerizer/docker.hpp @@ -105,7 +105,8 @@ public: virtual process::Future<Option<mesos::slave::ContainerTermination>> wait( const ContainerID& containerId); - virtual process::Future<bool> destroy(const ContainerID& containerId); + virtual process::Future<Option<mesos::slave::ContainerTermination>> destroy( + const ContainerID& containerId); virtual process::Future<hashset<ContainerID>> containers(); @@ -159,7 +160,7 @@ public: virtual process::Future<Option<mesos::slave::ContainerTermination>> wait( const ContainerID& containerId); - virtual process::Future<bool> destroy( + virtual process::Future<Option<mesos::slave::ContainerTermination>> destroy( const ContainerID& containerId, bool killed = true); // process is either killed or reaped. http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/containerizer/mesos/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp index 7473a87..d1d4c2a 100644 --- a/src/slave/containerizer/mesos/containerizer.cpp +++ b/src/slave/containerizer/mesos/containerizer.cpp @@ -625,7 +625,8 @@ Future<Option<ContainerTermination>> MesosContainerizer::wait( } -Future<bool> MesosContainerizer::destroy(const ContainerID& containerId) +Future<Option<ContainerTermination>> MesosContainerizer::destroy( + const ContainerID& containerId) { return dispatch(process.get(), &MesosContainerizerProcess::destroy, @@ -2297,7 +2298,7 @@ Future<ContainerStatus> MesosContainerizerProcess::status( } -Future<bool> MesosContainerizerProcess::destroy( +Future<Option<ContainerTermination>> MesosContainerizerProcess::destroy( const ContainerID& containerId, const Option<ContainerTermination>& termination) { @@ -2320,7 +2321,7 @@ Future<bool> MesosContainerizerProcess::destroy( // Move this logging into the callers. LOG(WARNING) << "Attempted to destroy unknown container " << containerId; - return false; + return None(); } const Owned<Container>& container = containers_.at(containerId); @@ -2332,7 +2333,7 @@ Future<bool> MesosContainerizerProcess::destroy( // 'destroy()' call to affect future calls to 'wait()'. See more // details in MESOS-7926. return undiscardable(container->termination.future()) - .then([]() { return true; }); + .then(Option<ContainerTermination>::some); } LOG_BASED_ON_CLASS(container->containerClass()) @@ -2345,13 +2346,13 @@ Future<bool> MesosContainerizerProcess::destroy( transition(containerId, DESTROYING); - list<Future<bool>> destroys; + list<Future<Option<ContainerTermination>>> destroys; foreach (const ContainerID& child, container->children) { destroys.push_back(destroy(child, termination)); } - await(destroys) - .then(defer(self(), [=](const list<Future<bool>>& futures) { + await(destroys).then(defer( + self(), [=](const list<Future<Option<ContainerTermination>>>& futures) { _destroy(containerId, termination, previousState, futures); return Nothing(); })); @@ -2362,7 +2363,7 @@ Future<bool> MesosContainerizerProcess::destroy( // to affect future calls to 'wait()'. See more details in // MESOS-7926. return undiscardable(container->termination.future()) - .then([]() { return true; }); + .then(Option<ContainerTermination>::some); } @@ -2370,7 +2371,7 @@ void MesosContainerizerProcess::_destroy( const ContainerID& containerId, const Option<ContainerTermination>& termination, const State& previousState, - const list<Future<bool>>& destroys) + const list<Future<Option<ContainerTermination>>>& destroys) { CHECK(containers_.contains(containerId)); @@ -2379,7 +2380,7 @@ void MesosContainerizerProcess::_destroy( CHECK_EQ(container->state, DESTROYING); vector<string> errors; - foreach (const Future<bool>& future, destroys) { + foreach (const Future<Option<ContainerTermination>>& future, destroys) { if (!future.isReady()) { errors.push_back(future.isFailed() ? future.failure() http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/containerizer/mesos/containerizer.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.hpp b/src/slave/containerizer/mesos/containerizer.hpp index cba4ed2..22405e6 100644 --- a/src/slave/containerizer/mesos/containerizer.hpp +++ b/src/slave/containerizer/mesos/containerizer.hpp @@ -107,7 +107,7 @@ public: virtual process::Future<Option<mesos::slave::ContainerTermination>> wait( const ContainerID& containerId); - virtual process::Future<bool> destroy( + virtual process::Future<Option<mesos::slave::ContainerTermination>> destroy( const ContainerID& containerId); virtual process::Future<bool> kill( @@ -179,7 +179,7 @@ public: const ContainerID& containerId, int_fd pipeWrite); - virtual process::Future<bool> destroy( + virtual process::Future<Option<mesos::slave::ContainerTermination>> destroy( const ContainerID& containerId, const Option<mesos::slave::ContainerTermination>& termination); @@ -245,7 +245,8 @@ private: const ContainerID& containerId, const Option<mesos::slave::ContainerTermination>& termination, const State& previousState, - const std::list<process::Future<bool>>& destroys); + const std::list< + process::Future<Option<mesos::slave::ContainerTermination>>>& destroys); // Continues '_destroy()' once isolators has completed. void __destroy( http://git-wip-us.apache.org/repos/asf/mesos/blob/ff8999b6/src/slave/http.cpp ---------------------------------------------------------------------- diff --git a/src/slave/http.cpp b/src/slave/http.cpp index d500fde..9e4525b 100644 --- a/src/slave/http.cpp +++ b/src/slave/http.cpp @@ -2589,7 +2589,7 @@ Future<Response> Http::_launchContainer( << (launchResult.isFailed() ? launchResult.failure() : "discarded"); slave->containerizer->destroy(containerId) - .onAny([=](const Future<bool>& destroy) { + .onAny([=](const Future<Option<ContainerTermination>>& destroy) { if (destroy.isReady()) { return; }
