From 52377e84cfaab576c0fcc95b38e5d700d68df054 Mon Sep 17 00:00:00 2001 From: Guillaume Boutry Date: Wed, 5 Jun 2024 19:14:44 +0200 Subject: [PATCH] [all] only publish active status at the end of the hook The status_compound by designed set the unit's status many times during hook execution. The unit's status is directly published to the controller. This leads to outside observers seeing active status (and lot of chatter around statuses) when the unit is in fact not ready. With this change, units will only publish an active status at the end of the hook execution. All other levels are still directly published to the controller. Units will no longer publish the WaitingStatus("no status yet"). This creates a lot of chatter and holds little value. Re-organize keystone __init__ not to publish false `Service not bootstrap` status. Closes-Bug: #2067016 Change-Id: Ie73b95972a44833ba4509f8fd2c2f52ed476004d --- charms/keystone-k8s/src/charm.py | 7 ++- charms/keystone-k8s/src/utils/manager.py | 15 +++--- .../sunbeam-clusterd/tests/unit/test_charm.py | 1 + ops-sunbeam/ops_sunbeam/charm.py | 13 +++++ ops-sunbeam/ops_sunbeam/compound_status.py | 47 ++++++++++++------- .../tests/unit_tests/test_compound_status.py | 3 ++ 6 files changed, 59 insertions(+), 27 deletions(-) diff --git a/charms/keystone-k8s/src/charm.py b/charms/keystone-k8s/src/charm.py index d93c309e..26d216d1 100755 --- a/charms/keystone-k8s/src/charm.py +++ b/charms/keystone-k8s/src/charm.py @@ -70,7 +70,6 @@ from ops.main import ( main, ) from ops.model import ( - ActiveStatus, MaintenanceStatus, ModelError, Relation, @@ -342,10 +341,12 @@ class KeystoneOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): SEND_CA_CERT_RELATION_NAME = "send-ca-cert" def __init__(self, framework): - super().__init__(framework) + # NOTE(gboutry): super().__init__ will call self.bootstrapped() which tries to + # make use of the keystone_manager self.keystone_manager = manager.KeystoneManager( self, KEYSTONE_CONTAINER ) + super().__init__(framework) self._state.set_default(admin_domain_name="admin_domain") self._state.set_default(admin_domain_id=None) self._state.set_default(default_domain_id=None) @@ -388,8 +389,6 @@ class KeystoneOperatorCharm(sunbeam_charm.OSBaseOperatorAPICharm): self.on.list_ca_certs_action, self._list_ca_certs_action, ) - if self.bootstrapped(): - self.bootstrap_status.set(ActiveStatus()) def _retrieve_or_set_secret( self, diff --git a/charms/keystone-k8s/src/utils/manager.py b/charms/keystone-k8s/src/utils/manager.py index 5fee497b..a192be20 100644 --- a/charms/keystone-k8s/src/utils/manager.py +++ b/charms/keystone-k8s/src/utils/manager.py @@ -21,6 +21,7 @@ from typing import ( ) import ops.pebble +import ops_sunbeam.charm as sunbeam_charm import ops_sunbeam.guard as sunbeam_guard from keystoneauth1 import ( session, @@ -31,9 +32,6 @@ from keystoneauth1.identity import ( from keystoneclient.v3 import ( client, ) -from ops import ( - framework, -) from ops.model import ( MaintenanceStatus, ) @@ -45,12 +43,15 @@ from utils.client import ( logger = logging.getLogger(__name__) -class KeystoneManager(framework.Object): +class KeystoneManager: """Class for managing interactions with keystone api.""" - def __init__(self, charm, container_name): + def __init__( + self, + charm: sunbeam_charm.OSBaseOperatorCharmK8S, + container_name: str, + ): """Setup the manager.""" - super().__init__(charm, "keystone-manager") self.charm = charm self.container_name = container_name self._api = None @@ -61,7 +62,7 @@ class KeystoneManager(framework.Object): pebble_handler = self.charm.get_named_pebble_handler( self.container_name ) - return pebble_handler.execute(cmd, exception_on_error=True, **kwargs) + return pebble_handler.execute(cmd, exception_on_error, **kwargs) @property def api(self): diff --git a/charms/sunbeam-clusterd/tests/unit/test_charm.py b/charms/sunbeam-clusterd/tests/unit/test_charm.py index ff93a918..df4f1243 100644 --- a/charms/sunbeam-clusterd/tests/unit/test_charm.py +++ b/charms/sunbeam-clusterd/tests/unit/test_charm.py @@ -65,6 +65,7 @@ class TestCharm(test_utils.CharmTestCase): self.harness.set_leader() self.harness.charm.on.config_changed.emit() + self.harness.evaluate_status() self.assertEqual(self.harness.charm.unit.status, ops.ActiveStatus()) self.ensure_snap_present.assert_called() self.harness.charm._clusterd.bootstrap.assert_called_once() diff --git a/ops-sunbeam/ops_sunbeam/charm.py b/ops-sunbeam/ops_sunbeam/charm.py index 3f82c9c1..f93e6a15 100644 --- a/ops-sunbeam/ops_sunbeam/charm.py +++ b/ops-sunbeam/ops_sunbeam/charm.py @@ -109,6 +109,9 @@ class OSBaseOperatorCharm(ops.charm.CharmBase): self.framework.observe(self.on.secret_changed, self._on_secret_changed) self.framework.observe(self.on.secret_rotate, self._on_secret_rotate) self.framework.observe(self.on.secret_remove, self._on_secret_remove) + self.framework.observe( + self.on.collect_unit_status, self._on_collect_unit_status_event + ) def can_add_handler( self, @@ -345,6 +348,16 @@ class OSBaseOperatorCharm(ops.charm.CharmBase): """ return {"database": self.service_name.replace("-", "_")} + def _on_collect_unit_status_event(self, event: ops.CollectStatusEvent): + """Publish the unit status. + + collect_unit_status is called at the end of the hook's execution, + making it the best place to publish an active status. + """ + status = self.status_pool.compute_status() + if status: + event.add_status(status) + def _on_config_changed(self, event: ops.framework.EventBase) -> None: self.configure_charm(event) diff --git a/ops-sunbeam/ops_sunbeam/compound_status.py b/ops-sunbeam/ops_sunbeam/compound_status.py index 16480fc2..841f3dbe 100644 --- a/ops-sunbeam/ops_sunbeam/compound_status.py +++ b/ops-sunbeam/ops_sunbeam/compound_status.py @@ -44,7 +44,6 @@ from ops.model import ( ActiveStatus, StatusBase, UnknownStatus, - WaitingStatus, ) from ops.storage import ( NoSnapshotError, @@ -222,10 +221,10 @@ class StatusPool(Object): self._charm.framework.save_snapshot(self._state) self._charm.framework._storage.commit() - def on_update(self) -> None: - """Update the unit status with the current highest priority status. + def compute_status(self) -> StatusBase | None: + """Compute the status to show to the user. - Use as a hook to run whenever a status is updated in the pool. + This is the highest priority status in the pool. """ status = ( sorted(self._pool.values(), key=lambda x: x.priority())[0] @@ -233,18 +232,34 @@ class StatusPool(Object): else None ) if status is None or status.status.name == "unknown": - self._charm.unit.status = WaitingStatus("no status set yet") - elif status.status.name == "active" and not status.message(): + return None + if status.status.name == "active" and not status.message(): # Avoid status name prefix if everything is active with no message. # If there's a message, then we want the prefix # to help identify where the message originates. - self._charm.unit.status = ActiveStatus("") - else: - message = status.message() - self._charm.unit.status = StatusBase.from_name( - status.status.name, - "({}){}".format( - status.label, - " " + message if message else "", - ), - ) + return ActiveStatus("") + return StatusBase.from_name( + status.status.name, + "({}){}".format( + status.label, + " " + status.message() if status.message() else "", + ), + ) + + def on_update(self) -> None: + """Update the unit status with the current highest priority status. + + Use as a hook to run whenever a status is updated in the pool. + on_update will never update the unit status to active, because this is + synced directly to the controller. Making the unit pass to active + multiple times during a hook while it's not. + + It is the charm's responsibility to set the unit status to active, + collect_unit_status is the best place to set a status at end of + the hook. + """ + status = self.compute_status() + if not status: + return + if status.name != "active": + self._charm.unit.status = status diff --git a/ops-sunbeam/tests/unit_tests/test_compound_status.py b/ops-sunbeam/tests/unit_tests/test_compound_status.py index aec6ab78..7e61ccbb 100644 --- a/ops-sunbeam/tests/unit_tests/test_compound_status.py +++ b/ops-sunbeam/tests/unit_tests/test_compound_status.py @@ -178,12 +178,15 @@ class TestCompoundStatus(test_utils.CharmTestCase): # also need to manually activate other default statuses pool._pool["container:my-service"].set(ActiveStatus("")) + self.harness.evaluate_status() # all empty messages should end up as an empty unit status self.assertEqual(self.harness.charm.unit.status, ActiveStatus("")) # if there's a message (on the highest priority status), # it should also show the status prefix status2.set(ActiveStatus("a message")) + + self.harness.evaluate_status() self.assertEqual( self.harness.charm.unit.status, ActiveStatus("(test2) a message") )