You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by gi...@apache.org on 2018/03/07 09:15:03 UTC
[09/12] mesos git commit: Handled hanging docker `stop`,
`inspect` commands in docker executor.
Handled hanging docker `stop`, `inspect` commands in docker executor.
Previosly, if `docker inspect` command hanged, the docker container
ended up in an unkillable state. This patch adds a timeout for inspect
command after receiving `killTask` analogically to `reaped` handler.
In addition we've added a timeout for `docker stop` command. If docker
`stop` or `inspect` command times out, we discard the related future,
thus the docker library kills previously spawned docker cli subprocess.
As a result, a scheduler can retry `killTask` operation to handle
nasty docker bugs that lead to hanging docker cli.
Review: https://reviews.apache.org/r/65713/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/29aa78ee
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/29aa78ee
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/29aa78ee
Branch: refs/heads/1.4.x
Commit: 29aa78ee05627768697b24bcf78f2a4f028857f9
Parents: d841ba4
Author: Andrei Budnik <ab...@mesosphere.com>
Authored: Fri Mar 2 15:38:59 2018 -0800
Committer: Gilbert Song <so...@gmail.com>
Committed: Mon Mar 5 18:11:12 2018 -0800
----------------------------------------------------------------------
src/docker/executor.cpp | 68 ++++++++++++++++++++++++++++++--------------
1 file changed, 47 insertions(+), 21 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/29aa78ee/src/docker/executor.cpp
----------------------------------------------------------------------
diff --git a/src/docker/executor.cpp b/src/docker/executor.cpp
index e580a48..5df8707 100644
--- a/src/docker/executor.cpp
+++ b/src/docker/executor.cpp
@@ -96,7 +96,6 @@ public:
killed(false),
terminated(false),
killedByHealthCheck(false),
- killingInProgress(false),
launcherDir(launcherDir),
docker(docker),
containerName(containerName),
@@ -298,6 +297,13 @@ public:
return Nothing();
}));
+ inspect
+ .after(DOCKER_INSPECT_TIMEOUT, [=](const Future<Nothing>&) {
+ LOG(WARNING) << "Docker inspect has not finished after "
+ << DOCKER_INSPECT_TIMEOUT;
+ return inspect;
+ });
+
inspect.onFailed(defer(self(), [=](const string& failure) {
LOG(ERROR) << "Failed to inspect container '" << containerName << "'"
<< ": " << failure;
@@ -417,9 +423,8 @@ private:
// TODO(alexr): If a kill is in progress, consider adjusting
// the grace period if a new one is provided.
- // Issue the kill signal if the container is running
- // and kill attempt is not in progress.
- if (run.isSome() && !killingInProgress) {
+ // Issue the kill signal if there was an attempt to launch the container.
+ if (run.isSome()) {
// We have to issue the kill after 'docker inspect' has
// completed, otherwise we may race with 'docker run'
// and docker may not know about the container. Note
@@ -428,6 +433,15 @@ private:
// issued the kill.
inspect
.onAny(defer(self(), &Self::_killTask, _taskId, gracePeriod));
+
+ // If the inspect takes too long we discard it to ensure we
+ // don't wait forever, however in this case there may be no
+ // TASK_RUNNING update.
+ inspect
+ .after(DOCKER_INSPECT_TIMEOUT, [=](const Future<Nothing>&) {
+ inspect.discard();
+ return inspect;
+ });
}
}
@@ -438,9 +452,7 @@ private:
CHECK_SOME(taskId);
CHECK_EQ(taskId_, taskId.get());
- if (!terminated && !killingInProgress) {
- killingInProgress = true;
-
+ if (!terminated) {
// Once the task has been transitioned to `killed`,
// there is no way back, even if the kill attempt
// failed. This also allows us to send TASK_KILLING
@@ -475,28 +487,43 @@ private:
}
}
+ // If a previous attempt to stop a Docker container is still in progress,
+ // we need to kill the hanging Docker CLI subprocess. Discarding this
+ // future triggers a callback in the Docker library that kills the
+ // subprocess.
+ if (stop.isPending()) {
+ LOG(WARNING) << "Previous docker stop has not terminated yet"
+ << " for container '" << containerName << "'";
+ stop.discard();
+ }
+
// TODO(bmahler): Replace this with 'docker kill' so
// that we can adjust the grace period in the case of
// a `KillPolicy` override.
+ //
+ // NOTE: `docker stop` may or may not finish. Our behaviour is to give
+ // the subprocess a chance to finish until next time `_killtask` is
+ // invoked. Also, invoking `docker stop` might be unsuccessful, in which
+ // case the container most probably does not receive the signal.
+ // In any case we should allow schedulers to retry the kill operation or,
+ // if the kill was initiated by a failing health check, retry ourselves.
+ // We do not bail out nor stop retrying to avoid sending a terminal
+ // status update while the container might still be running.
stop = docker->stop(containerName, gracePeriod);
- // Invoking `docker stop` might be unsuccessful, in which case the
- // container most probably does not receive the signal. In this case we
- // should allow schedulers to retry the kill operation or, if the kill
- // was initiated by a failing health check, retry ourselves. We do not
- // bail out nor stop retrying to avoid sending a terminal status update
- // while the container might still be running.
- //
- // NOTE: `docker stop` might also hang. We do not address this for now,
- // because there is no evidence that in this case docker daemon might
- // function properly, i.e., it's only the docker cli command that hangs,
- // and hence there is not so much we can do. See MESOS-6743.
+ if (killedByHealthCheck) {
+ stop
+ .after(KILL_RETRY_INTERVAL, defer(self(), [=](Future<Nothing>) {
+ LOG(INFO) << "Retrying to kill task";
+ _killTask(taskId_, gracePeriod);
+ return stop;
+ }));
+ }
+
stop.onFailed(defer(self(), [=](const string& failure) {
LOG(ERROR) << "Failed to stop container '" << containerName << "'"
<< ": " << failure;
- killingInProgress = false;
-
if (killedByHealthCheck) {
LOG(INFO) << "Retrying to kill task in " << KILL_RETRY_INTERVAL;
delay(
@@ -717,7 +744,6 @@ private:
bool terminated;
bool killedByHealthCheck;
- bool killingInProgress; // Guard against simultaneous kill attempts.
string launcherDir;
Owned<Docker> docker;