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'