This is an automated email from the ASF dual-hosted git repository. mzhu pushed a commit to branch 1.8.x in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 91c4a8eb07fd646df479cc142190507aae0c1701 Author: Meng Zhu <[email protected]> AuthorDate: Thu May 16 12:12:15 2019 +0200 Fix a bug where racing quota removal request could crash the master. Also added a test. Review: https://reviews.apache.org/r/70656 --- src/master/quota_handler.cpp | 7 +++++ src/tests/master_quota_tests.cpp | 60 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+) diff --git a/src/master/quota_handler.cpp b/src/master/quota_handler.cpp index 5d449e6..a18d8ba 100644 --- a/src/master/quota_handler.cpp +++ b/src/master/quota_handler.cpp @@ -735,6 +735,13 @@ Future<http::Response> Master::QuotaHandler::_remove( Future<http::Response> Master::QuotaHandler::__remove(const string& role) const { + // Double check if the quota still exists. It may have been removed + // by a previous removal already. + if (!master->quotas.contains(role)) { + return BadRequest( + "Failed to remove quota: Role '" + role + "' has no quota set"); + } + // Remove quota from the quota-related local state. We do this before // updating the registry in order to make sure that we are not already // trying to remove quota for this role (since this is a multi-phase event). diff --git a/src/tests/master_quota_tests.cpp b/src/tests/master_quota_tests.cpp index 66641f2..146b977 100644 --- a/src/tests/master_quota_tests.cpp +++ b/src/tests/master_quota_tests.cpp @@ -65,6 +65,7 @@ using process::Clock; using process::Future; using process::Owned; using process::PID; +using process::Promise; using process::http::BadRequest; using process::http::Conflict; @@ -508,6 +509,65 @@ TEST_F(MasterQuotaTest, RemoveSingleQuota) } +// This test ensures that master can handle two racing quota removal requests. +// This is a regression test for MESOS-9786. +TEST_F(MasterQuotaTest, RemoveQuotaRace) +{ + Clock::pause(); + + // Start master with a mock authorizer to block quota removal requests. + MockAuthorizer authorizer; + + Try<Owned<cluster::Master>> master = StartMaster(&authorizer); + ASSERT_SOME(master); + + // Use the force flag for setting quota that cannot be satisfied in + // this empty cluster without any agents. + const bool FORCE = true; + + // Wrap the `http::requestDelete` into a lambda for readability of the test. + auto removeQuota = [&master](const string& path) { + return process::http::requestDelete( + master.get()->pid, + path, + createBasicAuthHeaders(DEFAULT_CREDENTIAL)); + }; + + // Set quota for `ROLE1`. + { + Resources quotaResources = Resources::parse("cpus:1;mem:512").get(); + + Future<Response> response = process::http::post( + master.get()->pid, + "quota", + createBasicAuthHeaders(DEFAULT_CREDENTIAL), + createRequestBody(ROLE1, quotaResources, FORCE)); + + AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response); + } + + // Intercept both quota removal authorizations. + Future<Nothing> authorize1, authorize2; + Promise<bool> promise1, promise2; + EXPECT_CALL(authorizer, authorized(_)) + .WillOnce(DoAll(FutureSatisfy(&authorize1), Return(promise1.future()))) + .WillOnce(DoAll(FutureSatisfy(&authorize2), Return(promise2.future()))); + + Future<Response> response1 = removeQuota("quota/" + ROLE1); + AWAIT_READY(authorize1); + + Future<Response> response2 = removeQuota("quota/" + ROLE1); + AWAIT_READY(authorize2); + + // Authorize and process both requests. Only the first request will succeed. + promise1.set(true); + AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response1); + + promise2.set(true); + AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response2); +} + + // Tests whether we can retrieve the current quota status from // /master/quota endpoint via a GET request against /quota. TEST_F(MasterQuotaTest, Status)
