From 75238662d76f4ce715cc7bea337b8e2f37621597 Mon Sep 17 00:00:00 2001 From: Ashley James Date: Thu, 21 Mar 2024 14:33:57 +0530 Subject: [PATCH] fix: Raise custom exception instead of ErrorStatus (#191) --- src/charm.py | 24 +++++++++--------------- src/config.py | 2 +- src/service.py | 7 +++++++ tests/unit/test_charm.py | 19 ++++++------------- tests/unit/test_exporter.py | 27 +++++++++++++++++++-------- 5 files changed, 42 insertions(+), 37 deletions(-) diff --git a/src/charm.py b/src/charm.py index 1e36266c..2616660d 100755 --- a/src/charm.py +++ b/src/charm.py @@ -11,11 +11,12 @@ import ops from charms.grafana_agent.v0.cos_agent import COSAgentProvider from ops.framework import EventBase, StoredState -from ops.model import ActiveStatus, BlockedStatus, ErrorStatus, MaintenanceStatus, StatusBase +from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus from redfish import redfish_client from redfish.rest.v1 import InvalidCredentialsError from config import ( + EXPORTER_CRASH_MSG, EXPORTER_DEFAULT_PORT, EXPORTER_HEALTH_RETRY_COUNT, EXPORTER_HEALTH_RETRY_TIMEOUT, @@ -25,7 +26,7 @@ ) from hardware import get_bmc_address from hw_tools import HWToolHelper, bmc_hw_verifier -from service import Exporter +from service import Exporter, ExporterError logger = logging.getLogger(__name__) @@ -143,14 +144,12 @@ def _on_update_status(self, _: EventBase) -> None: # noqa: C901 if not self.exporter.check_health(): logger.warning("Exporter health check - failed.") - restart_ok, restart_status = self.restart_exporter() - if not restart_ok and restart_status is not None: - self.model.unit.status = restart_status - return + # if restart isn't successful, an ExporterError exception will be raised here + self.restart_exporter() self.model.unit.status = ActiveStatus("Unit is ready") - def restart_exporter(self) -> Tuple[bool, Optional[StatusBase]]: + def restart_exporter(self) -> None: """Restart exporter service with retry.""" try: for i in range(1, EXPORTER_HEALTH_RETRY_COUNT + 1): @@ -158,19 +157,14 @@ def restart_exporter(self) -> Tuple[bool, Optional[StatusBase]]: self.exporter.restart() sleep(EXPORTER_HEALTH_RETRY_TIMEOUT) if self.exporter.check_active(): - logger.info("Exporter restarted.") + logger.info("Exporter active after restart.") break if not self.exporter.check_active(): logger.error("Failed to restart the exporter.") - return False, ErrorStatus( - "Exporter crashed unexpectedly, please refer to systemd logs..." - ) + raise ExporterError(EXPORTER_CRASH_MSG) except Exception as err: # pylint: disable=W0718 logger.error("Exporter crashed unexpectedly: %s", err) - return False, ErrorStatus( - "Exporter crashed unexpectedly, please refer to systemd logs..." - ) - return True, None + raise ExporterError(EXPORTER_CRASH_MSG) from err def _on_config_changed(self, event: EventBase) -> None: """Reconfigure charm.""" diff --git a/src/config.py b/src/config.py index a10d097b..d322a232 100644 --- a/src/config.py +++ b/src/config.py @@ -13,7 +13,7 @@ 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..." # Redfish REDFISH_TIMEOUT = 10 diff --git a/src/service.py b/src/service.py index 7e8fa337..179c21d8 100644 --- a/src/service.py +++ b/src/service.py @@ -39,6 +39,13 @@ def wrapper(self: Any, *args: Tuple[Any], **kwargs: Dict[str, Any]) -> Any: return wrapper +class ExporterError(Exception): + """Custom exception for exporter errors. + + This will cause juju to set the charm in ErrorStatus. + """ + + class ExporterTemplate: """Jinja template helper class for exporter.""" diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py index d87053cc..3b4db824 100644 --- a/tests/unit/test_charm.py +++ b/tests/unit/test_charm.py @@ -9,12 +9,12 @@ import ops import ops.testing -from ops.model import ActiveStatus, BlockedStatus, ErrorStatus +from ops.model import ActiveStatus, BlockedStatus from parameterized import parameterized from redfish.rest.v1 import InvalidCredentialsError import charm -from charm import HardwareObserverCharm +from charm import ExporterError, HardwareObserverCharm from config import EXPORTER_DEFAULT_PORT, HWTool @@ -200,21 +200,14 @@ def test_09_update_status_exporter_crashed(self, mock_hw_tool_helper, mock_expor HWTool.IPMI_SEL, HWTool.IPMI_DCMI, ] - mock_hw_tool_helper.return_value.install.return_value = (True, "") mock_hw_tool_helper.return_value.check_installed.return_value = (True, "") - mock_exporter.return_value.install.return_value = True mock_exporter.return_value.check_health.return_value = False mock_exporter.return_value.restart.side_effect = Exception() - rid = self.harness.add_relation("cos-agent", "grafana-agent") + self.harness.add_relation("cos-agent", "grafana-agent") self.harness.begin() - self.harness.charm.on.install.emit() - self.harness.add_relation_unit(rid, "grafana-agent/0") - - self.harness.charm.on.update_status.emit() - self.assertEqual( - self.harness.charm.unit.status, - ErrorStatus("Exporter crashed unexpectedly, please refer to systemd logs..."), - ) + self.harness.charm._stored.resource_installed = True + with self.assertRaises(ExporterError): + self.harness.charm.on.update_status.emit() @mock.patch("charm.Exporter", return_value=mock.MagicMock()) def test_10_config_changed(self, mock_exporter): diff --git a/tests/unit/test_exporter.py b/tests/unit/test_exporter.py index 7eab3ac4..0fcca796 100644 --- a/tests/unit/test_exporter.py +++ b/tests/unit/test_exporter.py @@ -6,7 +6,7 @@ from unittest import mock import ops -from ops.model import ActiveStatus, BlockedStatus, ErrorStatus +from ops.model import ActiveStatus, BlockedStatus from ops.testing import Harness from parameterized import parameterized @@ -176,8 +176,10 @@ def test_31_stop_failed(self, mock_service_not_installed): self.harness.begin() self.harness.charm._stored.resource_installed = True self.harness.charm._stored.exporter_installed = True - self.harness.add_relation_unit(rid, "grafana-agent/0") - self.harness.remove_relation_unit(rid, "grafana-agent/0") + with self.assertRaises(service.ExporterError): + self.harness.add_relation_unit(rid, "grafana-agent/0") + with self.assertRaises(service.ExporterError): + self.harness.remove_relation_unit(rid, "grafana-agent/0") self.mock_systemd.service_stop.assert_not_called() self.mock_systemd.service_disable.assert_not_called() @@ -186,11 +188,6 @@ def test_31_stop_failed(self, mock_service_not_installed): (False, ActiveStatus("Unit is ready"), True), (True, ActiveStatus("Unit is ready"), True), (False, ActiveStatus("Unit is ready"), False), - ( - True, - ErrorStatus("Exporter crashed unexpectedly, please refer to systemd logs..."), - False, - ), ] ) @mock.patch.object(pathlib.Path, "exists", return_value=True) @@ -213,6 +210,20 @@ def test_40_check_health( self.harness.charm.on.update_status.emit() self.assertEqual(self.harness.charm.unit.status, expected_status) + @mock.patch.object(pathlib.Path, "exists", return_value=True) + def test_40_check_health_exporter_crash(self, mock_service_installed): + """Test check_health function when service is installed but exporter crashes.""" + rid = self.harness.add_relation(EXPORTER_RELATION_NAME, "grafana-agent") + self.harness.begin() + with mock.patch("builtins.open", new_callable=mock.mock_open) as _: + self.harness.charm.on.install.emit() + self.harness.add_relation_unit(rid, "grafana-agent/0") + + self.mock_systemd.service_running.return_value = False + self.mock_systemd.service_failed.return_value = True + with self.assertRaises(service.ExporterError): + self.harness.charm.on.update_status.emit() + @mock.patch.object(pathlib.Path, "exists", return_value=True) def test_50_check_relation_exists(self, mock_service_installed): """Test check_relation function when relation exists."""