Fix etcd.Client.machines

lavagetto · lavagetto · commit b8a61d9b3b7a · 2015-04-23T11:06:40.000+02:00
In our old version of the code, etcd.Client.api_execute called Client.machines in case of failure, which called api_execute in return. In the scenario of a client with a list of possible servers, and the first of the list being down, we throw an exception right now, because of this. Using directly urllib3 and skipping api_execute in Client.machines allows us to connect to the first available server. Closes jplana#51
diff --git a/src/etcd/client.py b/src/etcd/client.py
@@ -121,7 +121,11 @@ def uri(protocol, host, port):
             # we need the set of servers in the cluster in order to try
             # reconnecting upon error.
             self._machines_cache = self.machines
-            self._machines_cache.remove(self._base_uri)
+            if self._base_uri in self._machines_cache:
+                self._machines_cache.remove(self._base_uri)
+            else:
+                # This happens if the code includes the hostname and not the ip, or vice-versa
+                self._base_uri = self._machines_cache.pop(0)
         else:
             self._machines_cache = []
 
@@ -166,11 +170,27 @@ def machines(self):
         >>> print client.machines
         ['http://127.0.0.1:4001', 'http://127.0.0.1:4002']
         """
-        return [
-            node.strip() for node in self.api_execute(
-                self.version_prefix + '/machines',
-                self._MGET).data.decode('utf-8').split(',')
-        ]
+        # We can't use api_execute here, or it causes a logical loop
+        try:
+            uri = self._base_uri + self.version_prefix + '/machines'
+            response = self.http.request(
+                self._MGET,
+                uri,
+                timeout=self.read_timeout,
+                redirect=self.allow_redirect)
+
+            return [
+                node.strip() for node in
+                self._handle_server_response(response).data.decode('utf-8').split(',')
+            ]
+        except:
+            # We can't get the list of machines, if one server is in the machines cache, try on it
+            if self._machines_cache:
+                self._base_uri = self._machines_cache.pop(0)
+                # Call myself
+                return self.machines
+            else:
+                raise etcd.EtcdException("Could not get the list of servers, maybe you provided the wrong host(s) to connect to?")
 
     @property
     def leader(self):
diff --git a/src/etcd/tests/unit/test_old_request.py b/src/etcd/tests/unit/test_old_request.py
@@ -19,23 +19,6 @@ def getheaders(self):
 
 class TestClientRequest(unittest.TestCase):
 
-    def test_machines(self):
-        """ Can request machines """
-        client = etcd.Client()
-        client.api_execute = mock.Mock(
-            return_value=FakeHTTPResponse(200, data=
-                                          "http://127.0.0.1:4002,"
-                                          " http://127.0.0.1:4001,"
-                                          " http://127.0.0.1:4003,"
-                                          " http://127.0.0.1:4001")
-        )
-
-        assert client.machines == [
-            'http://127.0.0.1:4002',
-            'http://127.0.0.1:4001',
-            'http://127.0.0.1:4003',
-            'http://127.0.0.1:4001'
-        ]
 
     def test_leader(self):
         """ Can request the leader """
diff --git a/src/etcd/tests/unit/test_request.py b/src/etcd/tests/unit/test_request.py
@@ -105,13 +105,13 @@ class TestClientApiInterface(TestClientApiBase):
 
     If a test should be run only in this class, please override the method there.
     """
-
-    def test_machines(self):
+    @mock.patch('urllib3.request.RequestMethods.request')
+    def test_machines(self, mocker):
         """ Can request machines """
         data = ['http://127.0.0.1:4001',
                 'http://127.0.0.1:4002', 'http://127.0.0.1:4003']
         d = ','.join(data)
-        self._mock_api(200, d)
+        mocker.return_value = self._prepare_response(200, d)
         self.assertEquals(data, self.client.machines)
 
     def test_leader(self):
