From 4a1b809eadd8e919f576a8697c37adfc2385d92e Mon Sep 17 00:00:00 2001 From: Ray Chan <72372217+chanchiwai-ray@users.noreply.github.com> Date: Fri, 5 Apr 2024 20:31:25 +0800 Subject: [PATCH] Remove unnecessary use of `StoreState` for configs (#209) * Drop `_store.config`. * Remove unnecessary EXPORTER_DEFAULT_*. --- src/charm.py | 77 ++++++------------------ src/config.py | 2 - src/service.py | 4 +- tests/functional/utils.py | 4 +- tests/integration/export_mock_metrics.py | 4 +- tests/unit/test_charm.py | 52 ++++++++++++---- tests/unit/test_exporter.py | 7 ++- 7 files changed, 69 insertions(+), 81 deletions(-) diff --git a/src/charm.py b/src/charm.py index 62a450c5..ab1019ea 100755 --- a/src/charm.py +++ b/src/charm.py @@ -17,8 +17,6 @@ from config import ( EXPORTER_CRASH_MSG, - EXPORTER_DEFAULT_COLLECT_TIMEOUT, - EXPORTER_DEFAULT_PORT, EXPORTER_HEALTH_RETRY_COUNT, EXPORTER_HEALTH_RETRY_TIMEOUT, REDFISH_MAX_RETRY, @@ -42,9 +40,6 @@ def __init__(self, *args: Any) -> None: super().__init__(*args) self.hw_tool_helper = HWToolHelper() - collect_timeout = self.model.config.get( - "collect-timeout", EXPORTER_DEFAULT_COLLECT_TIMEOUT - ) # Add refresh_events to COSAgentProvider to update relation data when # config changed (default behavior) and upgrade charm. This is useful # for updating alert rules. @@ -56,7 +51,7 @@ def __init__(self, *args: Any) -> None: ], # Setting scrape_timeout as collect_timeout in the `duration` format specified in # https://prometheus.io/docs/prometheus/latest/configuration/configuration/#duration - scrape_configs=[{"scrape_timeout": f"{collect_timeout}s"}], + scrape_configs=[{"scrape_timeout": f"{int(self.model.config['collect-timeout'])}s"}], ) self.exporter = Exporter(self.charm_dir) @@ -73,7 +68,6 @@ def __init__(self, *args: Any) -> None: ) self._stored.set_default( - config={}, exporter_installed=False, resource_installed=False, ) @@ -93,11 +87,12 @@ def _on_install_or_upgrade(self, event: ops.InstallEvent) -> None: # Install exporter self.model.unit.status = MaintenanceStatus("Installing exporter...") - port = self.model.config.get("exporter-port", EXPORTER_DEFAULT_PORT) - level = self.model.config.get("exporter-log-level", "INFO") - scrape_timeout = self.model.config.get("scrape-timeout", EXPORTER_DEFAULT_COLLECT_TIMEOUT) - redfish_conn_params = self.get_redfish_conn_params() - success = self.exporter.install(port, level, redfish_conn_params, int(scrape_timeout)) + success = self.exporter.install( + int(self.model.config["exporter-port"]), + self.model.config["exporter-log-level"], + self.get_redfish_conn_params(), + int(self.model.config["collect-timeout"]), + ) self._stored.exporter_installed = success if not success: msg = "Failed to install exporter, please refer to `juju debug-log`" @@ -171,20 +166,6 @@ def restart_exporter(self) -> None: def _on_config_changed(self, event: EventBase) -> None: """Reconfigure charm.""" - # Keep track of what model config options + some extra config related - # information are changed. This can be helpful when we want to respond - # to the change of a specific config option. - change_set = set() - model_config: Dict[str, Optional[str]] = dict(self.model.config.items()) - for key, value in model_config.items(): - if ( - key not in self._stored.config # type: ignore[operator] - or self._stored.config[key] != value # type: ignore[index] - ): - logger.info("Setting %s to: %s", key, value) - self._stored.config[key] = value # type: ignore - change_set.add(key) - if not self._stored.resource_installed: # type: ignore[truthy-function] logging.info( # type: ignore[unreachable] "Config changed called before install complete, deferring event: %s", @@ -198,35 +179,17 @@ def _on_config_changed(self, event: EventBase) -> None: self.model.unit.status = BlockedStatus(message) return - exporter_configs = { - "exporter-port", - "exporter-log-level", - "redfish-host", - "redfish-username", - "redfish-password", - "collect-timeout", - } - if exporter_configs.intersection(change_set): - logger.info("Detected changes in exporter config.") - port = self.model.config.get("exporter-port", EXPORTER_DEFAULT_PORT) - level = self.model.config.get("exporter-log-level", "INFO") - collect_timeout = self.model.config.get( - "collect-timeout", EXPORTER_DEFAULT_COLLECT_TIMEOUT - ) - redfish_conn_params = self.get_redfish_conn_params() - success = self.exporter.template.render_config( - port=port, - level=level, - redfish_conn_params=redfish_conn_params, - collect_timeout=int(collect_timeout), - ) - if not success: - message = ( - "Failed to configure exporter, please check if the server is healthy." - ) - self.model.unit.status = BlockedStatus(message) - return - self.exporter.restart() + success = self.exporter.template.render_config( + port=int(self.model.config["exporter-port"]), + level=self.model.config["exporter-log-level"], + redfish_conn_params=self.get_redfish_conn_params(), + collect_timeout=int(self.model.config["collect-timeout"]), + ) + if not success: + message = "Failed to configure exporter, please check if the server is healthy." + self.model.unit.status = BlockedStatus(message) + return + self.exporter.restart() self._on_update_status(event) @@ -268,12 +231,12 @@ def get_redfish_conn_params(self) -> Dict[str, Any]: def validate_exporter_configs(self) -> Tuple[bool, str]: """Validate the static and runtime config options for the exporter.""" - port = int(self.model.config.get("exporter-port", EXPORTER_DEFAULT_PORT)) + port = int(self.model.config["exporter-port"]) if not 1 <= port <= 65535: logger.error("Invalid exporter-port: port must be in [1, 65535].") return False, "Invalid config: 'exporter-port'" - level = self.model.config.get("exporter-log-level", "") + level = self.model.config["exporter-log-level"] allowed_choices = {"DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"} if level.upper() not in allowed_choices: logger.error( diff --git a/src/config.py b/src/config.py index e22e77a5..fe7f6559 100644 --- a/src/config.py +++ b/src/config.py @@ -12,9 +12,7 @@ EXPORTER_SERVICE_TEMPLATE = f"{EXPORTER_NAME}.service.j2" EXPORTER_HEALTH_RETRY_COUNT = 3 EXPORTER_HEALTH_RETRY_TIMEOUT = 3 -EXPORTER_DEFAULT_PORT = "10200" EXPORTER_CRASH_MSG = "Exporter crashed unexpectedly, please refer to systemd logs..." -EXPORTER_DEFAULT_COLLECT_TIMEOUT = 10 # Redfish REDFISH_TIMEOUT = 10 diff --git a/src/service.py b/src/service.py index fc3c95b2..0c9040c2 100644 --- a/src/service.py +++ b/src/service.py @@ -86,7 +86,7 @@ def _uninstall(self, path: Path) -> bool: return success def render_config( - self, port: str, level: str, collect_timeout: int, redfish_conn_params: dict + self, port: int, level: str, collect_timeout: int, redfish_conn_params: dict ) -> bool: """Render and install exporter config file.""" hw_tools = get_hw_tool_white_list() @@ -130,7 +130,7 @@ def __init__(self, charm_dir: Path) -> None: self.template = ExporterTemplate(charm_dir) def install( - self, port: str, level: str, redfish_conn_params: dict, collect_timeout: int + self, port: int, level: str, redfish_conn_params: dict, collect_timeout: int ) -> bool: """Install the exporter.""" logger.info("Installing %s.", EXPORTER_NAME) diff --git a/tests/functional/utils.py b/tests/functional/utils.py index fbf3abd3..7c948cf3 100644 --- a/tests/functional/utils.py +++ b/tests/functional/utils.py @@ -8,8 +8,6 @@ from async_lru import alru_cache -from config import EXPORTER_DEFAULT_PORT - RESOURCES_DIR = Path("./resources/") @@ -62,7 +60,7 @@ async def get_metrics_output(ops_test, unit_name) -> Optional[dict[str, list[Met Raises MetricsFetchError if command to fetch metrics didn't execute successfully. """ - command = f"curl -s localhost:{EXPORTER_DEFAULT_PORT}" + command = "curl -s localhost:10200" # curl at default port (see config.yaml) results = await run_command_on_unit(ops_test, unit_name, command) if results.get("return-code") > 0: raise MetricsFetchError diff --git a/tests/integration/export_mock_metrics.py b/tests/integration/export_mock_metrics.py index 2af3c9df..d61f2282 100644 --- a/tests/integration/export_mock_metrics.py +++ b/tests/integration/export_mock_metrics.py @@ -10,8 +10,6 @@ from prometheus_client import REGISTRY, start_http_server from prometheus_client.core import GaugeMetricFamily -from config import EXPORTER_DEFAULT_PORT - class SyntheticCollector: """Collector for creating synthetic(mock) metrics.""" @@ -30,7 +28,7 @@ def collect(self): if __name__ == "__main__": - start_http_server(int(EXPORTER_DEFAULT_PORT)) + start_http_server(10200) # start at default port (see config.yaml) REGISTRY.register(SyntheticCollector()) while True: diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index e2026b59..87804dd3 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -15,7 +15,7 @@ import charm from charm import ExporterError, HardwareObserverCharm -from config import EXPORTER_DEFAULT_COLLECT_TIMEOUT, EXPORTER_DEFAULT_PORT, HWTool +from config import HWTool class TestCharm(unittest.TestCase): @@ -63,7 +63,6 @@ def test_harness(self) -> None: """Test charm initialize.""" self.harness.begin() self.assertFalse(self.harness.charm._stored.resource_installed) - self.assertTrue(isinstance(self.harness.charm._stored.config, ops.framework.StoredDict)) @mock.patch("charm.Exporter", return_value=mock.MagicMock()) @mock.patch("charm.HWToolHelper", return_value=mock.MagicMock()) @@ -131,10 +130,10 @@ def test_install_redfish_unavailable(self, mock_hw_tool_helper, mock_exporter) - self.assertTrue(self.harness.charm._stored.resource_installed) self.harness.charm.exporter.install.assert_called_with( - int(EXPORTER_DEFAULT_PORT), - "INFO", + 10200, # default in config.yaml + "INFO", # default in config.yaml {}, - int(EXPORTER_DEFAULT_COLLECT_TIMEOUT), + 10, # default int config.yaml ) @mock.patch("charm.Exporter", return_value=mock.MagicMock()) @@ -213,18 +212,47 @@ def test_update_status_exporter_crashed(self, mock_hw_tool_helper, mock_exporter with self.assertRaises(ExporterError): self.harness.charm.on.update_status.emit() + @mock.patch.object(charm, "bmc_hw_verifier", return_value=[]) + @mock.patch("charm.HWToolHelper") @mock.patch("charm.Exporter", return_value=mock.MagicMock()) - def test_config_changed(self, mock_exporter): - """Test config change event updates the charm's internal store.""" + def test_config_changed(self, mock_exporter, mock_hw_tool_helper, mock_bmc_hw_verifier): + """Test config change event renders config file.""" self.harness.begin() self.harness.charm._stored.resource_installed = True + self.harness.charm.num_cos_agent_relations = 1 # exporter enabled + self.harness.charm.hw_tool_helper.check_installed.return_value = ( + True, + "", + ) # hw tool install ok new_config = {"exporter-port": 80, "exporter-log-level": "DEBUG"} self.harness.update_config(new_config) self.harness.charm.on.config_changed.emit() - for k, v in self.harness.charm.model.config.items(): - self.assertEqual(self.harness.charm._stored.config.get(k), v) + self.harness.charm.exporter.template.render_config.assert_called() + + self.assertEqual(self.harness.charm.unit.status, ActiveStatus("Unit is ready")) + + @mock.patch.object(charm, "bmc_hw_verifier", return_value=[]) + @mock.patch("charm.HWToolHelper") + @mock.patch("charm.Exporter", return_value=mock.MagicMock()) + def test_config_changed_without_cos_agent_relation( + self, mock_exporter, mock_hw_tool_helper, mock_bmc_hw_verifier + ): + """Test config change event don't render config file if cos_agent relation is missing.""" + self.harness.begin() + self.harness.charm._stored.resource_installed = True + self.harness.charm.num_cos_agent_relations = 0 # exporter disabled + self.harness.charm.hw_tool_helper.check_installed.return_value = ( + True, + "", + ) # hw tool install ok + + new_config = {"exporter-port": 80, "exporter-log-level": "DEBUG"} + self.harness.update_config(new_config) + self.harness.charm.on.config_changed.emit() + + self.harness.charm.exporter.template.render_config.assert_not_called() self.assertEqual( self.harness.charm.unit.status, BlockedStatus("Missing relation: [cos-agent]") @@ -370,10 +398,10 @@ def test_install_redfish_enabled_with_correct_credential( self.assertTrue(self.harness.charm._stored.resource_installed) self.harness.charm.exporter.install.assert_called_with( - int(EXPORTER_DEFAULT_PORT), - "INFO", + 10200, # default in config.yaml + "INFO", # default in config.yaml self.harness.charm.get_redfish_conn_params(), - int(EXPORTER_DEFAULT_COLLECT_TIMEOUT), + 10, # default int config.yaml ) @parameterized.expand([(InvalidCredentialsError), (Exception)]) diff --git a/tests/unit/test_exporter.py b/tests/unit/test_exporter.py index b3ca7e63..b795e1e5 100644 --- a/tests/unit/test_exporter.py +++ b/tests/unit/test_exporter.py @@ -275,10 +275,13 @@ def test_60_config_changed_log_level_okay(self, mock_service_installed): self.mock_systemd.service_failed.return_value = False self.harness.charm.on.install.emit() self.harness.add_relation_unit(rid, "grafana-agent/0") + # this will trigger config changed event self.harness.update_config({"exporter-log-level": "DEBUG"}) - self.harness.charm.on.config_changed.emit() - self.assertEqual(self.harness.charm._stored.config.get("exporter-log-level"), "DEBUG") self.mock_systemd.service_restart.assert_called_once() + self.assertEqual( + self.harness.charm.unit.status, + ActiveStatus("Unit is ready"), + ) @mock.patch.object(pathlib.Path, "exists", return_value=True) def test_61_config_changed_not_okay(self, mock_service_installed):