This is an automated email from the ASF dual-hosted git repository. asekretenko pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 28ff20d4990ec2097852a5cd16966628d89c31d3 Author: Andrei Sekretenko <[email protected]> AuthorDate: Thu Sep 24 16:12:46 2020 +0200 Made offer constraints filter and protobuf non-optional inside the code. Given that Mesos now provides a guarantee that specifying no offer constraint in UPDATE_FRAMEWORK/SUBSCRIBE call is equivalent to specifying default-constructed `OfferConstraints`, and that we are intending to make the V0 scheduler driver always require offer constraints as an argument to the `updateFramework()`, it no longer makes sense to keep `OfferConstraints`/`OfferConstraintsFilter` optional inside the Mesos code base. This patch replaces a non-set `Option<OfferConstraints>` with default-constructed `OfferConstraints`, and a non-set `Option<OfferConstraintsFilter>` with a default-constructed filter. Review: https://reviews.apache.org/r/72897 --- include/mesos/allocator/allocator.hpp | 9 +++- .../allocator/mesos/offer_constraints_filter.cpp | 6 +++ src/master/framework.cpp | 10 ++--- src/master/http.cpp | 4 +- src/master/master.cpp | 50 ++++++++-------------- src/master/master.hpp | 18 ++++---- src/master/readonly_handler.cpp | 6 +-- 7 files changed, 49 insertions(+), 54 deletions(-) diff --git a/include/mesos/allocator/allocator.hpp b/include/mesos/allocator/allocator.hpp index b0a5d6a..c378f78 100644 --- a/include/mesos/allocator/allocator.hpp +++ b/include/mesos/allocator/allocator.hpp @@ -110,7 +110,12 @@ public: const Options& options, scheduler::OfferConstraints&& constraints); - OfferConstraintsFilter() = delete; + /* + * Constructs a no-op filter that does not exclude any agents/resources from + * being offered. This is equivalent to passing default-constructed + * `OfferConstraints` to the factory method `create()`. + */ + OfferConstraintsFilter(); // Definitions of these need `OfferConstraintsFilterImpl` to be a complete // type. @@ -149,7 +154,7 @@ struct FrameworkOptions /** * The internal representation of framework's offer constraints. */ - Option<OfferConstraintsFilter> offerConstraintsFilter; + OfferConstraintsFilter offerConstraintsFilter; }; diff --git a/src/master/allocator/mesos/offer_constraints_filter.cpp b/src/master/allocator/mesos/offer_constraints_filter.cpp index 441ebc1..45199ca 100644 --- a/src/master/allocator/mesos/offer_constraints_filter.cpp +++ b/src/master/allocator/mesos/offer_constraints_filter.cpp @@ -472,6 +472,12 @@ OfferConstraintsFilter::OfferConstraintsFilter( {} +OfferConstraintsFilter::OfferConstraintsFilter() + : OfferConstraintsFilter( + CHECK_NOTERROR(OfferConstraintsFilterImpl::create({Bytes(0), 0}, {}))) +{} + + OfferConstraintsFilter::OfferConstraintsFilter(OfferConstraintsFilter&&) = default; diff --git a/src/master/framework.cpp b/src/master/framework.cpp index 980828e..a93172a 100644 --- a/src/master/framework.cpp +++ b/src/master/framework.cpp @@ -35,7 +35,7 @@ Framework::Framework( Master* const master, const Flags& masterFlags, const FrameworkInfo& info, - Option<OfferConstraints>&& offerConstraints, + OfferConstraints&& offerConstraints, const process::UPID& pid, const Owned<ObjectApprovers>& approvers, const process::Time& time) @@ -57,7 +57,7 @@ Framework::Framework( Master* const master, const Flags& masterFlags, const FrameworkInfo& info, - Option<OfferConstraints>&& offerConstraints, + OfferConstraints&& offerConstraints, const StreamingHttpConnection<v1::scheduler::Event>& http, const Owned<ObjectApprovers>& approvers, const process::Time& time) @@ -83,7 +83,7 @@ Framework::Framework( master, masterFlags, info, - None(), + OfferConstraints{}, RECOVERED, false, nullptr, @@ -95,7 +95,7 @@ Framework::Framework( Master* const _master, const Flags& masterFlags, const FrameworkInfo& _info, - Option<OfferConstraints>&& offerConstraints, + OfferConstraints&& offerConstraints, State state, bool active_, const Owned<ObjectApprovers>& approvers, @@ -538,7 +538,7 @@ void Framework::removeOperation(Operation* operation) void Framework::update( const FrameworkInfo& newInfo, - Option<OfferConstraints>&& offerConstraints) + OfferConstraints&& offerConstraints) { // We only merge 'info' from the same framework 'id'. CHECK_EQ(info.id(), newInfo.id()); diff --git a/src/master/http.cpp b/src/master/http.cpp index 3d8e709..c4595cc 100644 --- a/src/master/http.cpp +++ b/src/master/http.cpp @@ -1301,9 +1301,7 @@ mesos::master::Response::GetFrameworks::Framework model( _framework.mutable_offered_resources()->Add()->CopyFrom(resource); } - if (framework.offerConstraints().isSome()) { - *_framework.mutable_offer_constraints() = *framework.offerConstraints(); - } + *_framework.mutable_offer_constraints() = framework.offerConstraints(); return _framework; } diff --git a/src/master/master.cpp b/src/master/master.cpp index 576ae10..d6d3ea7 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -2675,17 +2675,13 @@ void Master::subscribe( Option<Error> validationError = validateFramework(frameworkInfo, subscribe.suppressed_roles()); - Option<OfferConstraints> offerConstraints; - if (subscribe.has_offer_constraints()) { - offerConstraints = std::move(*subscribe.mutable_offer_constraints()); - } - allocator::FrameworkOptions allocatorOptions; // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). - if (validationError.isNone() && offerConstraints.isSome()) { + if (validationError.isNone()) { Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( - offerConstraintsFilterOptions, OfferConstraints(*offerConstraints)); + offerConstraintsFilterOptions, + OfferConstraints(subscribe.offer_constraints())); if (filter.isError()) { validationError = Error(std::move(filter.error())); @@ -2715,7 +2711,7 @@ void Master::subscribe( void (Master::*_subscribe)( StreamingHttpConnection<v1::scheduler::Event>, FrameworkInfo&&, - Option<OfferConstraints>&&, + OfferConstraints&&, bool, FrameworkOptions&&, const Future<Owned<ObjectApprovers>>&) = &Self::_subscribe; @@ -2728,7 +2724,7 @@ void Master::subscribe( _subscribe, http, std::move(frameworkInfo), - std::move(offerConstraints), + std::move(*subscribe.mutable_offer_constraints()), subscribe.force(), std::move(allocatorOptions), lambda::_1)); @@ -2738,7 +2734,7 @@ void Master::subscribe( void Master::_subscribe( StreamingHttpConnection<v1::scheduler::Event> http, FrameworkInfo&& frameworkInfo, - Option<OfferConstraints>&& offerConstraints, + OfferConstraints&& offerConstraints, bool force, ::mesos::allocator::FrameworkOptions&& options, const Future<Owned<ObjectApprovers>>& objectApprovers) @@ -2937,17 +2933,13 @@ void Master::subscribe( validationError = validateFrameworkAuthentication(frameworkInfo, from); } - Option<OfferConstraints> offerConstraints; - if (subscribe.has_offer_constraints()) { - offerConstraints = std::move(*subscribe.mutable_offer_constraints()); - } - allocator::FrameworkOptions allocatorOptions; // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). - if (validationError.isNone() && offerConstraints.isSome()) { + if (validationError.isNone()) { Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( - offerConstraintsFilterOptions, OfferConstraints(*offerConstraints)); + offerConstraintsFilterOptions, + OfferConstraints(subscribe.offer_constraints())); if (filter.isError()) { validationError = Error(std::move(filter.error())); @@ -2991,7 +2983,7 @@ void Master::subscribe( void (Master::*_subscribe)( const UPID&, FrameworkInfo&&, - Option<OfferConstraints>&&, + OfferConstraints&&, bool, ::mesos::allocator::FrameworkOptions&&, const Future<Owned<ObjectApprovers>>&) = &Self::_subscribe; @@ -3004,7 +2996,7 @@ void Master::subscribe( _subscribe, from, std::move(frameworkInfo), - std::move(offerConstraints), + std::move(*subscribe.mutable_offer_constraints()), subscribe.force(), std::move(allocatorOptions), lambda::_1)); @@ -3014,7 +3006,7 @@ void Master::subscribe( void Master::_subscribe( const UPID& from, FrameworkInfo&& frameworkInfo, - Option<OfferConstraints>&& offerConstraints, + OfferConstraints&& offerConstraints, bool force, ::mesos::allocator::FrameworkOptions&& options, const Future<Owned<ObjectApprovers>>& objectApprovers) @@ -3296,13 +3288,11 @@ Future<process::http::Response> Master::updateFramework( const bool frameworkInfoChanged = !typeutils::equivalent(framework->info, call.framework_info()); - Option<OfferConstraints> offerConstraints; allocator::FrameworkOptions allocatorOptions; - if (call.has_offer_constraints()) { - // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). - Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( - offerConstraintsFilterOptions, - OfferConstraints(call.offer_constraints())); + // TODO(asekretenko): Validate roles in offer constraints (see MESOS-10176). + Try<OfferConstraintsFilter> filter = OfferConstraintsFilter::create( + offerConstraintsFilterOptions, + OfferConstraints(call.offer_constraints())); if (filter.isError()) { return process::http::BadRequest( @@ -3310,9 +3300,7 @@ Future<process::http::Response> Master::updateFramework( filter.error()); } - allocatorOptions.offerConstraintsFilter = std::move(*filter); - offerConstraints = std::move(*call.mutable_offer_constraints()); - } + allocatorOptions.offerConstraintsFilter = std::move(*filter); ActionObject actionObject = ActionObject::frameworkRegistration(call.framework_info()); @@ -3338,7 +3326,7 @@ Future<process::http::Response> Master::updateFramework( updateFramework( framework, call.framework_info(), - std::move(offerConstraints), + std::move(*call.mutable_offer_constraints()), std::move(allocatorOptions)); if (frameworkInfoChanged) { @@ -7504,7 +7492,7 @@ void Master::unregisterSlave(const UPID& from, const SlaveID& slaveId) void Master::updateFramework( Framework* framework, const FrameworkInfo& frameworkInfo, - Option<OfferConstraints>&& offerConstraints, + OfferConstraints&& offerConstraints, ::mesos::allocator::FrameworkOptions&& allocatorOptions) { LOG(INFO) << "Updating framework " << *framework << " with roles " diff --git a/src/master/master.hpp b/src/master/master.hpp index 95aca58..83d8190 100644 --- a/src/master/master.hpp +++ b/src/master/master.hpp @@ -670,7 +670,7 @@ protected: void updateFramework( Framework* framework, const FrameworkInfo& frameworkInfo, - Option<::mesos::scheduler::OfferConstraints>&& offerConstraints, + ::mesos::scheduler::OfferConstraints&& offerConstraints, ::mesos::allocator::FrameworkOptions&& allocatorOptions); void sendFrameworkUpdates(const Framework& framework); @@ -904,7 +904,7 @@ private: void _subscribe( StreamingHttpConnection<v1::scheduler::Event> http, FrameworkInfo&& frameworkInfo, - Option<scheduler::OfferConstraints>&& offerConstraints, + scheduler::OfferConstraints&& offerConstraints, bool force, ::mesos::allocator::FrameworkOptions&& allocatorOptions, const process::Future<process::Owned<ObjectApprovers>>& objectApprovers); @@ -916,7 +916,7 @@ private: void _subscribe( const process::UPID& from, FrameworkInfo&& frameworkInfo, - Option<scheduler::OfferConstraints>&& offerConstraints, + scheduler::OfferConstraints&& offerConstraints, bool force, ::mesos::allocator::FrameworkOptions&& allocatorOptions, const process::Future<process::Owned<ObjectApprovers>>& objectApprovers); @@ -2416,7 +2416,7 @@ struct Framework Master* const master, const Flags& masterFlags, const FrameworkInfo& info, - Option<::mesos::scheduler::OfferConstraints>&& offerConstraints, + ::mesos::scheduler::OfferConstraints&& offerConstraints, const process::UPID& _pid, const process::Owned<ObjectApprovers>& objectApprovers, const process::Time& time = process::Clock::now()); @@ -2424,7 +2424,7 @@ struct Framework Framework(Master* const master, const Flags& masterFlags, const FrameworkInfo& info, - Option<::mesos::scheduler::OfferConstraints>&& offerConstraints, + ::mesos::scheduler::OfferConstraints&& offerConstraints, const StreamingHttpConnection<v1::scheduler::Event>& _http, const process::Owned<ObjectApprovers>& objectApprovers, const process::Time& time = process::Clock::now()); @@ -2493,7 +2493,7 @@ struct Framework // 'webui_url', 'capabilities', and 'labels'. void update( const FrameworkInfo& newInfo, - Option<::mesos::scheduler::OfferConstraints>&& offerConstraints); + ::mesos::scheduler::OfferConstraints&& offerConstraints); // Reactivate framework with new connection: update connection-related state // and mark the framework as CONNECTED, regardless of the previous state. @@ -2544,7 +2544,7 @@ struct Framework // action on object. Try<bool> approved(const authorization::ActionObject& actionObject) const; - const Option<::mesos::scheduler::OfferConstraints>& offerConstraints() const + const ::mesos::scheduler::OfferConstraints& offerConstraints() const { return offerConstraints_; } @@ -2635,7 +2635,7 @@ private: Framework(Master* const _master, const Flags& masterFlags, const FrameworkInfo& _info, - Option<::mesos::scheduler::OfferConstraints>&& offerConstraints, + ::mesos::scheduler::OfferConstraints&& offerConstraints, State state, bool active, const process::Owned<ObjectApprovers>& objectApprovers, @@ -2671,7 +2671,7 @@ private: process::Owned<ObjectApprovers> objectApprovers; // The last offer constraints with which the framework has been subscribed. - Option<::mesos::scheduler::OfferConstraints> offerConstraints_; + ::mesos::scheduler::OfferConstraints offerConstraints_; }; diff --git a/src/master/readonly_handler.cpp b/src/master/readonly_handler.cpp index df499d1..034419b 100644 --- a/src/master/readonly_handler.cpp +++ b/src/master/readonly_handler.cpp @@ -258,10 +258,8 @@ void FullFrameworkWriter::operator()(JSON::ObjectWriter* writer) const writer->field("labels", framework_->info.labels()); } - if (framework_->offerConstraints().isSome()) { - writer->field( - "offer_constraints", JSON::Protobuf(*framework_->offerConstraints())); - }; + writer->field( + "offer_constraints", JSON::Protobuf(framework_->offerConstraints())); }
