RUBY-1426 Don't remove unknown secondaries servers from the topology (#1003)

Author: saghm

lib/mongo/cluster/topology/replica_set.rb

@@ -213,6 +213,12 @@ def remove_hosts?(description)
         end
 
         # Whether a specific server in the cluster can be removed, given a description.
+        # As described in the SDAM spec, a server should be removed if the server's
+        # address does not match the "me" field of the isMaster response, if the server
+        # has a different replica set name, or if an isMaster response from the primary
+        # does not contain the server's address in the list of known hosts. Note that as
+        # described by the spec, a server determined to be of type Unknown from its
+        # isMaster response is NOT removed from the topology.
         #
         # @example Check if a specific server can be removed from the cluster.
         #   topology.remove_server?(description, server)
@@ -224,8 +230,10 @@ def remove_hosts?(description)
         #
         # @since 2.0.6
         def remove_server?(description, server)
+          ((server.address == description.address) && description.me_mismatch?) ||
           remove_self?(description, server) ||
             (member_of_this_set?(description) &&
+                description.server_type == :primary &&
                 !description.servers.empty? &&
                   !description.lists_server?(server))
         end
@@ -316,10 +324,14 @@ def member_of_this_set?(description)
             description.replica_set_name == replica_set_name
         end
 
+        # As described by the SDAM spec, a server should be removed from the
+        # topology upon receiving its isMaster response if no error occurred
+        # and replica set name does not match that of the topology.
         def remove_self?(description, server)
-          !member_of_this_set?(description) &&
-            description.is_server?(server) &&
-              !description.ghost?
+          !description.unknown? &&
+            !member_of_this_set?(description) &&
+              description.is_server?(server) &&
+                !description.ghost?
         end
       end
     end

lib/mongo/server/description.rb

@@ -640,7 +640,7 @@ def replica_set_member?
       #
       # @since 2.0.6
       def me_mismatch?
-        !!(address.to_s != me if me)
+        !!(address.to_s.downcase != me.downcase if me)
       end
 
       # Check equality of two descriptions.

spec/mongo/cluster/topology/replica_set_spec.rb

@@ -514,6 +514,9 @@
           allow(d).to receive(:replica_set_name).and_return('test')
           allow(d).to receive(:is_server?).and_return(true)
           allow(d).to receive(:ghost?).and_return(false)
+          allow(d).to receive(:address).and_return(address)
+          allow(d).to receive(:me_mismatch?).and_return(false)
+          allow(d).to receive(:unknown?).and_return(false)
         end
       end
 
@@ -533,6 +536,10 @@
             allow(d).to receive(:replica_set_name).and_return('testing')
             allow(d).to receive(:lists_server?).and_return(true)
             allow(d).to receive(:servers).and_return([double('server')])
+            allow(d).to receive(:address).and_return(address)
+            allow(d).to receive(:me_mismatch?).and_return(false)
+            allow(d).to receive(:unknown?).and_return(false)
+            allow(d).to receive(:server_type).and_return(:secondary)
           end
         end
 
@@ -543,19 +550,46 @@
 
       context 'when the description does not include the server in question' do
 
-        let(:description) do
-          double('description').tap do |d|
-            allow(d).to receive(:config).and_return({ 'setName' => 'testing' })
-            allow(d).to receive(:replica_set_member?).and_return(true)
-            allow(d).to receive(:replica_set_name).and_return('testing')
-            allow(d).to receive(:is_server?).and_return(false)
-            allow(d).to receive(:lists_server?).and_return(false)
-            allow(d).to receive(:servers).and_return([double('server')])
+        context 'when the description is primary' do
+          let(:description) do
+            double('description').tap do |d|
+              allow(d).to receive(:config).and_return({ 'setName' => 'testing' })
+              allow(d).to receive(:replica_set_member?).and_return(true)
+              allow(d).to receive(:replica_set_name).and_return('testing')
+              allow(d).to receive(:is_server?).and_return(false)
+              allow(d).to receive(:lists_server?).and_return(false)
+              allow(d).to receive(:servers).and_return([double('server')])
+              allow(d).to receive(:address).and_return("127.0.0.1:27018")
+              allow(d).to receive(:me_mismatch?).and_return(false)
+              allow(d).to receive(:unknown?).and_return(false)
+              allow(d).to receive(:server_type).and_return(:primary)
+            end
+          end
+
+          it 'returns true' do
+            expect(topology.remove_server?(description, secondary)).to eq(true)
           end
         end
 
-        it 'returns true' do
-          expect(topology.remove_server?(description, secondary)).to eq(true)
+        context 'when the description is not' do
+          let(:description) do
+            double('description').tap do |d|
+              allow(d).to receive(:config).and_return({ 'setName' => 'testing' })
+              allow(d).to receive(:replica_set_member?).and_return(true)
+              allow(d).to receive(:replica_set_name).and_return('testing')
+              allow(d).to receive(:is_server?).and_return(false)
+              allow(d).to receive(:lists_server?).and_return(false)
+              allow(d).to receive(:servers).and_return([double('server')])
+              allow(d).to receive(:address).and_return('127.0.0.1:27018')
+              allow(d).to receive(:me_mismatch?).and_return(false)
+              allow(d).to receive(:unknown?).and_return(false)
+              allow(d).to receive(:server_type).and_return(:secondary)
+            end
+          end
+
+          it 'returns false' do
+            expect(topology.remove_server?(description, secondary)).to eq(false)
+          end
         end
       end
     end
@@ -568,6 +602,8 @@
           allow(d).to receive(:replica_set_member?).and_return(true)
           allow(d).to receive(:replica_set_name).and_return('test')
           allow(d).to receive(:is_server?).and_return(false)
+          allow(d).to receive(:address).and_return('127.0.0.1:27018')
+          allow(d).to receive(:unknown?).and_return(false)
         end
       end
 

spec/support/sdam/rs/secondary_ignore_ok_0.yml

@@ -0,0 +1,85 @@
+description: "New primary"
+
+uri: "mongodb://a,b/?replicaSet=rs"
+
+phases: [
+
+    {
+        responses: [
+
+                ["a:27017", {
+
+                    ok: 1,
+                    ismaster: true,
+                    setName: "rs",
+                    hosts: ["a:27017", "b:27017"],
+                    minWireVersion: 0,
+                    maxWireVersion: 6
+                }],
+                ["b:27017", {
+
+                    ok: 1,
+                    ismaster: false,
+                    secondary: true,
+                    setName: "rs",
+                    hosts: ["a:27017", "b:27017"],
+                    minWireVersion: 0,
+                    maxWireVersion: 6
+                }]
+        ],
+
+        outcome: {
+
+            servers: {
+
+                "a:27017": {
+
+                    type: "RSPrimary",
+                    setName: "rs"
+                },
+
+                "b:27017": {
+
+                    type: "RSSecondary",
+                    setName: "rs"
+                }
+            },
+            topologyType: "ReplicaSetWithPrimary",
+            logicalSessionTimeoutMinutes: null,
+            setName: "rs"
+        }
+    },
+
+    {
+        responses: [
+
+                ["b:27017", {
+
+                    ok: 0,
+                    minWireVersion: 0,
+                    maxWireVersion: 6
+                }]
+        ],
+
+        outcome: {
+
+            servers: {
+                "a:27017": {
+
+                    type: "RSPrimary",
+                    setName: "rs"
+                },
+
+                "b:27017": {
+
+                    type: "Unknown",
+                    setName:
+                }
+
+            },
+            topologyType: "ReplicaSetWithPrimary",
+            logicalSessionTimeoutMinutes: null,
+            setName: "rs"
+        }
+    }
+]