Repository: mesos Updated Branches: refs/heads/master 7cec663fa -> eb075f9b7
Destroy Docker containers with stop instead of kill. Ensure docker calls stop if a docker_stop_timeout is provided. Added the flag docker_stop_timeout that defaults to 0, if the timeout is 0 the a docker kill will be run, otherwise a docker stop with the timeout in seconds is used. Review: https://reviews.apache.org/r/26736 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/eb075f9b Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/eb075f9b Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/eb075f9b Branch: refs/heads/master Commit: eb075f9b7b518a72249f5a68538f356ba63aa79d Parents: 7cec663 Author: Ryan Thomas <[email protected]> Authored: Thu Nov 13 17:16:00 2014 -0800 Committer: Timothy Chen <[email protected]> Committed: Thu Nov 13 17:19:17 2014 -0800 ---------------------------------------------------------------------- src/docker/docker.cpp | 18 +++++++++---- src/docker/docker.hpp | 14 +++++++--- src/slave/containerizer/docker.cpp | 12 ++++----- src/slave/flags.hpp | 7 +++++ src/tests/docker_containerizer_tests.cpp | 37 ++++++++++++++++----------- src/tests/docker_tests.cpp | 7 ++--- 6 files changed, 61 insertions(+), 34 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/eb075f9b/src/docker/docker.cpp ---------------------------------------------------------------------- diff --git a/src/docker/docker.cpp b/src/docker/docker.cpp index 0c0a1bf..f2730a7 100644 --- a/src/docker/docker.cpp +++ b/src/docker/docker.cpp @@ -486,10 +486,18 @@ Future<Nothing> Docker::run( return checkError(cmd, s.get()); } - -Future<Nothing> Docker::kill(const string& container, bool remove) const +Future<Nothing> Docker::stop( + const string& container, + const Duration& timeout, + bool remove) const { - const string cmd = path + " kill " + container; + int timeoutSecs = (int) timeout.secs(); + if (timeoutSecs < 0) { + return Failure("A negative timeout can not be applied to docker stop: " + + stringify(timeoutSecs)); + } + + string cmd = path + " stop -t " + stringify(timeoutSecs) + " " + container; VLOG(1) << "Running " << cmd; @@ -505,7 +513,7 @@ Future<Nothing> Docker::kill(const string& container, bool remove) const return s.get().status() .then(lambda::bind( - &Docker::_kill, + &Docker::_stop, *this, container, cmd, @@ -513,7 +521,7 @@ Future<Nothing> Docker::kill(const string& container, bool remove) const remove)); } -Future<Nothing> Docker::_kill( +Future<Nothing> Docker::_stop( const Docker& docker, const string& container, const string& cmd, http://git-wip-us.apache.org/repos/asf/mesos/blob/eb075f9b/src/docker/docker.hpp ---------------------------------------------------------------------- diff --git a/src/docker/docker.hpp b/src/docker/docker.hpp index 2dc692c..f004879 100644 --- a/src/docker/docker.hpp +++ b/src/docker/docker.hpp @@ -25,6 +25,7 @@ #include <process/future.hpp> #include <process/subprocess.hpp> +#include <stout/duration.hpp> #include <stout/json.hpp> #include <stout/none.hpp> #include <stout/nothing.hpp> @@ -90,10 +91,15 @@ public: const Option<mesos::Resources>& resources = None(), const Option<std::map<std::string, std::string> >& env = None()) const; - // Performs 'docker kill CONTAINER'. If remove is true then a rm -f - // will be called when kill failed, otherwise a failure is returned. - virtual process::Future<Nothing> kill( + // Performs 'docker stop -t TIMEOUT CONTAINER'. If remove is true then a rm -f + // will be called when stop failed, otherwise a failure is returned. The + // timeout parameter will be passed through to docker and is the amount of + // time for docker to wait after stopping a container before killing it. + // A value of zero (the default value) is the same as issuing a + // 'docker kill CONTAINER'. + process::Future<Nothing> stop( const std::string& container, + const Duration& timeout = Seconds(0), bool remove = false) const; // Performs 'docker rm (-f) CONTAINER'. @@ -128,7 +134,7 @@ protected: Docker(const std::string& _path) : path(_path) {}; private: - static process::Future<Nothing> _kill( + static process::Future<Nothing> _stop( const Docker& docker, const std::string& container, const std::string& cmd, http://git-wip-us.apache.org/repos/asf/mesos/blob/eb075f9b/src/slave/containerizer/docker.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp index 5978ec2..8351ee3 100644 --- a/src/slave/containerizer/docker.cpp +++ b/src/slave/containerizer/docker.cpp @@ -570,9 +570,8 @@ Future<Nothing> DockerContainerizerProcess::_recover( // Check if we're watching an executor for this container ID and // if not, rm -f the Docker container. if (!containers_.contains(id.get())) { - // TODO(benh): Retry 'docker rm -f' if it failed but the container - // still exists (asynchronously). - docker->kill(container.id, true); + // TODO(tnachen): Consider using executor_shutdown_grace_period. + docker->stop(container.id, flags.docker_stop_timeout, true); } } @@ -1289,11 +1288,10 @@ void DockerContainerizerProcess::_destroy( // a 'docker wait' on the container using the --override flag of // mesos-executor. - // TODO(benh): Retry 'docker rm -f' if it failed but the container - // still exists (asynchronously). + LOG(INFO) << "Running docker stop on container '" << containerId << "'"; - LOG(INFO) << "Running docker kill on container '" << containerId << "'"; - docker->kill(container->name(), false) + docker->stop(container->name(), + flags.docker_stop_timeout) .onAny(defer(self(), &Self::__destroy, containerId, killed, lambda::_1)); } http://git-wip-us.apache.org/repos/asf/mesos/blob/eb075f9b/src/slave/flags.hpp ---------------------------------------------------------------------- diff --git a/src/slave/flags.hpp b/src/slave/flags.hpp index 4ec5954..fee79e0 100644 --- a/src/slave/flags.hpp +++ b/src/slave/flags.hpp @@ -332,6 +332,12 @@ public: "}" ); + add(&Flags::docker_stop_timeout, + "docker_stop_timeout", + "The time as a duration for docker to wait after stopping an instance\n" + "before it kills that instance.", + Seconds(0)); + #ifdef WITH_NETWORK_ISOLATOR add(&Flags::ephemeral_ports_per_container, "ephemeral_ports_per_container", @@ -454,6 +460,7 @@ public: std::string docker_sandbox_directory; Duration docker_remove_delay; Option<ContainerInfo> default_container_info; + Duration docker_stop_timeout; #ifdef WITH_NETWORK_ISOLATOR uint16_t ephemeral_ports_per_container; Option<std::string> eth0_name; http://git-wip-us.apache.org/repos/asf/mesos/blob/eb075f9b/src/tests/docker_containerizer_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/docker_containerizer_tests.cpp b/src/tests/docker_containerizer_tests.cpp index 66552ad..59c2beb 100644 --- a/src/tests/docker_containerizer_tests.cpp +++ b/src/tests/docker_containerizer_tests.cpp @@ -24,6 +24,8 @@ #include <process/owned.hpp> #include <process/subprocess.hpp> +#include <stout/duration.hpp> + #include "linux/cgroups.hpp" #include "messages/messages.hpp" @@ -76,8 +78,8 @@ public: EXPECT_CALL(*this, logs(_, _)) .WillRepeatedly(Invoke(this, &MockDocker::_logs)); - EXPECT_CALL(*this, kill(_, _)) - .WillRepeatedly(Invoke(this, &MockDocker::_kill)); + EXPECT_CALL(*this, stop(_, _, _)) + .WillRepeatedly(Invoke(this, &MockDocker::_stop)); } MOCK_CONST_METHOD2( @@ -86,7 +88,11 @@ public: const string&, const string&)); - MOCK_CONST_METHOD2(kill, process::Future<Nothing>(const string&, bool)); + MOCK_CONST_METHOD3( + stop, + process::Future<Nothing>( + const string&, + const Duration&, bool)); process::Future<Nothing> _logs( const string& container, @@ -95,11 +101,12 @@ public: return Docker::logs(container, directory); } - process::Future<Nothing> _kill( + process::Future<Nothing> _stop( const string& container, + const Duration& timeout, bool remove) const { - return Docker::kill(container, remove); + return Docker::stop(container, timeout, remove); } }; @@ -1233,10 +1240,10 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Logs) Invoke((MockDocker*) docker.get(), &MockDocker::_logs))); - // We skip killing the docker container because killing a container + // We skip stopping the docker container because stopping a container // even when it terminated might not flush the logs and we end up // not getting stdout/stderr in our tests. - EXPECT_CALL(*mockDocker, kill(_, _)) + EXPECT_CALL(*mockDocker, stop(_, _, _)) .WillRepeatedly(Return(Nothing())); MockDockerContainerizer dockerContainerizer(flags, docker); @@ -1359,10 +1366,10 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Default_CMD) Invoke((MockDocker*) docker.get(), &MockDocker::_logs))); - // We skip killing the docker container because killing a container + // We skip stopping the docker container because stopping a container // even when it terminated might not flush the logs and we end up // not getting stdout/stderr in our tests. - EXPECT_CALL(*mockDocker, kill(_, _)) + EXPECT_CALL(*mockDocker, stop(_, _, _)) .WillRepeatedly(Return(Nothing())); MockDockerContainerizer dockerContainerizer(flags, docker); @@ -1486,10 +1493,10 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Default_CMD_Override) Invoke((MockDocker*) docker.get(), &MockDocker::_logs))); - // We skip killing the docker container because killing a container + // We skip stopping the docker container because stopping a container // even when it terminated might not flush the logs and we end up // not getting stdout/stderr in our tests. - EXPECT_CALL(*mockDocker, kill(_, _)) + EXPECT_CALL(*mockDocker, stop(_, _, _)) .WillRepeatedly(Return(Nothing())); MockDockerContainerizer dockerContainerizer(flags, docker); @@ -1618,10 +1625,10 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Default_CMD_Args) Invoke((MockDocker*) docker.get(), &MockDocker::_logs))); - // We skip killing the docker container because killing a container + // We skip stopping the docker container because stopping a container // even when it terminated might not flush the logs and we end up // not getting stdout/stderr in our tests. - EXPECT_CALL(*mockDocker, kill(_, _)) + EXPECT_CALL(*mockDocker, stop(_, _, _)) .WillRepeatedly(Return(Nothing())); MockDockerContainerizer dockerContainerizer(flags, docker); @@ -2105,10 +2112,10 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_PortMapping) Invoke((MockDocker*) docker.get(), &MockDocker::_logs))); - // We skip killing the docker container because killing a container + // We skip stopping the docker container because stopping a container // even when it terminated might not flush the logs and we end up // not getting stdout/stderr in our tests. - EXPECT_CALL(*mockDocker, kill(_, _)) + EXPECT_CALL(*mockDocker, stop(_, _, _)) .WillRepeatedly(Return(Nothing())); MockDockerContainerizer dockerContainerizer(flags, docker); http://git-wip-us.apache.org/repos/asf/mesos/blob/eb075f9b/src/tests/docker_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/docker_tests.cpp b/src/tests/docker_tests.cpp index ff06a01..ef05828 100644 --- a/src/tests/docker_tests.cpp +++ b/src/tests/docker_tests.cpp @@ -23,6 +23,7 @@ #include <process/owned.hpp> #include <process/subprocess.hpp> +#include <stout/duration.hpp> #include <stout/option.hpp> #include <stout/gtest.hpp> @@ -106,8 +107,8 @@ TEST(DockerTest, ROOT_DOCKER_interface) EXPECT_EQ("/" + containerName, container.get().name); EXPECT_SOME(container.get().pid); - // Kill the container. - status = docker->kill(containerName); + // Stop the container. + status = docker->stop(containerName); AWAIT_READY(status); // Now, the container should not appear in the result of ps(). @@ -156,7 +157,7 @@ TEST(DockerTest, ROOT_DOCKER_interface) } // Start the container again, this time we will do a "rm -f" - // directly, instead of killing and rm. + // directly, instead of stopping and rm. status = docker->run( containerInfo, commandInfo,
