Make different kazoo timeouts depend on loop_wait (#243)

* Make different kazoo timeouts dependant on loop_wait

ping timeout ~ 1/2 * loop_wait
connect_timeout ~ 1/2 * loop_wait

Originally these values were calculated from negotiated session timeout
and didn't worked very well, because it was taking significant time to
figure out that connection is dead and reconnect (up to session timeout)
and not giving us time to retry.

* Address the code review

tests/test_api.py

@@ -37,7 +37,6 @@ class MockPostgresql(object):
 
 class MockHa(object):
 
-    dcs = Mock()
     state_handler = MockPostgresql()
 
     @staticmethod
@@ -67,10 +66,9 @@ class MockHa(object):
 
 class MockPatroni(object):
 
-    nap_time = 10
-    config = Mock()
-    postgresql = MockPostgresql()
     ha = MockHa()
+    config = Mock()
+    postgresql = ha.state_handler
     dcs = Mock()
     tags = {}
     version = '0.00'
@@ -138,14 +136,14 @@ class TestRestApiHandler(unittest.TestCase):
         self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0'))
         MockRestApiServer(RestApiHandler, 'POST /restart HTTP/1.0\nAuthorization:')
 
-    @patch.object(MockHa, 'dcs')
+    @patch.object(MockPatroni, 'dcs')
     def test_do_GET_config(self, mock_dcs):
         mock_dcs.cluster.config.data = {}
         self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /config'))
         mock_dcs.cluster.config = None
         self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /config'))
 
-    @patch.object(MockHa, 'dcs')
+    @patch.object(MockPatroni, 'dcs')
     def test_do_PATCH_config(self, mock_dcs):
         config = {'postgresql': {'use_slots': False, 'use_pg_rewind': True, 'parameters': {'wal_level': 'logical'}}}
         mock_dcs.get_cluster.return_value.config = ClusterConfig.from_node(1, json.dumps(config))
@@ -161,7 +159,7 @@ class TestRestApiHandler(unittest.TestCase):
         mock_dcs.set_config_value.return_value = False
         MockRestApiServer(RestApiHandler, request)
 
-    @patch.object(MockHa, 'dcs')
+    @patch.object(MockPatroni, 'dcs')
     def test_do_PUT_config(self, mock_dcs):
         mock_dcs.get_cluster.return_value.config = ClusterConfig.from_node(1, '{}')
         request = 'PUT /config HTTP/1.0' + self._authorization + '\nContent-Length: '
@@ -181,9 +179,11 @@ class TestRestApiHandler(unittest.TestCase):
             MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0' + self._authorization)
         self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0' + self._authorization))
 
+    #@patch.object(MockPatroni, 'dcs')
     def test_do_POST_restart(self):
         request = 'POST /restart HTTP/1.0' + self._authorization
         self.assertIsNotNone(MockRestApiServer(RestApiHandler, request))
+
         with patch.object(MockHa, 'restart', Mock(side_effect=Exception)):
             MockRestApiServer(RestApiHandler, request)
 
@@ -221,13 +221,14 @@ class TestRestApiHandler(unittest.TestCase):
                 request = make_request('{"role": "master", "postgres_version": "9.5.2"}')
                 MockRestApiServer(RestApiHandler, request)
 
+    #@patch.object(MockPatroni, 'dcs')
     def test_do_DELETE_restart(self):
         for retval in (True, False):
             with patch.object(MockHa, 'delete_future_restart', Mock(return_value=retval)):
                 request = 'DELETE /restart HTTP/1.0' + self._authorization
                 self.assertIsNotNone(MockRestApiServer(RestApiHandler, request))
 
-    @patch.object(MockHa, 'dcs')
+    @patch.object(MockPatroni, 'dcs')
     def test_do_POST_reinitialize(self, dcs):
         cluster = dcs.get_cluster.return_value
         request = 'POST /reinitialize HTTP/1.0' + self._authorization
@@ -247,8 +248,9 @@ class TestRestApiHandler(unittest.TestCase):
             self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'GET /patroni'))
 
     @patch('time.sleep', Mock())
-    @patch.object(MockHa, 'dcs')
+    @patch.object(MockPatroni, 'dcs')
     def test_do_POST_failover(self, dcs):
+        dcs.loop_wait = 10
         cluster = dcs.get_cluster.return_value
 
         post = 'POST /failover HTTP/1.0' + self._authorization + '\nContent-Length: '
@@ -273,19 +275,27 @@ class TestRestApiHandler(unittest.TestCase):
         cluster.members = [Member(0, 'postgresql0', 30, {'api_url': 'http'}),
                            Member(0, 'postgresql2', 30, {'api_url': 'http'})]
         MockRestApiServer(RestApiHandler, request)
-        with patch.object(MockPatroni, 'dcs') as d:
-            cluster = d.get_cluster.return_value
-            cluster.leader.name = 'postgresql0'
-            MockRestApiServer(RestApiHandler, request)
-            cluster.leader.name = 'postgresql2'
-            MockRestApiServer(RestApiHandler, request)
-            cluster.leader.name = 'postgresql1'
-            cluster.failover = None
-            MockRestApiServer(RestApiHandler, request)
-            d.get_cluster = Mock(side_effect=Exception)
-            MockRestApiServer(RestApiHandler, request)
-            d.manual_failover.return_value = False
-            MockRestApiServer(RestApiHandler, request)
+
+        cluster.failover = None
+        MockRestApiServer(RestApiHandler, request)
+
+        dcs.get_cluster.side_effect = [cluster]
+        MockRestApiServer(RestApiHandler, request)
+
+        cluster2 = cluster.copy()
+        cluster2.leader.name = 'postgresql0'
+        dcs.get_cluster.side_effect = [cluster, cluster2]
+        MockRestApiServer(RestApiHandler, request)
+
+        cluster2.leader.name = 'postgresql2'
+        dcs.get_cluster.side_effect = [cluster, cluster2]
+        MockRestApiServer(RestApiHandler, request)
+
+        dcs.get_cluster.side_effect = None
+        dcs.manual_failover.return_value = False
+        MockRestApiServer(RestApiHandler, request)
+        dcs.manual_failover.return_value = True
+
         with patch.object(MockHa, 'fetch_nodes_statuses', Mock(return_value=[])):
             MockRestApiServer(RestApiHandler, request)
 

