From d7dc3c2d96efae456956db7df0ef9f022139b696 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 2 Dec 2021 11:35:30 +0100 Subject: [PATCH] Handle missing timelines in history file when deciding to rewind (#2120) When restore_command is configured Postgres is trying to fetch/apply all possible WAL segments and also fetch history files in order to select the correct timeline. It could result in a situation where the new history file will be missing some timelines. Example: - node1 demotes/crashes on timeline 1 - node2 promotes to timeline 2 and archives `00000002.history` and crashes - node1 recovers as a replica, "replays" `00000002.history` and promotes to timeline 3 As a result, the `00000003.history` will not have the line with timeline 2, because it never replayed any WAL segment from it. The `pg_rewind` tool is supposed to correctly handle such case when rewinding node2 from node1, but Patroni when deciding whether the rewind should happen was searching for the exact timeline in the history file from the new primary. The solution is to assume that rewind is required if the current replica timeline is missing. In addition to that this PR makes sure that the primary isn't running in recovery before starting the procedure of rewind check. Close https://github.com/zalando/patroni/issues/2118 and https://github.com/zalando/patroni/issues/2124 --- patroni/dcs/kubernetes.py | 2 +- patroni/ha.py | 3 --- patroni/postgresql/rewind.py | 14 ++++++++------ tests/test_rewind.py | 8 +++++--- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/patroni/dcs/kubernetes.py b/patroni/dcs/kubernetes.py index f18e8c5e..0aa9809a 100644 --- a/patroni/dcs/kubernetes.py +++ b/patroni/dcs/kubernetes.py @@ -1011,7 +1011,7 @@ class Kubernetes(AbstractDCS): def touch_member(self, data, permanent=False): cluster = self.cluster if cluster and cluster.leader and cluster.leader.name == self._name: - role = 'promoted' if data['role'] in ('replica', 'promoted') else 'master' + role = 'master' elif data['state'] == 'running' and data['role'] != 'master': role = data['role'] else: diff --git a/patroni/ha.py b/patroni/ha.py index d4b91f56..e9a4a641 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -192,9 +192,6 @@ class Ha(object): 'version': self.patroni.version } - # following two lines are mainly necessary for consul, to avoid creation of master service - if data['role'] == 'master' and not self.is_leader(): - data['role'] = 'promoted' if self.is_leader() and not self._rewind.checkpoint_after_promote(): data['checkpoint_after_promote'] = False tags = self.get_effective_tags() diff --git a/patroni/postgresql/rewind.py b/patroni/postgresql/rewind.py index 61b3b88c..21039327 100644 --- a/patroni/postgresql/rewind.py +++ b/patroni/postgresql/rewind.py @@ -161,11 +161,11 @@ class Rewind(object): if local_timeline is None or local_lsn is None: return - if isinstance(leader, Leader): - if leader.member.data.get('role') != 'master': - return - # standby cluster - elif not self.check_leader_is_not_in_recovery(self._conn_kwargs(leader, self._postgresql.config.replication)): + if isinstance(leader, Leader) and leader.member.data.get('role') != 'master': + return + + if not self.check_leader_is_not_in_recovery( + self._conn_kwargs(leader, self._postgresql.config.rewind_credentials)): return history = need_rewind = None @@ -200,9 +200,11 @@ class Rewind(object): need_rewind = True else: need_rewind = switchpoint != self._get_checkpoint_end(local_timeline, local_lsn) - break elif parent_timeline > local_timeline: + need_rewind = True break + else: + need_rewind = True self._log_master_history(history, i) self._state = need_rewind and REWIND_STATUS.NEED or REWIND_STATUS.NOT_NEED diff --git a/tests/test_rewind.py b/tests/test_rewind.py index b303d5eb..19e7e6ad 100644 --- a/tests/test_rewind.py +++ b/tests/test_rewind.py @@ -141,13 +141,15 @@ class TestRewind(BaseTestPostgresql): self.leader = self.leader.member self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader)) mock_check_leader_is_not_in_recovery.return_value = True - self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader)) + self.assertTrue(self.r.rewind_or_reinitialize_needed_and_possible(self.leader)) + self.r.reset_state() self.r.trigger_check_diverged_lsn() with patch('patroni.psycopg.connect', Mock(side_effect=Exception)): self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader)) self.r.trigger_check_diverged_lsn() - with patch.object(MockCursor, 'fetchone', Mock(side_effect=[('', 3, '0/0'), ('', b'3\t0/40159C0\tn\n')])): - self.assertFalse(self.r.rewind_or_reinitialize_needed_and_possible(self.leader)) + with patch.object(MockCursor, 'fetchone', Mock(side_effect=[('', 3, '0/0'), ('', b'1\t0/40159C0\tn\n')])): + self.assertTrue(self.r.rewind_or_reinitialize_needed_and_possible(self.leader)) + self.r.reset_state() self.r.trigger_check_diverged_lsn() with patch.object(MockCursor, 'fetchone', Mock(return_value=('', 1, '0/0'))): with patch.object(Rewind, '_get_local_timeline_lsn', Mock(return_value=(True, 1, '0/0'))):