fix: Raise custom exception instead of ErrorStatus (#191)

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,34 +144,27 @@ 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):
                 logger.warning("Restarting exporter - %d retry", i)
                 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."""

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

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."""
 

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):

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."""