From 01976ec10bca5c1869f44ae710124d3447123f44 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 31 Jul 2023 11:22:18 +0200 Subject: [PATCH] Don't allow stale primary to win the leader race (#2787) Consider a following situation: 1. node1 is stressed so much that Patroni heart-beat can't run regularly and the leader lock expires. 2. node2 notice that there is no leader, gets the lock, promotes, and gets to a situation like it is described in 1. 3. Patroni on node1 finally wakes up, notice that Postgres is running as a primary, but without a leader lock and "happily" acquires the lock. That is, node1 discarded promoting of node2, and the node2 after that it will not be possible to join the node2 back to the cluster, because pg_rewind is not possible when two nodes are on the same timeline. To partially mitigate the problem we introduce an additional timeline check. If postgres is running as primary Patroni will consider it as a perfect candidate only if timeline isn't behind the last known cluster timeline recorder in the `/history` key. If postgres timeline is behind the cluster timeline postgres will be demoted to read-only. Further behavior would depend on `maximum_lag_on_failover` and `check_timeline` settings. Since the `/history` key isn't updated instantly after promotion, there is still a short period of time when the issue could happen, but it seems that it is close to impossible to make it more reliable. Close https://github.com/zalando/patroni/issues/2779 --- patroni/ha.py | 19 ++++++++++++++++--- tests/test_ha.py | 6 ++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/patroni/ha.py b/patroni/ha.py index 95ca6e05..2445aa02 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -996,9 +996,22 @@ class Ha(object): return ret if self.state_handler.is_leader(): - # in pause leader is the healthiest only when no initialize or sysid matches with initialize! - return not self.is_paused() or not self.cluster.initialize\ - or self.state_handler.sysid == self.cluster.initialize + if self.is_paused(): + # in pause leader is the healthiest only when no initialize or sysid matches with initialize! + return not self.cluster.initialize or self.state_handler.sysid == self.cluster.initialize + + # We want to protect from the following scenario: + # 1. node1 is stressed so much that heart-beat isn't running regularly and the leader lock expires. + # 2. node2 promotes, gets heavy load and the situation described in 1 repeats. + # 3. Patroni on node1 comes back, notices that Postgres is running as primary but there is + # no leader key and "happily" acquires the leader lock. + # That is, node1 discarded promotion of node2. To avoid it we want to detect timeline change. + my_timeline = self.state_handler.get_primary_timeline() + if my_timeline < self.cluster.timeline: + logger.warning('My timeline %s is behind last known cluster timeline %s', + my_timeline, self.cluster.timeline) + return False + return True if self.is_paused(): return False diff --git a/tests/test_ha.py b/tests/test_ha.py index 1f5f903d..ae6be3e2 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -375,6 +375,12 @@ class TestHa(PostgresInit): def test_acquire_lock_as_primary(self): self.assertEqual(self.ha.run_cycle(), 'acquired session lock as a leader') + def test_leader_race_stale_primary(self): + with patch.object(Postgresql, 'get_primary_timeline', Mock(return_value=1)), \ + patch('patroni.ha.logger.warning') as mock_logger: + self.assertEqual(self.ha.run_cycle(), 'demoting self because i am not the healthiest node') + self.assertEqual(mock_logger.call_args[0][0], 'My timeline %s is behind last known cluster timeline %s') + def test_promoted_by_acquiring_lock(self): self.ha.is_healthiest_node = true self.p.is_leader = false