diff --git a/patroni/config.py b/patroni/config.py index e523bc08..78db1801 100644 --- a/patroni/config.py +++ b/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 = ( - 'name', - 'scope', - 'retry_timeout', - 'citus' - ) + if 'tags' in config: + self._validate_failover_tags(config['tags']) - 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: - """Check ``nofailover``/``failover_priority`` config and warn user if it's contradictory. + @staticmethod + 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.get('nofailover') - failover_priority_tag = parse_int(tags.get('failover_priority')) + nofailover_tag = tags_config.get('nofailover') + failover_priority_tag = parse_int(tags_config.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') diff --git a/tests/test_config.py b/tests/test_config.py index 7bf01f56..1b95af9b 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -155,34 +155,36 @@ 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} - mock_get.side_effect = [just_nofailover] * 2 - self.assertIsNone(config._validate_failover_tags()) + config = {"nofailover": True} + self.assertIsNone(Config._validate_failover_tags(config)) 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()) + + config = {"failover_priority": 1} + self.assertIsNone(Config._validate_failover_tags(config)) 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()) + config = {"nofailover": False, "failover_priority": 1} + self.assertIsNone(Config._validate_failover_tags(config)) + self.assertIn('nofailover', config) + self.assertIn('failover_priority', config) 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()) + + config = {"nofailover": True, "failover_priority": 0} + self.assertIsNone(Config._validate_failover_tags(config)) + self.assertIn('nofailover', config) + self.assertIn('failover_priority', config) 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()) + config = {"nofailover": False, "failover_priority": 0} + self.assertIsNone(Config._validate_failover_tags(config)) + self.assertIn('nofailover', config) + self.assertNotIn('failover_priority', config) mock_logger.warning.assert_called_once_with( 'Conflicting configuration between nofailover: %s and failover_priority: %s.' + ' Defaulting to nofailover: %s', @@ -191,9 +193,11 @@ class TestConfig(unittest.TestCase): 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()) + + config = {"nofailover": True, "failover_priority": 1} + self.assertIsNone(Config._validate_failover_tags(config)) + self.assertIn('nofailover', config) + self.assertNotIn('failover_priority', config) mock_logger.warning.assert_called_once_with( 'Conflicting configuration between nofailover: %s and failover_priority: %s.' + ' Defaulting to nofailover: %s',