unit should block if replan fails (#220)

Make sure the unit would go into BlockedStatus, if replan fails, instead of re-raising an exception (which is not caught anywhere).

Co-authored-by: Balbir Thomas <[email protected]>

src/charm.py

@@ -26,7 +26,7 @@
 from ops.charm import CharmBase
 from ops.main import main
 from ops.model import ActiveStatus, BlockedStatus, WaitingStatus
-from ops.pebble import Layer
+from ops.pebble import ChangeError, Layer
 
 from prometheus_server import Prometheus
 
@@ -120,24 +120,32 @@ def _configure(self, _):
         # Restart prometheus only if command line arguments have changed,
         # otherwise just reload its configuration.
         if current_services == new_layer.services:
+            # No change in layer; reload config to make sure it is valid
             external_url = urlparse(self._external_url)
-
             prometheus_server = Prometheus(web_route_prefix=external_url.path)
-
             reloaded = prometheus_server.reload_configuration()
             if not reloaded:
                 logger.error("Prometheus failed to reload the configuration")
                 self.unit.status = BlockedStatus(CORRUPT_PROMETHEUS_CONFIG_MESSAGE)
                 return
+
             logger.info("Prometheus configuration reloaded")
+
         else:
+            # Layer changed - replan.
             container.add_layer(self._name, new_layer, combine=True)
             try:
+                # If a config is invalid then prometheus would exit immediately.
+                # This would be caught by pebble (default timeout is 30 sec) and a ChangeError
+                # would be raised.
                 container.replan()
                 logger.info("Prometheus (re)started")
-            except Exception as e:
-                logger.error(f"Failed to replan; pebble plan: {container.get_plan().to_dict()}")
-                raise e
+            except ChangeError as e:
+                logger.error(
+                    "Failed to replan; pebble plan: %s; %s", container.get_plan().to_dict(), str(e)
+                )
+                self.unit.status = BlockedStatus(CORRUPT_PROMETHEUS_CONFIG_MESSAGE)
+                return
 
         if (
             isinstance(self.unit.status, BlockedStatus)

tests/unit/test_charm.py

@@ -47,12 +47,12 @@ def test_invalid_log_level_defaults_to_debug(self):
         bad_log_config = {"log_level": "bad-level"}
         with self.assertLogs(level="ERROR") as logger:
             self.harness.update_config(bad_log_config)
-            expected_logs = [
+            expected_logs = {
                 "ERROR:root:Invalid loglevel: bad-level given, "
                 "debug/info/warn/error/fatal allowed. "
                 "defaulting to DEBUG loglevel."
-            ]
-            self.assertEqual(sorted(logger.output), expected_logs)
+            }
+            self.assertGreaterEqual(set(logger.output), expected_logs)
 
         plan = self.harness.get_container_pebble_plan("prometheus")
         self.assertEqual(cli_arg(plan, "--log.level"), "debug")

tests/unit/test_charm_status.py

@@ -0,0 +1,82 @@
+#!/usr/bin/env python3
+# Copyright 2021 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+import datetime
+import logging
+import unittest
+from unittest.mock import Mock, patch
+
+from helpers import patch_network_get
+from ops.model import ActiveStatus, BlockedStatus
+from ops.pebble import Change, ChangeError, ChangeID
+from ops.testing import Harness
+
+from charm import PrometheusCharm
+
+logger = logging.getLogger(__name__)
+
+
+class TestActiveStatus(unittest.TestCase):
+    """Feature: Charm's status should reflect the correctness of the config / relations.
+
+    Background: When launched on its own, the charm should always end up with active status.
+    In some cases (e.g. Ingress conflicts) the charm should go into blocked state.
+    """
+
+    def setUp(self) -> None:
+        self.app_name = "prometheus-k8s"
+        self.harness = Harness(PrometheusCharm)
+        self.addCleanup(self.harness.cleanup)
+        self.peer_rel_id = self.harness.add_relation("prometheus-peers", self.app_name)
+
+        # GIVEN a total of three units present
+        for i in range(1, 3):
+            self.harness.add_relation_unit(self.peer_rel_id, f"{self.app_name}/{i}")
+
+        # AND the current unit is a leader
+        self.harness.set_leader(True)
+
+    @patch_network_get(private_address="1.1.1.1")
+    @patch("charm.KubernetesServicePatch", lambda x, y: None)
+    def test_unit_is_active_if_deployed_without_relations_or_config(self):
+        """Scenario: Unit is deployed without any user-provided config or regular relations."""
+        # GIVEN reload configuration succeeds
+        with patch("prometheus_server.Prometheus.reload_configuration", lambda *a, **kw: True):
+            self.harness.begin_with_initial_hooks()
+
+            # WHEN no config is provided or relations created
+
+            # THEN the unit goes into active state
+            self.assertIsInstance(self.harness.charm.unit.status, ActiveStatus)
+
+            # AND pebble plan is not empty
+            plan = self.harness.get_container_pebble_plan(self.harness.charm._name)
+            self.assertTrue(plan.to_dict())
+
+    @patch_network_get(private_address="1.1.1.1")
+    @patch("charm.KubernetesServicePatch", lambda x, y: None)
+    def test_unit_is_blocked_if_reload_configuration_fails(self):
+        """Scenario: Unit is deployed but reload configuration fails."""
+        # GIVEN reload configuration fails
+        # Construct mock objects
+        cid = ChangeID("0")
+        spawn_time = datetime.datetime.now()
+        change = Change(cid, "kind", "summary", "status", [], False, None, spawn_time, None)
+        replan_patch = patch(
+            "ops.model.Container.replan", Mock(side_effect=ChangeError("err", change))
+        )
+        reload_patch = patch(
+            "prometheus_server.Prometheus.reload_configuration", lambda *a, **kw: False
+        )
+        with replan_patch, reload_patch:
+            self.harness.begin_with_initial_hooks()
+
+            # WHEN no config is provided or relations created
+
+            # THEN the unit goes into blocked state
+            self.assertIsInstance(self.harness.charm.unit.status, BlockedStatus)
+
+            # AND pebble plan is not empty
+            plan = self.harness.get_container_pebble_plan(self.harness.charm._name)
+            self.assertTrue(plan.to_dict())