Re-engineering status changes

src/charm.py

@@ -11,15 +11,27 @@
 from ops.charm import CharmBase, InstallEvent, SecretChangedEvent
 from ops.framework import EventBase
 from ops.main import main
-from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus
+from ops.model import BlockedStatus, MaintenanceStatus, WaitingStatus
 
 from core.cluster import ClusterState
 from events.password_actions import PasswordActionEvents
 from events.requirer import RequirerEvents
 
 # from events.provider import ProviderEvents
 from events.tls import TLSEvents
-from literals import CHARM_KEY, CHARM_USERS, PEER, RESTART_TIMEOUT, SUBSTRATE
+from helpers import clear_status
+from literals import (
+    CHARM_KEY,
+    CHARM_USERS,
+    MSG_INSTALLING,
+    MSG_STARTING,
+    MSG_STARTING_SERVER,
+    MSG_WAITING_FOR_PEER,
+    MSG_WAITING_FOR_USER_CREDENTIALS,
+    PEER,
+    RESTART_TIMEOUT,
+    SUBSTRATE,
+)
 from managers.config import ConfigManager
 from managers.tls import TLSManager
 from workload import ODWorkload
@@ -84,21 +96,23 @@ def __init__(self, *args):
 
     def _on_install(self, event: InstallEvent) -> None:
         """Handler for the `on_install` event."""
-        self.unit.status = MaintenanceStatus("installing Opensearch Dashboards...")
+        self.unit.status = MaintenanceStatus(MSG_INSTALLING)
 
         install = self.workload.install()
         if not install:
             self.unit.status = BlockedStatus("unable to install Opensearch Dashboards")
 
         # don't complete install until passwords set
         if not self.state.peer_relation:
-            self.unit.status = WaitingStatus("waiting for peer relation")
+            self.unit.status = WaitingStatus(MSG_WAITING_FOR_PEER)
             event.defer()
             return
+        clear_status(self.unit, MSG_WAITING_FOR_PEER)
 
         if self.unit.is_leader() and not self.state.cluster.internal_user_credentials:
             for user in CHARM_USERS:
                 self.state.cluster.update({f"{user}-password": self.workload.generate_password()})
+        clear_status(self.unit, [MSG_INSTALLING, MSG_WAITING_FOR_USER_CREDENTIALS])
 
     def reconcile(self, event: EventBase) -> None:
         """Generic handler for all 'something changed, update' events across all relations."""
@@ -134,66 +148,58 @@ def _on_secret_changed(self, event: SecretChangedEvent):
             self.state.cluster.relation.id,
             None,  # type:ignore noqa
         ):  # Changes with the soon upcoming new version of DP-libs STILL within this POC
-
-            if (
-                self.config_manager.config_changed()
-                and self.state.unit_server.started
-                # and self.upgrade_events.idle
-            ):
-                logger.info(f"Secret {event.secret.label} changed.")
+            logger.info(f"Secret {event.secret.label} changed.")
+            self.reconcile(event)
 
     def _start(self, event: EventBase) -> None:
         """Forces a rolling-restart event.
 
         Necessary for ensuring that `on_start` restarts roll.
         """
         # if not self.state.peer_relation or not self.state.stable or not self.upgrade_events.idle:
-        self.unit.status = MaintenanceStatus("Starting...")
+        self.unit.status = MaintenanceStatus(MSG_STARTING)
         if not self.state.peer_relation or not self.state.stable:
             event.defer()
             return
 
-        # not needed during application init
-        # only needed for scenarios where the LXD goes down (e.g PC shutdown)
-        if not self.workload.alive():
-            self.on[f"{self.restart.name}"].acquire_lock.emit()
-        else:
-            self.unit.status = ActiveStatus()
+        self.reconcile(event)
+        clear_status(self.unit, MSG_STARTING)
 
     def _restart(self, event: EventBase) -> None:
         """Handler for emitted restart events."""
         # if not self.state.stable or not self.upgrade_events.idle:
-        if not self.state.stable:
-            event.defer()
+        #     event.defer()
+        #     return
+        if not self.state.unit_server.started:
+            self.reconcile(event)
             return
 
-        logger.info(f"{self.unit.name} (re)starting...")
+        logger.info(f"{self.unit.name} restarting...")
         self.workload.restart()
 
         start_time = time.time()
         while not self.workload.alive() and time.time() - start_time < RESTART_TIMEOUT:
             time.sleep(5)
 
-        self.unit.status = ActiveStatus()
+        clear_status(self.unit, [MSG_STARTING, MSG_STARTING_SERVER])
 
     # --- CONVENIENCE METHODS ---
 
     def init_server(self):
         """Calls startup functions for server start."""
         # don't run if leader has not yet created passwords
         if not self.state.cluster.internal_user_credentials:
