From deb9cc6b732a52580920fdc1f1e373f4b3842fc5 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 24 Mar 2025 08:50:49 +0100 Subject: [PATCH] Fix bug with switchover in synchronous_mode=quorum (#3310) When the candidate is specified we don't have to check quorum requirements. The problem was introduced in #3278 Close #3307 --- patroni/ha.py | 16 ++++++++++++---- tests/test_ha.py | 44 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index f03bb065..527c2072 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -1377,7 +1377,14 @@ class Ha(object): quorum_votes += 1 # In case of quorum replication we need to make sure that there is enough healthy synchronous replicas! - return quorum_votes >= (self.cluster.sync.quorum if self.quorum_commit_mode_is_active() else 0) + # However, when failover candidate is set, we can ignore quorum requirements. + check_quorum = self.quorum_commit_mode_is_active() and\ + not (self.cluster.failover and self.cluster.failover.candidate and not exclude_failover_candidate) + if check_quorum and quorum_votes < self.cluster.sync.quorum: + logger.info('Quorum requirement %d can not be reached', self.cluster.sync.quorum) + return False + + return quorum_votes >= 0 def manual_failover_process_no_leader(self) -> Optional[bool]: """Handles manual failover/switchover when the old leader already stepped down. @@ -2418,8 +2425,9 @@ class Ha(object): return False # Don't spend time on "nofailover" nodes checking. # We also don't need nodes which we can't query with the api in the list. - return node.name not in exclude and \ - not node.nofailover and bool(node.api_url) and \ - (not failover or not failover.candidate or node.name == failover.candidate) + # And, if exclude_failover_candidate is True we want to skip node.name == failover.candidate check. + return node.name not in exclude and not node.nofailover and bool(node.api_url) and \ + (exclude_failover_candidate or not failover + or not failover.candidate or node.name == failover.candidate) return list(filter(is_eligible, self.cluster.members)) diff --git a/tests/test_ha.py b/tests/test_ha.py index a260478b..599a8b25 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -68,7 +68,8 @@ def get_cluster_initialized_without_leader(leader=False, failover=None, sync=Non 'tags': {'clonefrom': True}, 'scheduled_restart': {'schedule': "2100-01-01 10:53:07.560445+00:00", 'postgres_version': '99.0.0'}}) - syncstate = SyncState(0 if sync else None, sync and sync[0], sync and sync[1], 0) + syncstate = SyncState(0 if sync else None, sync and sync[0], + sync and sync[1], sync[2] if sync and len(sync) > 2 else 0) failsafe = {m.name: m.api_url for m in (m1, m2)} if failsafe else None return get_cluster(SYSID, leader, [m1, m2], failover, syncstate, cluster_config, failsafe) @@ -1015,7 +1016,7 @@ class TestHa(PostgresInit): # switchover from a specific leader, but the only sync node (us, postgresql0) has nofailover tag self.ha.cluster = get_cluster_initialized_without_leader(failover=Failover(0, 'leader', '', None), - sync=('postgresql0')) + sync=('postgresql0', None)) self.ha.patroni.nofailover = True self.assertEqual(self.ha.run_cycle(), 'following a different leader because I am not allowed to promote') @@ -1844,3 +1845,42 @@ class TestHa(PostgresInit): # Test that _process_quorum_replication doesn't take longer than loop_wait with patch('time.time', Mock(side_effect=[30, 60, 90, 120])): self.ha.process_sync_replication() + + def test_is_failover_possible(self): + self.p._major_version = 140000 # supports_multiple_sync + self.p.name = 'leader' + self.ha.fetch_node_status = get_node_status() + self.ha.cluster = get_cluster_initialized_with_leader(sync=('leader', 'foo,other', 1), + failover=Failover(0, 'leader', 'other', None)) + self.ha.cluster.members.append(Member(0, 'foo', 28, {'api_url': 'http://127.0.0.1:8011/patroni'})) + # switchover when synchronous_mode = off + self.assertTrue(self.ha.is_failover_possible()) + + with patch.object(global_config.__class__, 'is_synchronous_mode', PropertyMock(return_value=True)): + # switchover to synchronous node when synchronous_mode = on + self.assertTrue(self.ha.is_failover_possible()) + with patch.object(global_config.__class__, 'is_quorum_commit_mode', PropertyMock(return_value=True)): + # switchover to synchronous node when synchronous_mode = quorum + self.assertTrue(self.ha.is_failover_possible()) # success, despite the fact that quorum is low + # failover candidate is unhealthy, we are checking if there are other good candidates, but quorum is low + self.assertFalse(self.ha.is_failover_possible(exclude_failover_candidate=True)) + # now we satisfy quorum requirements + with patch.object(SyncState, 'quorum', PropertyMock(return_value=0)): + self.assertTrue(self.ha.is_failover_possible(exclude_failover_candidate=True)) + + self.ha.cluster = get_cluster_initialized_with_leader(sync=('leader', 'foo,other', 1), + failover=Failover(0, '', 'foo', None)) + # failover to missing node foo + self.assertFalse(self.ha.is_failover_possible()) + + self.ha.cluster = get_cluster_initialized_with_leader(sync=('leader', 'foo,other', 1), + failover=Failover(0, 'leader', '', None)) + # switchover from leader when synchronous_mode = off + self.assertTrue(self.ha.is_failover_possible()) + + with patch.object(global_config.__class__, 'is_synchronous_mode', PropertyMock(return_value=True)): + # switchover from leader when synchronous_mode = on + self.assertTrue(self.ha.is_failover_possible()) + with patch.object(global_config.__class__, 'is_quorum_commit_mode', PropertyMock(return_value=True)): + # switchover from leader when synchronous_mode = quorum + self.assertFalse(self.ha.is_failover_possible()) # failure, because quorum is low