This is an automated email from the ASF dual-hosted git repository. bmahler pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 4189143ab0ea778bf59b85fc883ec1cda7482e95 Author: Benjamin Mahler <[email protected]> AuthorDate: Mon Feb 5 18:56:43 2018 -0800 Added validation of QuotaRequest. This obviates the need for the old-style validation of quotaInfo, but leaves the latter in order to keep the v0 code path untouched. A TODO has been added to remove the old validation and replace it with the one in this patch. Review: https://reviews.apache.org/r/65784 --- src/master/quota.cpp | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++ src/master/quota.hpp | 26 +++++++++++++ 2 files changed, 130 insertions(+) diff --git a/src/master/quota.cpp b/src/master/quota.cpp index bef18c0..671626c 100644 --- a/src/master/quota.cpp +++ b/src/master/quota.cpp @@ -21,9 +21,11 @@ #include <mesos/mesos.hpp> #include <mesos/resources.hpp> #include <mesos/roles.hpp> +#include <mesos/values.hpp> #include <stout/error.hpp> #include <stout/option.hpp> +#include <stout/set.hpp> #include "common/resources_utils.hpp" @@ -188,6 +190,108 @@ Option<Error> quotaInfo(const QuotaInfo& quotaInfo) } // namespace validation { +Option<Error> validate(const QuotaRequest& request) +{ + if (!request.has_role()) { + return Error("'QuotaRequest.role' must be set"); + } + + // Check the provided role is valid. + Option<Error> error = roles::validate(request.role()); + if (error.isSome()) { + return Error("Invalid 'QuotaRequest.role': " + error->message); + } + + // Disallow quota for '*' role. + // + // TODO(alexr): Consider allowing setting quota for '*' role, + // see MESOS-3938. + if (request.role() == "*") { + return Error("Invalid 'QuotaRequest.role': setting quota for the" + " default '*' role is not supported"); + } + + // Define a helper for use against guarantee and limit. + auto validateQuotaResources = []( + const RepeatedPtrField<Resource>& resources) -> Option<Error> { + hashset<string> names; + + // Check that each resource does not contain fields + // that are disallowed for quota resources. + foreach (const Resource& resource, resources) { + if (resource.has_reservation()) { + return Error("'Resource.reservation' must not be set"); + } + + if (resource.reservations_size() > 0) { + return Error("'Resource.reservations' must not be set"); + } + + if (resource.has_disk()) { + return Error("'Resource.disk' must not be set"); + } + + if (resource.has_revocable()) { + return Error("'Resource.revocable' must not be set"); + } + + if (resource.type() != Value::SCALAR) { + return Error("'Resource.type' must be 'SCALAR'"); + } + + if (resource.has_shared()) { + return Error("'Resource.shared' must not be set"); + } + + if (resource.has_provider_id()) { + return Error("'Resource.provider_id' must not be set"); + } + + // Check that resource names do not repeat. + if (names.contains(resource.name())) { + return Error("Duplicate '" + resource.name() + "'; only a single" + " entry for each resource is supported"); + } + + names.insert(resource.name()); + } + + // Finally, ensure the resources are valid. + return Resources::validate(resources); + }; + + error = validateQuotaResources(request.guarantee()); + if (error.isSome()) { + return Error("Invalid 'QuotaRequest.guarantee': " + error->message); + } + + error = validateQuotaResources(request.limit()); + if (error.isSome()) { + return Error("Invalid 'QuotaRequest.limit': " + error->message); + } + + // Validate that guarantee <= limit. + Resources guarantee = request.guarantee(); + Resources limit = request.limit(); + + // This needs to be checked on a per-resource basis for those + // resources that are specified in both the guarantee and limit. + foreach (const string& name, guarantee.names() & limit.names()) { + Value::Scalar guaranteeScalar = + CHECK_NOTNONE(guarantee.get<Value::Scalar>(name)); + Value::Scalar limitScalar = + CHECK_NOTNONE(limit.get<Value::Scalar>(name)); + + if (!(guaranteeScalar <= limitScalar)) { + return Error("'QuotaRequest.guarantee' (" + stringify(guarantee) + ")" + " is not contained within the 'QuotaRequest.limit'" + " (" + stringify(limit) + ")"); + } + } + + return None(); +} + } // namespace quota { } // namespace master { } // namespace internal { diff --git a/src/master/quota.hpp b/src/master/quota.hpp index 40447bc..5cd2bb0 100644 --- a/src/master/quota.hpp +++ b/src/master/quota.hpp @@ -103,10 +103,36 @@ namespace validation { // - Irrelevant fields in `Resources` are not set // (e.g. `ReservationInfo`). // - Request only contains scalar `Resources`. +// +// TODO(bmahler): Remove this in favor of `validate` below. This +// requires some new logic outside of this function to prevent +// users from setting `limit` explicitly in the old API and +// setting the `limit` implicitly for users of the old API before +// calling into this. Option<Error> quotaInfo(const mesos::quota::QuotaInfo& quotaInfo); } // namespace validation { +/** + * A `QuotaRequest` is valid if the following conditions are met: + * + * (1) The request has a valid non-"*" role. + * + * (2) The guarantee and limit contain only valid non-revocable + * scalar resources without reservations, disk info, and + * only 1 entry for each resource name. + * + * (3) If both guarantee and limit are set for a particular + * resource, then guarantee <= limit for that resource. + * + * TODO(bmahler): Remove the old validation function in favor of + * this one. This requires some new logic outside of this function + * to prevent users from setting `limit` explicitly in the old API + * and setting the `limit` implicitly for users of the old API before + * calling into this. + */ +Option<Error> validate(const mesos::quota::QuotaRequest& request); + } // namespace quota { } // namespace master { } // namespace internal {
