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
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 @@
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 @@
/**
* 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(
+ 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 @@
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 @@
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 @@
master,
masterFlags,
info,
- None(),
+ OfferConstraints{},
RECOVERED,
false,
nullptr,
@@ -95,7 +95,7 @@
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::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 @@
_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 @@
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)(
StreamingHttpConnection<v1::scheduler::Event>,
FrameworkInfo&&,
- Option<OfferConstraints>&&,
+ OfferConstraints&&,
bool,
FrameworkOptions&&,
const Future<Owned<ObjectApprovers>>&) = &Self::_subscribe;
@@ -2728,7 +2724,7 @@
_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(
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 @@
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)(
const UPID&,
FrameworkInfo&&,
- Option<OfferConstraints>&&,
+ OfferConstraints&&,
bool,
::mesos::allocator::FrameworkOptions&&,
const Future<Owned<ObjectApprovers>>&) = &Self::_subscribe;
@@ -3004,7 +2996,7 @@
_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(
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 @@
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 @@
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 @@
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::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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
// '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 @@
// 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 @@
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 @@
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 @@
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()));
}