-            self.unit.status = MaintenanceStatus("waiting for passwords to be created")
+            self.unit.status = MaintenanceStatus(MSG_WAITING_FOR_USER_CREDENTIALS)
             return
 
-        self.unit.status = MaintenanceStatus("starting Opensearch Dashboards server")
+        self.unit.status = MaintenanceStatus(MSG_STARTING_SERVER)
         logger.info(f"{self.unit.name} initializing...")
 
         logger.debug("setting properties")
         self.config_manager.set_dashboard_properties()
 
         logger.debug("starting Opensearch Dashboards service")
         self.workload.start()
-        self.unit.status = ActiveStatus()
 
         # unit flags itself as 'started' so it can be retrieved by the leader
         logger.info(f"{self.unit.name} started")
@@ -204,6 +210,7 @@ def init_server(self):
                 "state": "started",
             }
         )
+        clear_status(self.unit, MSG_STARTING_SERVER)
 
 
 if __name__ == "__main__":

src/events/tls.py

@@ -102,10 +102,6 @@ def _on_certificate_available(self, event: CertificateAvailableEvent) -> None:
             logger.error("Can't use certificate, found unknown CSR")
             return
 
-        # if certificate already exists, this event must be new, flag restart
-        if self.charm.state.unit_server.certificate:
-            self.charm.on[f"{self.charm.restart.name}"].acquire_lock.emit()
-
         self.charm.state.unit_server.update(
             {"certificate": event.certificate, "ca-cert": event.ca}
         )
@@ -116,8 +112,6 @@ def _on_certificate_available(self, event: CertificateAvailableEvent) -> None:
         # self.charm.tls_manager.set_truststore()
         # self.charm.tls_manager.set_p12_keystore()
 
-        self.charm.on[f"{self.charm.restart.name}"].acquire_lock.emit()
-
     def _on_certificate_expiring(self, _: EventBase) -> None:
         """Handler for `certificates_expiring` event when certs need renewing."""
         if not (self.charm.state.unit_server.private_key or self.charm.state.unit_server.csr):

src/helpers.py

@@ -0,0 +1,16 @@
+#!/usr/bin/env python3
+# Copyright 2022 Canonical Ltd.
+# See LICENSE file for licensing details.
+
+"""Charmed Machine Operator for Apache Opensearch Dashboards."""
+
+from ops.model import ActiveStatus, Application, Unit
+
+
+def clear_status(scope_obj: Unit | Application, messages: str | list[str]) -> None:
+    """Clear status if set."""
+    if not isinstance(messages, list):
+        messages = [messages]
+
+    if any([scope_obj.status.message == message for message in messages]):
+        scope_obj.status = ActiveStatus()

src/literals.py

@@ -33,3 +33,12 @@
 PEER_UNIT_SECRETS = ["ca-cert", "csr", "certificate", "private-key"]
 
 RESTART_TIMEOUT = 30
+
+
+# Status messages
+
+MSG_INSTALLING = "installing Opensearch Dashboards..."
+MSG_STARTING = "starting..."
+MSG_STARTING_SERVER = "starting Opensearch Dashboards server..."
+MSG_WAITING_FOR_USER_CREDENTIALS = "waiting for passwords to be created"
+MSG_WAITING_FOR_PEER = "waiting for peer relation"

tests/unit/test_charm.py

@@ -9,10 +9,11 @@
 import pytest
 import yaml
 from ops.framework import EventBase
-from ops.model import ActiveStatus, BlockedStatus
+from ops.model import ActiveStatus, BlockedStatus, MaintenanceStatus, WaitingStatus
 from ops.testing import Harness
 
 from charm import OpensearchDasboardsCharm
+from helpers import clear_status
 from literals import CHARM_KEY, CONTAINER, PEER, SUBSTRATE
 
 logger = logging.getLogger(__name__)
@@ -37,6 +38,20 @@ def harness():
     return harness
 
 
+def test_clear_status(harness):
+    harness.charm.unit.status = MaintenanceStatus("x")
+    clear_status(harness.charm.unit, "x")
+    assert isinstance(harness.charm.unit.status, ActiveStatus)
+
+    harness.charm.unit.status = WaitingStatus("y")
+    clear_status(harness.charm.unit, "y")
+    assert isinstance(harness.charm.unit.status, ActiveStatus)
+
+    harness.charm.unit.status = BlockedStatus("z")
+    clear_status(harness.charm.unit, "z")
+    assert isinstance(harness.charm.unit.status, ActiveStatus)
+
+
 def test_install_fails_create_passwords_until_peer_relation(harness):
     with harness.hooks_disabled():
         harness.set_leader(True)
@@ -197,51 +212,28 @@ def test_relation_changed_restarts(harness):
         patched_restart.assert_called_once()
 
 
