From 2afcaa9d8395e67e2a8643baa79621a470b81ab6 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Mon, 6 Mar 2023 16:33:32 +0100 Subject: [PATCH] Don't write to PGDATA if major version is not known (#2583) It could happen that Patroni is started up before PGDATA was mounted. In this case Patroni can't determine major Postgres version from PG_VERSION file. Later, when PGDATA is mounted, Patroni was trying to create the recovery.conf even if the actual Postgres major version is newver than 12. To mitigate the problem we double check that the `Postgresql._major_version` is set before writing recovery configuration or starting postgres up. Close https://github.com/zalando/patroni/issues/2434 --- patroni/postgresql/__init__.py | 49 ++++++++++++++++++++++++++++------ patroni/postgresql/misc.py | 4 +-- tests/test_bootstrap.py | 1 + tests/test_ha.py | 3 ++- tests/test_postgresql.py | 5 ++++ 5 files changed, 51 insertions(+), 11 deletions(-) diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index 16718108..c8c7ef99 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -13,6 +13,7 @@ from datetime import datetime from dateutil import tz from psutil import TimeoutExpired from threading import current_thread, Lock +from typing import Optional from .bootstrap import Bootstrap from .callback_executor import CallbackAction, CallbackExecutor @@ -25,6 +26,7 @@ from .postmaster import PostmasterProcess from .slots import SlotsHandler from .sync import SyncHandler from .. import psycopg +from ..dcs import Member from ..exceptions import PostgresConnectionException from ..utils import Retry, RetryFailedError, polling_loop, data_directory_is_empty, parse_int @@ -200,7 +202,10 @@ class Postgresql(object): def _version_file_exists(self): return not self.data_directory_empty() and os.path.isfile(self._version_file) - def get_major_version(self): + def get_major_version(self) -> int: + """Reads major version from PG_VERSION file + + :returns: major PostgreSQL version in integer format or 0 in case of missing file or errors""" if self._version_file_exists(): try: with open(self._version_file) as f: @@ -557,7 +562,8 @@ class Postgresql(object): Waits for postmaster to open ports or terminate so pg_isready can be used to check startup completion or failure. - :returns: True if start was initiated and postmaster ports are open, False if start failed""" + :returns: True if start was initiated and postmaster ports are open, + False if start failed, and None if postgres is still starting up""" # make sure we close all connections established against # the former node, otherwise, we might get a stalled one # after kill -9, which would report incorrect data to @@ -578,8 +584,8 @@ class Postgresql(object): self._pending_restart = False try: - if not self._major_version: - self.configure_server_parameters() + if not self.ensure_major_version_is_known(): + return None configuration = self.config.effective_configuration except Exception: return None @@ -926,7 +932,27 @@ class Postgresql(object): except Exception: logger.exception('Failed to read and parse %s', (history_path,)) - def follow(self, member, role='replica', timeout=None, do_reload=False): + def follow(self, + member: Member, + role: Optional[str] = 'replica', + timeout: Optional[float] = None, + do_reload: Optional[bool] = False) -> Optional[bool]: + """Reconfigure postgres to follow a new member or use different recovery parameters. + + Method may call `on_role_change` callback if role is changing. + + :param member: The member to follow + :param role: The desired role, normally 'replica', but could also be a 'standby_leader' + :param timeout: start timeout, how long should the `start()` method wait for postgres accepting connections + :param do_reload: indicates that after updating postgresql.conf we just need to do a reload instead of restart + + :returns: True - if restart/reload were successfully performed, + False - if restart/reload failed + None - if nothing was done or if Postgres is still in starting state after `timeout` seconds.""" + + if not self.ensure_major_version_is_known(): + return None + recovery_params = self.config.build_recovery_params(member) self.config.write_recovery_conf(recovery_params) @@ -1050,7 +1076,15 @@ class Postgresql(object): def configure_server_parameters(self): self._major_version = self.get_major_version() self.config.setup_server_parameters() - return True + + def ensure_major_version_is_known(self) -> bool: + """Calls configure_server_parameters() if `_major_version` is not known + + :returns: `True` if `_major_version` is set, otherwise `False`""" + + if not self._major_version: + self.configure_server_parameters() + return self._major_version > 0 def pg_wal_realpath(self): """Returns a dict containing the symlink (key) and target (value) for the wal directory""" @@ -1143,8 +1177,7 @@ class Postgresql(object): 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.ensure_major_version_is_known() self.slots_handler.schedule() self.citus_handler.schedule_cache_rebuild() self._sysid = None diff --git a/patroni/postgresql/misc.py b/patroni/postgresql/misc.py index 798bed35..462e5d43 100644 --- a/patroni/postgresql/misc.py +++ b/patroni/postgresql/misc.py @@ -7,7 +7,7 @@ from patroni.exceptions import PostgresException logger = logging.getLogger(__name__) -def postgres_version_to_int(pg_version): +def postgres_version_to_int(pg_version: str) -> int: """Convert the server_version to integer >>> postgres_version_to_int('9.5.3') @@ -45,7 +45,7 @@ def postgres_version_to_int(pg_version): return int(''.join('{0:02d}'.format(c) for c in components)) -def postgres_major_version_to_int(pg_version): +def postgres_major_version_to_int(pg_version: str) -> int: """ >>> postgres_major_version_to_int('10') 100000 diff --git a/tests/test_bootstrap.py b/tests/test_bootstrap.py index 9bbc8112..1c16306f 100644 --- a/tests/test_bootstrap.py +++ b/tests/test_bootstrap.py @@ -112,6 +112,7 @@ class TestBootstrap(BaseTestPostgresql): config = {'users': {'replicator': {'password': 'rep-pass', 'options': ['replication']}}} with patch.object(Postgresql, 'is_running', Mock(return_value=False)),\ + patch.object(Postgresql, 'get_major_version', Mock(return_value=140000)),\ patch('multiprocessing.Process', Mock(side_effect=Exception)),\ patch('multiprocessing.get_context', Mock(side_effect=Exception), create=True): self.assertRaises(Exception, self.b.bootstrap, config) diff --git a/tests/test_ha.py b/tests/test_ha.py index 65a3460c..10ae87af 100644 --- a/tests/test_ha.py +++ b/tests/test_ha.py @@ -182,6 +182,8 @@ def run_async(self, func, args=()): @patch.object(CancellableSubprocess, 'call', Mock(return_value=0)) @patch.object(Postgresql, 'get_replica_timeline', Mock(return_value=2)) @patch.object(Postgresql, 'get_primary_timeline', Mock(return_value=2)) +@patch.object(Postgresql, 'get_major_version', Mock(return_value=140000)) +@patch.object(Postgresql, 'resume_wal_replay', Mock()) @patch.object(ConfigHandler, 'restore_configuration_files', Mock()) @patch.object(etcd.Client, 'write', etcd_write) @patch.object(etcd.Client, 'read', etcd_read) @@ -449,7 +451,6 @@ class TestHa(PostgresInit): self.p.is_leader = false self.assertEqual(self.ha.run_cycle(), 'not promoting because failed to update leader lock in DCS') - @patch.object(Postgresql, 'major_version', PropertyMock(return_value=130000)) def test_follow(self): self.ha.cluster.is_unlocked = false self.p.is_leader = false diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 71f5df4d..29dcbe93 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -115,6 +115,9 @@ class TestPostgresql(BaseTestPostgresql): self.assertTrue(self.p.start()) mock_is_running.return_value = None + with patch.object(Postgresql, 'ensure_major_version_is_known', Mock(return_value=False)): + self.assertIsNone(self.p.start()) + mock_postmaster = MockPostmaster() with patch.object(PostmasterProcess, 'start', return_value=mock_postmaster): pg_conf = os.path.join(self.p.data_dir, 'postgresql.conf') @@ -324,6 +327,8 @@ class TestPostgresql(BaseTestPostgresql): self.p.call_nowait(CallbackAction.ON_START) m = RemoteMember('1', {'restore_command': '2', 'primary_slot_name': 'foo', 'conn_kwargs': {'host': 'bar'}}) self.p.follow(m) + with patch.object(Postgresql, 'ensure_major_version_is_known', Mock(return_value=False)): + self.assertIsNone(self.p.follow(m)) @patch.object(MockCursor, 'execute', Mock(side_effect=psycopg.OperationalError)) def test__query(self):