SERVER-66050 findSelfInConfig should attempt fast path for every HostAndPort before trying slow pathr4.4.19-rc1

(cherry picked from commit 0c0f80372c2adee56c16c106ec2711d39ea6b45f)

10 files changed, 167 insertions, 7 deletions

diff --git a/src/mongo/db/repl/isself.cpp b/src/mongo/db/repl/isself.cpp
index 1c561ff9f5d..17d01625e94 100644
--- a/src/mongo/db/repl/isself.cpp
+++ b/src/mongo/db/repl/isself.cpp
@@ -164,10 +164,17 @@ std::vector<std::string> getAddrsForHost(const std::string& iporhost,
 
 }  // namespace
 
-bool isSelf(const HostAndPort& hostAndPort, ServiceContext* const ctx) {
+bool isSelf(const HostAndPort& hostAndPort, ServiceContext* const ctx, Milliseconds timeout) {
+    if (isSelfFastPath(hostAndPort)) {
+        return true;
+    }
+    return isSelfSlowPath(hostAndPort, ctx, timeout);
+}
+
+bool isSelfFastPath(const HostAndPort& hostAndPort) {
     if (MONGO_unlikely(failIsSelfCheck.shouldFail())) {
         LOGV2(356490,
-              "failIsSelfCheck failpoint activated, returning false from isSelf",
+              "failIsSelfCheck failpoint activated, returning false from isSelfFastPath",
               "hostAndPort"_attr = hostAndPort);
         return false;
     }
@@ -223,12 +230,24 @@ bool isSelf(const HostAndPort& hostAndPort, ServiceContext* const ctx) {
             }
         }
     }
+    return false;
+}
 
