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 4dd00c6ad3d8af1d38d496a51f5407ee0e4b1970 Author: Meng Zhu <[email protected]> AuthorDate: Tue Sep 10 11:51:09 2019 -0700 Allowed setting quota the default "*" role. There is no clear argument against setting quota on the default "*" role. This patch allows doing so. Tests are updated to check against regressions. Review: https://reviews.apache.org/r/71464 --- docs/quota.md | 2 +- src/master/quota.cpp | 13 ------------- src/tests/hierarchical_allocator_tests.cpp | 3 ++- src/tests/master_quota_tests.cpp | 8 +++++--- src/tests/master_validation_tests.cpp | 14 ++++---------- 5 files changed, 12 insertions(+), 28 deletions(-) diff --git a/docs/quota.md b/docs/quota.md index 3df7d7b..a252a88 100644 --- a/docs/quota.md +++ b/docs/quota.md @@ -223,9 +223,9 @@ documentation: [https://github.com/apache/mesos/blob/1.8.0/docs/quota.md](https: ## Implementation Notes -* Quota is not supported for the default `*` role. * Quota is not supported on nested roles (e.g. `eng/prod`). * During failover, in order to correctly enforce limits, the allocator will be paused and will not issue offers until at least 80% agents re-register or 10 minutes elapses. These parameters will be made configurable: [MESOS-4073](https://issues.apache.org/jira/browse/MESOS-4073) +* Quota is SUPPORTED for the default `*` role now [MESOS-3938](https://issues.apache.org/jira/browse/MESOS-3938). diff --git a/src/master/quota.cpp b/src/master/quota.cpp index c9f39f3..4ecd326 100644 --- a/src/master/quota.cpp +++ b/src/master/quota.cpp @@ -158,12 +158,6 @@ Option<Error> quotaInfo(const QuotaInfo& quotaInfo) return Error("QuotaInfo with invalid role: " + roleError->message); } - // Disallow quota for '*' role. - // TODO(alexr): Consider allowing setting quota for '*' role, see MESOS-3938. - if (quotaInfo.role() == "*") { - return Error("QuotaInfo must not specify the default '*' role"); - } - // Check that `QuotaInfo` contains at least one guarantee. // TODO(alexr): Relaxing this may make sense once quota is the // only way that we offer non-revocable resources. Setting quota @@ -220,13 +214,6 @@ Option<Error> validate(const QuotaConfig& config) return Error("Invalid 'QuotaConfig.role': " + error->message); } - // Disallow quota for '*' role. - if (config.role() == "*") { - return Error( - "Invalid 'QuotaConfig.role': setting quota for the" - " default '*' role is not supported"); - } - // Validate scalar values. foreach (auto&& guarantee, config.guarantees()) { Option<Error> error = diff --git a/src/tests/hierarchical_allocator_tests.cpp b/src/tests/hierarchical_allocator_tests.cpp index da7489b..56c8caf 100644 --- a/src/tests/hierarchical_allocator_tests.cpp +++ b/src/tests/hierarchical_allocator_tests.cpp @@ -3651,11 +3651,12 @@ TEST_F(HierarchicalAllocatorTest, QuotaProvidesGuarantee) // When a role has limits set, its frameworks allocations are restricted based // on its quota limits. +// We set quota on the default "*" role as a regression test for MESOS-3938. TEST_F(HierarchicalAllocatorTest, QuotaProvidesLimit) { Clock::pause(); - const string QUOTA_ROLE{"quota-limits-role"}; + const string QUOTA_ROLE{"*"}; const string NO_QUOTA_ROLE{"no-quota-role"}; initialize(); diff --git a/src/tests/master_quota_tests.cpp b/src/tests/master_quota_tests.cpp index 5832869..cb421b5 100644 --- a/src/tests/master_quota_tests.cpp +++ b/src/tests/master_quota_tests.cpp @@ -414,6 +414,8 @@ TEST_F(MasterQuotaTest, UpdateAndGetQuota) // This ensures that `UPDATE_QUOTA` call with multiple quota configs are // updated all-or-nothing. +// The second role is intentionally set to the default `*` role as a regression +// test for MESOS-3938. TEST_F(MasterQuotaTest, UpdateQuotaMultipleRoles) { MockAuthorizer authorizer; @@ -427,7 +429,7 @@ TEST_F(MasterQuotaTest, UpdateQuotaMultipleRoles) string request = createUpdateQuotaRequestBody( { createQuotaConfig(ROLE1, "cpus:1;mem:1024", "cpus:2;mem:2048"), - createQuotaConfig(ROLE2, "cpus:1;mem:1024", "cpus:2;mem:2048"), + createQuotaConfig("*", "cpus:1;mem:1024", "cpus:2;mem:2048"), }, true); @@ -502,7 +504,7 @@ TEST_F(MasterQuotaTest, UpdateQuotaMultipleRoles) " \"cpus\": 1.0," " \"mem\": 1024.0" " }," - " \"role\": \"role1\"" + " \"role\": \"*\"" " }" "}"); @@ -518,7 +520,7 @@ TEST_F(MasterQuotaTest, UpdateQuotaMultipleRoles) " \"cpus\": 1.0," " \"mem\": 1024.0" " }," - " \"role\": \"role2\"" + " \"role\": \"role1\"" " }" "}"); diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp index 90b8447..3658a88 100644 --- a/src/tests/master_validation_tests.cpp +++ b/src/tests/master_validation_tests.cpp @@ -118,19 +118,13 @@ TEST(MasterCallValidationTest, UpdateQuota) EXPECT_TRUE(strings::contains(error->message, "Invalid 'QuotaConfig.role'")) << error->message; - config.set_role("*"); - - error = mesos::internal::master::quota::validate(config); - ASSERT_SOME(error); - EXPECT_TRUE(strings::contains( - error->message, - "Invalid 'QuotaConfig.role':" - " setting quota for the default '*' role is not supported")) - << error->message; - // Once a role is set, it is valid to have no guarantee and no limit. config.set_role("role"); + error = mesos::internal::master::quota::validate(config); + ASSERT_NONE(error); + // Setting quota on "*" role is allowed. + config.set_role("*"); error = mesos::internal::master::quota::validate(config); ASSERT_NONE(error);
