Repository: zalando/patroni
Pull request #2992 (2023-12-21 15:13:50)

Not filter out contradictory nofailover tag
* 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 



coveralls: ## Pull Request Test Coverage Report for [Build 7289956429](https://coveralls.io/builds/64711583)


* **6** of **6**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at **99.859%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/64711583/badge)](https://coveralls.io/builds/64711583) |
| :-- | --: |
| Change from base [Build 7285720031](https://coveralls.io/builds/64703697): |  0.0% |
| Covered Lines: | 13460 |
| Relevant Lines: | 13479 |

---
##### 💛  - [Coveralls](https://coveralls.io)


Patch:
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/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/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


Test patch:
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'))
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),