You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2018/01/23 03:36:14 UTC
[1/3] mesos git commit: Removed the misleading `isRemovable` helper
in the master.
Repository: mesos
Updated Branches:
refs/heads/master 62d392704 -> e91ce42ed
Removed the misleading `isRemovable` helper in the master.
In the past, the notion of a "removable" task meant: the task is
terminal and acknowledged. However, the `isRemovable` helper only
defines removability by the task state (terminal or unreachable)
but not whether the terminal update is acknowledged.
As a result, the code that is calling `isRemovable` is unintuitive.
One example of a confusing piece of code is within updateTask. Here,
we have logic which says, if the task is removable, recover the
resources but don't remove it. This seems more intuitive if directly
described as: "if the task is no longer consuming resources (e.g.
transitioned to terminal or unreachable) then recover the resources".
If one looks up the documentation of `isRemovable`, it says "When a
task becomes removable, it is erased from the master's primary task
data structures", but that isn't accurate since this function doesn't
say whether the terminal task has been acknowledged, which is
required for a task to be removable.
To make an immediate improvement, this patch removes `isRemovable`
(no pun intended) in favor of directly checking the states we care
about. In the future, we may want to introduce some helpers like
`isAllocatedResources` to describe whether the task's resources are
considered allocated.
If we do introduce a notion of `isRemovable` in the future, it seems
it should take the `Task*` so that it can say whether the task could
be "removed" from the master, which includes checking that terminal
tasks have been acknowledged. However, "remove" is somewhat of a
confusing term, since what we're actually doing is "completing" the
task.
Review: https://reviews.apache.org/r/65106
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/5db5d8d6
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/5db5d8d6
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/5db5d8d6
Branch: refs/heads/master
Commit: 5db5d8d68692093128434c370b867c8dee85c8f0
Parents: 62d3927
Author: Benjamin Mahler <bm...@apache.org>
Authored: Thu Jan 11 17:09:30 2018 -0800
Committer: Benjamin Mahler <bm...@apache.org>
Committed: Mon Jan 22 19:22:10 2018 -0800
----------------------------------------------------------------------
src/master/master.cpp | 96 ++++++++++++++++++++++++++++------------------
src/master/master.hpp | 40 ++++++++++---------
2 files changed, 81 insertions(+), 55 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/5db5d8d6/src/master/master.cpp
----------------------------------------------------------------------
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 7b6b5c4..c731f6e 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -10158,38 +10158,33 @@ void Master::updateTask(Task* task, const StatusUpdate& update)
latestState = update.latest_state();
}
- // Indicated whether we should send a notification to all subscribers if the
- // task transitioned to a new state.
- bool sendSubscribersUpdate = false;
+ // Determine whether the task transitioned to terminal or
+ // unreachable prior to changing the task state.
+ auto isTerminalOrUnreachableState = [](const TaskState& state) {
+ return protobuf::isTerminalState(state) || state == TASK_UNREACHABLE;
+ };
- // Set 'removable' to true if this is the first time the task
- // transitioned to a removable state. Also set the latest state.
- bool removable;
- if (latestState.isSome()) {
- removable = !isRemovable(task->state()) && isRemovable(latestState.get());
-
- // If the task has already transitioned to a terminal state,
- // do not update its state.
- if (!protobuf::isTerminalState(task->state())) {
- if (latestState.get() != task->state()) {
- sendSubscribersUpdate = true;
- }
+ bool transitionedToTerminalOrUnreachable =
+ !isTerminalOrUnreachableState(task->state()) &&
+ isTerminalOrUnreachableState(latestState.getOrElse(status.state()));
- task->set_state(latestState.get());
- }
- } else {
- removable = !isRemovable(task->state()) && isRemovable(status.state());
-
- // If the task has already transitioned to a terminal state, do not update
- // its state. Note that we are being defensive here because this should not
- // happen unless there is a bug in the master code.
- if (!protobuf::isTerminalState(task->state())) {
- if (status.state() != task->state()) {
- sendSubscribersUpdate = true;
- }
+ // Indicates whether we should send a notification to subscribers,
+ // set if the task transitioned to a new state.
+ bool sendSubscribersUpdate = false;
- task->set_state(status.state());
+ // If the task has already transitioned to a terminal state,
+ // do not update its state. Note that we are being defensive
+ // here because this should not happen unless there is a bug
+ // in the master code.
+ //
+ // TODO(bmahler): Check that we're not transitioning from
+ // TASK_UNREACHABLE to another state.
+ if (!protobuf::isTerminalState(task->state())) {
+ if (status.state() != task->state()) {
+ sendSubscribersUpdate = true;
}
+
+ task->set_state(latestState.getOrElse(status.state()));
}
// TODO(brenden): Consider wiping the `message` field?
@@ -10217,8 +10212,9 @@ void Master::updateTask(Task* task, const StatusUpdate& update)
<< " (latest state: " << task->state()
<< ", status update state: " << status.state() << ")";
- // Once the task becomes removable, recover the resources.
- if (removable) {
+ // Once the task transitioned to terminal or unreachable,
+ // recover the resources.
+ if (transitionedToTerminalOrUnreachable) {
allocator->recoverResources(
task->framework_id(),
task->slave_id(),
@@ -10297,17 +10293,25 @@ void Master::removeTask(Task* task, bool unreachable)
// Conversion is safe, as resources have already passed validation.
const Resources resources = task->resources();
- if (!isRemovable(task->state())) {
+ // The invariant here is that the master will recover the resources
+ // prior to removing terminal or unreachable tasks. If the task is
+ // not terminal or unreachable, we must recover the resources here.
+ //
+ // TODO(bmahler): Currently, only `Master::finalize()` will call
+ // `removeTask()` with a non-terminal task. Consider fixing this
+ // and instead CHECKing here to simplify the logic.
+ if (!protobuf::isTerminalState(task->state()) &&
+ task->state() != TASK_UNREACHABLE) {
+ CHECK(!unreachable) << task->task_id();
+
// Note that we use `Resources` for output as it's faster than
// logging raw protobuf data.
LOG(WARNING) << "Removing task " << task->task_id()
<< " with resources " << resources
<< " of framework " << task->framework_id()
<< " on agent " << *slave
- << " in non-removable state " << task->state();
+ << " in non-terminal state " << task->state();
- // If the task is not removable, then the resources have
- // not yet been recovered.
allocator->recoverResources(
task->framework_id(),
task->slave_id(),
@@ -11517,7 +11521,11 @@ void Slave::addTask(Task* task)
// Conversion is safe, as resources have already passed validation.
const Resources resources = task->resources();
- if (!Master::isRemovable(task->state())) {
+ CHECK(task->state() != TASK_UNREACHABLE)
+ << "Task '" << taskId << "' of framework " << frameworkId
+ << " added in TASK_UNREACHABLE state";
+
+ if (!protobuf::isTerminalState(task->state())) {
usedResources[frameworkId] += resources;
}
@@ -11538,7 +11546,11 @@ void Slave::recoverResources(Task* task)
const TaskID& taskId = task->task_id();
const FrameworkID& frameworkId = task->framework_id();
- CHECK(Master::isRemovable(task->state()));
+ CHECK(protobuf::isTerminalState(task->state()) ||
+ task->state() == TASK_UNREACHABLE)
+ << "Task '" << taskId << "' of framework " << frameworkId
+ << " is in unexpected state " << task->state();
+
CHECK(tasks.at(frameworkId).contains(taskId))
<< "Unknown task " << taskId << " of framework " << frameworkId;
@@ -11557,7 +11569,17 @@ void Slave::removeTask(Task* task)
CHECK(tasks.at(frameworkId).contains(taskId))
<< "Unknown task " << taskId << " of framework " << frameworkId;
- if (!Master::isRemovable(task->state())) {
+ // The invariant here is that the master will have already called
+ // `recoverResources()` prior to removing terminal or unreachable tasks.
+ //
+ // TODO(bmahler): The unreachable case could be avoided if
+ // we updated `removeSlave` in the allocator to recover the
+ // resources (see MESOS-621) so that the master could just
+ // remove the unreachable agent from the allocator.
+ if (!protobuf::isTerminalState(task->state()) &&
+ task->state() != TASK_UNREACHABLE) {
+ // We cannot call `Slave::recoverResources()` here because
+ // it expects the task to be terminal or unreachable.
usedResources[frameworkId] -= task->resources();
if (usedResources[frameworkId].empty()) {
usedResources.erase(frameworkId);
http://git-wip-us.apache.org/repos/asf/mesos/blob/5db5d8d6/src/master/master.hpp
----------------------------------------------------------------------
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 1fa428d..b434d23 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -1096,22 +1096,6 @@ private:
bool isWhitelistedRole(const std::string& name) const;
/**
- * Indicates whether a task in the given state can safely be removed
- * from the master's in-memory state. When a task becomes removable,
- * it is erased from the master's primary task data structures; a
- * limited number of such tasks are kept as a cache (see
- * `framework.unreachableTasks` and `framework.completedTasks`).
- */
- static bool isRemovable(const TaskState& state)
- {
- if (state == TASK_UNREACHABLE) {
- return true;
- }
-
- return protobuf::isTerminalState(state);
- }
-
- /**
* Inner class used to namespace the handling of quota requests.
*
* It operates inside the Master actor. It is responsible for validating
@@ -2241,7 +2225,20 @@ struct Framework
tasks[task->task_id()] = task;
- if (!Master::isRemovable(task->state())) {
+ // Unreachable tasks should be added via `addUnreachableTask`.
+ CHECK(task->state() != TASK_UNREACHABLE)
+ << "Task '" << task->task_id() << "' of framework " << id()
+ << " added in TASK_UNREACHABLE state";
+
+ // Since we track terminal but unacknowledged tasks within
+ // `tasks` rather than `completedTasks`, we need to handle
+ // them here: don't count them as consuming resources.
+ //
+ // TODO(bmahler): Users currently get confused because
+ // terminal tasks can show up as "active" tasks in the UI and
+ // endpoints. Ideally, we show the terminal unacknowledged
+ // tasks as "completed" as well.
+ if (!protobuf::isTerminalState(task->state())) {
// Note that we explicitly convert from protobuf to `Resources` once
// and then use the result for calculations to avoid performance penalty
// for multiple conversions and validations implied by `+=` with protobuf
@@ -2347,13 +2344,20 @@ struct Framework
<< "Unknown task " << task->task_id()
<< " of framework " << task->framework_id();
- if (!Master::isRemovable(task->state())) {
+ // The invariant here is that the master will have already called
+ // `recoverResources()` prior to removing terminal or unreachable tasks.
+ if (!protobuf::isTerminalState(task->state()) &&
+ task->state() != TASK_UNREACHABLE) {
recoverResources(task);
}
if (unreachable) {
addUnreachableTask(*task);
} else {
+ CHECK(task->state() != TASK_UNREACHABLE);
+
+ // TODO(bmahler): This moves a potentially non-terminal task into
+ // the completed list!
addCompletedTask(Task(*task));
}
[2/3] mesos git commit: Updated the task state metrics to be more
readable.
Posted by bm...@apache.org.
Updated the task state metrics to be more readable.
Review: https://reviews.apache.org/r/65107
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/9a83e417
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9a83e417
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9a83e417
Branch: refs/heads/master
Commit: 9a83e4172b383a419df081138b9bbd5be29638d7
Parents: 5db5d8d
Author: Benjamin Mahler <bm...@apache.org>
Authored: Thu Jan 11 17:16:46 2018 -0800
Committer: Benjamin Mahler <bm...@apache.org>
Committed: Mon Jan 22 19:22:17 2018 -0800
----------------------------------------------------------------------
src/master/master.cpp | 48 ++++++++++++++++------------------------------
1 file changed, 17 insertions(+), 31 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/9a83e417/src/master/master.cpp
----------------------------------------------------------------------
diff --git a/src/master/master.cpp b/src/master/master.cpp
index c731f6e..b97ebae 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -10233,38 +10233,24 @@ void Master::updateTask(Task* task, const StatusUpdate& update)
}
switch (status.state()) {
- case TASK_FINISHED:
- ++metrics->tasks_finished;
- break;
- case TASK_FAILED:
- ++metrics->tasks_failed;
- break;
- case TASK_KILLED:
- ++metrics->tasks_killed;
- break;
- case TASK_LOST:
- ++metrics->tasks_lost;
- break;
- case TASK_ERROR:
- ++metrics->tasks_error;
- break;
- case TASK_DROPPED:
- ++metrics->tasks_dropped;
- break;
- case TASK_GONE:
- ++metrics->tasks_gone;
- break;
- case TASK_GONE_BY_OPERATOR:
- ++metrics->tasks_gone_by_operator;
- break;
- case TASK_STARTING:
- case TASK_STAGING:
- case TASK_RUNNING:
- case TASK_KILLING:
- case TASK_UNREACHABLE:
- break;
+ case TASK_FINISHED: ++metrics->tasks_finished; break;
+ case TASK_FAILED: ++metrics->tasks_failed; break;
+ case TASK_KILLED: ++metrics->tasks_killed; break;
+ case TASK_LOST: ++metrics->tasks_lost; break;
+ case TASK_ERROR: ++metrics->tasks_error; break;
+ case TASK_DROPPED: ++metrics->tasks_dropped; break;
+ case TASK_GONE: ++metrics->tasks_gone; break;
+ case TASK_GONE_BY_OPERATOR: ++metrics->tasks_gone_by_operator; break;
+
+ // The following are non-terminal and use gauge based metrics.
+ case TASK_STARTING: break;
+ case TASK_STAGING: break;
+ case TASK_RUNNING: break;
+ case TASK_KILLING: break;
+ case TASK_UNREACHABLE: break;
+
+ // Should not happen.
case TASK_UNKNOWN:
- // Should not happen.
LOG(FATAL) << "Unexpected TASK_UNKNOWN for in-memory task";
break;
}
[3/3] mesos git commit: Added documentation for
`protobuf::isTerminalState`.
Posted by bm...@apache.org.
Added documentation for `protobuf::isTerminalState`.
Review: https://reviews.apache.org/r/65108
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/e91ce42e
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/e91ce42e
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/e91ce42e
Branch: refs/heads/master
Commit: e91ce42ed56c5ab65220fbba740a8a50c7f835ae
Parents: 9a83e41
Author: Benjamin Mahler <bm...@apache.org>
Authored: Thu Jan 11 17:19:25 2018 -0800
Committer: Benjamin Mahler <bm...@apache.org>
Committed: Mon Jan 22 19:22:17 2018 -0800
----------------------------------------------------------------------
src/common/protobuf_utils.hpp | 5 +++++
1 file changed, 5 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/e91ce42e/src/common/protobuf_utils.hpp
----------------------------------------------------------------------
diff --git a/src/common/protobuf_utils.hpp b/src/common/protobuf_utils.hpp
index fd728b7..0ff7239 100644
--- a/src/common/protobuf_utils.hpp
+++ b/src/common/protobuf_utils.hpp
@@ -67,6 +67,11 @@ bool frameworkHasCapability(
FrameworkInfo::Capability::Type capability);
+// Returns whether the task state is terminal. Terminal states
+// mean that the resources are released and the task cannot
+// transition back to a non-terminal state. Note that
+// `TASK_UNREACHABLE` is not a terminal state, but still
+// releases the resources.
bool isTerminalState(const TaskState& state);