Repository: mesos Updated Branches: refs/heads/master 5b183ee21 -> d5cc1a606
Pulled out call validation. Review: https://reviews.apache.org/r/36919 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/d5cc1a60 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/d5cc1a60 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/d5cc1a60 Branch: refs/heads/master Commit: d5cc1a606ca65821447daef378ee45e0da02864b Parents: 5b183ee Author: Benjamin Mahler <[email protected]> Authored: Wed Jul 29 12:22:10 2015 -0700 Committer: Benjamin Mahler <[email protected]> Committed: Wed Jul 29 13:37:26 2015 -0700 ---------------------------------------------------------------------- src/master/master.cpp | 88 ++++++--------------------- src/master/master.hpp | 2 +- src/master/validation.cpp | 91 ++++++++++++++++++++++++++++ src/master/validation.hpp | 11 ++++ src/scheduler/scheduler.cpp | 124 +++------------------------------------ 5 files changed, 130 insertions(+), 186 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/d5cc1a60/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index c584cb9..2bfec2f 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -1626,27 +1626,15 @@ void Master::receive( { // TODO(vinod): Add metrics for calls. - if (call.type() == scheduler::Call::SUBSCRIBE) { - if (!call.has_subscribe()) { - drop(from, call, "Expecting 'subscribe' to be present"); - return; - } + Option<Error> error = validation::scheduler::call::validate(call); - if (!(call.subscribe().framework_info().id() == call.framework_id())) { - drop(from, - call, - "Framework id in the call doesn't match the framework id" - " in the 'subscribe' message"); - return; - } - - subscribe(from, call.subscribe()); + if (error.isSome()) { + drop(from, call, error.get().message); return; } - // All calls except SUBSCRIBE should have framework id set. - if (!call.has_framework_id()) { - drop(from, call, "Expecting framework id to be present"); + if (call.type() == scheduler::Call::SUBSCRIBE) { + subscribe(from, call.subscribe()); return; } @@ -1665,90 +1653,50 @@ void Master::receive( } switch (call.type()) { - case scheduler::Call::TEARDOWN: { + case scheduler::Call::TEARDOWN: removeFramework(framework); break; - } - case scheduler::Call::ACCEPT: { - if (!call.has_accept()) { - drop(from, call, "Expecting 'accept' to be present"); - return; - } + case scheduler::Call::ACCEPT: accept(framework, call.accept()); break; - } - case scheduler::Call::DECLINE: { - if (!call.has_decline()) { - drop(from, call, "Expecting 'decline' to be present"); - return; - } + case scheduler::Call::DECLINE: decline(framework, call.decline()); break; - } - case scheduler::Call::REVIVE: { + case scheduler::Call::REVIVE: revive(framework); break; - } - case scheduler::Call::KILL: { - if (!call.has_kill()) { - drop(from, call, "Expecting 'kill' to be present"); - return; - } + case scheduler::Call::KILL: kill(framework, call.kill()); break; - } - case scheduler::Call::SHUTDOWN: { - if (!call.has_shutdown()) { - drop(from, call, "Expecting 'shutdown' to be present"); - return; - } + case scheduler::Call::SHUTDOWN: shutdown(framework, call.shutdown()); break; - } - case scheduler::Call::ACKNOWLEDGE: { - if (!call.has_acknowledge()) { - drop(from, call, "Expecting 'acknowledge' to be present"); - return; - } + case scheduler::Call::ACKNOWLEDGE: acknowledge(framework, call.acknowledge()); break; - } - case scheduler::Call::RECONCILE: { - if (!call.has_reconcile()) { - drop(from, call, "Expecting 'reconcile' to be present"); - return; - } + case scheduler::Call::RECONCILE: reconcile(framework, call.reconcile()); break; - } - case scheduler::Call::MESSAGE: { - if (!call.has_message()) { - drop(from, call, "Expecting 'message' to be present"); - return; - } + case scheduler::Call::MESSAGE: message(framework, call.message()); break; - } - case scheduler::Call::REQUEST: { - if (!call.has_request()) { - drop(from, call, "Expecting 'request' to be present"); - return; - } + case scheduler::Call::REQUEST: request(framework, call.request()); break; - } default: - drop(from, call, "Unknown call type"); + // Should be caught during call validation above. + LOG(FATAL) << "Unexpected " << call.type() << " call" + << " from framework " << call.framework_id() << " at " << from; break; } } http://git-wip-us.apache.org/repos/asf/mesos/blob/d5cc1a60/src/master/master.hpp ---------------------------------------------------------------------- diff --git a/src/master/master.hpp b/src/master/master.hpp index 2331173..1135049 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -30,7 +30,7 @@ #include <mesos/mesos.hpp> #include <mesos/resources.hpp> -#include <mesos/scheduler.hpp> +#include <mesos/scheduler/scheduler.hpp> #include <mesos/type_utils.hpp> #include <mesos/master/allocator.hpp> http://git-wip-us.apache.org/repos/asf/mesos/blob/d5cc1a60/src/master/validation.cpp ---------------------------------------------------------------------- diff --git a/src/master/validation.cpp b/src/master/validation.cpp index 9d128aa..ffb7bf0 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -51,6 +51,97 @@ static bool invalid(char c) return iscntrl(c) || c == '/' || c == '\\'; } + +namespace scheduler { +namespace call { + +Option<Error> validate(const mesos::scheduler::Call& call) +{ + if (!call.IsInitialized()) { + return Error("Not initialized: " + call.InitializationErrorString()); + } + + if (call.type() == mesos::scheduler::Call::SUBSCRIBE) { + if (!call.has_subscribe()) { + return Error("Expecting 'subscribe' to be present"); + } + + if (!(call.subscribe().framework_info().id() == call.framework_id())) { + return Error("'framework_id' differs from 'subscribe.framework_info.id'"); + } + + return None(); + } + + // All calls except SUBSCRIBE should have framework id set. + if (!call.has_framework_id()) { + return Error("Expecting 'framework_id' to be present"); + } + + switch (call.type()) { + case mesos::scheduler::Call::TEARDOWN: + return None(); + + case mesos::scheduler::Call::ACCEPT: + if (!call.has_accept()) { + return Error("Expecting 'accept' to be present"); + } + return None(); + + case mesos::scheduler::Call::DECLINE: + if (!call.has_decline()) { + return Error("Expecting 'decline' to be present"); + } + return None(); + + case mesos::scheduler::Call::REVIVE: + return None(); + + case mesos::scheduler::Call::KILL: + if (!call.has_kill()) { + return Error("Expecting 'kill' to be present"); + } + return None(); + + case mesos::scheduler::Call::SHUTDOWN: + if (!call.has_shutdown()) { + return Error("Expecting 'shutdown' to be present"); + } + return None(); + + case mesos::scheduler::Call::ACKNOWLEDGE: + if (!call.has_acknowledge()) { + return Error("Expecting 'acknowledge' to be present"); + } + return None(); + + case mesos::scheduler::Call::RECONCILE: + if (!call.has_reconcile()) { + return Error("Expecting 'reconcile' to be present"); + } + return None(); + + case mesos::scheduler::Call::MESSAGE: + if (!call.has_message()) { + return Error("Expecting 'message' to be present"); + } + return None(); + + case mesos::scheduler::Call::REQUEST: + if (!call.has_request()) { + return Error("Expecting 'request' to be present"); + } + return None(); + + default: + return Error("Unknown call type"); + } +} + +} // namespace call { +} // namespace scheduler { + + namespace resource { // Validates the ReservationInfos specified in the given resources (if http://git-wip-us.apache.org/repos/asf/mesos/blob/d5cc1a60/src/master/validation.hpp ---------------------------------------------------------------------- diff --git a/src/master/validation.hpp b/src/master/validation.hpp index 469d6f5..43b8d84 100644 --- a/src/master/validation.hpp +++ b/src/master/validation.hpp @@ -20,6 +20,7 @@ #include <mesos/mesos.hpp> #include <mesos/resources.hpp> +#include <mesos/scheduler/scheduler.hpp> #include <stout/error.hpp> #include <stout/option.hpp> @@ -35,6 +36,16 @@ struct Slave; namespace validation { +namespace scheduler { +namespace call { + +// Validates that a scheduler call is well-formed. +// TODO(bmahler): Add unit tests. +Option<Error> validate(const mesos::scheduler::Call& call); + +} // namespace call { +} // namespace scheduler { + namespace resource { // Validates resources specified by frameworks. http://git-wip-us.apache.org/repos/asf/mesos/blob/d5cc1a60/src/scheduler/scheduler.cpp ---------------------------------------------------------------------- diff --git a/src/scheduler/scheduler.cpp b/src/scheduler/scheduler.cpp index 6887ed1..97fa2c0 100644 --- a/src/scheduler/scheduler.cpp +++ b/src/scheduler/scheduler.cpp @@ -64,6 +64,7 @@ #include "common/protobuf_utils.hpp" #include "master/detector.hpp" +#include "master/validation.hpp" #include "local/local.hpp" @@ -192,12 +193,6 @@ public: return; } - // Only a SUBSCRIBE call may not have set the framework ID. - if (call.type() != Call::SUBSCRIBE && !call.has_framework_id()) { - drop(call, "Expecting 'framework_id' to be present"); - return; - } - // If no user was specified in FrameworkInfo, use the current user. // TODO(benh): Make FrameworkInfo.user be optional. if (call.type() == Call::SUBSCRIBE && @@ -208,118 +203,17 @@ public: call.mutable_subscribe()->mutable_framework_info()->set_user(user.get()); } - if (!call.IsInitialized()) { - drop(call, "Call is not properly initialized: " + - call.InitializationErrorString()); + Option<Error> error = validation::scheduler::call::validate(call); + + if (error.isSome()) { + drop(call, error.get().message); return; } - switch (call.type()) { - case Call::SUBSCRIBE: { - if (!call.has_subscribe()) { - drop(call, "Expecting 'subscribe' to be present"); - return; - } - - if (!(call.subscribe().framework_info().id() == call.framework_id())) { - drop(call, "Framework id in the call doesn't match the framework id" - " in the 'subscribe' message"); - return; - } - - send(master.get(), call); - break; - } - - case Call::TEARDOWN: { - send(master.get(), call); - break; - } - - case Call::ACCEPT: { - if (!call.has_accept()) { - drop(call, "Expecting 'accept' to be present"); - return; - } - send(master.get(), call); - break; - } - - case Call::DECLINE: { - if (!call.has_decline()) { - drop(call, "Expecting 'decline' to be present"); - return; - } - send(master.get(), call); - break; - } - - case Call::REVIVE: { - send(master.get(), call); - break; - } - - case Call::KILL: { - if (!call.has_kill()) { - drop(call, "Expecting 'kill' to be present"); - return; - } - send(master.get(), call); - break; - } - - case Call::SHUTDOWN: { - if (!call.has_shutdown()) { - drop(call, "Expecting 'shutdown' to be present"); - return; - } - send(master.get(), call); - break; - } - - case Call::ACKNOWLEDGE: { - if (!call.has_acknowledge()) { - drop(call, "Expecting 'acknowledge' to be present"); - return; - } - send(master.get(), call); - break; - } - - case Call::RECONCILE: { - if (!call.has_reconcile()) { - drop(call, "Expecting 'reconcile' to be present"); - return; - } - send(master.get(), call); - break; - } - - case Call::MESSAGE: { - if (!call.has_message()) { - drop(call, "Expecting 'message' to be present"); - return; - } - // TODO(vinod): Add support for sending the call directly to - // the slave, instead of relaying it through the master, as - // the scheduler driver does. - send(master.get(), call); - break; - } - - case Call::REQUEST: { - if (!call.has_request()) { - drop(call, "Expecting 'request' to be present"); - return; - } - send(master.get(), call); - break; - } - - default: - LOG(ERROR) << "Unexpected call " << stringify(call.type()); - break; - } + // TODO(vinod): Add support for sending MESSAGE calls directly + // to the slave, instead of relaying it through the master, as + // the scheduler driver does. + send(master.get(), call); } protected:
