Add behave test

features/priority_failover.feature

@@ -21,3 +21,14 @@ 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
+    When I issue a GET request to http://127.0.0.1:8010/patroni
+    Then I receive a response tags {'nofailover': True}
+    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"

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"

features/steps/patroni_api.py

@@ -128,6 +128,11 @@ 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 set {tag:w} tag in {pg_name:w} config')
+def add_bool_tag_to_config(context, tag, pg_name):
+    context.pctl.add_tag_to_config(pg_name, tag, True)
+
+
 @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)

patroni/config.py

@@ -145,7 +145,6 @@ class Config(object):
         if validator:  # patronictl uses validator=None and we don't want to load anything from local cache in this case
             self._load_cache()
         self._cache_needs_saving = False
-        self._validate_failover_tags()
 
     @property
     def config_file(self) -> Optional[str]:
@@ -746,14 +745,11 @@ class Config(object):
             dcs = bootstrap.setdefault('dcs', {})
             dcs.setdefault('synchronous_mode', True)
 
-        updated_fields = (
+        if 'tags' in config:
-            'name',
+            self._validate_failover_tags(config['tags'])
-            'scope',
-            'retry_timeout',
-            'citus'
-        )
 
-        pg_config.update({p: config[p] for p in updated_fields if p in config})
+        # Add params required inside Postgresql class to PG config
+        pg_config.update({p: config[p] for p in ('name', 'scope', 'retry_timeout', 'citus') if p in config})
 
         return config
 
@@ -801,8 +797,11 @@ class Config(object):
         """
         return deepcopy(self.__effective_configuration)
 
-    def _validate_failover_tags(self) -> None:
+    @staticmethod
-        """Check ``nofailover``/``failover_priority`` config and warn user if it's contradictory.
+    def _validate_failover_tags(tags_config: Dict[str, Any]) -> None:
+        """Check ``nofailover``/``failover_priority`` config, remove contradictory tag and warn user.
+
+        :param tags_config: dictionary representing values under the ``tags`` configuration section.
 
         .. note::
           To preserve sanity (and backwards compatibility) the ``nofailover`` tag will still exist. A contradictory
@@ -813,11 +812,11 @@ class Config(object):
           The behaviour is as if ``failover_priority`` were not provided (i.e ``nofailover`` is the
           bedrock source of truth)
         """
-        tags = self.get('tags', {})
+        nofailover_tag = tags_config.get('nofailover')
-        nofailover_tag = tags.get('nofailover')
+        failover_priority_tag = parse_int(tags_config.get('failover_priority'))
-        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):
             logger.warning('Conflicting configuration between nofailover: %s and failover_priority: %s. '
                            'Defaulting to nofailover: %s', nofailover_tag, failover_priority_tag, nofailover_tag)
+            tags_config.pop('failover_priority')

postgres0.yml

@@ -132,7 +132,7 @@ postgresql:
 #  safety_margin: 5
 
 tags:
-    nofailover: false
+    # failover_priority: 1
     noloadbalance: false
     clonefrom: false
     nosync: false

postgres1.yml

@@ -124,6 +124,6 @@ postgresql:
   #pre_promote: /path/to/pre_promote.sh
 
 tags:
-    nofailover: false
+    # failover_priority: 1
     noloadbalance: false
     clonefrom: false

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

tests/test_config.py

@@ -155,52 +155,39 @@ class TestConfig(unittest.TestCase):
     def test_invalid_path(self):
         self.assertRaises(ConfigParseError, Config, 'postgres0')
 
-    @patch.object(Config, 'get')
     @patch('patroni.config.logger')
-    def test__validate_failover_tags(self, mock_logger, mock_get):
+    def test__validate_failover_tags(self, mock_logger):
         """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}
+        for tags_config in [{"nofailover": True}, {"failover_priority": 1}]:
-        mock_get.side_effect = [just_nofailover] * 2
+            self.assertIsNone(Config._validate_failover_tags(tags_config))
-        self.assertIsNone(config._validate_failover_tags())
+            mock_logger.warning.assert_not_called()
-        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()
         # Providing both `nofailover` and `failover_priority` is fine if consistent
-        consistent_false = {"nofailover": False, "failover_priority": 1}
+        for tags_config in [
-        mock_get.side_effect = [consistent_false] * 2
+                {"nofailover": False, "failover_priority": 1},
-        self.assertIsNone(config._validate_failover_tags())
+                {"nofailover": True, "failover_priority": 0}]:
-        mock_logger.warning.assert_not_called()
+            self.assertIsNone(Config._validate_failover_tags(tags_config))
-        consistent_true = {"nofailover": True, "failover_priority": 0}
+            self.assertIn('nofailover', tags_config)
-        mock_get.side_effect = [consistent_true] * 2
+            self.assertIn('failover_priority', tags_config)
-        self.assertIsNone(config._validate_failover_tags())
+            mock_logger.warning.assert_not_called()
-        mock_logger.warning.assert_not_called()
+
         # Providing both inconsistently should log a warning
-        inconsistent_false = {"nofailover": False, "failover_priority": 0}
+        for tags_config in [
-        mock_get.side_effect = [inconsistent_false] * 2
+                {"nofailover": False, "failover_priority": 0},
-        self.assertIsNone(config._validate_failover_tags())
+                {"nofailover": True, "failover_priority": 1}]:
-        mock_logger.warning.assert_called_once_with(
+            initial_config = tags_config.copy()
-            'Conflicting configuration between nofailover: %s and failover_priority: %s.'
+            self.assertIsNone(Config._validate_failover_tags(tags_config))
-            + ' Defaulting to nofailover: %s',
+            self.assertIn('nofailover', tags_config)
-            False,
+            self.assertNotIn('failover_priority', tags_config)
-            0,
+            mock_logger.warning.assert_called_once_with(
-            False
+                'Conflicting configuration between nofailover: %s and failover_priority: %s.'
-        )
+                + ' Defaulting to nofailover: %s',
-        mock_logger.warning.reset_mock()
+                initial_config['nofailover'],
-        inconsistent_true = {"nofailover": True, "failover_priority": 1}
+                initial_config['failover_priority'],
-        mock_get.side_effect = [inconsistent_true] * 2
+                initial_config['nofailover']
-        self.assertIsNone(config._validate_failover_tags())
+            )
-        mock_logger.warning.assert_called_once_with(
+            mock_logger.warning.reset_mock()
-            'Conflicting configuration between nofailover: %s and failover_priority: %s.'
-            + ' Defaulting to nofailover: %s',
-            True,
-            1,
-            True
-        )
 
     def test__process_postgresql_parameters(self):
         expected_params = {