SERVER-19897 Cannot overtake lock if ping document is deleted in RS config servers

Author: renctan

src/mongo/s/catalog/replset/replset_dist_lock_manager.cpp

@@ -152,17 +152,21 @@ void ReplSetDistLockManager::doTask() {
 StatusWith<bool> ReplSetDistLockManager::canOvertakeLock(LocksType lockDoc) {
     const auto& processID = lockDoc.getProcess();
     auto pingStatus = _catalog->getPing(processID);
-    if (!pingStatus.isOK()) {
-        return pingStatus.getStatus();
-    }
 
-    const auto& pingDoc = pingStatus.getValue();
-    Status pingDocValidationStatus = pingDoc.validate();
-    if (!pingDocValidationStatus.isOK()) {
-        return {ErrorCodes::UnsupportedFormat,
-                str::stream() << "invalid ping document for " << processID << ": "
-                              << pingDocValidationStatus.toString()};
-    }
+    Date_t pingValue;
+    if (pingStatus.isOK()) {
+        const auto& pingDoc = pingStatus.getValue();
+        Status pingDocValidationStatus = pingDoc.validate();
+        if (!pingDocValidationStatus.isOK()) {
+            return {ErrorCodes::UnsupportedFormat,
+                    str::stream() << "invalid ping document for " << processID << ": "
+                                  << pingDocValidationStatus.toString()};
+        }
+
+        pingValue = pingDoc.getPing();
+    } else if (pingStatus.getStatus() != ErrorCodes::NoMatchingDocument) {
+        return pingStatus.getStatus();
+    } // else use default pingValue if ping document does not exist.
 
     Timer timer(_serviceContext->getTickSource());
     auto serverInfoStatus = _catalog->getServerInfo();
@@ -175,7 +179,6 @@ StatusWith<bool> ReplSetDistLockManager::canOvertakeLock(LocksType lockDoc) {
     // time from the config server.
     milliseconds delay(timer.millis() / 2);  // Assuming symmetrical delay.
 
-    Date_t pingValue = pingDoc.getPing();
     const auto& serverInfo = serverInfoStatus.getValue();
 
     stdx::lock_guard<stdx::mutex> lk(_mutex);

src/mongo/s/catalog/replset/replset_dist_lock_manager_test.cpp

@@ -115,7 +115,6 @@ class ReplSetDistLockManagerFixture : public mongo::unittest::Test {
         _mgr.shutDown(true);
     }
 
-    TickSourceMock _tickSource;
     std::unique_ptr<DistLockCatalogMock> _dummyDoNotUse;  // dummy placeholder
     DistLockCatalogMock* _mockCatalog;
     string _processID;
@@ -1746,5 +1745,89 @@ TEST_F(ReplSetDistLockManagerFixture, CannotOvertakeIfConfigServerClockGoesBackw
     }
 }
 
+/**
+ * Test scenario:
+ * 1. Attempt to grab lock fails because lock is already owned.
+ * 2. Try to get ping data (does not exist) and config server clock.
+ * 3. Since we don't have previous ping data to compare with, we cannot
+ *    decide whether it's ok to overtake, so we can't.
+ * 4. Lock expiration has elapsed and the ping still does not exist.
+ * 5. 2nd attempt to grab lock still fails for the same reason.
+ * 6. But since the ping has not been updated, dist lock manager should overtake lock.
+ */
+TEST_F(RSDistLockMgrWithMockTickSource, CanOvertakeIfNoPingDocument) {
+    getMockCatalog()->expectGrabLock(
+        [](StringData, const OID&, StringData, StringData, Date_t, StringData) {
+            // Don't care
+        },
+        {ErrorCodes::LockStateChangeFailed, "nMod 0"});
+
+    LocksType currentLockDoc;
+    currentLockDoc.setName("bar");
+    currentLockDoc.setState(LocksType::LOCKED);
+    currentLockDoc.setProcess("otherProcess");
+    currentLockDoc.setLockID(OID("5572007fda9e476582bf3716"));
+    currentLockDoc.setWho("me");
+    currentLockDoc.setWhy("why");
+
+    getMockCatalog()->expectGetLockByName([](StringData name) { ASSERT_EQUALS("bar", name); },
+                                          currentLockDoc);
+
+    getMockCatalog()->expectGetPing([](StringData process) {
+        ASSERT_EQUALS("otherProcess", process);
+    }, {ErrorCodes::NoMatchingDocument, "no ping"});
+
+    getMockCatalog()->expectGetServerInfo([]() {}, DistLockCatalog::ServerInfo(Date_t(), OID()));
+
+    // First attempt will record the ping data.
+    {
+        auto status = getMgr()->lock("bar", "", Milliseconds(0), Milliseconds(0)).getStatus();
+        ASSERT_NOT_OK(status);
+        ASSERT_EQUALS(ErrorCodes::LockBusy, status.code());
+    }
+
+    OID lastTS;
+    getMockCatalog()->expectGrabLock(
+        [&lastTS](StringData, const OID& newTS, StringData, StringData, Date_t, StringData) {
+            lastTS = newTS;
+        },
+        {ErrorCodes::LockStateChangeFailed, "nMod 0"});
+
+    getMockCatalog()->expectGetLockByName([](StringData name) { ASSERT_EQUALS("bar", name); },
+                                          currentLockDoc);
+
+    getMockCatalog()->expectGetPing([](StringData process) {
+        ASSERT_EQUALS("otherProcess", process);
+    }, {ErrorCodes::NoMatchingDocument, "no ping"});
+
+    getMockCatalog()->expectGetServerInfo(
+        []() {}, DistLockCatalog::ServerInfo(Date_t() + kLockExpiration + Milliseconds(1), OID()));
+
+    getMockCatalog()->expectOvertakeLock(
+        [this, &lastTS, &currentLockDoc](StringData lockID,
+                                         const OID& lockSessionID,
+                                         const OID& currentHolderTS,
+                                         StringData who,
+                                         StringData processId,
+                                         Date_t time,
+                                         StringData why) {
+            ASSERT_EQUALS("bar", lockID);
+            ASSERT_EQUALS(lastTS, lockSessionID);
+            ASSERT_EQUALS(currentLockDoc.getLockID(), currentHolderTS);
+            ASSERT_EQUALS(getProcessID(), processId);
+            ASSERT_EQUALS("foo", why);
+        },
+        currentLockDoc);  // return arbitrary valid lock document, for testing purposes only.
+
+    getMockCatalog()->expectUnLock(
+        [](const OID&) {
+            // Don't care
+        },
+        Status::OK());
+
+    // Second attempt should overtake lock.
+    { ASSERT_OK(getMgr()->lock("bar", "foo", Milliseconds(0), Milliseconds(0)).getStatus()); }
+}
+
 }  // unnamed namespace
 }  // namespace mongo