tests/test_ha.py

@@ -82,7 +82,6 @@ zookeeper:
         self.api = Mock()
         self.tags = {'foo': 'bar'}
         self.nofailover = None
-        self.nap_time = 10
         self.replicatefrom = None
         self.api.connection_string = 'http://127.0.0.1:8008'
         self.clonefrom = None

tests/test_patroni.py

@@ -74,7 +74,7 @@ class TestPatroni(unittest.TestCase):
     def test_schedule_next_run(self):
         self.p.ha.dcs.watch = Mock(return_value=True)
         self.p.schedule_next_run()
-        self.p.next_run = time.time() - self.p.nap_time - 1
+        self.p.next_run = time.time() - self.p.dcs.loop_wait - 1
         self.p.schedule_next_run()
 
     def test_noloadbalance(self):

tests/test_zookeeper.py

@@ -58,11 +58,16 @@ class MockKazooClient(Mock):
             raise TypeError("Invalid type for 'path' (string expected)")
         if not isinstance(value, (six.binary_type,)):
             raise TypeError("Invalid type for 'value' (must be a byte string)")
+        if value == b'Exception':
+            raise Exception
         if path.endswith('/initialize') or path == '/service/test/optime/leader':
             raise Exception
         elif value == b'retry' or (value == b'exists' and self.exists):
             raise NodeExistsError
 
+    def create_async(self, path, value=b"", acl=None, ephemeral=False, sequence=False, makepath=False):
+        return self.create(path, value, acl, ephemeral, sequence, makepath) or Mock()
+
     @staticmethod
     def set(path, value, version=-1):
         if not isinstance(path, six.string_types):
@@ -80,6 +85,9 @@ class MockKazooClient(Mock):
                 return
         raise NoNodeError
 
+    def set_async(self, path, value, version=-1):
+        return self.set(path, value, version) or Mock()
+
     def delete(self, path, version=-1, recursive=False):
         if not isinstance(path, six.string_types):
             raise TypeError("Invalid type for 'path' (string expected)")
@@ -92,6 +100,9 @@ class MockKazooClient(Mock):
         elif path.endswith('/') or path.endswith('/initialize') or path == '/service/test/members/bar':
             raise NoNodeError
 
+    def delete_async(self, path, version=-1, recursive=False):
+        return self.delete(path, version, recursive) or Mock()
+
 
 class TestPatroniSequentialThreadingHandler(unittest.TestCase):
 
@@ -109,16 +120,14 @@ class TestZooKeeper(unittest.TestCase):
     @patch('patroni.dcs.zookeeper.KazooClient', MockKazooClient)
     def setUp(self):
         self.zk = ZooKeeper({'hosts': ['localhost:2181'], 'scope': 'test',
-                             'name': 'foo', 'ttl': 30, 'retry_timeout': 10})
+                             'name': 'foo', 'ttl': 30, 'retry_timeout': 10, 'loop_wait': 10})
 
     def test_session_listener(self):
         self.zk.session_listener(KazooState.SUSPENDED)
 
-    def test_set_ttl(self):
-        self.zk.set_ttl(20)
-
-    def test_set_retry_timeout(self):
-        self.zk.set_retry_timeout(10)
+    def test_reload_config(self):
+        self.zk.reload_config({'ttl': 20, 'retry_timeout': 10, 'loop_wait': 10})
+        self.zk.reload_config({'ttl': 20, 'retry_timeout': 10, 'loop_wait': 5})
 
     def test_get_node(self):
         self.assertIsNone(self.zk.get_node('/no_node'))
@@ -165,7 +174,7 @@ class TestZooKeeper(unittest.TestCase):
         self.zk.touch_member('new')
         self.zk._name = 'na'
         self.zk._client.exists = 1
-        self.zk.touch_member('exists')
+        self.zk.touch_member('Exception')
         self.zk._name = 'bar'
         self.zk.touch_member('retry')
         self.zk._fetch_cluster = True
@@ -183,8 +192,12 @@ class TestZooKeeper(unittest.TestCase):
     def test_write_leader_optime(self):
         self.zk.last_leader_operation = '0'
         self.zk.write_leader_optime('1')
+        with patch.object(MockKazooClient, 'create_async', Mock()):
+            self.zk.write_leader_optime('1')
+        with patch.object(MockKazooClient, 'set_async', Mock()):
+            self.zk.write_leader_optime('2')
         self.zk._base_path = self.zk._base_path.replace('test', 'bla')
-        self.zk.write_leader_optime('2')
+        self.zk.write_leader_optime('3')
 
     def test_delete_cluster(self):
         self.assertTrue(self.zk.delete_cluster())
@@ -193,3 +206,8 @@ class TestZooKeeper(unittest.TestCase):
         self.zk.watch(0)
         self.zk.event.isSet = lambda: True
         self.zk.watch(0)
+
+    def test__kazoo_connect(self):
+        self.zk._client._retry.deadline = 1
+        self.zk._orig_kazoo_connect = Mock(return_value=(0, 0))
+        self.zk._kazoo_connect(None, None)