-def test_restart_fails_not_related(harness):
-    with harness.hooks_disabled():
-        peer_rel_id = harness.add_relation(PEER, CHARM_KEY)
-        harness.add_relation_unit(peer_rel_id, f"{CHARM_KEY}/0")
-        harness.set_planned_units(1)
-
-    with (
-        patch("workload.ODWorkload.restart") as patched,
-        patch("ops.framework.EventBase.defer"),
-    ):
-        harness.charm._restart(EventBase)
-        patched.assert_not_called()
-
-
 def test_restart_fails_not_started(harness):
     with harness.hooks_disabled():
         peer_rel_id = harness.add_relation(PEER, CHARM_KEY)
         harness.add_relation_unit(peer_rel_id, f"{CHARM_KEY}/0")
         harness.set_planned_units(1)
 
     with (
-        patch("workload.ODWorkload.restart") as patched,
-        patch("ops.framework.EventBase.defer"),
-    ):
-        harness.charm._restart(EventBase)
-        patched.assert_not_called()
-
-
-def test_restart_fails_not_added(harness):
-    with harness.hooks_disabled():
-        peer_rel_id = harness.add_relation(PEER, CHARM_KEY)
-        harness.add_relation_unit(peer_rel_id, f"{CHARM_KEY}/0")
-        harness.set_planned_units(1)
-        harness.update_relation_data(peer_rel_id, f"{CHARM_KEY}/0", {"state": "started"})
-
-    with (
-        patch("workload.ODWorkload.restart") as patched,
-        patch("ops.framework.EventBase.defer"),
+        patch("workload.ODWorkload.restart") as patched_restart,
+        patch("workload.ODWorkload.start") as patched_start,
+        patch(
+            "core.models.ODCluster.internal_user_credentials",
+            new_callable=PropertyMock,
+            return_value={"user": "password"},
+        ),
+        patch("managers.config.ConfigManager.set_dashboard_properties"),
     ):
         harness.charm._restart(EventBase)
-        patched.assert_not_called()
+        patched_restart.assert_not_called()
+        patched_start.assert_called_once()
 
 
-@pytest.mark.parametrize("stable, restarts", [(True, 1), (False, 0)])
-def test_restart_restarts_with_sleep(harness, stable, restarts):
+def test_restart_restarts_with_sleep(harness):
     with harness.hooks_disabled():
         peer_rel_id = harness.add_relation(PEER, CHARM_KEY)
         harness.add_relation_unit(peer_rel_id, f"{CHARM_KEY}/0")
@@ -252,28 +244,10 @@ def test_restart_restarts_with_sleep(harness, stable, restarts):
     with (
         patch("workload.ODWorkload.restart") as patched_restart,
         patch("time.sleep") as patched_sleep,
-        patch("core.cluster.ClusterState.stable", new_callable=PropertyMock, return_value=stable),
-    ):
-        harness.charm._restart(EventBase(harness.charm))
-        assert patched_restart.call_count == restarts
-        assert patched_sleep.call_count >= restarts
-
-
-def test_restart_restarts_snap_sets_active_status(harness):
-    with harness.hooks_disabled():
-        peer_rel_id = harness.add_relation(PEER, CHARM_KEY)
-        harness.add_relation_unit(peer_rel_id, f"{CHARM_KEY}/0")
-        harness.set_planned_units(1)
-        harness.update_relation_data(peer_rel_id, f"{CHARM_KEY}/0", {"state": "started"})
-        harness.update_relation_data(peer_rel_id, f"{CHARM_KEY}", {"0": "added"})
-
-    with (
-        patch("workload.ODWorkload.restart"),
-        patch("core.cluster.ClusterState.stable", new_callable=PropertyMock, return_value=True),
-        patch("time.sleep"),
     ):
         harness.charm._restart(EventBase(harness.charm))
-        assert isinstance(harness.model.unit.status, ActiveStatus)
+        patched_restart.assert_called_once()
+        assert patched_sleep.call_count >= 1
 
 
 def test_init_server_calls_necessary_methods(harness):
@@ -310,22 +284,6 @@ def test_config_changed_applies_relation_data(harness):
         patched.assert_called_once()
 
 
-def test_restart_defers_if_not_stable(harness):
-    with harness.hooks_disabled():
-        _ = harness.add_relation(PEER, CHARM_KEY)
-        harness.set_leader(True)
-
-    with (
-        patch("core.cluster.ClusterState.stable", new_callable=PropertyMock(return_value=False)),
-        patch("managers.config.ConfigManager.config_changed") as patched_apply,
-        patch("ops.framework.EventBase.defer") as patched_defer,
-    ):
-        harness.charm._restart(EventBase)
-
-        patched_apply.assert_not_called()
-        patched_defer.assert_called_once()
-
-
 # def test_port_updates_if_tls(harness):
 #     with harness.hooks_disabled():
 #         harness.add_relation(PEER, CHARM_KEY)