From 8acefefc4238b9d32cda2e8670ba53e212837c7c Mon Sep 17 00:00:00 2001 From: zhjwpku Date: Fri, 29 Dec 2023 16:01:46 +0800 Subject: [PATCH 1/4] Fix Citus bootstrap - CREATE DATABASE cannot be executed from a function (#2994) This was introduced by #2990: pod cannot be started and show the following logs: ``` 2023-12-26 03:29:25.569 UTC [47] CONTEXT: SQL statement "CREATE DATABASE "citus"" PL/pgSQL function inline_code_block line 5 at SQL statement 2023-12-26 03:29:25.569 UTC [47] STATEMENT: DO $$ BEGIN PERFORM * FROM pg_catalog.pg_database WHERE datname = 'citus'; IF NOT FOUND THEN CREATE DATABASE "citus"; END IF; END;$$ 2023-12-26 03:29:25,570 ERROR: post_bootstrap Traceback (most recent call last): File "/usr/local/lib/python3.11/dist-packages/patroni/postgresql/bootstrap.py", line 474, in post_bootstrap self._postgresql.citus_handler.bootstrap() File "/usr/local/lib/python3.11/dist-packages/patroni/postgresql/mpp/citus.py", line 401, in bootstrap cur.execute(sql.encode('utf-8')) psycopg2.errors.ActiveSqlTransaction: CREATE DATABASE cannot be executed from a function CONTEXT: SQL statement "CREATE DATABASE "citus"" PL/pgSQL function inline_code_block line 5 at SQL statement ``` --------- Signed-off-by: Zhao Junwang --- patroni/postgresql/mpp/citus.py | 15 +++++---------- patroni/psycopg.py | 5 ++++- tests/__init__.py | 2 ++ tests/test_citus.py | 8 ++++++++ 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/patroni/postgresql/mpp/citus.py b/patroni/postgresql/mpp/citus.py index b8c205ce..f3d6394c 100644 --- a/patroni/postgresql/mpp/citus.py +++ b/patroni/postgresql/mpp/citus.py @@ -8,7 +8,7 @@ from typing import Any, Dict, List, Optional, Union, Tuple, TYPE_CHECKING from . import AbstractMPP, AbstractMPPHandler from ...dcs import Cluster -from ...psycopg import connect, quote_ident, quote_literal +from ...psycopg import connect, quote_ident, DuplicateDatabase from ...utils import parse_int if TYPE_CHECKING: # pragma: no cover @@ -389,16 +389,11 @@ class CitusHandler(Citus, AbstractMPPHandler, Thread): if self._config['database'] != self._postgresql.database: conn = connect(**conn_kwargs) try: - database = self._config['database'] - sql = """DO $$ -BEGIN - PERFORM * FROM pg_catalog.pg_database WHERE datname = {0}; - IF NOT FOUND THEN - CREATE DATABASE {1}; - END IF; -END;$$""".format(quote_literal(database), quote_ident(database, conn)) with conn.cursor() as cur: - cur.execute(sql.encode('utf-8')) + cur.execute('CREATE DATABASE {0}'.format( + quote_ident(self._config['database'], conn)).encode('utf-8')) + except DuplicateDatabase as e: + logger.debug('Exception when creating database: %r', e) finally: conn.close() diff --git a/patroni/psycopg.py b/patroni/psycopg.py index 4a92047c..5d47ad5c 100644 --- a/patroni/psycopg.py +++ b/patroni/psycopg.py @@ -9,7 +9,8 @@ if TYPE_CHECKING: # pragma: no cover from psycopg import Connection from psycopg2 import connection, cursor -__all__ = ['connect', 'quote_ident', 'quote_literal', 'DatabaseError', 'Error', 'OperationalError', 'ProgrammingError'] +__all__ = ['connect', 'quote_ident', 'quote_literal', 'DatabaseError', 'Error', 'OperationalError', 'ProgrammingError', + 'DuplicateDatabase'] _legacy = False try: @@ -18,6 +19,7 @@ try: if parse_version(__version__) < MIN_PSYCOPG2: raise ImportError from psycopg2 import connect as _connect, Error, DatabaseError, OperationalError, ProgrammingError + from psycopg2.errors import DuplicateDatabase from psycopg2.extensions import adapt try: @@ -43,6 +45,7 @@ try: return value.getquoted().decode('utf-8') except ImportError: from psycopg import connect as __connect, sql, Error, DatabaseError, OperationalError, ProgrammingError + from psycopg.errors import DuplicateDatabase def _connect(dsn: Optional[str] = None, **kwargs: Any) -> 'Connection[Any]': """Call :func:`psycopg.connect` with *dsn* and ``**kwargs``. diff --git a/tests/__init__.py b/tests/__init__.py index 986bd88b..2f3730f6 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -129,6 +129,8 @@ class MockCursor(object): sql = sql.decode('utf-8') if sql.startswith('blabla'): raise psycopg.ProgrammingError() + if sql.startswith('CREATE DATABASE'): + raise psycopg.DuplicateDatabase() elif sql == 'CHECKPOINT' or sql.startswith('SELECT pg_catalog.pg_create_'): raise psycopg.OperationalError() elif sql.startswith('RetryFailedError'): diff --git a/tests/test_citus.py b/tests/test_citus.py index dbf6d9cf..f1d8a020 100644 --- a/tests/test_citus.py +++ b/tests/test_citus.py @@ -157,3 +157,11 @@ class TestCitus(BaseTestPostgresql): 'type': 'logical', 'database': 'citus', 'plugin': 'pgoutput'})) self.assertTrue(self.c.ignore_replication_slot({'name': 'citus_shard_split_slot_1_2_3', 'type': 'logical', 'database': 'citus', 'plugin': 'citus'})) + + @patch('patroni.postgresql.mpp.citus.logger.debug') + @patch('patroni.postgresql.mpp.citus.connect', psycopg_connect) + @patch('patroni.postgresql.mpp.citus.quote_ident', Mock()) + def test_bootstrap_duplicate_database(self, mock_logger): + self.c.bootstrap() + mock_logger.assert_called_once() + self.assertTrue(mock_logger.call_args[0][0].startswith('Exception when creating database')) From 71ccf91e3672be7879f5e5d7317d13b58f87220f Mon Sep 17 00:00:00 2001 From: Polina Bungina <27892524+hughcapet@users.noreply.github.com> Date: Tue, 2 Jan 2024 11:30:18 +0300 Subject: [PATCH 2/4] Don't filter out contradictory nofailover tag (#2992) * Ensure that nofailover will always be used if both nofailover and failover_priority tags are provided * Call _validate_failover_tags from reload_local_configuration() as well * Properly check values in the _validate_failover_tags(): nofailover value should be casted to boolean like it is done when accessed in other places --- features/priority_failover.feature | 18 ++++++++ features/steps/basic_replication.py | 2 +- features/steps/patroni_api.py | 6 +++ patroni/config.py | 13 +++--- patroni/tags.py | 10 +++-- postgres0.yml | 2 +- postgres1.yml | 2 +- postgres2.yml | 2 +- tests/test_config.py | 68 +++++++++++++---------------- tests/test_patroni.py | 16 +++++++ 10 files changed, 89 insertions(+), 50 deletions(-) diff --git a/features/priority_failover.feature b/features/priority_failover.feature index b33dd045..acb1cb5a 100644 --- a/features/priority_failover.feature +++ b/features/priority_failover.feature @@ -21,3 +21,21 @@ Feature: priority replication And I sleep for 5 seconds Then postgres3 role is the primary after 10 seconds And there is one of ["postgres3 has equally tolerable WAL position and priority 2, while this node has priority 1","Wal position of postgres3 is ahead of my wal position"] INFO in the postgres2 patroni log after 5 seconds + + Scenario: check conflicting configuration handling + When I set nofailover tag in postgres2 config + And I issue an empty POST request to http://127.0.0.1:8010/reload + Then I receive a response code 202 + And there is one of ["Conflicting configuration between nofailover: True and failover_priority: 1. Defaulting to nofailover: True"] WARNING in the postgres2 patroni log after 5 seconds + And "members/postgres2" key in DCS has tags={'failover_priority': '1', 'nofailover': True} after 10 seconds + When I issue a POST request to http://127.0.0.1:8010/failover with {"candidate": "postgres2"} + Then I receive a response code 412 + And I receive a response text "failover is not possible: no good candidates have been found" + When I reset nofailover tag in postgres1 config + And I issue an empty POST request to http://127.0.0.1:8009/reload + Then I receive a response code 202 + And there is one of ["Conflicting configuration between nofailover: False and failover_priority: 0. Defaulting to nofailover: False"] WARNING in the postgres1 patroni log after 5 seconds + And "members/postgres1" key in DCS has tags={'failover_priority': '0', 'nofailover': False} after 10 seconds + And I issue a POST request to http://127.0.0.1:8010/failover with {"candidate": "postgres1"} + Then I receive a response code 200 + And postgres1 role is the primary after 10 seconds diff --git a/features/steps/basic_replication.py b/features/steps/basic_replication.py index d70c6d0e..f2db7110 100644 --- a/features/steps/basic_replication.py +++ b/features/steps/basic_replication.py @@ -123,6 +123,6 @@ def check_patroni_log(context, message_list, level, node, timeout): messsages_of_level = context.pctl.read_patroni_log(node, level) if any(any(message in line for line in messsages_of_level) for message in message_list): break - time.sleep(1) + sleep(1) else: assert False, f"There were none of {message_list} {level} in the {node} patroni log after {timeout} seconds" diff --git a/features/steps/patroni_api.py b/features/steps/patroni_api.py index 2c76d32d..74a7c0da 100644 --- a/features/steps/patroni_api.py +++ b/features/steps/patroni_api.py @@ -128,6 +128,12 @@ def scheduled_restart(context, url, in_seconds, data): context.execute_steps(u"""Given I issue a POST request to {0}/restart with {1}""".format(url, json.dumps(data))) +@step('I {action:w} {tag:w} tag in {pg_name:w} config') +def add_bool_tag_to_config(context, action, tag, pg_name): + value = action == 'set' + context.pctl.add_tag_to_config(pg_name, tag, value) + + @step('I add tag {tag:w} {value:w} to {pg_name:w} config') def add_tag_to_config(context, tag, value, pg_name): context.pctl.add_tag_to_config(pg_name, tag, value) diff --git a/patroni/config.py b/patroni/config.py index e523bc08..f7d648d5 100644 --- a/patroni/config.py +++ b/patroni/config.py @@ -142,10 +142,10 @@ class Config(object): self.__effective_configuration = self._build_effective_configuration({}, self._local_configuration) self._data_dir = self.__effective_configuration.get('postgresql', {}).get('data_dir', "") self._cache_file = os.path.join(self._data_dir, self.__CACHE_FILENAME) - if validator: # patronictl uses validator=None and we don't want to load anything from local cache in this case - self._load_cache() + if validator: # patronictl uses validator=None + self._load_cache() # we don't want to load anything from local cache for ctl + self._validate_failover_tags() # irrelevant for ctl self._cache_needs_saving = False - self._validate_failover_tags() @property def config_file(self) -> Optional[str]: @@ -356,6 +356,7 @@ class Config(object): new_configuration = self._build_effective_configuration(self._dynamic_configuration, configuration) self._local_configuration = configuration self.__effective_configuration = new_configuration + self._validate_failover_tags() return True else: logger.info('No local configuration items changed.') @@ -814,10 +815,12 @@ class Config(object): bedrock source of truth) """ tags = self.get('tags', {}) + if 'nofailover' not in tags: + return nofailover_tag = tags.get('nofailover') failover_priority_tag = parse_int(tags.get('failover_priority')) if failover_priority_tag is not None \ - and (nofailover_tag is True and failover_priority_tag > 0 - or nofailover_tag is False and failover_priority_tag <= 0): + and (bool(nofailover_tag) is True and failover_priority_tag > 0 + or bool(nofailover_tag) is False and failover_priority_tag <= 0): logger.warning('Conflicting configuration between nofailover: %s and failover_priority: %s. ' 'Defaulting to nofailover: %s', nofailover_tag, failover_priority_tag, nofailover_tag) diff --git a/patroni/tags.py b/patroni/tags.py index 998ff693..eedc9674 100644 --- a/patroni/tags.py +++ b/patroni/tags.py @@ -22,14 +22,18 @@ class Tags(abc.ABC): A custom tag is any tag added to the configuration ``tags`` section that is not one of ``clonefrom``, ``nofailover``, ``noloadbalance`` or ``nosync``. - For the Patroni predefined tags, the returning object will only contain them if they are enabled as they - all are boolean values that default to disabled. + For most of the Patroni predefined tags, the returning object will only contain them if they are enabled as + they all are boolean values that default to disabled. + However ``nofailover`` tag is always returned if ``failover_priority`` tag is defined. In this case, we need + both values to see if they are contradictory and the ``nofailover`` value should be used. :returns: a dictionary of tags set for this node. The key is the tag name, and the value is the corresponding tag value. """ return {tag: value for tag, value in tags.items() - if tag not in ('clonefrom', 'nofailover', 'noloadbalance', 'nosync') or value} + if any((tag not in ('clonefrom', 'nofailover', 'noloadbalance', 'nosync'), + value, + tag == 'nofailover' and 'failover_priority' in tags))} @property @abc.abstractmethod diff --git a/postgres0.yml b/postgres0.yml index 8a975156..84796a46 100644 --- a/postgres0.yml +++ b/postgres0.yml @@ -132,7 +132,7 @@ postgresql: # safety_margin: 5 tags: - nofailover: false + # failover_priority: 1 noloadbalance: false clonefrom: false nosync: false diff --git a/postgres1.yml b/postgres1.yml index 6ca2aa64..c86e8790 100644 --- a/postgres1.yml +++ b/postgres1.yml @@ -124,6 +124,6 @@ postgresql: #pre_promote: /path/to/pre_promote.sh tags: - nofailover: false + # failover_priority: 1 noloadbalance: false clonefrom: false diff --git a/postgres2.yml b/postgres2.yml index ee61a023..7384568e 100644 --- a/postgres2.yml +++ b/postgres2.yml @@ -114,7 +114,7 @@ postgresql: # krb_server_keyfile: /var/spool/keytabs/postgres unix_socket_directories: '..' # parent directory of data_dir tags: - nofailover: false + # failover_priority: 1 noloadbalance: false clonefrom: false # replicatefrom: postgresql1 diff --git a/tests/test_config.py b/tests/test_config.py index 7bf01f56..a02a33fd 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -159,48 +159,40 @@ class TestConfig(unittest.TestCase): @patch('patroni.config.logger') def test__validate_failover_tags(self, mock_logger, mock_get): """Ensures that only one of `nofailover` or `failover_priority` can be provided""" - mock_logger.warning.reset_mock() config = Config("postgres0.yml") + # Providing one of `nofailover` or `failover_priority` is fine - just_nofailover = {"nofailover": True} - mock_get.side_effect = [just_nofailover] * 2 - self.assertIsNone(config._validate_failover_tags()) - mock_logger.warning.assert_not_called() - just_failover_priority = {"failover_priority": 1} - mock_get.side_effect = [just_failover_priority] * 2 - self.assertIsNone(config._validate_failover_tags()) - mock_logger.warning.assert_not_called() + for single_param in ({"nofailover": True}, {"failover_priority": 1}, {"failover_priority": 0}): + mock_get.side_effect = [single_param] * 2 + self.assertIsNone(config._validate_failover_tags()) + mock_logger.warning.assert_not_called() + # Providing both `nofailover` and `failover_priority` is fine if consistent - consistent_false = {"nofailover": False, "failover_priority": 1} - mock_get.side_effect = [consistent_false] * 2 - self.assertIsNone(config._validate_failover_tags()) - mock_logger.warning.assert_not_called() - consistent_true = {"nofailover": True, "failover_priority": 0} - mock_get.side_effect = [consistent_true] * 2 - self.assertIsNone(config._validate_failover_tags()) - mock_logger.warning.assert_not_called() + for consistent_state in ( + {"nofailover": False, "failover_priority": 1}, + {"nofailover": True, "failover_priority": 0}, + {"nofailover": "False", "failover_priority": 0} + ): + mock_get.side_effect = [consistent_state] * 2 + self.assertIsNone(config._validate_failover_tags()) + mock_logger.warning.assert_not_called() + # Providing both inconsistently should log a warning - inconsistent_false = {"nofailover": False, "failover_priority": 0} - mock_get.side_effect = [inconsistent_false] * 2 - self.assertIsNone(config._validate_failover_tags()) - mock_logger.warning.assert_called_once_with( - 'Conflicting configuration between nofailover: %s and failover_priority: %s.' - + ' Defaulting to nofailover: %s', - False, - 0, - False - ) - mock_logger.warning.reset_mock() - inconsistent_true = {"nofailover": True, "failover_priority": 1} - mock_get.side_effect = [inconsistent_true] * 2 - self.assertIsNone(config._validate_failover_tags()) - mock_logger.warning.assert_called_once_with( - 'Conflicting configuration between nofailover: %s and failover_priority: %s.' - + ' Defaulting to nofailover: %s', - True, - 1, - True - ) + for inconsistent_state in ( + {"nofailover": False, "failover_priority": 0}, + {"nofailover": True, "failover_priority": 1}, + {"nofailover": "False", "failover_priority": 1}, + {"nofailover": "", "failover_priority": 0} + ): + mock_get.side_effect = [inconsistent_state] * 2 + self.assertIsNone(config._validate_failover_tags()) + mock_logger.warning.assert_called_once_with( + 'Conflicting configuration between nofailover: %s and failover_priority: %s.' + + ' Defaulting to nofailover: %s', + inconsistent_state['nofailover'], + inconsistent_state['failover_priority'], + inconsistent_state['nofailover']) + mock_logger.warning.reset_mock() def test__process_postgresql_parameters(self): expected_params = { diff --git a/tests/test_patroni.py b/tests/test_patroni.py index 8e08d406..2f8428b1 100644 --- a/tests/test_patroni.py +++ b/tests/test_patroni.py @@ -175,6 +175,20 @@ class TestPatroni(unittest.TestCase): self.p.next_run = time.time() - self.p.dcs.loop_wait - 1 self.p.schedule_next_run() + def test__filter_tags(self): + tags = {'noloadbalance': False, 'clonefrom': False, 'nosync': False, 'smth': 'random'} + self.assertEqual(self.p._filter_tags(tags), {'smth': 'random'}) + + tags['clonefrom'] = True + tags['smth'] = False + self.assertEqual(self.p._filter_tags(tags), {'clonefrom': True, 'smth': False}) + + tags = {'nofailover': False, 'failover_priority': 0} + self.assertEqual(self.p._filter_tags(tags), tags) + + tags = {'nofailover': True, 'failover_priority': 1} + self.assertEqual(self.p._filter_tags(tags), tags) + def test_noloadbalance(self): self.p.tags['noloadbalance'] = True self.assertTrue(self.p.noloadbalance) @@ -186,9 +200,11 @@ class TestPatroni(unittest.TestCase): # Setting `nofailover: True` has precedence (True, 0, True), (True, 1, True), + ('False', 1, True), # because we use bool() for the value # Similarly, setting `nofailover: False` has precedence (False, 0, False), (False, 1, False), + ('', 0, False), # Only when we have `nofailover: None` should we got based on priority (None, 0, True), (None, 1, False), From 3390ee9dea84ad00eed7fba901922dc4e25d9b53 Mon Sep 17 00:00:00 2001 From: Sophia Ruan <104968314+XiuhuaRuan@users.noreply.github.com> Date: Thu, 4 Jan 2024 19:30:03 +0800 Subject: [PATCH 3/4] call freeze_support in main module to solve pyinstaller frozen issue (#2996) Close #2995 --- patroni/__main__.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/patroni/__main__.py b/patroni/__main__.py index 2c253da4..c4c9a497 100644 --- a/patroni/__main__.py +++ b/patroni/__main__.py @@ -229,11 +229,6 @@ def patroni_main(configfile: str) -> None: :param configfile: path to Patroni configuration file. """ - from multiprocessing import freeze_support - - # Windows executables created by PyInstaller are frozen, thus we need to enable frozen support for - # :mod:`multiprocessing` to avoid :class:`RuntimeError` exceptions. - freeze_support() abstract_main(Patroni, configfile) @@ -335,6 +330,12 @@ def main() -> None: ``patroni`` daemon as another process. In that case relevant signals received by the main process and forwarded to ``patroni`` daemon process. """ + from multiprocessing import freeze_support + + # Executables created by PyInstaller are frozen, thus we need to enable frozen support for + # :mod:`multiprocessing` to avoid :class:`RuntimeError` exceptions. + freeze_support() + check_psycopg() args = process_arguments() From 4e5b2ee2491847ae8787c17f7d20bd3e7aacfad3 Mon Sep 17 00:00:00 2001 From: Israel Date: Thu, 4 Jan 2024 08:30:28 -0300 Subject: [PATCH 4/4] Close the doors for a possible future bug in the config generator (#3000) The `AbstractConfigGenerator._format_config` method was missing a comma in the declaration of a tuple. As a consequence it was concatenating the strings `ctl` and `citus` instead of creating two separate items in the tuple. There is currently no observed bug from that issue in the code because the template configuration created by the method `AbstractConfigGenerator.get_template_config` doesn't include either of `ctl` or `citus` keys. However, it is still important that we close the doors for possible future bugs that would come up if we ever attempt to use either of those keys in the template, for example. References: PAT-231. --- patroni/config_generator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/patroni/config_generator.py b/patroni/config_generator.py index c2b133c6..3593bea3 100644 --- a/patroni/config_generator.py +++ b/patroni/config_generator.py @@ -178,7 +178,7 @@ class AbstractConfigGenerator(abc.ABC): :yields: formatted lines or blocks that represent a text output of the YAML document. """ - for name in ('scope', 'namespace', 'name', 'log', 'restapi', 'ctl' 'citus', + for name in ('scope', 'namespace', 'name', 'log', 'restapi', 'ctl', 'citus', 'consul', 'etcd', 'etcd3', 'exhibitor', 'kubernetes', 'raft', 'zookeeper'): yield from self._format_config_section(name)