Recheck annotations when reading leader object on 409 (patroni#3174)

There are cases when we may send the same PATCH request more than one time to K8s API server and it could happen that the first request actually successfully updated the target and we cancelled while waiting for a response. The second PATCH request in this case will fail due to resource_version mismatch.

So far our strategy for update_leader() method was - re-read the object and repeat the request with the new resource_version. However, we can avoid the update by comparing annotations on the read object with annotations that we wanted to set.

Author: CyberDem0n

patroni/dcs/kubernetes.py

@@ -1215,16 +1215,21 @@ def _retry(*args: Any, **kwargs: Any) -> Any:
 
         self._kinds.set(self.leader_path, kind)
 
-        if not retry.ensure_deadline(0.5):
-            return False
-
         kind_annotations = kind and kind.metadata.annotations or EMPTY_DICT
         kind_resource_version = kind and kind.metadata.resource_version
 
         # There is different leader or resource_version in cache didn't change
         if kind and (kind_annotations.get(self._LEADER) != self._name or kind_resource_version == resource_version):
             return False
 
+        # We can get 409 because we do at least one retry, and the first update might have succeeded,
+        # therefore we will check if annotations on the read object match expectations.
+        if all(kind_annotations.get(k) == v for k, v in annotations.items()):
+            return True
+
+        if not retry.ensure_deadline(0.5):
+            return False
+
         return bool(_run_and_handle_exceptions(self._patch_or_create, self.leader_path, annotations,
                                                kind_resource_version, ips=ips, retry=_retry))
 

tests/test_kubernetes.py

@@ -422,14 +422,18 @@ def test__update_leader_with_retry(self, mock_patch, mock_read):
         self.assertFalse(self.k.update_leader(cluster, '123'))
         mock_patch.side_effect = RetryFailedError('')
         self.assertRaises(KubernetesError, self.k.update_leader, cluster, '123')
-        mock_patch.side_effect = k8s_client.rest.ApiException(409, '')
+        mock_patch.side_effect = [k8s_client.rest.ApiException(409, ''),
+                                  k8s_client.rest.ApiException(409, ''), mock_namespaced_kind()]
+        mock_read.return_value.metadata.resource_version = '2'
         with patch('time.time', Mock(side_effect=[0, 0, 100, 200, 0, 0, 0, 0, 0, 100, 200])):
             self.assertFalse(self.k.update_leader(cluster, '123'))
             self.assertFalse(self.k.update_leader(cluster, '123'))
+        mock_patch.side_effect = k8s_client.rest.ApiException(409, '')
         self.assertFalse(self.k.update_leader(cluster, '123'))
         mock_patch.side_effect = [k8s_client.rest.ApiException(409, ''), mock_namespaced_kind()]
-        mock_read.return_value.metadata.resource_version = '2'
-        self.assertIsNotNone(self.k._update_leader_with_retry({}, '1', []))
+        self.assertTrue(self.k._update_leader_with_retry({}, '1', []))
+        mock_patch.side_effect = [k8s_client.rest.ApiException(409, ''), mock_namespaced_kind()]
+        self.assertIsNotNone(self.k._update_leader_with_retry({'foo': 'bar'}, '1', []))
         mock_patch.side_effect = k8s_client.rest.ApiException(409, '')
         mock_read.side_effect = RetryFailedError('')
         self.assertRaises(KubernetesError, self.k.update_leader, cluster, '123')