+bool isSelfSlowPath(const HostAndPort& hostAndPort,
+                    ServiceContext* const ctx,
+                    Milliseconds timeout) {
     ctx->waitForStartupComplete();
+    if (MONGO_unlikely(failIsSelfCheck.shouldFail())) {
+        LOGV2(6605000,
+              "failIsSelfCheck failpoint activated, returning false from isSelfSlowPath",
+              "hostAndPort"_attr = hostAndPort);
+        return false;
+    }
 
     try {
         DBClientConnection conn;
-        conn.setSoTimeout(30);  // 30 second timeout
+        double timeoutSeconds = static_cast<double>(durationCount<Milliseconds>(timeout)) / 1000.0;
+        conn.setSoTimeout(timeoutSeconds);
 
         // We need to avoid the isMaster call triggered by a normal connect, which would
         // cause a deadlock. 'isSelf' is called by the Replication Coordinator when validating
diff --git a/src/mongo/db/repl/isself.h b/src/mongo/db/repl/isself.h
index 6358f53037d..5f554af135b 100644
--- a/src/mongo/db/repl/isself.h
+++ b/src/mongo/db/repl/isself.h
@@ -33,6 +33,7 @@
 #include <vector>
 
 #include "mongo/bson/oid.h"
+#include "mongo/util/duration.h"
 
 namespace mongo {
 struct HostAndPort;
@@ -49,7 +50,21 @@ extern OID instanceId;
 /**
  * Returns true if "hostAndPort" identifies this instance.
  */
-bool isSelf(const HostAndPort& hostAndPort, ServiceContext* ctx);
+bool isSelf(const HostAndPort& hostAndPort,
+            ServiceContext* ctx,
+            Milliseconds timeout = Seconds(30));
+
+/**
+ * Returns true if "hostAndPort" identifies this instance by checking our bound IP addresses,
+ * without going out to the network and running the _isSelf command on the node.
+ */
+bool isSelfFastPath(const HostAndPort& hostAndPort);
+
+/**
+ * Returns true if "hostAndPort" identifies this instance by running the _isSelf command on the
+ * node.
+ */
+bool isSelfSlowPath(const HostAndPort& hostAndPort, ServiceContext* ctx, Milliseconds timeout);
 
 /**
  * Returns all the IP addresses bound to the network interfaces of this machine.
diff --git a/src/mongo/db/repl/isself_test.cpp b/src/mongo/db/repl/isself_test.cpp
index 511ef122ecf..91ad0ad3803 100644
--- a/src/mongo/db/repl/isself_test.cpp
+++ b/src/mongo/db/repl/isself_test.cpp
@@ -55,6 +55,7 @@ TEST_F(ServiceContextTest, DetectsSameHostIPv4) {
     // Fastpath should agree with the result of getBoundAddrs
     // since it uses it...
     for (std::vector<string>::const_iterator it = addrs.begin(); it != addrs.end(); ++it) {
+        ASSERT(isSelfFastPath(HostAndPort(*it, serverGlobalParams.port)));
         ASSERT(isSelf(HostAndPort(*it, serverGlobalParams.port), getGlobalServiceContext()));
     }
 #else
@@ -72,6 +73,7 @@ TEST_F(ServiceContextTest, DetectsSameHostIPv6) {
     // Fastpath should agree with the result of getBoundAddrs
     // since it uses it...
     for (std::vector<string>::const_iterator it = addrs.begin(); it != addrs.end(); ++it) {
+        ASSERT(isSelfFastPath(HostAndPort(*it, serverGlobalParams.port)));
         ASSERT(isSelf(HostAndPort(*it, serverGlobalParams.port), getGlobalServiceContext()));
     }
 #else
diff --git a/src/mongo/db/repl/repl_set_config_checks.cpp b/src/mongo/db/repl/repl_set_config_checks.cpp
index 9888ff5d556..af0f9cc3fd1 100644
--- a/src/mongo/db/repl/repl_set_config_checks.cpp
+++ b/src/mongo/db/repl/repl_set_config_checks.cpp
@@ -327,11 +327,21 @@ StatusWith<int> findSelfInConfig(ReplicationCoordinatorExternalState* externalSt
     for (ReplSetConfig::MemberIterator iter = newConfig.membersBegin();
          iter != newConfig.membersEnd();
          ++iter) {
-        if (externalState->isSelf(iter->getHostAndPort(), ctx)) {
+        if (externalState->isSelfFastPath(iter->getHostAndPort())) {
             meConfigs.push_back(iter);
         }
     }
     if (meConfigs.empty()) {
+        // No self-hosts were found using the fastpath; check with the slow path.
+        for (ReplSetConfig::MemberIterator iter = newConfig.membersBegin();
+             iter != newConfig.membersEnd();
+             ++iter) {
+            if (externalState->isSelfSlowPath(iter->getHostAndPort(), ctx, Seconds(30))) {
+                meConfigs.push_back(iter);
+            }
+        }
+    }
+    if (meConfigs.empty()) {
         return StatusWith<int>(ErrorCodes::NodeNotFound,
                                str::stream() << "No host described in new configuration with "
                                              << newConfig.getConfigVersionAndTerm().toString()
diff --git a/src/mongo/db/repl/repl_set_config_checks_test.cpp b/src/mongo/db/repl/repl_set_config_checks_test.cpp
index dc90eb6d115..81e77bc81f7 100644
--- a/src/mongo/db/repl/repl_set_config_checks_test.cpp
+++ b/src/mongo/db/repl/repl_set_config_checks_test.cpp
@@ -1154,6 +1154,60 @@ TEST_F(ServiceContextTest, FindSelfInConfig) {
             .getStatus());
 }
 
+TEST_F(ServiceContextTest, FindSelfInConfigFastAndSlow) {
+    ReplSetConfig newConfig;
+    ASSERT_OK(newConfig.initialize(BSON("_id"
+                                        << "rs0"
+                                        << "version" << 2 << "protocolVersion" << 1 << "members"
+                                        << BSON_ARRAY(BSON("_id" << 1 << "host"
+                                                                 << "h1")
+                                                      << BSON("_id" << 2 << "host"
+                                                                    << "h2")
+                                                      << BSON("_id" << 3 << "host"
+                                                                    << "h3")))));
+
+    {
+        // Present once only on the slow path, but fast enough to be found
+        ReplicationCoordinatorExternalStateMock presentOnceExternalState;
+        presentOnceExternalState.addSelfSlow(HostAndPort("h2"), Seconds(29));
+        ASSERT_EQUALS(1,
+                      unittest::assertGet(findSelfInConfig(
+                          &presentOnceExternalState, newConfig, getServiceContext())));
+    }
+
+    {
+        // Present twice only on the slow path, but fast enough to be found both times.
+        ReplicationCoordinatorExternalStateMock presentTwiceExternalState;
+        presentTwiceExternalState.addSelfSlow(HostAndPort("h2"), Seconds(29));
+        presentTwiceExternalState.addSelfSlow(HostAndPort("h3"), Seconds(29));
+        ASSERT_EQUALS(ErrorCodes::InvalidReplicaSetConfig,
+                      findSelfInConfig(&presentTwiceExternalState, newConfig, getServiceContext())
+                          .getStatus());
+    }
+
+    {
+        // Present once on the fast path, once on the slow path. This is expected to erroneously
+        // succeed, because we should not check the slow path if we got a unique result on the fast
+        // path.
+        ReplicationCoordinatorExternalStateMock presentFastAndSlowExternalState;
+        presentFastAndSlowExternalState.addSelf(HostAndPort("h2"));
+        presentFastAndSlowExternalState.addSelfSlow(HostAndPort("h3"), Seconds(29));
+        ASSERT_EQUALS(1,
+                      unittest::assertGet(findSelfInConfig(
+                          &presentFastAndSlowExternalState, newConfig, getServiceContext())));
+    }
+
+    {
+        // Present only on the slow path, with a long timeout.  This will fail.
+        ReplicationCoordinatorExternalStateMock presentLongTimeoutExternalState;
+        presentLongTimeoutExternalState.addSelfSlow(HostAndPort("h2"), Seconds(31));
+        ASSERT_EQUALS(
+            ErrorCodes::NodeNotFound,
+            findSelfInConfig(&presentLongTimeoutExternalState, newConfig, getServiceContext())
+                .getStatus());
+    }
+}
+
 }  // namespace
 }  // namespace repl
 }  // namespace mongo
diff --git a/src/mongo/db/repl/replication_coordinator_external_state.h b/src/mongo/db/repl/replication_coordinator_external_state.h
index d78510ee0c1..35314f6ecdf 100644
--- a/src/mongo/db/repl/replication_coordinator_external_state.h
+++ b/src/mongo/db/repl/replication_coordinator_external_state.h
@@ -160,6 +160,20 @@ public:
     virtual bool isSelf(const HostAndPort& host, ServiceContext* service) = 0;
 
     /**
+     * Returns true if "host" is one of the network identities of this node, without actually
+     * going out to the network and checking.
+     */
+    virtual bool isSelfFastPath(const HostAndPort& host) = 0;
+
+    /**
+     * Returns true if "host" is one of the network identities of this node, without
+     * checking the fast path first.
+     */
+    virtual bool isSelfSlowPath(const HostAndPort& host,
+                                ServiceContext* service,
+                                Milliseconds timeout) = 0;
+
+    /**
      * Gets the replica set config document from local storage, or returns an error.
      */
     virtual StatusWith<BSONObj> loadLocalConfigDocument(OperationContext* opCtx) = 0;
diff --git a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
index cabdbe1eece..5744e7a3cd0 100644
--- a/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
+++ b/src/mongo/db/repl/replication_coordinator_external_state_impl.cpp
@@ -750,6 +750,16 @@ bool ReplicationCoordinatorExternalStateImpl::isSelf(const HostAndPort& host, Se
     return repl::isSelf(host, ctx);
 }
 
+bool ReplicationCoordinatorExternalStateImpl::isSelfFastPath(const HostAndPort& host) {
+    return repl::isSelfFastPath(host);
+}
+
+bool ReplicationCoordinatorExternalStateImpl::isSelfSlowPath(const HostAndPort& host,
+                                                             ServiceContext* ctx,
+                                                             Milliseconds timeout) {
+    return repl::isSelfSlowPath(host, ctx, timeout);
+}
+
 HostAndPort ReplicationCoordinatorExternalStateImpl::getClientHostAndPort(
     const OperationContext* opCtx) {
     return HostAndPort(opCtx->getClient()->clientAddress(true));
diff --git a/src/mongo/db/repl/replication_coordinator_external_state_impl.h b/src/mongo/db/repl/replication_coordinator_external_state_impl.h
index cd2648042c0..87e50558b61 100644
--- a/src/mongo/db/repl/replication_coordinator_external_state_impl.h
+++ b/src/mongo/db/repl/replication_coordinator_external_state_impl.h
@@ -85,6 +85,10 @@ public:
     OpTime onTransitionToPrimary(OperationContext* opCtx) override;
     virtual void forwardSlaveProgress();
     virtual bool isSelf(const HostAndPort& host, ServiceContext* service);
+    bool isSelfFastPath(const HostAndPort& host) final;
+    bool isSelfSlowPath(const HostAndPort& host,
+                        ServiceContext* service,
+                        Milliseconds timeout) final;
     Status createLocalLastVoteCollection(OperationContext* opCtx) final;
     virtual StatusWith<BSONObj> loadLocalConfigDocument(OperationContext* opCtx);
     virtual Status storeLocalConfigDocument(OperationContext* opCtx,
diff --git a/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp b/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp
index 0a381a5fa2f..54f4879269b 100644
--- a/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp
+++ b/src/mongo/db/repl/replication_coordinator_external_state_mock.cpp
@@ -101,9 +101,24 @@ void ReplicationCoordinatorExternalStateMock::forwardSlaveProgress() {}
 
 bool ReplicationCoordinatorExternalStateMock::isSelf(const HostAndPort& host,
                                                      ServiceContext* const service) {
+    return sequenceContains(_selfHosts, host) || _selfHostsSlow.find(host) != _selfHostsSlow.end();
+}
+
+bool ReplicationCoordinatorExternalStateMock::isSelfFastPath(const HostAndPort& host) {
     return sequenceContains(_selfHosts, host);
 }
 
+bool ReplicationCoordinatorExternalStateMock::isSelfSlowPath(const HostAndPort& host,
+                                                             ServiceContext* const service,
+                                                             Milliseconds timeout) {
+    if (sequenceContains(_selfHosts, host))
+        return true;
+    auto iter = _selfHostsSlow.find(host);
+    if (iter == _selfHostsSlow.end())
+        return false;
+    return iter->second <= timeout;
+}
+
 void ReplicationCoordinatorExternalStateMock::addSelf(const HostAndPort& host) {
     _selfHosts.push_back(host);
 }
@@ -112,6 +127,11 @@ void ReplicationCoordinatorExternalStateMock::clearSelfHosts() {
     _selfHosts.clear();
 }
 
+void ReplicationCoordinatorExternalStateMock::addSelfSlow(const HostAndPort& host,
+                                                          Milliseconds timeout) {
+    _selfHostsSlow.emplace(host, timeout);
+}
+
 HostAndPort ReplicationCoordinatorExternalStateMock::getClientHostAndPort(
     const OperationContext* opCtx) {
     return _clientHostAndPort;
diff --git a/src/mongo/db/repl/replication_coordinator_external_state_mock.h b/src/mongo/db/repl/replication_coordinator_external_state_mock.h
index deb1192cd1e..f52ad5aff7b 100644
--- a/src/mongo/db/repl/replication_coordinator_external_state_mock.h
+++ b/src/mongo/db/repl/replication_coordinator_external_state_mock.h
@@ -75,6 +75,10 @@ public:
     OpTime onTransitionToPrimary(OperationContext* opCtx) override;
     virtual void forwardSlaveProgress();
     virtual bool isSelf(const HostAndPort& host, ServiceContext* service);
+    bool isSelfFastPath(const HostAndPort& host) final;
+    bool isSelfSlowPath(const HostAndPort& host,
+                        ServiceContext* service,
+                        Milliseconds timeout) final;
     virtual HostAndPort getClientHostAndPort(const OperationContext* opCtx);
     virtual StatusWith<BSONObj> loadLocalConfigDocument(OperationContext* opCtx);
     virtual Status storeLocalConfigDocument(OperationContext* opCtx,
@@ -107,13 +111,20 @@ public:
 
     /**
      * Adds "host" to the list of hosts that this mock will match when responding to "isSelf"
-     * messages.
+     * messages, including "isSelfFastPath" and "isSelfSlowPath".
      */
     void addSelf(const HostAndPort& host);
 
     /**
+     * Adds "host" to the list of hosts that this mock will match when responding to
+     * "isSelfSlowPath" messages with a timeout less than or equal to that given,
+     * but not "isSelfFastPath" messages.
+     */
+    void addSelfSlow(const HostAndPort& host, Milliseconds timeout);
+
+    /**
      * Remove all hosts from the list of hosts that this mock will match when responding to "isSelf"
-     * messages.
+     * messages.  Clears both regular and slow hosts.
      */
     void clearSelfHosts();
 
@@ -204,6 +215,7 @@ private:
     StatusWith<OpTime> _lastOpTime;
     StatusWith<Date_t> _lastWallTime;
     std::vector<HostAndPort> _selfHosts;
+    stdx::unordered_map<HostAndPort, Milliseconds> _selfHostsSlow;
     bool _canAcquireGlobalSharedLock;
     Status _storeLocalConfigDocumentStatus;
     Status _storeLocalLastVoteDocumentStatus;