[mesos] 01/02: Fix a bug where racing quota removal request could crash the master.

[mz...@apache.org]

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 <mz...@mesosphere.io>
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)