SERVER-18671 SecondaryPreferred can end up using unversioned connections

(cherry picked from commit 1d611a8c7ee346929a4186f524c21007ef7a279d)

7 files changed, 111 insertions, 47 deletions

diff --git a/jstests/sharding/secondary_query_routing.js b/jstests/sharding/secondary_query_routing.js
new file mode 100644
index 00000000000..aae33b02ab2
--- /dev/null
+++ b/jstests/sharding/secondary_query_routing.js
@@ -0,0 +1,38 @@
+/**
+ * Test that queries can be routed to the right secondaries that are caught
+ * up with the primary.
+ */
+(function() {
+
+var rsOpts = { nodes: 2 };
+var st = new ShardingTest({ mongos: 2, shards: { rs0: rsOpts, rs1: rsOpts }});
+
+st.s0.adminCommand({ enableSharding: 'test' });
+
+st.ensurePrimaryShard('test', 'test-rs0');
+st.s0.adminCommand({ shardCollection: 'test.user', key: { x: 1 }});
+st.s0.adminCommand({ split: 'test.user', middle: { x: 0 }});
+
+st.s1.setReadPref('secondary');
+var testDB = st.s1.getDB('test');
+// This establishes the shard version Mongos #1's view.
+testDB.user.insert({ x: 1 });
+
+// Mongos #0 bumps up the version without Mongos #1 knowledge.
+// Note: moveChunk has implicit { w: 2 } write concern.
+st.s0.adminCommand({ moveChunk: 'test.user',
+                     find: { x: 0 },
+                     to: 'test-rs1',
+                     _waitForDelete: true });
+
+// Clear all the connections to make sure that Mongos #1 will attempt to establish
+// the shard version.
+assert.commandWorked(testDB.adminCommand({ connPoolSync: 1 }));
+
+// Mongos #1 performs a query to the secondary.
+var res = testDB.runCommand({ count: 'user', query: { x: 1 }});
+assert(res.ok);
+assert.eq(1, res.n, tojson(res));
+
+st.stop();
+})();
diff --git a/src/mongo/base/error_codes.err b/src/mongo/base/error_codes.err
index 41d9ce6c581..83459e8b7e3 100644
--- a/src/mongo/base/error_codes.err
+++ b/src/mongo/base/error_codes.err
@@ -117,6 +117,7 @@ error_code("InitialSyncOplogSourceMissing", 114)
 error_code("CommandNotSupported", 115)
 error_code("DocTooLargeForCapped", 116)
 error_code("ConflictingOperationInProgress", 117)
+error_code("FailedToSatisfyReadPreference", 133)
 
 # backported
 error_code("OplogStartMissing", 120)
