This is an automated email from the ASF dual-hosted git repository. chhsiao pushed a commit to branch 1.6.x in repository https://gitbox.apache.org/repos/asf/mesos.git
commit a74efea12583df73cc83d604a55a61076ce3f744 Author: Chun-Hung Hsiao <chhs...@mesosphere.io> AuthorDate: Fri Dec 14 11:01:29 2018 -0800 Made master process all authorization results for `LAUNCH_GROUP`. This patch fixes a bug where the master does not complete the processing of authorization results for `LAUNCH_GROUP`, causing a subsequent operation to drop if one of the remaining authorization is denied. Review: https://reviews.apache.org/r/69571 --- src/master/master.cpp | 79 +++++++++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 34 deletions(-) diff --git a/src/master/master.cpp b/src/master/master.cpp index 67fa5a8..2aaff39 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -4647,8 +4647,8 @@ void Master::_accept( // The order of the conversions is important and preserved. vector<ResourceConversion> conversions; - // The order of `authorizations` must match the order of the operations in - // `accept.operations()`, as they are iterated through simultaneously. + // The order of `authorizations` must match the order of the operations and/or + // tasks in `accept.operations()` as they are iterated through simultaneously. CHECK_READY(_authorizations); list<Future<bool>> authorizations = _authorizations.get(); @@ -4656,6 +4656,7 @@ void Master::_accept( switch (operation.type()) { // The RESERVE operation allows a principal to reserve resources. case Offer::Operation::RESERVE: { + CHECK(!authorizations.empty()); Future<bool> authorization = authorizations.front(); authorizations.pop_front(); @@ -4731,6 +4732,7 @@ void Master::_accept( // The UNRESERVE operation allows a principal to unreserve resources. case Offer::Operation::UNRESERVE: { + CHECK(!authorizations.empty()); Future<bool> authorization = authorizations.front(); authorizations.pop_front(); @@ -4796,6 +4798,7 @@ void Master::_accept( } case Offer::Operation::CREATE: { + CHECK(!authorizations.empty()); Future<bool> authorization = authorizations.front(); authorizations.pop_front(); @@ -4873,6 +4876,7 @@ void Master::_accept( } case Offer::Operation::DESTROY: { + CHECK(!authorizations.empty()); Future<bool> authorization = authorizations.front(); authorizations.pop_front(); @@ -4966,6 +4970,7 @@ void Master::_accept( } case Offer::Operation::GROW_VOLUME: { + CHECK(!authorizations.empty()); Future<bool> authorization = authorizations.front(); authorizations.pop_front(); @@ -5048,6 +5053,7 @@ void Master::_accept( } case Offer::Operation::SHRINK_VOLUME: { + CHECK(!authorizations.empty()); Future<bool> authorization = authorizations.front(); authorizations.pop_front(); @@ -5131,6 +5137,7 @@ void Master::_accept( case Offer::Operation::LAUNCH: { foreach (const TaskInfo& task, operation.launch().task_infos()) { + CHECK(!authorizations.empty()); Future<bool> authorization = authorizations.front(); authorizations.pop_front(); @@ -5362,51 +5369,50 @@ void Master::_accept( // TODO(bmahler): Consider injecting some default (cpus, mem, disk) // resources when the framework omits the executor resources. - // See if there are any validation or authorization errors. + // See if there are any authorization or validation errors. // Note that we'll only report the first error we encounter // for the group. // // TODO(anindya_sinha): If task group uses shared resources, this // validation needs to be enhanced to accommodate multiple copies // of shared resources across tasks within the task group. - Option<Error> error = validation::task::group::validate( - taskGroup, executor, framework, slave, _offeredResources); + Option<Error> error; + Option<TaskStatus::Reason> reason; - Option<TaskStatus::Reason> reason = None(); - - if (error.isSome()) { - reason = TaskStatus::REASON_TASK_GROUP_INVALID; - } else { - foreach (const TaskInfo& task, taskGroup.tasks()) { - Future<bool> authorization = authorizations.front(); - authorizations.pop_front(); - - CHECK(!authorization.isDiscarded()); - - if (authorization.isFailed()) { - error = Error("Failed to authorize task" - " '" + stringify(task.task_id()) + "'" - ": " + authorization.failure()); + // NOTE: We check for the authorization errors first and never break the + // loop to ensure that all authorization futures for this task group are + // iterated through. + foreach (const TaskInfo& task, taskGroup.tasks()) { + CHECK(!authorizations.empty()); + Future<bool> authorization = authorizations.front(); + authorizations.pop_front(); - reason = TaskStatus::REASON_TASK_GROUP_UNAUTHORIZED; + CHECK(!authorization.isDiscarded()); - break; + if (authorization.isFailed()) { + error = Error("Failed to authorize task" + " '" + stringify(task.task_id()) + "'" + ": " + authorization.failure()); + } else if (!authorization.get()) { + string user = framework->info.user(); // Default user. + if (task.has_command() && task.command().has_user()) { + user = task.command().user(); } - if (!authorization.get()) { - string user = framework->info.user(); // Default user. - if (task.has_command() && task.command().has_user()) { - user = task.command().user(); - } - - error = Error("Task '" + stringify(task.task_id()) + "'" - " is not authorized to launch as" - " user '" + user + "'"); + error = Error("Task '" + stringify(task.task_id()) + "'" + " is not authorized to launch as" + " user '" + user + "'"); + } + } - reason = TaskStatus::REASON_TASK_GROUP_UNAUTHORIZED; + if (error.isSome()) { + reason = TaskStatus::REASON_TASK_GROUP_UNAUTHORIZED; + } else { + error = validation::task::group::validate( + taskGroup, executor, framework, slave, _offeredResources); - break; - } + if (error.isSome()) { + reason = TaskStatus::REASON_TASK_GROUP_INVALID; } } @@ -5713,6 +5719,11 @@ void Master::_accept( } } + CHECK(authorizations.empty()) + << "Authorization results not processed: " + << stringify( + vector<Future<bool>>(authorizations.begin(), authorizations.end())); + // Update the allocator based on the operations. if (!conversions.empty()) { allocator->updateAllocation(