You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by al...@apache.org on 2018/03/27 09:38:43 UTC
[1/8] mesos git commit: Changed the signature of a DefaultExecutor
kill method.
Repository: mesos
Updated Branches:
refs/heads/master a741b15e8 -> 4e7bbe67f
Changed the signature of a DefaultExecutor kill method.
This method is being passed the container to be killed as
`Owned<Container>`.
The container object is owned by the executor and stored
in `LinkedHashMap<TaskID, Owner<Container>> containers`.
We should pass around raw pointers and not make copies of
the stored `Owned` object, so this patch changes the
signature of the `kill` method.
Review: https://reviews.apache.org/r/65692/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/73766e63
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/73766e63
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/73766e63
Branch: refs/heads/master
Commit: 73766e635c6619efe94eeef8fa8b7002ee2b6f50
Parents: a741b15
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:36:29 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:36:29 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/73766e63/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index 906836f..d5bd20e 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -994,7 +994,7 @@ protected:
continue;
}
- kill(container_);
+ kill(container_.get());
}
}
}
@@ -1036,7 +1036,7 @@ protected:
continue;
}
- killResponses.push_back(kill(container));
+ killResponses.push_back(kill(container.get()));
}
// It is possible that the agent process can fail while we are
@@ -1072,7 +1072,7 @@ protected:
}
Future<Nothing> kill(
- Owned<Container> container,
+ Container* container,
const Option<KillPolicy>& killPolicy = None())
{
CHECK_EQ(SUBSCRIBED, state);
@@ -1238,7 +1238,7 @@ protected:
return;
}
- kill(container, killPolicy);
+ kill(container.get(), killPolicy);
}
void taskCheckUpdated(
[5/8] mesos git commit: Made the default executor log kill failures.
Posted by al...@apache.org.
Made the default executor log kill failures.
Review: https://reviews.apache.org/r/66233/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/7d0b4489
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/7d0b4489
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/7d0b4489
Branch: refs/heads/master
Commit: 7d0b4489bb940bf0b116342589b22edc247b65b9
Parents: 501eb34
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:37:44 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:37:44 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/7d0b4489/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index a2f1ec5..57bcdf9 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -1245,7 +1245,13 @@ protected:
return;
}
- kill(container.get(), killPolicy);
+ const ContainerID& containerId = container->containerId;
+ kill(container.get(), killPolicy)
+ .onFailed(defer(self(), [=](const string& failure) {
+ LOG(WARNING) << "Failed to kill the task '" << taskId
+ << "' running in child container " << containerId << ": "
+ << failure;
+ }));
}
void taskCheckUpdated(
[7/8] mesos git commit: Fixed variable shadowing in the default
executor.
Posted by al...@apache.org.
Fixed variable shadowing in the default executor.
Review: https://reviews.apache.org/r/66234/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/554429fe
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/554429fe
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/554429fe
Branch: refs/heads/master
Commit: 554429fec0cebb15673d0f675ad2642b07403f18
Parents: cbc7d0e
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:38:06 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:38:06 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/554429fe/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index 95c4a47..d4d913e 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -958,16 +958,16 @@ protected:
container->killingTaskGroup = true;
foreach (const TaskInfo& task, container->taskGroup.tasks()) {
- const TaskID& taskId = task.task_id();
+ const TaskID& taskId_ = task.task_id();
// Ignore if it's the same task that triggered this callback or
// if the task is no longer active.
- if (taskId == container->taskInfo.task_id() ||
- !containers.contains(taskId)) {
+ if (taskId_ == container->taskInfo.task_id() ||
+ !containers.contains(taskId_)) {
continue;
}
- Container* container_ = containers.at(taskId).get();
+ Container* container_ = containers.at(taskId_).get();
container_->killingTaskGroup = true;
// Ignore if the task is already being killed. This can happen
[6/8] mesos git commit: Avoided copying `Owned` pointers in the
default executor.
Posted by al...@apache.org.
Avoided copying `Owned` pointers in the default executor.
`Owned` pointers are copied in multiple places of the default executor.
This violates the semantic of owned pointers and works only because
`Owned` is currently implemented with `shared_ptr`, it would otherwise
lead to double-freeing the pointers.
This patch changes those places to use references to the original
`Owned` objects or raw pointers instead of copies.
Review: https://reviews.apache.org/r/65962/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/cbc7d0e2
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/cbc7d0e2
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/cbc7d0e2
Branch: refs/heads/master
Commit: cbc7d0e2654310063d6ed856b40e3775901e2333
Parents: 7d0b448
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:37:59 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:37:59 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 52 ++++++++++++++--------------------
1 file changed, 21 insertions(+), 31 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbc7d0e2/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index 57bcdf9..95c4a47 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -172,7 +172,7 @@ public:
// Disconnect all active connections used for
// waiting on child containers.
- foreachvalue (Owned<Container> container, containers) {
+ foreachvalue (Owned<Container>& container, containers) {
if (container->waiting.isSome()) {
container->waiting->disconnect();
container->waiting = None();
@@ -180,7 +180,7 @@ public:
}
// Pause all checks and health checks.
- foreachvalue (Owned<Container> container, containers) {
+ foreachvalue (Owned<Container>& container, containers) {
if (container->checker.isSome()) {
container->checker->get()->pause();
}
@@ -214,7 +214,7 @@ public:
}
// Resume all checks and health checks.
- foreachvalue (Owned<Container> container, containers) {
+ foreachvalue (Owned<Container>& container, containers) {
if (container->checker.isSome()) {
container->checker->get()->resume();
}
@@ -726,7 +726,7 @@ protected:
CHECK_SOME(connectionId);
CHECK(containers.contains(taskId));
- Owned<Container> container = containers.at(taskId);
+ Container* container = containers.at(taskId).get();
LOG(INFO) << "Waiting for child container " << container->containerId
<< " of task '" << taskId << "'";
@@ -767,7 +767,7 @@ protected:
CHECK_EQ(SUBSCRIBED, state);
CHECK(containers.contains(taskId));
- Owned<Container> container = containers.at(taskId);
+ Container* container = containers.at(taskId).get();
CHECK_SOME(container->waiting);
@@ -932,34 +932,16 @@ protected:
forward(taskStatus);
- CHECK(containers.contains(taskId));
- containers.erase(taskId);
-
LOG(INFO)
<< "Child container " << container->containerId << " of task '" << taskId
<< "' completed in state " << stringify(taskState)
<< ": " << message.get();
- // Shutdown the executor if all the active child containers have terminated.
- if (containers.empty()) {
- _shutdown();
- return;
- }
-
- // Ignore if the executor is already in the process of shutting down.
- if (shuttingDown) {
- return;
- }
-
- // Ignore if this task group is already in the process of being killed.
- if (container->killingTaskGroup) {
- return;
- }
-
// The default restart policy for a task group is to kill all the
// remaining child containers if one of them terminated with a
// non-zero exit code.
- if (taskState == TASK_FAILED || taskState == TASK_KILLED) {
+ if (!shuttingDown && !container->killingTaskGroup &&
+ (taskState == TASK_FAILED || taskState == TASK_KILLED)) {
// Needed for logging.
auto taskIds = [container]() {
list<TaskID> taskIds_;
@@ -985,7 +967,7 @@ protected:
continue;
}
- Owned<Container> container_ = containers.at(taskId);
+ Container* container_ = containers.at(taskId).get();
container_->killingTaskGroup = true;
// Ignore if the task is already being killed. This can happen
@@ -996,9 +978,17 @@ protected:
continue;
}
- kill(container_.get());
+ kill(container_);
}
}
+
+ CHECK(containers.contains(taskId));
+ containers.erase(taskId);
+
+ // Shutdown the executor if all the active child containers have terminated.
+ if (containers.empty()) {
+ _shutdown();
+ }
}
void shutdown()
@@ -1238,7 +1228,7 @@ protected:
return;
}
- const Owned<Container>& container = containers.at(taskId);
+ Container* container = containers.at(taskId).get();
if (container->killing) {
LOG(WARNING) << "Ignoring kill for task '" << taskId
<< "' as it is in the process of getting killed";
@@ -1246,7 +1236,7 @@ protected:
}
const ContainerID& containerId = container->containerId;
- kill(container.get(), killPolicy)
+ kill(container, killPolicy)
.onFailed(defer(self(), [=](const string& failure) {
LOG(WARNING) << "Failed to kill the task '" << taskId
<< "' running in child container " << containerId << ": "
@@ -1386,7 +1376,7 @@ private:
}
CHECK(containers.contains(taskId));
- const Owned<Container>& container = containers.at(taskId);
+ const Container* container = containers.at(taskId).get();
// TODO(alexr): Augment health information in a way similar to
// `CheckStatusInfo`. See MESOS-6417 for more details.
@@ -1504,7 +1494,7 @@ private:
CHECK_SOME(connectionId);
CHECK(containers.contains(taskId));
- const Owned<Container>& container = containers.at(taskId);
+ const Container* container = containers.at(taskId).get();
if (!connection.isReady()) {
LOG(ERROR)
[4/8] mesos git commit: Made the default executor's handling of kill
escalations more robust.
Posted by al...@apache.org.
Made the default executor's handling of kill escalations more robust.
This patch makes the default executor retry SIGKILL escalations if the
executor is disconnected from the agent or the kill call fails.
Review: https://reviews.apache.org/r/65694/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/501eb344
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/501eb344
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/501eb344
Branch: refs/heads/master
Commit: 501eb34467812143a6462441e9d8447be474c675
Parents: 801aee6
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:37:28 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:37:28 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/501eb344/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index 1109175..a2f1ec5 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -1200,7 +1200,19 @@ protected:
<< " did not terminate after " << timeout << ", sending SIGKILL"
<< " to the container";
- kill(containerId, SIGKILL);
+ kill(containerId, SIGKILL).onFailed([=](const string& failure) {
+ const Duration duration = Seconds(1);
+
+ LOG(WARNING)
+ << "Escalation to SIGKILL the task '" << taskId
+ << "' running in child container " << containerId
+ << " failed: " << failure << "; Retrying in " << duration;
+
+ process::delay(
+ duration, self(), &Self::escalated, containerId, taskId, timeout);
+
+ return;
+ });
}
void killTask(
[2/8] mesos git commit: Made the default executor fail kills if the
response isn't "200 OK".
Posted by al...@apache.org.
Made the default executor fail kills if the response isn't "200 OK".
The default executor's `Future<Nothing> kill(const ContainerID&, int)`
method returns `Nothing()` if the agent responded to the
`KILL_NESTED_CONTAINER` call, regardless of the response.
This patch updates the method, so that it returns a failure if the
response is not "200 OK".
Review: https://reviews.apache.org/r/65693/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/1e9bfbf3
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/1e9bfbf3
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/1e9bfbf3
Branch: refs/heads/master
Commit: 1e9bfbf3705cc9668a0a40829d311493a679712d
Parents: 73766e6
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:37:06 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:37:06 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/1e9bfbf3/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index d5bd20e..73e087a 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -30,6 +30,7 @@
#include <process/collect.hpp>
#include <process/defer.hpp>
#include <process/delay.hpp>
+#include <process/future.hpp>
#include <process/id.hpp>
#include <process/owned.hpp>
#include <process/process.hpp>
@@ -62,6 +63,7 @@ using mesos::executor::Event;
using mesos::v1::executor::Mesos;
using process::Clock;
+using process::Failure;
using process::Future;
using process::Owned;
using process::UPID;
@@ -1170,7 +1172,15 @@ protected:
kill->set_signal(signal);
return post(None(), call)
- .then([](const Response& /* response */) {
+ .then([=](const Response& response) -> Future<Nothing> {
+ if (response.code != process::http::Status::OK) {
+ return Failure(
+ stringify("The agent failed to send signal") +
+ " " + strsignal(signal) + " (" + stringify(signal) + ")" +
+ " to the container " + stringify(containerId) +
+ ": " + response.body);
+ }
+
return Nothing();
});
}
[8/8] mesos git commit: Added a reference to MESOS-8557 to the
default executor.
Posted by al...@apache.org.
Added a reference to MESOS-8557 to the default executor.
Review: https://reviews.apache.org/r/66235/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/4e7bbe67
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/4e7bbe67
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/4e7bbe67
Branch: refs/heads/master
Commit: 4e7bbe67f55fbaa560466fc1d0a2f5e5bdb6ab32
Parents: 554429f
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:38:13 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:38:13 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/4e7bbe67/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index d4d913e..05cd012 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -1142,6 +1142,19 @@ protected:
forward(status);
}
+
+ // Ideally we should detect and act on this kill's failure, and perform the
+ // following actions only once the kill is successful:
+ //
+ // 1) Stop (health) checking.
+ // 2) Send a `TASK_KILLING` task status update.
+ // 3) Schedule the kill escalation.
+ // 4) Set `container->killing` to `true`
+ //
+ // If the kill fails or times out, we could do one of the following options:
+ //
+ // 1) Automatically retry the kill (MESOS-8726).
+ // 2) Let the scheduler request another kill.
return kill(containerId, SIGTERM);
}
@@ -1218,7 +1231,7 @@ protected:
// TODO(anand): Add support for adjusting the remaining grace period if
// we receive another kill request while a task is being killed but has
// not terminated yet. See similar comments in the command executor
- // for more context.
+ // for more context. See MESOS-8557 for more details.
LOG(INFO) << "Received kill for task '" << taskId << "'";
[3/8] mesos git commit: Removed unnecessary/invalid checks from the
default executor.
Posted by al...@apache.org.
Removed unnecessary/invalid checks from the default executor.
The default executor uses checks to ensure that it is subscribed to the
agent before killing a task/container; if it is not, it will skip kill
calls or crash.
When killing a task/container, the default executor performs the
following actions:
1) Calls `KILL_NESTED_CONTAINER`.
2) Enqueues a task status update (for tasks only).
None of these actions require an active subscription.
When the checks were added:
1) The default executor supported launching only one task group, so it
was correct to assume that killing a task would eventually trigger the
destruction of all the child containers.
2) It was assumed that the executor will only be unsubscribed for brief
periods of time, mostly during agent recovery, when kill calls are
likely to fail.
3) There was no kill/escalation retry logic.
The checks were added as a way of working around the lack of retry logic
for kill requests, relying on the fact that crashing the executor leads
to the destruction of all the child containers.
This chain adds retry logic for failed kill call escalation, making the
workaround unnecessary.
Now that the default executor supports running multiple task groups, the
checks are not just unnecessary, but also invalid and dangerous. If a
check fails, all the containers started by the executor will be killed,
regardless of which task group they belong to. This is bad and could
lead to data loss.
This patch removes these unnecessary and sometimes invalid checks from
the default executor.
Review: https://reviews.apache.org/r/66232/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/801aee6b
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/801aee6b
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/801aee6b
Branch: refs/heads/master
Commit: 801aee6b9c95f695d3d33b8ee444e2b875347786
Parents: 1e9bfbf
Author: Gaston Kleiman <ga...@mesosphere.io>
Authored: Tue Mar 27 11:37:15 2018 +0200
Committer: Alexander Rukletsov <al...@apache.org>
Committed: Tue Mar 27 11:37:15 2018 +0200
----------------------------------------------------------------------
src/launcher/default_executor.cpp | 15 ---------------
1 file changed, 15 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/801aee6b/src/launcher/default_executor.cpp
----------------------------------------------------------------------
diff --git a/src/launcher/default_executor.cpp b/src/launcher/default_executor.cpp
index 73e087a..1109175 100644
--- a/src/launcher/default_executor.cpp
+++ b/src/launcher/default_executor.cpp
@@ -1077,8 +1077,6 @@ protected:
Container* container,
const Option<KillPolicy>& killPolicy = None())
{
- CHECK_EQ(SUBSCRIBED, state);
-
if (!container->launched) {
// We can get here if we're killing a task group for which multiple
// containers failed to launch.
@@ -1140,7 +1138,6 @@ protected:
delay(gracePeriod,
self(),
&Self::escalated,
- connectionId.get(),
containerId,
container->taskInfo.task_id(),
gracePeriod);
@@ -1160,8 +1157,6 @@ protected:
Future<Nothing> kill(const ContainerID& containerId, int signal)
{
- CHECK_EQ(SUBSCRIBED, state);
-
agent::Call call;
call.set_type(agent::Call::KILL_NESTED_CONTAINER);
@@ -1186,18 +1181,10 @@ protected:
}
void escalated(
- const id::UUID& _connectionId,
const ContainerID& containerId,
const TaskID& taskId,
const Duration& timeout)
{
- if (connectionId != _connectionId) {
- VLOG(1) << "Ignoring signal escalation timeout from a stale connection";
- return;
- }
-
- CHECK_EQ(SUBSCRIBED, state);
-
// It might be possible that the container is already terminated.
// If that happens, don't bother escalating to SIGKILL.
if (!containers.contains(taskId)) {
@@ -1226,8 +1213,6 @@ protected:
return;
}
- CHECK_EQ(SUBSCRIBED, state);
-
// TODO(anand): Add support for adjusting the remaining grace period if
// we receive another kill request while a task is being killed but has
// not terminated yet. See similar comments in the command executor