SERVER-20418 Make sure that we always start the distlock pinger when running with mirrored config servers

Author: stbrody

src/mongo/dbtests/framework.cpp

@@ -45,8 +45,8 @@
 #include "mongo/dbtests/dbtests.h"
 #include "mongo/dbtests/framework_options.h"
 #include "mongo/s/catalog/catalog_manager.h"
-#include "mongo/s/grid.h"
 #include "mongo/s/catalog/legacy/legacy_dist_lock_manager.h"
+#include "mongo/s/grid.h"
 #include "mongo/stdx/mutex.h"
 #include "mongo/util/assert_util.h"
 #include "mongo/util/exit.h"
@@ -77,20 +77,10 @@ int runDbTests(int argc, char** argv) {
         auto connectHook = stdx::make_unique<CustomConnectHook>(txn.get());
         ConnectionString::setConnectionHook(connectHook.get());
         ON_BLOCK_EXIT([] { ConnectionString::setConnectionHook(nullptr); });
+        LegacyDistLockManager::disablePinger();
         ShardingState::get(txn.get())->initialize(txn.get(), "$dummy:10000");
     }
 
-    // Note: ShardingState::initialize also initializes the distLockMgr.
-    {
-        auto txn = cc().makeOperationContext();
-        auto distLockMgr = dynamic_cast<LegacyDistLockManager*>(
-            grid.forwardingCatalogManager()->getDistLockManager());
-        if (distLockMgr) {
-            distLockMgr->enablePinger(false);
-        }
-    }
-
-
     int ret = unittest::Suite::run(frameworkGlobalParams.suites,
                                    frameworkGlobalParams.filter,
                                    frameworkGlobalParams.runsPerTest);

src/mongo/s/catalog/legacy/catalog_manager_legacy.cpp

