From 3d527f5728fa8c6fadc55284cf854eead6a30a9b Mon Sep 17 00:00:00 2001 From: Alexander Kukushkin Date: Wed, 25 Oct 2023 14:23:57 +0200 Subject: [PATCH] Improve formatting of generated config and validation of ints (#2928) - order sections similar to sample configs - add warnings and comments to `bootstrap.dcs` section. - add `tags` and `log` sections. - use discovered IPs in `postgresql.connect_address` and `postgresql.listen` - set `wal_level` to `replica` for PostgreSQL 9.6+ - make unit tests pass with python 3.6 - improve config validator so it doesn't complain when some ints are strings in YAML file. --- patroni/config_generator.py | 133 ++++++++++++++++++++++----------- patroni/validator.py | 59 ++++++++------- tests/test_config_generator.py | 60 ++++++++++----- 3 files changed, 161 insertions(+), 91 deletions(-) diff --git a/patroni/config_generator.py b/patroni/config_generator.py index 4bcb2819..c2b133c6 100644 --- a/patroni/config_generator.py +++ b/patroni/config_generator.py @@ -9,7 +9,7 @@ import yaml from getpass import getuser, getpass from contextlib import contextmanager -from typing import Any, Dict, Iterator, List, Optional, Tuple, TYPE_CHECKING, Union +from typing import Any, Dict, Iterator, List, Optional, TextIO, Tuple, TYPE_CHECKING, Union if TYPE_CHECKING: # pragma: no cover from psycopg import Cursor from psycopg2 import cursor @@ -17,6 +17,7 @@ if TYPE_CHECKING: # pragma: no cover from . import psycopg from .config import Config from .exceptions import PatroniException +from .log import PatroniLogger from .postgresql.config import ConfigHandler, parse_dsn from .postgresql.misc import postgres_major_version_to_int from .utils import get_major_version, parse_bool, patch_config, read_stripped @@ -93,10 +94,20 @@ class AbstractConfigGenerator(abc.ABC): template_config: Dict[str, Any] = { 'scope': NO_VALUE_MSG, 'name': cls._HOSTNAME, + 'restapi': { + 'connect_address': cls._IP + ':8008', + 'listen': cls._IP + ':8008' + }, + 'log': { + 'level': PatroniLogger.DEFAULT_LEVEL, + 'traceback_level': PatroniLogger.DEFAULT_TRACEBACK_LEVEL, + 'format': PatroniLogger.DEFAULT_FORMAT, + 'max_queue_size': PatroniLogger.DEFAULT_MAX_QUEUE_SIZE + }, 'postgresql': { 'data_dir': NO_VALUE_MSG, - 'connect_address': NO_VALUE_MSG + ':5432', - 'listen': NO_VALUE_MSG + ':5432', + 'connect_address': cls._IP + ':5432', + 'listen': cls._IP + ':5432', 'bin_dir': '', 'authentication': { 'superuser': { @@ -109,9 +120,11 @@ class AbstractConfigGenerator(abc.ABC): } } }, - 'restapi': { - 'connect_address': cls._IP + ':8008', - 'listen': cls._IP + ':8008' + 'tags': { + 'failover_priority': 1, + 'noloadbalance': False, + 'clonefrom': True, + 'nosync': False, } } @@ -130,6 +143,72 @@ class AbstractConfigGenerator(abc.ABC): def generate(self) -> None: """Generate config and store in :attr:`~AbstractConfigGenerator.config`.""" + @staticmethod + def _format_block(block: Any, line_prefix: str = '') -> str: + """Format a single YAML block. + + .. note:: + Optionally the formatted block could be indented with the *line_prefix* + + :param block: the object that should be formatted to YAML. + :param line_prefix: is used for indentation. + + :returns: a formatted and indented *block*. + """ + return line_prefix + yaml.safe_dump(block, default_flow_style=False, line_break='\n', + allow_unicode=True, indent=2).strip().replace('\n', '\n' + line_prefix) + + def _format_config_section(self, section_name: str) -> Iterator[str]: + """Format and yield as single section of the current :attr:`~AbstractConfigGenerator.config`. + + .. note:: + If the section is a :class:`dict` object we put an empty line before it. + + :param section_name: a section name in the :attr:`~AbstractConfigGenerator.config`. + + :yields: a formatted section in case if it exists in the :attr:`~AbstractConfigGenerator.config`. + """ + if section_name in self.config: + if isinstance(self.config[section_name], dict): + yield '' + yield self._format_block({section_name: self.config[section_name]}) + + def _format_config(self) -> Iterator[str]: + """Format current :attr:`~AbstractConfigGenerator.config` and enrich it with some comments. + + :yields: formatted lines or blocks that represent a text output of the YAML document. + """ + for name in ('scope', 'namespace', 'name', 'log', 'restapi', 'ctl' 'citus', + 'consul', 'etcd', 'etcd3', 'exhibitor', 'kubernetes', 'raft', 'zookeeper'): + yield from self._format_config_section(name) + + if 'bootstrap' in self.config: + yield '\n# The bootstrap configuration. Works only when the cluster is not yet initialized.' + yield '# If the cluster is already initialized, all changes in the `bootstrap` section are ignored!' + yield 'bootstrap:' + if 'dcs' in self.config['bootstrap']: + yield ' # This section will be written into :///config after initializing' + yield ' # new cluster and all other cluster members will use it as a `global configuration`.' + yield ' # WARNING! If you want to change any of the parameters that were set up' + yield ' # via `bootstrap.dcs` section, please use `patronictl edit-config`!' + yield ' dcs:' + for name in ('loop_wait', 'retry_timeout', 'ttl'): + if name in self.config['bootstrap']['dcs']: + yield self._format_block({name: self.config['bootstrap']['dcs'].pop(name)}, ' ') + + for name, value in self.config['bootstrap']['dcs'].items(): + yield self._format_block({name: value}, ' ') + + for name in ('postgresql', 'watchdog', 'tags'): + yield from self._format_config_section(name) + + def _write_config_to_fd(self, fd: TextIO) -> None: + """Format and write current :attr:`~AbstractConfigGenerator.config` to provided file descriptor. + + :param fd: where to write the config file. Could be ``sys.stdout`` or the real file. + """ + fd.write('\n'.join(self._format_config())) + def write_config(self) -> None: """Write current :attr:`~AbstractConfigGenerator.config` to the output file if provided, to stdout otherwise.""" if self.output_file: @@ -137,9 +216,9 @@ class AbstractConfigGenerator(abc.ABC): if dir_path and not os.path.isdir(dir_path): os.makedirs(dir_path) with open(self.output_file, 'w', encoding='UTF-8') as output_file: - yaml.safe_dump(self.config, output_file, default_flow_style=False, allow_unicode=True) + self._write_config_to_fd(output_file) else: - yaml.safe_dump(self.config, sys.stdout, default_flow_style=False, allow_unicode=True) + self._write_config_to_fd(sys.stdout) class SampleConfigGenerator(AbstractConfigGenerator): @@ -182,6 +261,9 @@ class SampleConfigGenerator(AbstractConfigGenerator): self.config['bootstrap']['dcs']['postgresql']['parameters'][wal_keep_param] = \ ConfigHandler.CMDLINE_OPTIONS[wal_keep_param][0] + wal_level = 'hot_standby' if self.pg_major < 90600 else 'replica' + self.config['bootstrap']['dcs']['postgresql']['parameters']['wal_level'] = wal_level + self.config['bootstrap']['dcs']['postgresql']['use_pg_rewind'] = True if self.pg_major >= 110000: self.config['postgresql']['authentication'].setdefault( @@ -411,41 +493,6 @@ class RunningClusterConfigGenerator(AbstractConfigGenerator): def generate_config(output_file: str, sample: bool, dsn: Optional[str]) -> None: """Generate Patroni configuration file. - Gather all the available non-internal GUC values having configuration file, postmaster command line or environment - variable as a source and store them in the appropriate part of Patroni configuration (``postgresql.parameters`` or - ``bootstrap.dcs.postgresql.parameters``). Either the provided DSN (takes precedence) or PG ENV vars will be used - for the connection. If password is not provided, it should be entered via prompt. - - The created configuration contains: - * ``scope``: ``cluster_name`` GUC value or ``PATRONI_SCOPE ENV`` variable value if available. - * ``name``: ``PATRONI_NAME`` ENV variable value if set, otherwise hostname. - - * ``bootstrap.dcs``: section with all the parameters (incl. the majority of PG GUCs) set to their default values - defined by Patroni and adjusted by the source instances's configuration values. - - * ``postgresql.parameters``: the source instance's ``archive_command``, ``restore_command``, - ``archive_cleanup_command``, ``recovery_end_command``, ``ssl_passphrase_command``, ``hba_file``, ``ident_file``, - ``config_file`` GUC values. - - * ``postgresql.bin_dir``: path to Postgres binaries gathered from the running instance or, if not available, - the value of ``PATRONI_POSTGRESQL_BIN_DIR`` ENV variable. Otherwise, an empty string. - - * ``postgresql.datadir``: the value gathered from the corresponding PG GUC. - * ``postgresql.listen``: source instance's ``listen_addresses`` and port GUC values. - * ``postgresql.connect_address``: if possible, generated from the connection params. - * ``postgresql.authentication``: - - * superuser and replication users defined (if possible, usernames are set from the respective Patroni ENV vars, - otherwise the default ``postgres`` and ``replicator`` values are used). - If not a sample config, either DSN or PG ENV vars are used to define superuser authentication parameters. - - * rewind user is defined only for sample config, if PG version can be defined and PG version is >=11 - (if possible, username is set from the respective Patroni ENV var). - - * ``bootstrap.dcs.postgresql.use_pg_rewind`` set to ``True`` for a sample config only. - * ``postgresql.pg_hba`` defaults or the lines gathered from the source instance's ``hba_file``. - * ``postgresql.pg_ident`` the lines gathered from the source instance's ``ident_file``. - :param output_file: Full path to the configuration file to be used. If not provided, result is sent to ``stdout``. :param sample: Optional flag. If set, no source instance will be used - generate config with some sane defaults. :param dsn: Optional DSN string for the local instance to get GUC values from. diff --git a/patroni/validator.py b/patroni/validator.py index 122b795e..10bc7b2a 100644 --- a/patroni/validator.py +++ b/patroni/validator.py @@ -814,27 +814,28 @@ def assert_(condition: bool, message: str = "Wrong value") -> None: class IntValidator(object): """Validate an integer setting. - :cvar expected_type: the expected Python type for an integer setting (:class:`int`). :ivar min: minimum allowed value for the setting, if any. :ivar max: maximum allowed value for the setting, if any. :ivar base_unit: the base unit to convert the value to before checking if it's within *min* and *max* range. + :ivar expected_type: the expected Python type. :ivar raise_assert: if an ``assert`` test should be performed regarding expected type and valid range. """ - expected_type = int - def __init__(self, min: OptionalType[int] = None, max: OptionalType[int] = None, - base_unit: OptionalType[str] = None, raise_assert: bool = False) -> None: + base_unit: OptionalType[str] = None, expected_type: Any = None, raise_assert: bool = False) -> None: """Create an :class:`IntValidator` object with the given rules. :param min: minimum allowed value for the setting, if any. :param max: maximum allowed value for the setting, if any. :param base_unit: the base unit to convert the value to before checking if it's within *min* and *max* range. + :param expected_type: the expected Python type. :param raise_assert: if an ``assert`` test should be performed regarding expected type and valid range. """ self.min = min self.max = max self.base_unit = base_unit + if expected_type: + self.expected_type = expected_type self.raise_assert = raise_assert def __call__(self, value: Any) -> bool: @@ -953,36 +954,36 @@ schema = Schema({ Optional("allowlist_include_members"): bool, Optional("http_extra_headers"): dict, Optional("https_extra_headers"): dict, - Optional("request_queue_size"): IntValidator(min=0, max=4096, raise_assert=True) + Optional("request_queue_size"): IntValidator(min=0, max=4096, expected_type=int, raise_assert=True) }, Optional("bootstrap"): { "dcs": { - Optional("ttl"): int, - Optional("loop_wait"): int, - Optional("retry_timeout"): int, - Optional("maximum_lag_on_failover"): int, - Optional("maximum_lag_on_syncnode"): int, + Optional("ttl"): IntValidator(min=20, raise_assert=True), + Optional("loop_wait"): IntValidator(min=1, raise_assert=True), + Optional("retry_timeout"): IntValidator(min=3, raise_assert=True), + Optional("maximum_lag_on_failover"): IntValidator(min=0, raise_assert=True), + Optional("maximum_lag_on_syncnode"): IntValidator(min=-1, raise_assert=True), Optional("postgresql"): { Optional("parameters"): { - Optional("max_connections"): int, - Optional("max_locks_per_transaction"): int, - Optional("max_prepared_transactions"): int, - Optional("max_replication_slots"): int, - Optional("max_wal_senders"): int, - Optional("max_worker_processes"): int + Optional("max_connections"): IntValidator(1, 262143, raise_assert=True), + Optional("max_locks_per_transaction"): IntValidator(10, 2147483647, raise_assert=True), + Optional("max_prepared_transactions"): IntValidator(0, 262143, raise_assert=True), + Optional("max_replication_slots"): IntValidator(0, 262143, raise_assert=True), + Optional("max_wal_senders"): IntValidator(0, 262143, raise_assert=True), + Optional("max_worker_processes"): IntValidator(0, 262143, raise_assert=True), }, Optional("use_pg_rewind"): bool, Optional("pg_hba"): [str], Optional("pg_ident"): [str], - Optional("pg_ctl_timeout"): int, + Optional("pg_ctl_timeout"): IntValidator(min=0, raise_assert=True), Optional("use_slots"): bool, }, - Optional("primary_start_timeout"): int, - Optional("primary_stop_timeout"): int, + Optional("primary_start_timeout"): IntValidator(min=0, raise_assert=True), + Optional("primary_stop_timeout"): IntValidator(min=0, raise_assert=True), Optional("standby_cluster"): { Or("host", "port", "restore_command"): Case({ "host": str, - "port": int, + "port": IntValidator(max=65535, expected_type=int, raise_assert=True), "restore_command": str }), Optional("primary_slot_name"): str, @@ -992,7 +993,7 @@ schema = Schema({ }, Optional("synchronous_mode"): bool, Optional("synchronous_mode_strict"): bool, - Optional("synchronous_node_count"): int + Optional("synchronous_node_count"): IntValidator(min=1, raise_assert=True), }, Optional("initdb"): [Or(str, dict)], Optional("method"): str @@ -1003,7 +1004,7 @@ schema = Schema({ "host": validate_host_port, "url": str }), - Optional("port"): int, + Optional("port"): IntValidator(max=65535, expected_type=int, raise_assert=True), Optional("scheme"): str, Optional("token"): str, Optional("verify"): bool, @@ -1023,8 +1024,8 @@ schema = Schema({ "etcd3": validate_etcd, "exhibitor": { "hosts": [str], - "port": IntValidator(max=65535, raise_assert=True), - Optional("pool_interval"): int + "port": IntValidator(max=65535, expected_type=int, raise_assert=True), + Optional("poll_interval"): IntValidator(min=1, expected_type=int, raise_assert=True), }, "raft": { "self_addr": validate_connect_address, @@ -1055,14 +1056,14 @@ schema = Schema({ Optional("tmp_role_label"): str, Optional("use_endpoints"): bool, Optional("pod_ip"): Or(is_ipv4_address, is_ipv6_address), - Optional("ports"): [{"name": str, "port": int}], + Optional("ports"): [{"name": str, "port": IntValidator(max=65535, expected_type=int, raise_assert=True)}], Optional("cacert"): str, Optional("retriable_http_codes"): Or(int, [int]), }, }), Optional("citus"): { "database": str, - "group": int + "group": IntValidator(min=0, expected_type=int, raise_assert=True), }, "postgresql": { "listen": validate_host_port_listen_multiple_hosts, @@ -1089,18 +1090,18 @@ schema = Schema({ }, Optional("pg_hba"): [str], Optional("pg_ident"): [str], - Optional("pg_ctl_timeout"): int, + Optional("pg_ctl_timeout"): IntValidator(min=0, raise_assert=True), Optional("use_pg_rewind"): bool }, Optional("watchdog"): { Optional("mode"): validate_watchdog_mode, Optional("device"): str, - Optional("safety_margin"): int + Optional("safety_margin"): IntValidator(min=-1, expected_type=int, raise_assert=True), }, Optional("tags"): { AtMostOne("nofailover", "failover_priority"): Case({ "nofailover": bool, - "failover_priority": IntValidator(min=0, raise_assert=True), + "failover_priority": IntValidator(min=0, expected_type=int, raise_assert=True), }), Optional("clonefrom"): bool, Optional("noloadbalance"): bool, diff --git a/tests/test_config_generator.py b/tests/test_config_generator.py index e702f957..1b436bb0 100644 --- a/tests/test_config_generator.py +++ b/tests/test_config_generator.py @@ -1,14 +1,16 @@ import os import psutil import unittest +import yaml from . import MockConnect, MockCursor, MockConnectionInfo from copy import deepcopy -from mock import MagicMock, Mock, PropertyMock, mock_open, patch +from mock import MagicMock, Mock, PropertyMock, mock_open as _mock_open, patch from patroni.__main__ import main as _main from patroni.config import Config from patroni.config_generator import AbstractConfigGenerator, get_address, NO_VALUE_MSG +from patroni.log import PatroniLogger from patroni.utils import patch_config from . import psycopg_connect @@ -17,6 +19,14 @@ HOSTNAME = 'test_hostname' IP = '1.9.8.4' +def mock_open(*args, **kwargs): + ret = _mock_open(*args, **kwargs) + ret.return_value.__iter__ = lambda o: iter(o.readline, '') + if not kwargs.get('read_data'): + ret.return_value.readline = Mock(return_value=None) + return ret + + @patch('patroni.psycopg.connect', psycopg_connect) @patch('builtins.open', MagicMock()) @patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 16.2")) @@ -51,13 +61,23 @@ class TestGenerateConfig(unittest.TestCase): self.config = { 'scope': self.environ['PATRONI_SCOPE'], 'name': HOSTNAME, + 'log': { + 'level': PatroniLogger.DEFAULT_LEVEL, + 'traceback_level': PatroniLogger.DEFAULT_TRACEBACK_LEVEL, + 'format': PatroniLogger.DEFAULT_FORMAT, + 'max_queue_size': PatroniLogger.DEFAULT_MAX_QUEUE_SIZE + }, + 'restapi': { + 'connect_address': self.environ['PATRONI_RESTAPI_CONNECT_ADDRESS'], + 'listen': self.environ['PATRONI_RESTAPI_LISTEN'] + }, 'bootstrap': { 'dcs': dynamic_config }, 'postgresql': { - 'connect_address': NO_VALUE_MSG + ':5432', + 'connect_address': IP + ':5432', 'data_dir': NO_VALUE_MSG, - 'listen': NO_VALUE_MSG + ':5432', + 'listen': IP + ':5432', 'pg_hba': ['host all all all md5', f'host replication {self.environ["PATRONI_REPLICATION_USERNAME"]} all md5'], 'authentication': {'superuser': {'username': self.environ['PATRONI_SUPERUSER_USERNAME'], @@ -68,10 +88,6 @@ class TestGenerateConfig(unittest.TestCase): 'bin_dir': self.environ['PATRONI_POSTGRESQL_BIN_DIR'], 'bin_name': {'postgres': self.environ['PATRONI_POSTGRESQL_BIN_POSTGRES']}, 'parameters': {'password_encryption': 'md5'} - }, - 'restapi': { - 'connect_address': self.environ['PATRONI_RESTAPI_CONNECT_ADDRESS'], - 'listen': self.environ['PATRONI_RESTAPI_LISTEN'] } } @@ -119,6 +135,12 @@ class TestGenerateConfig(unittest.TestCase): }, 'rewind': None }, + }, + 'tags': { + 'failover_priority': 1, + 'noloadbalance': False, + 'clonefrom': True, + 'nosync': False, } } patch_config(self.config, conf) @@ -139,22 +161,21 @@ class TestGenerateConfig(unittest.TestCase): ] @patch('os.makedirs') - @patch('yaml.safe_dump') - def test_generate_sample_config_pre_13_dir_creation(self, mock_config_dump, mock_makedir): + def test_generate_sample_config_pre_13_dir_creation(self, mock_makedir): with patch('sys.argv', ['patroni.py', '--generate-sample-config', '/foo/bar.yml']), \ patch('subprocess.check_output', Mock(return_value=b"postgres (PostgreSQL) 9.4.3")) as pg_bin_mock, \ + patch('builtins.open', _mock_open()) as mocked_file, \ self.assertRaises(SystemExit) as e: _main() + self.assertEqual(self.config, yaml.safe_load(mocked_file().write.call_args_list[0][0][0])) self.assertEqual(e.exception.code, 0) - self.assertEqual(self.config, mock_config_dump.call_args[0][0]) mock_makedir.assert_called_once() pg_bin_mock.assert_called_once_with([os.path.join(self.environ['PATRONI_POSTGRESQL_BIN_DIR'], self.environ['PATRONI_POSTGRESQL_BIN_POSTGRES']), '--version']) @patch('os.makedirs', Mock()) - @patch('yaml.safe_dump') - def test_generate_sample_config_16(self, mock_config_dump): + def test_generate_sample_config_16(self): conf = { 'bootstrap': { 'dcs': { @@ -182,14 +203,15 @@ class TestGenerateConfig(unittest.TestCase): patch_config(self.config, conf) with patch('sys.argv', ['patroni.py', '--generate-sample-config', '/foo/bar.yml']), \ + patch('builtins.open', _mock_open()) as mocked_file, \ self.assertRaises(SystemExit) as e: _main() + self.assertEqual(self.config, yaml.safe_load(mocked_file().write.call_args_list[0][0][0])) self.assertEqual(e.exception.code, 0) - self.assertEqual(self.config, mock_config_dump.call_args[0][0]) @patch('os.makedirs', Mock()) - @patch('yaml.safe_dump') - def test_generate_config_running_instance_16(self, mock_config_dump): + @patch('sys.stdout') + def test_generate_config_running_instance_16(self, mock_sys_stdout): self._set_running_instance_config_vals() with patch('builtins.open', Mock(side_effect=self._get_running_instance_open_res())), \ @@ -198,11 +220,11 @@ class TestGenerateConfig(unittest.TestCase): self.assertRaises(SystemExit) as e: _main() self.assertEqual(e.exception.code, 0) - self.assertEqual(self.config, mock_config_dump.call_args[0][0]) + self.assertEqual(self.config, yaml.safe_load(mock_sys_stdout.write.call_args_list[0][0][0])) @patch('os.makedirs', Mock()) - @patch('yaml.safe_dump') - def test_generate_config_running_instance_16_connect_from_env(self, mock_config_dump): + @patch('sys.stdout') + def test_generate_config_running_instance_16_connect_from_env(self, mock_sys_stdout): self._set_running_instance_config_vals() # su auth params and connect host from env os.environ['PGCHANNELBINDING'] = \ @@ -245,7 +267,7 @@ class TestGenerateConfig(unittest.TestCase): self.assertRaises(SystemExit) as e: _main() self.assertEqual(e.exception.code, 0) - self.assertEqual(self.config, mock_config_dump.call_args[0][0]) + self.assertEqual(self.config, yaml.safe_load(mock_sys_stdout.write.call_args_list[0][0][0])) def test_generate_config_running_instance_errors(self): # 1. Wrong DSN format