diff --git a/src/mongo/client/dbclient_rs.cpp b/src/mongo/client/dbclient_rs.cpp
index d8e3e0d4485..dccd9c3d356 100644
--- a/src/mongo/client/dbclient_rs.cpp
+++ b/src/mongo/client/dbclient_rs.cpp
@@ -327,7 +327,7 @@ DBClientConnection* DBClientReplicaSet::checkMaster() {
 
     if (newConn == NULL || !errmsg.empty()) {
         monitor->failedHost(_masterHost);
-        uasserted(13639,
+        uasserted(ErrorCodes::FailedToSatisfyReadPreference,
                   str::stream() << "can't connect to new replica set master ["
                                 << _masterHost.toString() << "]"
                                 << (errmsg.empty() ? "" : ", err: ") << errmsg);
diff --git a/src/mongo/client/parallel.cpp b/src/mongo/client/parallel.cpp
index 7907113a7ef..d3c594344a2 100644
--- a/src/mongo/client/parallel.cpp
+++ b/src/mongo/client/parallel.cpp
@@ -560,60 +560,55 @@ void ParallelSortClusteredCursor::setupVersionAndHandleSlaveOk(PCStatePtr state,
     const DBClientBase* rawConn = state->conn->getRawConn();
     bool allowShardVersionFailure = rawConn->type() == ConnectionString::SET &&
         DBClientReplicaSet::isSecondaryQuery(_qSpec.ns(), _qSpec.query(), _qSpec.options());
-    bool connIsDown = rawConn->isFailed();
-    if (allowShardVersionFailure && !connIsDown) {
-        // If the replica set connection believes that it has a valid primary that is up,
-        // confirm that the replica set monitor agrees that the suspected primary is indeed up.
+
+    // Skip shard version checking if primary is known to be down.
+    if (allowShardVersionFailure) {
         const DBClientReplicaSet* replConn = dynamic_cast<const DBClientReplicaSet*>(rawConn);
         invariant(replConn);
         ReplicaSetMonitorPtr rsMonitor = ReplicaSetMonitor::get(replConn->getSetName());
-        if (!rsMonitor->isHostUp(replConn->getSuspectedPrimaryHostAndPort())) {
-            connIsDown = true;
+        if (!rsMonitor->isKnownToHaveGoodPrimary()) {
+            state->conn->donotCheckVersion();
+
+            // A side effect of this short circuiting is the mongos will not be able figure out
+            // that the primary is now up on it's own and has to rely on other threads to refresh
+            // node states.
+
+            OCCASIONALLY {
+                const DBClientReplicaSet* repl = dynamic_cast<const DBClientReplicaSet*>(rawConn);
+                dassert(repl);
+                warning() << "Primary for " << repl->getServerAddress()
+                          << " was down before, bypassing setShardVersion."
+                          << " The local replica set view and targeting may be stale.";
+            }
+
+            return;
         }
     }
 
-    if (allowShardVersionFailure && connIsDown) {
-        // If we're doing a secondary-allowed query and the primary is down, don't attempt to
-        // set the shard version.
-
-        state->conn->donotCheckVersion();
-
-        // A side effect of this short circuiting is the mongos will not be able figure out that
-        // the primary is now up on it's own and has to rely on other threads to refresh node
-        // states.
-
-        OCCASIONALLY {
-            const DBClientReplicaSet* repl = dynamic_cast<const DBClientReplicaSet*>(rawConn);
-            dassert(repl);
-            warning() << "Primary for " << repl->getServerAddress()
-                      << " was down before, bypassing setShardVersion."
-                      << " The local replica set view and targeting may be stale." << endl;
+    try {
+        if (state->conn->setVersion()) {
+            LOG(pc) << "needed to set remote version on connection to value "
+                    << "compatible with " << vinfo;
         }
-    } else {
-        try {
-            if (state->conn->setVersion()) {
-                // It's actually okay if we set the version here, since either the
-                // manager will be verified as compatible, or if the manager doesn't
-                // exist, we don't care about version consistency
-                LOG(pc) << "needed to set remote version on connection to value "
-                        << "compatible with " << vinfo << endl;
-            }
-        } catch (const DBException&) {
-            if (allowShardVersionFailure) {
-                // It's okay if we don't set the version when talking to a secondary, we can
-                // be stale in any case.
-
-                OCCASIONALLY {
-                    const DBClientReplicaSet* repl =
-                        dynamic_cast<const DBClientReplicaSet*>(state->conn->getRawConn());
-                    dassert(repl);
-                    warning() << "Cannot contact primary for " << repl->getServerAddress()
-                              << " to check shard version."
-                              << " The local replica set view and targeting may be stale." << endl;
-                }
-            } else {
-                throw;
+    } catch (const DBException& dbExcep) {
+        auto errCode = dbExcep.getCode();
+        if (allowShardVersionFailure &&
+            (errCode == ErrorCodes::NotMaster || errCode == ErrorCodes::NotMasterNoSlaveOkCode ||
+             errCode == ErrorCodes::FailedToSatisfyReadPreference ||
+             errCode == 9001)) {  // socket exception
+            // It's okay if we don't set the version when talking to a secondary, we can
+            // be stale in any case.
+
+            OCCASIONALLY {
+                const DBClientReplicaSet* repl =
+                    dynamic_cast<const DBClientReplicaSet*>(state->conn->getRawConn());
+                dassert(repl);
+                warning() << "Cannot contact primary for " << repl->getServerAddress()
+                          << " to check shard version."
+                          << " The local replica set view and targeting may be stale.";
             }
+        } else {
+            throw;
         }
     }
 }
diff --git a/src/mongo/client/replica_set_monitor.cpp b/src/mongo/client/replica_set_monitor.cpp
index 8b126fc00f2..3a2276c252f 100644
--- a/src/mongo/client/replica_set_monitor.cpp
+++ b/src/mongo/client/replica_set_monitor.cpp
@@ -495,6 +495,18 @@ void ReplicaSetMonitor::cleanup() {
     seedServers = StringMap<set<HostAndPort>>();
 }
 
+bool ReplicaSetMonitor::isKnownToHaveGoodPrimary() const {
+    boost::mutex::scoped_lock lk(_state->mutex);
+
+    for (const auto& node : _state->nodes) {
+        if (node.isMaster) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 Refresher::Refresher(const SetStatePtr& setState)
     : _set(setState), _scan(setState->currentScan), _startedNewScan(false) {
     if (_scan)
diff --git a/src/mongo/client/replica_set_monitor.h b/src/mongo/client/replica_set_monitor.h
index 17046ace6f5..0c5f8ef8b62 100644
--- a/src/mongo/client/replica_set_monitor.h
+++ b/src/mongo/client/replica_set_monitor.h
@@ -137,6 +137,11 @@ public:
     void appendInfo(BSONObjBuilder& b) const;
 
     /**
+     * Returns true if the monitor knows a usable primary from it's interal view.
+     */
+    bool isKnownToHaveGoodPrimary() const;
+
+    /**
      * Creates a new ReplicaSetMonitor, if it doesn't already exist.
      */
     static void createIfNeeded(const std::string& name, const std::set<HostAndPort>& servers);
diff --git a/src/mongo/client/replica_set_monitor_test.cpp b/src/mongo/client/replica_set_monitor_test.cpp
index 11b67a66aea..fba7e718159 100644
--- a/src/mongo/client/replica_set_monitor_test.cpp
+++ b/src/mongo/client/replica_set_monitor_test.cpp
@@ -958,3 +958,16 @@ TEST(ReplicaSetMonitorTests, StalePrimaryWithObsoleteElectionId) {
     ASSERT_EQUALS(ns.step, NextStep::DONE);
     ASSERT(ns.host.empty());
 }
+
+TEST(ReplicaSetMonitor, NoPrimaryUpCheck) {
+    SetStatePtr state(new SetState("name", basicSeedsSet));
+    ReplicaSetMonitor rsm(state);
+    ASSERT_FALSE(rsm.isKnownToHaveGoodPrimary());
+}
+
+TEST(ReplicaSetMonitor, PrimaryIsUpCheck) {
+    SetStatePtr state(new SetState("name", basicSeedsSet));
+    state->nodes.front().isMaster = true;
+    ReplicaSetMonitor rsm(state);
+    ASSERT_TRUE(rsm.isKnownToHaveGoodPrimary());
+}