@@ -206,7 +206,6 @@ Status CatalogManagerLegacy::init(const ConnectionString& configDBCS,
 
     _distLockManager =
         stdx::make_unique<LegacyDistLockManager>(_configServerConnectionString, distLockProcessId);
-    _distLockManager->startUp();
 
     {
         stdx::lock_guard<stdx::mutex> lk(_mutex);
@@ -223,17 +222,14 @@ Status CatalogManagerLegacy::startup(OperationContext* txn, bool allowNetworking
         return Status::OK();
     }
 
-    if (!allowNetworking) {
-        // Config servers shouldn't call dbHash on themselves and shards don't need to
-        // run the checker.
-        _started = true;
-        return Status::OK();
+    if (allowNetworking) {
+        Status status = _startConfigServerChecker();
+        if (!status.isOK()) {
+            return status;
+        }
     }
 
-    Status status = _startConfigServerChecker();
-    if (!status.isOK()) {
-        return status;
-    }
+    _distLockManager->startUp();
 
     _started = true;
     return Status::OK();

src/mongo/s/catalog/legacy/legacy_dist_lock_manager.cpp

@@ -49,17 +49,20 @@ const stdx::chrono::seconds kDefaultSocketTimeout(30);
 const milliseconds kDefaultPingInterval(30 * 1000);
 }  // unnamed namespace
 
+bool LegacyDistLockManager::_pingerEnabled = true;
+
 LegacyDistLockManager::LegacyDistLockManager(ConnectionString configServer,
                                              const std::string& processId)
-    : _configServer(std::move(configServer)),
-      _processId(processId),
-      _isStopped(false),
-      _pingerEnabled(true) {}
+    : _configServer(std::move(configServer)), _processId(processId), _isStopped(false) {}
 
 void LegacyDistLockManager::startUp() {
     stdx::lock_guard<stdx::mutex> sl(_mutex);
     invariant(!_pinger);
     _pinger = stdx::make_unique<LegacyDistLockPinger>();
+
+    if (_pingerEnabled) {
+        uassertStatusOK(_pinger->startup(_configServer, _processId, kDefaultPingInterval));
+    }
 }
 
 void LegacyDistLockManager::shutDown(OperationContext* txn, bool allowNetworking) {
@@ -93,13 +96,6 @@ StatusWith<DistLockManager::ScopedDistLock> LegacyDistLockManager::lock(
         if (_isStopped) {
             return Status(ErrorCodes::LockBusy, "legacy distlock manager is stopped");
         }
-
-        if (_pingerEnabled) {
-            auto pingStatus = _pinger->startPing(*(distLock.get()), kDefaultPingInterval);
-            if (!pingStatus.isOK()) {
-                return pingStatus;
-            }
-        }
     }
 
     auto lastStatus =
@@ -227,11 +223,6 @@ Status LegacyDistLockManager::checkStatus(OperationContext* txn, const DistLockH
     return distLock->checkStatus(durationCount<Seconds>(kDefaultSocketTimeout));
 }
 
-void LegacyDistLockManager::enablePinger(bool enable) {
-    stdx::lock_guard<stdx::mutex> sl(_mutex);
-    _pingerEnabled = enable;
-}
-
 void LegacyDistLockManager::unlockAll(OperationContext* txn, const std::string& processID) {
     fassertFailed(34367);  // Only supported for CSRS
 }

src/mongo/s/catalog/legacy/legacy_dist_lock_manager.h

@@ -63,8 +63,10 @@ class LegacyDistLockManager : public DistLockManager {
 
     virtual void unlockAll(OperationContext* txn, const std::string& processID) override;
 
-    // For testing only.
-    void enablePinger(bool enable);
+    // For testing only.  Must be called before any calls to startUp().
+    static void disablePinger() {
+        _pingerEnabled = false;
+    }
 
 protected:
     virtual void unlock(OperationContext* txn,
@@ -85,8 +87,6 @@ class LegacyDistLockManager : public DistLockManager {
     std::unique_ptr<LegacyDistLockPinger> _pinger;
 
     bool _isStopped;
-
-    // For testing only.
-    bool _pingerEnabled;
+    static bool _pingerEnabled;
 };
 }

src/mongo/s/catalog/legacy/legacy_dist_lock_pinger.cpp

@@ -220,11 +220,10 @@ void LegacyDistLockPinger::distLockPingThread(ConnectionString addr,
     }
 }
 
-Status LegacyDistLockPinger::startPing(const DistributedLock& lock,
-                                       stdx::chrono::milliseconds sleepTime) {
-    const ConnectionString& conn = lock.getRemoteConnection();
-    const string& processId = lock.getProcessId();
-    string pingID = pingThreadId(conn, processId);
+Status LegacyDistLockPinger::startup(const ConnectionString& configServerConnectionString,
+                                     const std::string& processID,
+                                     Milliseconds sleepTime) {
+    string pingID = pingThreadId(configServerConnectionString, processID);
 
     {
         // Make sure we don't start multiple threads for a process id.
@@ -239,22 +238,11 @@ Status LegacyDistLockPinger::startPing(const DistributedLock& lock,
             return Status::OK();
         }
 
-        // Check the config server clock skew.
-        if (lock.isRemoteTimeSkewed()) {
-            return Status(ErrorCodes::DistributedClockSkewed,
-                          str::stream() << "clock skew of the cluster " << conn.toString()
-                                        << " is too far out of bounds "
-                                        << "to allow distributed locking.");
-        }
-    }
-
-    {
-        stdx::lock_guard<stdx::mutex> lk(_mutex);
         stdx::thread thread(stdx::bind(&LegacyDistLockPinger::distLockPingThread,
                                        this,
-                                       conn,
+                                       configServerConnectionString,
                                        getJSTimeVirtualThreadSkew(),
-                                       processId,
+                                       processID,
                                        sleepTime));
         _pingThreads.insert(std::make_pair(pingID, std::move(thread)));
 

src/mongo/s/catalog/legacy/legacy_dist_lock_pinger.h

@@ -50,11 +50,13 @@ class LegacyDistLockPinger {
     LegacyDistLockPinger() = default;
 
     /**
-     * Starts pinging the process id for the given lock.
-     * Note: this pinger does not support calling startPing on a lock that has previously
-     * been stopped by a call to stopPing on its underlying processId.
+     * Starts the pinger thread for a given processID.
+     * Note: this pinger does not support being started up after it was stopped, either by a call
+     * to stopPing or shutdown.
      */
-    Status startPing(const DistributedLock& lock, Milliseconds sleepTime);
+    Status startup(const ConnectionString& configServerConnectionString,
+                   const std::string& processID,
+                   Milliseconds sleepTime);
 
     /**
      * Adds a distributed lock that has the given id to the unlock list. The unlock list