From 3d29cb7e5080fae222c08311a8655ce46bfba095 Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Thu, 10 Oct 2019 14:49:30 +0200 Subject: [PATCH] Perform pg_ctl reload regardless of config changes (#1204) It is possible that some config files are not controlled by Patroni and when somebody is doing reload via REST API or by sending SIGHUP to Patroni process the usual expectation is that postgres will also be reloaded, but it didn't happen when there were no changes in the postgresql section of Patroni config. For example one might replace ssl_cert_file and ssl_key_file on the filesystem and starting from PostgreSQL 10 it just requires a reload, but Patroni wasn't doing it. In addition to that fix the issue with handling of `wal_buffers`. The default value depends on `shared_buffers` and `wal_segment_size` and therefore Patroni was exposing pending_restart when the new value in the config was explicitly set to -1 (default). Close https://github.com/zalando/patroni/issues/1198 --- features/patroni_api.feature | 5 +-- patroni/__init__.py | 10 +++-- patroni/api.py | 14 +------ patroni/config.py | 8 +--- patroni/postgresql/__init__.py | 4 +- patroni/postgresql/config.py | 76 ++++++++++++++++++++++++---------- tests/__init__.py | 3 ++ tests/test_api.py | 5 +-- tests/test_config.py | 3 +- tests/test_patroni.py | 3 ++ tests/test_postgresql.py | 7 +++- 11 files changed, 80 insertions(+), 58 deletions(-) diff --git a/features/patroni_api.feature b/features/patroni_api.feature index 95c80858..4351bab9 100644 --- a/features/patroni_api.feature +++ b/features/patroni_api.feature @@ -28,10 +28,7 @@ Scenario: check API requests on a stand-alone server And I receive a response text "Failover could be performed only to a specific candidate" Scenario: check local configuration reload - Given I issue an empty POST request to http://127.0.0.1:8008/reload - Then I receive a response code 200 - And I receive a response text nothing changed - When I add tag new_tag new_value to postgres0 config + Given I add tag new_tag new_value to postgres0 config And I issue an empty POST request to http://127.0.0.1:8008/reload Then I receive a response code 202 diff --git a/patroni/__init__.py b/patroni/__init__.py index fff80243..37802a33 100644 --- a/patroni/__init__.py +++ b/patroni/__init__.py @@ -66,14 +66,14 @@ class Patroni(object): def nosync(self): return bool(self.tags.get('nosync', False)) - def reload_config(self): + def reload_config(self, sighup=False): try: self.tags = self.get_tags() self.logger.reload_config(self.config.get('log', {})) - self.dcs.reload_config(self.config) self.watchdog.reload_config(self.config) self.api.reload_config(self.config['restapi']) - self.postgresql.reload_config(self.config['postgresql']) + self.postgresql.reload_config(self.config['postgresql'], sighup) + self.dcs.reload_config(self.config) except Exception: logger.exception('Failed to reload config_file=%s', self.config.config_file) @@ -121,7 +121,9 @@ class Patroni(object): if self._received_sighup: self._received_sighup = False if self.config.reload_local_configuration(): - self.reload_config() + self.reload_config(True) + else: + self.postgresql.config.reload_config(self.config['postgresql'], True) logger.info(self.ha.run_cycle()) diff --git a/patroni/api.py b/patroni/api.py index 60dcb457..b8e77b5b 100644 --- a/patroni/api.py +++ b/patroni/api.py @@ -187,18 +187,8 @@ class RestApiHandler(BaseHTTPRequestHandler): @check_auth def do_POST_reload(self): - try: - if self.server.patroni.config.reload_local_configuration(True): - status_code = 202 - response = 'reload scheduled' - self.server.patroni.sighup_handler() - else: - status_code = 200 - response = 'nothing changed' - except Exception as e: - status_code = 500 - response = str(e) - self._write_response(status_code, response) + self.server.patroni.sighup_handler() + self._write_response(202, 'reload scheduled') @staticmethod def parse_schedule(schedule, action): diff --git a/patroni/config.py b/patroni/config.py index f9716b4c..ab737f92 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -168,23 +168,19 @@ class Config(object): except Exception: logger.exception('Exception when setting dynamic_configuration') - def reload_local_configuration(self, dry_run=False): + def reload_local_configuration(self): if self.config_file: try: configuration = self._load_config_file() if not deep_compare(self._local_configuration, configuration): new_configuration = self._build_effective_configuration(self._dynamic_configuration, configuration) - if dry_run: - return not deep_compare(new_configuration, self.__effective_configuration) self._local_configuration = configuration self.__effective_configuration = new_configuration return True else: - logger.info('No configuration items changed, nothing to reload.') + logger.info('No local configuration items changed.') except Exception: logger.exception('Exception when reloading local configuration from %s', self.config_file) - if dry_run: - raise @staticmethod def _process_postgresql_parameters(parameters, is_local=False): diff --git a/patroni/postgresql/__init__.py b/patroni/postgresql/__init__.py index 2f2a87ec..f3654a02 100644 --- a/patroni/postgresql/__init__.py +++ b/patroni/postgresql/__init__.py @@ -187,8 +187,8 @@ class Postgresql(object): 3: STATE_UNKNOWN} return return_codes.get(ret, STATE_UNKNOWN) - def reload_config(self, config): - self.config.reload_config(config) + def reload_config(self, config, sighup=False): + self.config.reload_config(config, sighup) self._is_leader_retry.deadline = self.retry.deadline = config['retry_timeout']/2.0 @property diff --git a/patroni/postgresql/config.py b/patroni/postgresql/config.py index 76aa9f56..40efe4f7 100644 --- a/patroni/postgresql/config.py +++ b/patroni/postgresql/config.py @@ -4,6 +4,7 @@ import re import shutil import socket import stat +import time from requests.structures import CaseInsensitiveDict from six.moves.urllib_parse import urlparse, parse_qsl, unquote @@ -695,7 +696,23 @@ class ConfigHandler(object): self._postgresql.set_connection_kwargs(self.local_connect_kwargs) - def reload_config(self, config): + @staticmethod + def _handle_wal_buffers(old_values, changes): + wal_block_size = parse_int(old_values['wal_block_size'][1]) + wal_segment_size = old_values['wal_segment_size'] + wal_segment_size = parse_int(wal_segment_size[1]) * parse_int(wal_segment_size[2], 'B') / wal_block_size + default_wal_buffers = min(max(parse_int(old_values['shared_buffers'][1]) / 32, 8), wal_segment_size) + + wal_buffers = old_values['wal_buffers'] + new_value = str(changes['wal_buffers'] or -1) + + new_value = default_wal_buffers if new_value == '-1' else parse_int(new_value, wal_buffers[2]) + old_value = default_wal_buffers if wal_buffers[1] == '-1' else parse_int(*wal_buffers[1:3]) + + if new_value == old_value: + del changes['wal_buffers'] + + def reload_config(self, config, sighup=False): self._superuser = config['authentication'].get('superuser', {}) server_parameters = self.get_server_parameters(config) @@ -704,42 +721,47 @@ class ConfigHandler(object): changes = CaseInsensitiveDict({p: v for p, v in server_parameters.items() if '.' not in p}) changes.update({p: None for p in self._server_parameters.keys() if not ('.' in p or p in changes)}) if changes: + if 'wal_buffers' in changes: # we need to calculate the default value of wal_buffers + undef = [p for p in ('shared_buffers', 'wal_segment_size', 'wal_block_size') if p not in changes] + changes.update({p: None for p in undef}) # XXX: query can raise an exception - for r in self._postgresql.query(('SELECT name, setting, unit, vartype, context ' - + 'FROM pg_catalog.pg_settings ' + - ' WHERE pg_catalog.lower(name) IN (' - + ', '.join(['%s'] * len(changes)) + - ')'), *(k.lower() for k in changes.keys())): + old_values = {r[0]: r for r in self._postgresql.query(('SELECT name, setting, unit, vartype, context ' + + 'FROM pg_catalog.pg_settings ' + + ' WHERE pg_catalog.lower(name) = ANY(%s)'), + [k.lower() for k in changes.keys()])} + if 'wal_buffers' in changes: + self._handle_wal_buffers(old_values, changes) + for p in undef: + del changes[p] + + for r in old_values.values(): if r[4] != 'internal' and r[0] in changes: new_value = changes.pop(r[0]) if new_value is None or not compare_values(r[3], r[2], r[1], new_value): + conf_changed = True if r[4] == 'postmaster': pending_restart = True - logger.info('Changed %s from %s to %s (restart required)', r[0], r[1], new_value) + logger.info('Changed %s from %s to %s (restart might be required)', + r[0], r[1], new_value) if config.get('use_unix_socket') and r[0] == 'unix_socket_directories'\ or r[0] in ('listen_addresses', 'port'): local_connection_address_changed = True else: logger.info('Changed %s from %s to %s', r[0], r[1], new_value) - conf_changed = True for param in changes: if param in server_parameters: logger.warning('Removing invalid parameter `%s` from postgresql.parameters', param) server_parameters.pop(param) # Check that user-defined-paramters have changed (parameters with period in name) - if not conf_changed: - for p, v in server_parameters.items(): - if '.' in p and (p not in self._server_parameters or str(v) != str(self._server_parameters[p])): - logger.info('Changed %s from %s to %s', p, self._server_parameters.get(p), v) - conf_changed = True - break - if not conf_changed: - for p, v in self._server_parameters.items(): - if '.' in p and (p not in server_parameters or str(v) != str(server_parameters[p])): - logger.info('Changed %s from %s to %s', p, v, server_parameters.get(p)) - conf_changed = True - break + for p, v in server_parameters.items(): + if '.' in p and (p not in self._server_parameters or str(v) != str(self._server_parameters[p])): + logger.info('Changed %s from %s to %s', p, self._server_parameters.get(p), v) + conf_changed = True + for p, v in self._server_parameters.items(): + if '.' in p and (p not in server_parameters or str(v) != str(server_parameters[p])): + logger.info('Changed %s from %s to %s', p, v, server_parameters.get(p)) + conf_changed = True if not server_parameters.get('hba_file') and config.get('pg_hba'): hba_changed = self._config.get('pg_hba', []) != config['pg_hba'] @@ -769,10 +791,18 @@ class ConfigHandler(object): if ident_changed: self.replace_pg_ident() - if conf_changed or hba_changed or ident_changed: - logger.info('PostgreSQL configuration items changed, reloading configuration.') + if sighup or conf_changed or hba_changed or ident_changed: + logger.info('Reloading PostgreSQL configuration.') self._postgresql.reload() - elif not pending_restart: + if self._postgresql.major_version >= 90500: + time.sleep(1) + try: + pending_restart = self._postgresql.query('SELECT COUNT(*) FROM pg_catalog.pg_settings' + ' WHERE pending_restart').fetchone()[0] > 0 + self._postgresql.set_pending_restart(pending_restart) + except Exception as e: + logger.warning('Exception %r when running query', e) + else: logger.info('No PostgreSQL configuration items changed, nothing to reload.') def set_synchronous_standby(self, name): diff --git a/tests/__init__.py b/tests/__init__.py index 58ec0505..14d9d4a5 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -106,6 +106,9 @@ class MockCursor(object): self.results = [('', 0, '', '', '', '', False, replication_info)] elif sql.startswith('SELECT name, setting'): self.results = [('wal_segment_size', '2048', '8kB', 'integer', 'internal'), + ('wal_block_size', '8192', None, 'integer', 'internal'), + ('shared_buffers', '16384', '8kB', 'integer', 'postmaster'), + ('wal_buffers', '-1', '8kB', 'integer', 'postmaster'), ('search_path', 'public', None, 'string', 'user'), ('port', '5433', None, 'integer', 'postmaster'), ('listen_addresses', '*', None, 'string', 'postmaster'), diff --git a/tests/test_api.py b/tests/test_api.py index 327dbf8e..8aae6e0a 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -233,11 +233,8 @@ class TestRestApiHandler(unittest.TestCase): mock_dcs.get_cluster.return_value.config = ClusterConfig.from_node(1, config) MockRestApiServer(RestApiHandler, request) - @patch.object(MockPatroni, 'sighup_handler', Mock(side_effect=Exception)) + @patch.object(MockPatroni, 'sighup_handler', Mock()) def test_do_POST_reload(self): - with patch.object(MockPatroni, 'config') as mock_config: - mock_config.reload_local_configuration.return_value = False - MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0' + self._authorization) self.assertIsNotNone(MockRestApiServer(RestApiHandler, 'POST /reload HTTP/1.0' + self._authorization)) @patch.object(MockPatroni, 'dcs') diff --git a/tests/test_config.py b/tests/test_config.py index 357a227b..a60046f2 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -70,8 +70,7 @@ class TestConfig(unittest.TestCase): config = Config() with patch.object(Config, '_load_config_file', Mock(return_value={'restapi': {}})): with patch.object(Config, '_build_effective_configuration', Mock(side_effect=Exception)): - self.assertRaises(Exception, config.reload_local_configuration, True) - self.assertTrue(config.reload_local_configuration(True)) + config.reload_local_configuration() self.assertTrue(config.reload_local_configuration()) self.assertIsNone(config.reload_local_configuration()) diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 36b0c42b..ab72484f 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -125,6 +125,9 @@ class TestPatroni(unittest.TestCase): self.p.logger.start = Mock() self.p.config._dynamic_configuration = {} self.assertRaises(SleepException, self.p.run) + with patch('patroni.config.Config.reload_local_configuration', Mock(return_value=False)): + self.p.sighup_handler() + self.assertRaises(SleepException, self.p.run) with patch('patroni.config.Config.set_dynamic_configuration', Mock(return_value=True)): self.assertRaises(SleepException, self.p.run) with patch('patroni.postgresql.Postgresql.data_directory_empty', Mock(return_value=False)): diff --git a/tests/test_postgresql.py b/tests/test_postgresql.py index 343b2e10..b4116684 100644 --- a/tests/test_postgresql.py +++ b/tests/test_postgresql.py @@ -424,13 +424,18 @@ class TestPostgresql(BaseTestPostgresql): self.p.config._config['foo'] = {'command': 'bar'} self.assertFalse(self.p.replica_method_can_work_without_replication_connection('foo')) + @patch('time.sleep', Mock()) @patch.object(Postgresql, 'is_running', Mock(return_value=True)) - def test_reload_config(self): + @patch.object(MockCursor, 'fetchone') + def test_reload_config(self, mock_fetchone): + mock_fetchone.return_value = (1,) parameters = self._PARAMETERS.copy() parameters.pop('f.oo') + parameters['wal_buffers'] = '512' config = {'pg_hba': [''], 'pg_ident': [''], 'use_unix_socket': True, 'authentication': {}, 'retry_timeout': 10, 'listen': '*', 'krbsrvname': 'postgres', 'parameters': parameters} self.p.reload_config(config) + mock_fetchone.side_effect = Exception parameters['b.ar'] = 'bar' self.p.reload_config(config) parameters['autovacuum'] = 'on'