From febb83d9892425701535e13e9a9efa23fd3456b4 Mon Sep 17 00:00:00 2001 From: Dragomir Penev <6687393+dragomirp@users.noreply.github.com> Date: Thu, 14 Mar 2024 19:56:23 +0200 Subject: [PATCH] [DPE-3593] Only check config values against the DB in on_config_changed (#395) * Don't check the DB if config values are the defaults * Unit test * Move config DB validation in _on_config_changed --- src/charm.py | 34 ++++++++--------- tests/integration/test_config.py | 7 ++++ tests/unit/test_charm.py | 65 ++++++++++++++++++++++++++++++-- 3 files changed, 85 insertions(+), 21 deletions(-) diff --git a/src/charm.py b/src/charm.py index c1694849ea..1fdd086288 100755 --- a/src/charm.py +++ b/src/charm.py @@ -11,6 +11,7 @@ import subprocess from typing import Dict, List, Literal, Optional, Set, get_args +import psycopg2 from charms.data_platform_libs.v0.data_interfaces import DataPeer, DataPeerUnit from charms.data_platform_libs.v0.data_models import TypedCharmBase from charms.grafana_agent.v0.cos_agent import COSAgentProvider @@ -862,19 +863,25 @@ def _on_leader_elected(self, event: LeaderElectedEvent) -> None: else: self.unit.status = WaitingStatus(PRIMARY_NOT_REACHABLE_MESSAGE) - def _on_config_changed(self, _) -> None: + def _on_config_changed(self, event) -> None: """Handle configuration changes, like enabling plugins.""" if not self.is_cluster_initialised: - logger.debug("Early exit on_config_changed: cluster not initialised yet") + logger.debug("Defer on_config_changed: cluster not initialised yet") + event.defer() return if not self.upgrade.idle: - logger.debug("Early exit on_config_changed: upgrade in progress") + logger.debug("Defer on_config_changed: upgrade in progress") + event.defer() return - try: + self._validate_config_options() # update config on every run self.update_config() + except psycopg2.OperationalError: + logger.debug("Defer on_config_changed: Cannot connect to database") + event.defer() + return except ValueError as e: self.unit.status = BlockedStatus("Configuration Error. Please check the logs") logger.error("Invalid configuration: %s", str(e)) @@ -1476,7 +1483,6 @@ def update_config(self, is_creating_backup: bool = False) -> bool: if not self._can_connect_to_postgresql: logger.warning("Early exit update_config: Cannot connect to Postgresql") return False - self._validate_config_options() self._patroni.bulk_update_parameters_controller_by_patroni({ "max_connections": max(4 * os.cpu_count(), 100), @@ -1504,24 +1510,18 @@ def update_config(self, is_creating_backup: bool = False) -> bool: def _validate_config_options(self) -> None: """Validates specific config options that need access to the database or to the TLS status.""" if ( - self.config.instance_default_text_search_config is not None - and self.config.instance_default_text_search_config + self.config.instance_default_text_search_config not in self.postgresql.get_postgresql_text_search_configs() ): - raise Exception( + raise ValueError( "instance_default_text_search_config config option has an invalid value" ) - if self.config.request_date_style is not None and not self.postgresql.validate_date_style( - self.config.request_date_style - ): - raise Exception("request_date_style config option has an invalid value") + if not self.postgresql.validate_date_style(self.config.request_date_style): + raise ValueError("request_date_style config option has an invalid value") - if ( - self.config.request_time_zone is not None - and self.config.request_time_zone not in self.postgresql.get_postgresql_timezones() - ): - raise Exception("request_time_zone config option has an invalid value") + if self.config.request_time_zone not in self.postgresql.get_postgresql_timezones(): + raise ValueError("request_time_zone config option has an invalid value") def _handle_postgresql_restart_need(self, enable_tls: bool) -> None: """Handle PostgreSQL restart need based on the TLS configuration and configuration changes.""" diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index 251ed1d5ce..c75731053d 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -81,6 +81,13 @@ async def test_config_parameters(ops_test: OpsTest) -> None: { "vacuum_vacuum_freeze_table_age": ["-1", "150000000"] }, # config option is between 0 and 2000000000 + { + "instance_default_text_search_config": [test_string, "pg_catalog.simple"] + }, # config option is validated against the db + { + "request_date_style": [test_string, "ISO, MDY"] + }, # config option is validated against the db + {"request_time_zone": [test_string, "UTC"]}, # config option is validated against the db ] charm_config = {} diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index 3666878d5a..c925274d6f 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -25,6 +25,7 @@ ) from ops.testing import Harness from parameterized import parameterized +from psycopg2 import OperationalError from tenacity import RetryError, wait_fixed from charm import ( @@ -258,12 +259,18 @@ def test_is_cluster_initialised(self): ) self.assertTrue(self.charm.is_cluster_initialised) + @patch("charm.PostgresqlOperatorCharm._validate_config_options") @patch("charm.PostgresqlOperatorCharm.update_config") @patch("relations.db.DbProvides.set_up_relation") @patch("charm.PostgresqlOperatorCharm.enable_disable_extensions") @patch("charm.PostgresqlOperatorCharm.is_cluster_initialised", new_callable=PropertyMock) def test_on_config_changed( - self, _is_cluster_initialised, _enable_disable_extensions, _set_up_relation, _update_config + self, + _is_cluster_initialised, + _enable_disable_extensions, + _set_up_relation, + _update_config, + _validate_config_options, ): # Test when the cluster was not initialised yet. _is_cluster_initialised.return_value = False @@ -274,9 +281,17 @@ def test_on_config_changed( # Test when the unit is not the leader. _is_cluster_initialised.return_value = True self.charm.on.config_changed.emit() + _validate_config_options.assert_called_once() _enable_disable_extensions.assert_not_called() _set_up_relation.assert_not_called() + # Test unable to connect to db + _update_config.reset_mock() + _validate_config_options.side_effect = OperationalError + self.charm.on.config_changed.emit() + assert not _update_config.called + _validate_config_options.side_effect = None + # Test after the cluster was initialised. with self.harness.hooks_disabled(): self.harness.set_leader() @@ -1166,7 +1181,6 @@ def test_restart(self, _are_all_members_ready, _restart_postgresql): @patch("charm.snap.SnapCache") @patch("charm.PostgresqlOperatorCharm._handle_postgresql_restart_need") @patch("charm.Patroni.bulk_update_parameters_controller_by_patroni") - @patch("charm.PostgresqlOperatorCharm._validate_config_options") @patch("charm.Patroni.member_started", new_callable=PropertyMock) @patch("charm.PostgresqlOperatorCharm._is_workload_running", new_callable=PropertyMock) @patch("charm.Patroni.render_patroni_yml_file") @@ -1178,10 +1192,9 @@ def test_update_config( _is_workload_running, _member_started, _, - __, _handle_postgresql_restart_need, + __, ___, - ____, ): with patch.object(PostgresqlOperatorCharm, "postgresql", Mock()) as postgresql_mock: # Mock some properties. @@ -1321,6 +1334,50 @@ def test_on_cluster_topology_change_clear_blocked( _primary_endpoint.assert_called_once_with() self.assertTrue(isinstance(self.harness.model.unit.status, ActiveStatus)) + @patch("charm.PostgresqlOperatorCharm.postgresql", new_callable=PropertyMock) + @patch("config.subprocess") + def test_validate_config_options(self, _, _charm_lib): + _charm_lib.return_value.get_postgresql_text_search_configs.return_value = [] + _charm_lib.return_value.validate_date_style.return_value = [] + _charm_lib.return_value.get_postgresql_timezones.return_value = [] + + # Test instance_default_text_search_config exception + with self.harness.hooks_disabled(): + self.harness.update_config({"instance_default_text_search_config": "pg_catalog.test"}) + + with self.assertRaises(ValueError) as e: + self.charm._validate_config_options() + assert ( + e.msg == "instance_default_text_search_config config option has an invalid value" + ) + + _charm_lib.return_value.get_postgresql_text_search_configs.assert_called_once_with() + _charm_lib.return_value.get_postgresql_text_search_configs.return_value = [ + "pg_catalog.test" + ] + + # Test request_date_style exception + with self.harness.hooks_disabled(): + self.harness.update_config({"request_date_style": "ISO, TEST"}) + + with self.assertRaises(ValueError) as e: + self.charm._validate_config_options() + assert e.msg == "request_date_style config option has an invalid value" + + _charm_lib.return_value.validate_date_style.assert_called_once_with("ISO, TEST") + _charm_lib.return_value.validate_date_style.return_value = ["ISO, TEST"] + + # Test request_time_zone exception + with self.harness.hooks_disabled(): + self.harness.update_config({"request_time_zone": "TEST_ZONE"}) + + with self.assertRaises(ValueError) as e: + self.charm._validate_config_options() + assert e.msg == "request_time_zone config option has an invalid value" + + _charm_lib.return_value.get_postgresql_timezones.assert_called_once_with() + _charm_lib.return_value.get_postgresql_timezones.return_value = ["TEST_ZONE"] + @patch_network_get(private_address="1.1.1.1") @patch("charm.snap.SnapCache") @patch("charm.PostgresqlOperatorCharm._update_relation_endpoints")