From 8a8409999d5a98283e27d49bcfa29afba9ffd858 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Fri, 18 Sep 2020 08:25:00 +0200 Subject: [PATCH] Change the behavior in pause (#1687) 1. Don't call bootstrap if PGDATA is missing/empty, because it might be for purpose, and someone/something working on it. 2. Consider postgres running as a leader in pause not healthy if pg_control sysid doesn't match with the /initialize key (empty initialize key will allow the "race" and the leader will "restore" initialize key). 3. Don't exit on sysid mismatch in pause, only log a warning. 4. Cover corner cases when Patroni started in pause with empty PGDATA and it was restored by somebody else 5. Empty string is a valid `recovery_target`. --- features/patroni_api.feature | 10 ++++++++-- patroni/ha.py | 21 ++++++++++++++++----- patroni/postgresql/__init__.py | 9 +++++++-- patroni/postgresql/validator.py | 2 +- tests/test_ha.py | 15 +++++++++++++++ 5 files changed, 47 insertions(+), 10 deletions(-) diff --git a/features/patroni_api.feature b/features/patroni_api.feature index edda1220..17ef19d7 100644 --- a/features/patroni_api.feature +++ b/features/patroni_api.feature @@ -61,9 +61,15 @@ Scenario: check the scheduled restart And postgres0 role is the primary after 10 seconds Scenario: check API requests for the primary-replica pair in the pause mode - Given I run patronictl.py pause batman + Given I start postgres1 + Then replication works from postgres0 to postgres1 after 20 seconds + When I run patronictl.py pause batman + Then I receive a response returncode 0 + When I kill postmaster on postgres1 + And I issue a GET request to http://127.0.0.1:8009/replica + Then I receive a response code 503 + When I run patronictl.py restart batman postgres1 --force Then I receive a response returncode 0 - When I start postgres1 Then replication works from postgres0 to postgres1 after 20 seconds When I issue a GET request to http://127.0.0.1:8009/replica Then I receive a response code 200 diff --git a/patroni/ha.py b/patroni/ha.py index 4c4e2171..f29f702e 100644 --- a/patroni/ha.py +++ b/patroni/ha.py @@ -765,8 +765,10 @@ class Ha(object): if self.state_handler.is_starting(): # postgresql still starting up is unhealthy return False - if self.state_handler.is_leader(): # leader is always the healthiest - return True + 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(): return False @@ -1352,6 +1354,8 @@ class Ha(object): self.release_leader_key_voluntarily() return 'released leader key voluntarily as data dir empty and currently leader' + if self.is_paused(): + return 'running with empty data directory' return self.bootstrap() # new node else: # check if we are allowed to join @@ -1363,9 +1367,16 @@ class Ha(object): if self.sysid_valid(self.cluster.initialize): if self.cluster.initialize != data_sysid: - logger.fatal("system ID mismatch, node %s belongs to a different cluster: %s != %s", - self.state_handler.name, self.cluster.initialize, data_sysid) - sys.exit(1) + if self.is_paused(): + logger.warning('system ID has changed while in paused mode. Patroni will exit when resuming' + ' unless system ID is reset: %s != %s', self.cluster.initialize, data_sysid) + if self.has_lock(): + self.release_leader_key_voluntarily() + return 'released leader key voluntarily due to the system ID mismatch' + else: + logger.fatal('system ID mismatch, node %s belongs to a different cluster: %s != %s', + self.state_handler.name, self.cluster.initialize, data_sysid) + sys.exit(1) elif self.cluster.is_unlocked() and not self.is_paused(): # "bootstrap", but data directory is not empty if not self.state_handler.cb_called and self.state_handler.is_running() \ diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index 05f90e9e..0b8359b6 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -459,6 +459,8 @@ class Postgresql(object): self._pending_restart = False try: + if not self._major_version: + self.configure_server_parameters() configuration = self.config.effective_configuration except Exception: return None @@ -1001,8 +1003,11 @@ class Postgresql(object): def schedule_sanity_checks_after_pause(self): """ After coming out of pause we have to: - 1. sync replication slots, because it might happen that slots were removed - 2. get new 'Database system identifier' to make sure that it wasn't changed + 1. configure server parameters if necessary + 2. sync replication slots, because it might happen that slots were removed + 3. get new 'Database system identifier' to make sure that it wasn't changed """ + if not self._major_version: + self.configure_server_parameters() self.slots_handler.schedule() self._sysid = None diff --git a/patroni/postgresql/validator.py b/patroni/postgresql/validator.py index 8fbaf646..886beac9 100644 --- a/patroni/postgresql/validator.py +++ b/patroni/postgresql/validator.py @@ -484,7 +484,7 @@ recovery_parameters = CaseInsensitiveDict({ 'promote_trigger_file': String(120000, None), 'recovery_end_command': String(90300, None), 'recovery_min_apply_delay': Integer(90400, None, 0, 2147483647, 'ms'), - 'recovery_target': Enum(90400, None, ('immediate',)), + 'recovery_target': Enum(90400, None, ('immediate', '')), 'recovery_target_action': Enum(90500, None, ('pause', 'promote', 'shutdown')), 'recovery_target_inclusive': Bool(90300, None), 'recovery_target_lsn': String(100000, None), diff --git a/tests/test_ha.py b/tests/test_ha.py index cedf23ce..e3998203 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -1112,3 +1112,18 @@ class TestHa(PostgresInit): self.assertEqual(self.ha.run_cycle(), 'Unexpected exception raised, please report it as a BUG') self.ha.dcs.touch_member = Mock(side_effect=PatroniFatalException('foo')) self.assertRaises(PatroniFatalException, self.ha.run_cycle) + + def test_empty_directory_in_pause(self): + self.ha.is_paused = true + self.p.data_directory_empty = true + self.assertEqual(self.ha.run_cycle(), 'PAUSE: running with empty data directory') + self.assertEqual(self.p.role, 'uninitialized') + + @patch('patroni.ha.Ha.sysid_valid', MagicMock(return_value=True)) + def test_sysid_no_match_in_pause(self): + self.ha.is_paused = true + self.p.controldata = lambda: {'Database cluster state': 'in recovery', 'Database system identifier': '123'} + self.assertEqual(self.ha.run_cycle(), 'PAUSE: continue to run as master without lock') + + self.ha.has_lock = true + self.assertEqual(self.ha.run_cycle(), 'PAUSE: released leader key voluntarily due to the system ID mismatch')