This is an automated email from the ASF dual-hosted git repository. mzhu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 294b70a2190f01952f7ec6f542c60999373ea0fc Author: Meng Zhu <[email protected]> AuthorDate: Thu Mar 21 19:07:48 2019 -0700 Removed `limits` from `QuotaInfo` and `QuotaRequest` protobuf. `Limits` was added as an experimental field and the related calls were not implementation complete. `QuotaConfig` has been added where limits can be specified. This patch cleans up the experimental field and related validation and tests. Review: https://reviews.apache.org/r/70268 --- include/mesos/quota/quota.proto | 2 - include/mesos/v1/quota/quota.proto | 41 +------------ src/master/quota.cpp | 122 +------------------------------------ src/tests/master_quota_tests.cpp | 49 --------------- 4 files changed, 5 insertions(+), 209 deletions(-) diff --git a/include/mesos/quota/quota.proto b/include/mesos/quota/quota.proto index 34d4b69..682c0cc 100644 --- a/include/mesos/quota/quota.proto +++ b/include/mesos/quota/quota.proto @@ -31,7 +31,6 @@ message QuotaInfo { optional string role = 1; optional string principal = 2; repeated Resource guarantee = 3; - repeated Resource limit = 4; } @@ -42,7 +41,6 @@ message QuotaRequest { optional bool force = 1 [default = false]; optional string role = 2; repeated Resource guarantee = 3; - repeated Resource limit = 4; } diff --git a/include/mesos/v1/quota/quota.proto b/include/mesos/v1/quota/quota.proto index 8a6a64a..b109ca2 100644 --- a/include/mesos/v1/quota/quota.proto +++ b/include/mesos/v1/quota/quota.proto @@ -25,7 +25,7 @@ option java_outer_classname = "Protos"; /** - * Describes the resource guarantees and limits for a role. + * Describes the resource guarantees for a role. * Persisted in the registry. */ message QuotaInfo { @@ -35,7 +35,6 @@ message QuotaInfo { optional string principal = 2; repeated Resource guarantee = 3; - repeated Resource limit = 4; } @@ -43,12 +42,10 @@ message QuotaInfo { * Describes an update to a role's quota. This is a copy of * `QuotaInfo` which omits the principal since it is determined * during authentication. Also allows the user to force the update - * in the case of a guarantee overcommit or a limit exceeding the - * parent's limit (or overall cluster size if a top level role). + * in the case of a guarantee overcommit. */ message QuotaRequest { - // See `guarantee` and `limit` for the behavior of `force` - // on these two fields. + // See `guarantee` for the behavior of `force`. optional bool force = 1; optional string role = 2; @@ -76,38 +73,6 @@ message QuotaRequest { // NOTE: The resources must be scalars without additional // metadata like reservations, disk information, etc. repeated Resource guarantee = 3; - - // EXPERIMENTAL DO NOT USE. - // - // This feature is not implementation complete. - // - // Imposes a limit on the amount of resources allocated to the - // role. Mesos will try its best to ensure that the role does - // not exceed this limit. Despite this, the limit can be exceeded - // when: - // (1) The limit is lowered below the allocation. - // (2) Some agents are partitioned and re-connect with - // resources allocated to the role. - // - // The provided limit will be validated to ensure it does not - // exceed the total cluster size. The operator can disable - // this check via `QuotaRequest.force`. - // - // If the limit is omitted, there is no limit. - // - // Operators may want to set up alerting to let them know - // when the limit is exceeded. - // - // NOTE: The resources must be scalars without additional - // metadata like reservations, disk information, etc. - // - // NOTE: The limit was introduced alongside the v1 `UPDATE_QUOTA` - // `master::Call`. Note that the old v0 `POST /quota` endpoint and - // the v1 `SetQuota` `master::Call` continue to implicitly set - // limit to the guarantee for backwards compatibility. Users - // should switch to the v1 `UPDATE_QUOTA` `master::Call` which - // does not implicitly set the limit. - repeated Resource limit = 4; } /** diff --git a/src/master/quota.cpp b/src/master/quota.cpp index 62b788f..bad19a2 100644 --- a/src/master/quota.cpp +++ b/src/master/quota.cpp @@ -95,14 +95,12 @@ namespace { QuotaInfo createQuotaInfo( const string& role, - const RepeatedPtrField<Resource>& guarantee, - const RepeatedPtrField<Resource>& limit) + const RepeatedPtrField<Resource>& guarantee) { QuotaInfo quota; quota.set_role(role); quota.mutable_guarantee()->CopyFrom(guarantee); - quota.mutable_limit()->CopyFrom(limit); return quota; } @@ -112,7 +110,7 @@ QuotaInfo createQuotaInfo( QuotaInfo createQuotaInfo(const QuotaRequest& request) { - return createQuotaInfo(request.role(), request.guarantee(), request.limit()); + return createQuotaInfo(request.role(), request.guarantee()); } @@ -175,127 +173,11 @@ Option<Error> quotaInfo(const QuotaInfo& quotaInfo) names.insert(resource.name()); } - // TODO(bmahler): We ensure the limit is not set here to enforce - // that the v0 API and v1 SET_QUOTA Call cannot set an explicit - // limit. Ideally this check would be done during call validation - // but, at the time of writing this comment, call validation does - // not validate the call type specific messages within `Call`. - // Once we start to allow limit to be set via UPDATE_QUOTA, this - // check will have to be moved into a SET_QUOTA validator and - // a v0 /quota validator. - if (quotaInfo.limit_size() > 0) { - return Error("Setting QuotaInfo.limit is not supported via" - " /quota and the SET_QUOTA Call," - "please use the UPDATE_QUOTA Call"); - } - return None(); } } // 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(); -} - Option<Error> validate(const QuotaConfig& config) { if (!config.has_role()) { diff --git a/src/tests/master_quota_tests.cpp b/src/tests/master_quota_tests.cpp index 3e7a94c..23aa831 100644 --- a/src/tests/master_quota_tests.cpp +++ b/src/tests/master_quota_tests.cpp @@ -268,55 +268,6 @@ TEST_F(MasterQuotaTest, InvalidSetRequest) } -// v0 /quota requests and SET_QUOTA calls cannot specify a limit. -TEST_F(MasterQuotaTest, V0WithLimitDisallowed) -{ - Try<Owned<cluster::Master>> master = StartMaster(); - ASSERT_SOME(master); - - // Ensure a /quota POST request with limit is rejected. - QuotaRequest quotaRequest; - quotaRequest.set_role("role"); - *quotaRequest.mutable_guarantee() = - CHECK_NOTERROR(Resources::parse("cpus:10;mem:20")); - *quotaRequest.mutable_limit() = - CHECK_NOTERROR(Resources::parse("cpus:20;mem:40")); - - process::http::Headers headers = createBasicAuthHeaders(DEFAULT_CREDENTIAL); - - Future<Response> response = process::http::post( - master.get()->pid, - "quota", - headers, - string(jsonify(JSON::Protobuf(quotaRequest)))); - - AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response); - EXPECT_TRUE(strings::contains( - response->body, - "Setting QuotaInfo.limit is not supported")) - << response->body; - - // Ensure a SET_QUOTA Call with limit is rejected. - mesos::master::Call call; - call.set_type(mesos::master::Call::SET_QUOTA); - *call.mutable_set_quota()->mutable_quota_request() = quotaRequest; - - headers["Content-Type"] = APPLICATION_PROTOBUF; - - response = process::http::post( - master.get()->pid, - "api/v1", - headers, - call.SerializeAsString()); - - AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response); - EXPECT_TRUE(strings::contains( - response->body, - "Setting QuotaInfo.limit is not supported")) - << response->body; -} - - // Checks that a quota set request is not satisfied if an invalid // field is set or provided data are not supported. TEST_F(MasterQuotaTest, SetRequestWithInvalidData)
