From 91e8f9dc63af521fe3ebf10864652ab46906b2d4 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 27 Aug 2026 05:19:25 +0000 Subject: [PATCH 1/7] fix(garm): gate teardown before state reconstruction Use Juju planned units as the local teardown signal before the paas-charm state decorator runs. Keep inherited handlers inert and prevent update-status from refreshing ingress after teardown begins. --- charms/garm/src/charm.py | 39 +++++++++- charms/garm/tests/unit/test_charm.py | 108 +++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 2 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 8edcac2a..509af6ba 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -16,6 +16,7 @@ import ops import paas_charm.go from paas_charm.app import WorkloadConfig +from paas_charm.charm_state import CharmState as PaasCharmState from paas_charm.charm_utils import block_if_invalid_data from charm_state import ( @@ -168,11 +169,41 @@ def __init__(self, *args: typing.Any) -> None: self.framework.observe(self.on.update_status, self._reconcile) self.framework.observe(self.on.remove, self._on_remove) + def _is_tearing_down(self) -> bool: + """Return whether Juju plans no remaining units for the local application.""" + return self.app.planned_units() == 0 + + def _reconcile(self, event: ops.EventBase) -> None: + """Skip normal reconciliation once local application teardown starts.""" + if self._is_tearing_down(): + logger.info( + "Skipping normal GARM reconciliation for %s during local teardown", + event.handle.kind, + ) + return + self._normal_reconcile(event) + @block_if_invalid_data - def _reconcile(self, _: ops.EventBase) -> None: - """Reconcile charm state.""" + def _normal_reconcile(self, _: ops.EventBase) -> None: + """Reconcile active GARM charm state.""" self.restart() + def _create_charm_state(self) -> PaasCharmState: + """Create framework state without reading relations during local teardown.""" + if self._is_tearing_down(): + return PaasCharmState( + framework=self._framework_name, + is_secret_storage_ready=False, + ) + return super()._create_charm_state() + + def _on_update_status(self, event: ops.HookEvent) -> None: + """Run the framework update-status handler only while active.""" + if self._is_tearing_down(): + logger.info("Skipping update-status handling during local teardown") + return + super()._on_update_status(event) + def _on_remove(self, _: ops.RemoveEvent) -> None: """Drain GARM resources before Juju removes the application.""" if not self.unit.is_leader(): @@ -287,6 +318,10 @@ def restart(self, rerun_migrations: bool = False) -> None: Args: rerun_migrations: Passed through to the parent restart. """ + if self._is_tearing_down(): + logger.info("Skipping GARM workload restart during local teardown") + return + self._ensure_secrets() if not self.is_ready(): diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 0922a4cf..116cd8f0 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1070,3 +1070,111 @@ def test_every_observed_event_reconciles( ctx.run(event(ctx, state), state) garm_api.auth.from_login.assert_called_once() + + +# The external secret is included in State because Scenario requires the exact object for +# secret-changed events. Teardown must not resolve it through normal charm-state construction. +_TEARDOWN_SECRET = Secret(id="secret:externalabcdefghijkl", tracked_content={"value": "external"}) + + +_TEARDOWN_EVENTS = [ + pytest.param(lambda ctx, _: ctx.on.config_changed(), id="config-changed"), + pytest.param( + lambda ctx, _: ctx.on.secret_changed(_TEARDOWN_SECRET), + id="secret-changed", + ), + pytest.param(lambda ctx, _: ctx.on.update_status(), id="update-status"), + pytest.param( + lambda ctx, state: ctx.on.pebble_ready(state.get_container(CONTAINER_NAME)), + id="pebble-ready", + ), + pytest.param( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, GARM_CONFIGURATOR_RELATION_NAME), remote_unit=0 + ), + id="configurator-relation-departed", + ), + pytest.param( + lambda ctx, state: ctx.on.relation_broken( + _relation(state, GARM_CONFIGURATOR_RELATION_NAME) + ), + id="configurator-relation-broken", + ), + pytest.param( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, DEBUG_SSH_INTEGRATION_NAME), remote_unit=0 + ), + id="debug-ssh-relation-departed", + ), + pytest.param( + lambda ctx, state: ctx.on.relation_broken(_relation(state, DEBUG_SSH_INTEGRATION_NAME)), + id="debug-ssh-relation-broken", + ), + pytest.param( + lambda ctx, state: ctx.on.relation_broken(_relation(state, "postgresql")), + id="postgresql-relation-broken", + ), +] + + +@pytest.mark.parametrize("event", _TEARDOWN_EVENTS) +def test_teardown_events_skip_framework_state_and_garm_reconciliation( + ctx: Context, garm_api: _GarmApiMocks, event: typing.Callable +): + """ + arrange: No GARM units remain planned, and normal state seams fail if called. + act: Emit an event that can arrive during teardown. + assert: The event returns without reconstructing state or reconciling GARM. + """ + state = _state(debug_ssh_related=True, planned_units=0, secrets=[_TEARDOWN_SECRET]) + + with ( + patch( + "paas_charm.charm_state.CharmState.from_charm", + side_effect=AssertionError("framework charm state was reconstructed"), + ), + patch( + "charm_state.CharmState.from_charm", + side_effect=AssertionError("GARM charm state was reconstructed"), + ), + patch.object( + GarmCharm, "_ensure_secrets", side_effect=AssertionError("secrets were read") + ), + patch.object( + GarmCharm, + "_get_postgresql_config", + side_effect=AssertionError("postgresql relation was read"), + ), + patch.object( + GarmCharm, + "_get_configurator_provider_configs", + side_effect=AssertionError("configurator relation was read"), + ), + ): + out = ctx.run(event(ctx, state), state) + + assert out is not None + garm_api.client.assert_not_called() + garm_api.auth.from_login.assert_not_called() + garm_api.github.assert_not_called() + garm_api.entity.assert_not_called() + garm_api.scaleset.assert_not_called() + + +def test_teardown_update_status_skips_ingress_refresh(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: The local GARM service is tearing down. + act: Emit update-status while ingress refresh would fail if invoked. + assert: The outer status guard returns before the base ingress refresh. + """ + state = _state(planned_units=0) + with ctx(ctx.on.update_status(), state) as manager: + with patch.object( + manager.charm._ingress, + "_publish_auto_data", + side_effect=AssertionError("ingress refresh ran during teardown"), + ): + out = manager.run() + + assert out is not None + garm_api.auth.from_login.assert_not_called() From 7de24e7e9cc1d899fddd9368c60f6b2595371f0e Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 27 Aug 2026 07:04:39 +0000 Subject: [PATCH 2/7] refactor(garm): route inherited hooks through teardown gate --- charms/garm/src/charm.py | 70 ++++++++++++++++++++++++---- charms/garm/tests/unit/test_charm.py | 25 ++++++++-- 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 509af6ba..d585eadc 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -16,7 +16,6 @@ import ops import paas_charm.go from paas_charm.app import WorkloadConfig -from paas_charm.charm_state import CharmState as PaasCharmState from paas_charm.charm_utils import block_if_invalid_data from charm_state import ( @@ -183,19 +182,65 @@ def _reconcile(self, event: ops.EventBase) -> None: return self._normal_reconcile(event) + def _reconcile_with_migrations(self, event: ops.EventBase) -> None: + """Dispatch a database event through the teardown gate.""" + if self._is_tearing_down(): + logger.info( + "Skipping normal GARM reconciliation for %s during local teardown", + event.handle.kind, + ) + return + self._normal_reconcile_with_migrations(event) + @block_if_invalid_data def _normal_reconcile(self, _: ops.EventBase) -> None: """Reconcile active GARM charm state.""" self.restart() - def _create_charm_state(self) -> PaasCharmState: - """Create framework state without reading relations during local teardown.""" - if self._is_tearing_down(): - return PaasCharmState( - framework=self._framework_name, - is_secret_storage_ready=False, - ) - return super()._create_charm_state() + @block_if_invalid_data + def _normal_reconcile_with_migrations(self, _: ops.EventBase) -> None: + """Reconcile active GARM state and rerun database migrations.""" + self.restart(rerun_migrations=True) + + def _on_config_changed(self, event: ops.EventBase) -> None: + """Route config changes through GARM's teardown gate.""" + self._reconcile(event) + + def _on_secret_changed(self, event: ops.EventBase) -> None: + """Route secret changes through GARM's teardown gate.""" + self._reconcile(event) + + def _on_secret_storage_relation_changed(self, event: ops.RelationEvent) -> None: + """Route secret-storage changes through GARM's teardown gate.""" + self._reconcile(event) + + def _on_secret_storage_relation_departed(self, event: ops.HookEvent) -> None: + """Route secret-storage departures through GARM's teardown gate.""" + self._reconcile(event) + + def _on_postgresql_database_database_created(self, event: ops.EventBase) -> None: + """Route PostgreSQL database creation through GARM's teardown gate.""" + self._reconcile_with_migrations(event) + + def _on_postgresql_database_endpoints_changed(self, event: ops.EventBase) -> None: + """Route PostgreSQL endpoint changes through GARM's teardown gate.""" + self._reconcile_with_migrations(event) + + def _on_postgresql_database_relation_broken(self, event: ops.RelationBrokenEvent) -> None: + """Route PostgreSQL relation loss through GARM's teardown gate.""" + self._reconcile(event) + + def _on_ingress_ready(self, event: ops.HookEvent) -> None: + """Route ingress readiness through GARM's teardown gate.""" + self._reconcile(event) + + def _on_ingress_revoked(self, event: ops.HookEvent) -> None: + """Route ingress revocation through GARM's teardown gate.""" + self._reconcile(event) + + def _on_pebble_ready(self, event: ops.PebbleReadyEvent) -> None: + """Route Pebble readiness through GARM's teardown gate.""" + self._reconcile(event) def _on_update_status(self, event: ops.HookEvent) -> None: """Run the framework update-status handler only while active.""" @@ -204,6 +249,13 @@ def _on_update_status(self, event: ops.HookEvent) -> None: return super()._on_update_status(event) + def _on_rotate_secret_key_action(self, event: ops.ActionEvent) -> None: + """Reject secret rotation during teardown before the base decorator runs.""" + if self._is_tearing_down(): + event.fail("cannot rotate the secret key during local teardown") + return + super()._on_rotate_secret_key_action(event) + def _on_remove(self, _: ops.RemoveEvent) -> None: """Drain GARM resources before Juju removes the application.""" if not self.unit.is_leader(): diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 116cd8f0..c7242bd3 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1129,9 +1129,10 @@ def test_teardown_events_skip_framework_state_and_garm_reconciliation( state = _state(debug_ssh_related=True, planned_units=0, secrets=[_TEARDOWN_SECRET]) with ( - patch( - "paas_charm.charm_state.CharmState.from_charm", - side_effect=AssertionError("framework charm state was reconstructed"), + patch.object( + GarmCharm, + "_create_charm_state", + side_effect=AssertionError("charm state was created during teardown"), ), patch( "charm_state.CharmState.from_charm", @@ -1161,6 +1162,24 @@ def test_teardown_events_skip_framework_state_and_garm_reconciliation( garm_api.scaleset.assert_not_called() +def test_teardown_handlers_skip_charm_state_creation(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: No GARM units remain planned. + act: Emit an inherited config-changed event while state creation is forbidden. + assert: The subclass observer guard returns before the framework state factory. + """ + state = _state(planned_units=0) + with patch.object( + GarmCharm, + "_create_charm_state", + side_effect=AssertionError("charm state was created during teardown"), + ): + out = ctx.run(ctx.on.config_changed(), state) + + assert out is not None + garm_api.auth.from_login.assert_not_called() + + def test_teardown_update_status_skips_ingress_refresh(ctx: Context, garm_api: _GarmApiMocks): """ arrange: The local GARM service is tearing down. From 612b3af323d98bc9439d28c0fb6da507359ffd2d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 27 Aug 2026 09:00:07 +0000 Subject: [PATCH 3/7] refactor(garm): loop over reconcile observers --- charms/garm/src/charm.py | 54 ++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 35 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index d585eadc..467199e2 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -130,42 +130,26 @@ def __init__(self, *args: typing.Any) -> None: args: Passed through to CharmBase. """ super().__init__(*args) - self.framework.observe(self.on.install, self._reconcile) - self.framework.observe(self.on.leader_elected, self._reconcile) - self.framework.observe( - self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_joined, - self._reconcile, - ) - self.framework.observe( - self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_changed, - self._reconcile, - ) - self.framework.observe( - self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_departed, - self._reconcile, - ) - self.framework.observe( - self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_broken, - self._reconcile, - ) + for event in ( + self.on.install, + self.on.leader_elected, + self.on.update_status, + ): + self.framework.observe(event, self._reconcile) + + for relation_events in ( + self.on[GARM_CONFIGURATOR_RELATION_NAME], + self.on[DEBUG_SSH_INTEGRATION_NAME], + ): + for event in ( + relation_events.relation_joined, + relation_events.relation_changed, + relation_events.relation_departed, + relation_events.relation_broken, + ): + self.framework.observe(event, self._reconcile) + self.framework.observe(self.on.get_credentials_action, self._on_get_credentials_action) - self.framework.observe( - self.on[DEBUG_SSH_INTEGRATION_NAME].relation_joined, - self._reconcile, - ) - self.framework.observe( - self.on[DEBUG_SSH_INTEGRATION_NAME].relation_changed, - self._reconcile, - ) - self.framework.observe( - self.on[DEBUG_SSH_INTEGRATION_NAME].relation_departed, - self._reconcile, - ) - self.framework.observe( - self.on[DEBUG_SSH_INTEGRATION_NAME].relation_broken, - self._reconcile, - ) - self.framework.observe(self.on.update_status, self._reconcile) self.framework.observe(self.on.remove, self._on_remove) def _is_tearing_down(self) -> bool: From bf32e1e9459314b2fad9450bca1777c9405d019d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 27 Aug 2026 09:18:41 +0000 Subject: [PATCH 4/7] refactor(garm): isolate teardown dispatch --- charms/garm/src/charm.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 467199e2..3ad5e6f6 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -157,25 +157,26 @@ def _is_tearing_down(self) -> bool: return self.app.planned_units() == 0 def _reconcile(self, event: ops.EventBase) -> None: - """Skip normal reconciliation once local application teardown starts.""" + """Reconcile GARM, or handle local teardown before normal state construction.""" if self._is_tearing_down(): - logger.info( - "Skipping normal GARM reconciliation for %s during local teardown", - event.handle.kind, - ) + self._teardown(event) return self._normal_reconcile(event) def _reconcile_with_migrations(self, event: ops.EventBase) -> None: - """Dispatch a database event through the teardown gate.""" + """Reconcile a database event, preserving the teardown gate.""" if self._is_tearing_down(): - logger.info( - "Skipping normal GARM reconciliation for %s during local teardown", - event.handle.kind, - ) + self._teardown(event) return self._normal_reconcile_with_migrations(event) + def _teardown(self, event: ops.EventBase) -> None: + """Handle a local teardown event without normal reconciliation.""" + logger.info( + "Skipping normal GARM reconciliation for %s during local teardown", + event.handle.kind, + ) + @block_if_invalid_data def _normal_reconcile(self, _: ops.EventBase) -> None: """Reconcile active GARM charm state.""" From ebfac444a5555345f0082f1b400cd024e1f71c96 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 27 Aug 2026 09:52:35 +0000 Subject: [PATCH 5/7] refactor(garm): alias inherited reconcile hooks --- charms/garm/src/charm.py | 52 +++++++++++++--------------------------- 1 file changed, 17 insertions(+), 35 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 3ad5e6f6..359d330a 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -187,45 +187,27 @@ def _normal_reconcile_with_migrations(self, _: ops.EventBase) -> None: """Reconcile active GARM state and rerun database migrations.""" self.restart(rerun_migrations=True) - def _on_config_changed(self, event: ops.EventBase) -> None: - """Route config changes through GARM's teardown gate.""" + def _route_reconcile(self, event: ops.EventBase) -> None: + """Route an inherited framework event through GARM's teardown gate.""" self._reconcile(event) - def _on_secret_changed(self, event: ops.EventBase) -> None: - """Route secret changes through GARM's teardown gate.""" - self._reconcile(event) - - def _on_secret_storage_relation_changed(self, event: ops.RelationEvent) -> None: - """Route secret-storage changes through GARM's teardown gate.""" - self._reconcile(event) - - def _on_secret_storage_relation_departed(self, event: ops.HookEvent) -> None: - """Route secret-storage departures through GARM's teardown gate.""" - self._reconcile(event) - - def _on_postgresql_database_database_created(self, event: ops.EventBase) -> None: - """Route PostgreSQL database creation through GARM's teardown gate.""" - self._reconcile_with_migrations(event) - - def _on_postgresql_database_endpoints_changed(self, event: ops.EventBase) -> None: - """Route PostgreSQL endpoint changes through GARM's teardown gate.""" + # PaasCharm.__init__ resolves these hook names dynamically. Aliasing them keeps the + # teardown check before the inherited decorators without registering duplicate observers. + _on_config_changed = _route_reconcile + _on_secret_changed = _route_reconcile + _on_secret_storage_relation_changed = _route_reconcile + _on_secret_storage_relation_departed = _route_reconcile + _on_postgresql_database_relation_broken = _route_reconcile + _on_ingress_ready = _route_reconcile + _on_ingress_revoked = _route_reconcile + _on_pebble_ready = _route_reconcile + + def _route_reconcile_with_migrations(self, event: ops.EventBase) -> None: + """Route an inherited database event through GARM's migration gate.""" self._reconcile_with_migrations(event) - def _on_postgresql_database_relation_broken(self, event: ops.RelationBrokenEvent) -> None: - """Route PostgreSQL relation loss through GARM's teardown gate.""" - self._reconcile(event) - - def _on_ingress_ready(self, event: ops.HookEvent) -> None: - """Route ingress readiness through GARM's teardown gate.""" - self._reconcile(event) - - def _on_ingress_revoked(self, event: ops.HookEvent) -> None: - """Route ingress revocation through GARM's teardown gate.""" - self._reconcile(event) - - def _on_pebble_ready(self, event: ops.PebbleReadyEvent) -> None: - """Route Pebble readiness through GARM's teardown gate.""" - self._reconcile(event) + _on_postgresql_database_database_created = _route_reconcile_with_migrations + _on_postgresql_database_endpoints_changed = _route_reconcile_with_migrations def _on_update_status(self, event: ops.HookEvent) -> None: """Run the framework update-status handler only while active.""" From 600bd6a8e4833d095e370bce69c65f71ec89653f Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 2 Sep 2026 04:10:48 +0000 Subject: [PATCH 6/7] [verified] fix(garm): harden teardown lifecycle compatibility Cover database migration and inherited guard routes, and fail loudly when the pinned paas-charm hook contract changes. Add the PR-scoped teardown decision-tree architecture diagram. --- charms/garm/src/charm.py | 36 ++- charms/garm/tests/unit/test_charm.py | 362 +++++++++++++++++++++++---- 2 files changed, 352 insertions(+), 46 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 359d330a..bc54a047 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -120,6 +120,36 @@ def _parse_pre_install_scripts(raw: str) -> dict[str, str]: return {} +_PAAS_CHARM_HOOKS: typing.Final[tuple[str, ...]] = ( + "_on_config_changed", + "_on_rotate_secret_key_action", + "_on_secret_changed", + "_on_secret_storage_relation_changed", + "_on_secret_storage_relation_departed", + "_on_postgresql_database_database_created", + "_on_postgresql_database_endpoints_changed", + "_on_postgresql_database_relation_broken", + "_on_ingress_ready", + "_on_ingress_revoked", + "_on_pebble_ready", + "_on_update_status", +) + + +def _validate_paas_charm_hook_contract() -> None: + """Fail loudly if the pinned paas-charm lifecycle hooks have changed.""" + base_classes = paas_charm.go.Charm.__mro__ + missing = [ + hook + for hook in _PAAS_CHARM_HOOKS + if not any(hook in base_class.__dict__ for base_class in base_classes) + ] + if missing: + raise RuntimeError( + "Unsupported paas-charm lifecycle API; missing expected hooks: " + ", ".join(missing) + ) + + class GarmCharm(paas_charm.go.Charm): """GARM charm — manages the GARM service via Pebble.""" @@ -129,6 +159,7 @@ def __init__(self, *args: typing.Any) -> None: Args: args: Passed through to CharmBase. """ + _validate_paas_charm_hook_contract() super().__init__(*args) for event in ( self.on.install, @@ -191,8 +222,9 @@ def _route_reconcile(self, event: ops.EventBase) -> None: """Route an inherited framework event through GARM's teardown gate.""" self._reconcile(event) - # PaasCharm.__init__ resolves these hook names dynamically. Aliasing them keeps the - # teardown check before the inherited decorators without registering duplicate observers. + # PaasCharm.__init__ resolves these hook names dynamically. These aliases keep the + # teardown check before block_if_invalid_data without registering duplicate observers. + # The compatibility check above makes this adapter fail loudly when the base API changes. _on_config_changed = _route_reconcile _on_secret_changed = _route_reconcile _on_secret_storage_relation_changed = _route_reconcile diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index c7242bd3..d15c7b26 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -30,6 +30,7 @@ except ImportError: import tomli as tomllib # type: ignore[no-redef] +import charm as charm_module import garm_template from charm import GARM_ADMIN_CREDENTIALS_LABEL, GARM_PORT, GARM_SECRETS_LABEL, GarmCharm from charm_state import DEBUG_SSH_INTEGRATION_NAME, GARM_CONFIGURATOR_RELATION_NAME @@ -178,9 +179,12 @@ def _state( leader: bool = True, can_connect: bool = True, postgresql_data: dict | None = None, + postgresql_local_unit_data: dict | None = None, configurator_related: bool = True, configurator_units_data: dict[int, dict] | None = None, debug_ssh_related: bool = False, + secret_storage_peers_data: dict[int, dict] | None = None, + ingress_data: dict | None = None, secrets: typing.Sequence[Secret] = (), unit_status: ops.StatusBase | None = None, app_status: ops.StatusBase | None = None, @@ -192,16 +196,31 @@ def _state( connection data, and whose single configurator unit publishes a full OpenStack provider and scaleset spec — the state in which restart() runs end to end. """ + postgresql_relation = Relation( + endpoint="postgresql", + remote_app_name="postgresql", + remote_app_data=_POSTGRESQL_DATA if postgresql_data is None else postgresql_data, + ) + if postgresql_local_unit_data is not None: + postgresql_relation = dataclasses.replace( + postgresql_relation, local_unit_data=postgresql_local_unit_data + ) relations: list[Relation | PeerRelation] = [ - Relation( - endpoint="postgresql", - remote_app_name="postgresql", - remote_app_data=_POSTGRESQL_DATA if postgresql_data is None else postgresql_data, - ), + postgresql_relation, PeerRelation( - endpoint="secret-storage", local_app_data={_SECRET_STORAGE_KEY: "peer-secret-key"} + endpoint="secret-storage", + local_app_data={_SECRET_STORAGE_KEY: "peer-secret-key"}, + peers_data={} if secret_storage_peers_data is None else secret_storage_peers_data, ), ] + if ingress_data is not None: + relations.append( + Relation( + endpoint="ingress", + remote_app_name="traefik", + remote_app_data=ingress_data, + ) + ) if configurator_related: relations.append( Relation( @@ -1013,15 +1032,54 @@ def test_controller_urls_derive_from_the_ingress_url( # --- Event wiring ------------------------------------------------------------------------- +@dataclasses.dataclass(frozen=True) +class _EventCase: + """A Scenario event and the state needed to emit it.""" + + emit: typing.Callable[[Context, State], object] + state_factory: typing.Callable[[], State] + + +def _event_case( + emit: typing.Callable[[Context, State], object], + state_factory: typing.Callable[[], State] = _state, +) -> _EventCase: + """Build an event case with a default ready state.""" + return _EventCase(emit=emit, state_factory=state_factory) + + def _relation(state: State, endpoint: str) -> Relation: """Return the state's relation on the given endpoint.""" return next(relation for relation in state.relations if relation.endpoint == endpoint) # type: ignore[return-value] +def _ready_state() -> State: + """Return a ready state with both GARM relation types present.""" + return _state(debug_ssh_related=True) + + +def _postgresql_relation_changed(ctx: Context, state: State) -> object: + """Emit the raw relation event that produces a database custom event.""" + return ctx.on.relation_changed(_relation(state, "postgresql")) + + +def _postgresql_endpoints_changed_state() -> State: + """Return a state whose PostgreSQL endpoint differs from the previous snapshot.""" + previous_data = {**_POSTGRESQL_DATA, "endpoints": "10.0.0.4:5432"} + return _state( + debug_ssh_related=True, + postgresql_local_unit_data={"data": json.dumps(previous_data)}, + ) + + _OBSERVED_EVENTS = [ - pytest.param(lambda ctx, _: ctx.on.install(), id="install"), - pytest.param(lambda ctx, _: ctx.on.leader_elected(), id="leader-elected"), - pytest.param(lambda ctx, _: ctx.on.update_status(), id="update-status"), + pytest.param(_event_case(lambda ctx, _: ctx.on.install(), _ready_state), id="install"), + pytest.param( + _event_case(lambda ctx, _: ctx.on.leader_elected(), _ready_state), id="leader-elected" + ), + pytest.param( + _event_case(lambda ctx, _: ctx.on.update_status(), _ready_state), id="update-status" + ), ] for _endpoint, _id in ( (GARM_CONFIGURATOR_RELATION_NAME, "configurator"), @@ -1029,35 +1087,60 @@ def _relation(state: State, endpoint: str) -> Relation: ): _OBSERVED_EVENTS += [ pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_joined( - _relation(state, endpoint) + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_joined( + _relation(state, endpoint) + ), + _ready_state, ), id=f"{_id}-relation-joined", ), pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_changed( - _relation(state, endpoint) + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_changed( + _relation(state, endpoint) + ), + _ready_state, ), id=f"{_id}-relation-changed", ), pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_departed( - _relation(state, endpoint), remote_unit=0 + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_departed( + _relation(state, endpoint), remote_unit=0 + ), + _ready_state, ), id=f"{_id}-relation-departed", ), pytest.param( - lambda ctx, state, endpoint=_endpoint: ctx.on.relation_broken( - _relation(state, endpoint) + _event_case( + lambda ctx, state, endpoint=_endpoint: ctx.on.relation_broken( + _relation(state, endpoint) + ), + _ready_state, ), id=f"{_id}-relation-broken", ), ] -@pytest.mark.parametrize("event", _OBSERVED_EVENTS) +_MIGRATION_EVENTS = [ + pytest.param( + _event_case(_postgresql_relation_changed, _ready_state), + id="postgresql-database-created", + ), + pytest.param( + _event_case(_postgresql_relation_changed, _postgresql_endpoints_changed_state), + id="postgresql-endpoints-changed", + ), +] +_OBSERVED_EVENTS += _MIGRATION_EVENTS + + +@pytest.mark.parametrize("event_case", _OBSERVED_EVENTS) def test_every_observed_event_reconciles( - ctx: Context, garm_api: _GarmApiMocks, event: typing.Callable + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase ): """ arrange: A ready charm related to garm-configurator and debug-ssh. @@ -1065,68 +1148,181 @@ def test_every_observed_event_reconciles( assert: Every one drives a full reconcile. The charm holds no per-event delta logic, so an observer that is missed leaves GARM unsynced until the next hook happens to fire. """ - state = _state(debug_ssh_related=True) + state = event_case.state_factory() - ctx.run(event(ctx, state), state) + ctx.run(event_case.emit(ctx, state), state) garm_api.auth.from_login.assert_called_once() +@pytest.mark.parametrize("event_case", _MIGRATION_EVENTS) +def test_database_events_reconcile_with_migrations(ctx: Context, event_case: _EventCase): + """ + arrange: An active charm receives a PostgreSQL database lifecycle event. + act: Run database-created or endpoints-changed. + assert: The active route calls restart with migrations enabled. + """ + state = event_case.state_factory() + + with patch.object(GarmCharm, "restart") as restart: + ctx.run(event_case.emit(ctx, state), state) + + restart.assert_called_once_with(rerun_migrations=True) + + # The external secret is included in State because Scenario requires the exact object for # secret-changed events. Teardown must not resolve it through normal charm-state construction. _TEARDOWN_SECRET = Secret(id="secret:externalabcdefghijkl", tracked_content={"value": "external"}) +def _teardown_state() -> State: + """Return a state for events that explicitly enter the teardown fallthrough.""" + return _state(debug_ssh_related=True, planned_units=0, secrets=[_TEARDOWN_SECRET]) + + +def _teardown_endpoints_changed_state() -> State: + """Return a teardown state that emits PostgreSQL endpoints-changed.""" + state = _postgresql_endpoints_changed_state() + return dataclasses.replace(state, planned_units=0, secrets=[_TEARDOWN_SECRET]) + + _TEARDOWN_EVENTS = [ - pytest.param(lambda ctx, _: ctx.on.config_changed(), id="config-changed"), pytest.param( - lambda ctx, _: ctx.on.secret_changed(_TEARDOWN_SECRET), + _event_case(lambda ctx, _: ctx.on.config_changed(), _teardown_state), id="config-changed" + ), + pytest.param( + _event_case( + lambda ctx, _: ctx.on.secret_changed(_TEARDOWN_SECRET), + _teardown_state, + ), id="secret-changed", ), - pytest.param(lambda ctx, _: ctx.on.update_status(), id="update-status"), pytest.param( - lambda ctx, state: ctx.on.pebble_ready(state.get_container(CONTAINER_NAME)), + _event_case(lambda ctx, _: ctx.on.update_status(), _teardown_state), id="update-status" + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.pebble_ready(state.get_container(CONTAINER_NAME)), + _teardown_state, + ), id="pebble-ready", ), pytest.param( - lambda ctx, state: ctx.on.relation_departed( - _relation(state, GARM_CONFIGURATOR_RELATION_NAME), remote_unit=0 + _event_case( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, GARM_CONFIGURATOR_RELATION_NAME), remote_unit=0 + ), + _teardown_state, ), id="configurator-relation-departed", ), pytest.param( - lambda ctx, state: ctx.on.relation_broken( - _relation(state, GARM_CONFIGURATOR_RELATION_NAME) + _event_case( + lambda ctx, state: ctx.on.relation_broken( + _relation(state, GARM_CONFIGURATOR_RELATION_NAME) + ), + _teardown_state, ), id="configurator-relation-broken", ), pytest.param( - lambda ctx, state: ctx.on.relation_departed( - _relation(state, DEBUG_SSH_INTEGRATION_NAME), remote_unit=0 + _event_case( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, DEBUG_SSH_INTEGRATION_NAME), remote_unit=0 + ), + _teardown_state, ), id="debug-ssh-relation-departed", ), pytest.param( - lambda ctx, state: ctx.on.relation_broken(_relation(state, DEBUG_SSH_INTEGRATION_NAME)), + _event_case( + lambda ctx, state: ctx.on.relation_broken( + _relation(state, DEBUG_SSH_INTEGRATION_NAME) + ), + _teardown_state, + ), id="debug-ssh-relation-broken", ), pytest.param( - lambda ctx, state: ctx.on.relation_broken(_relation(state, "postgresql")), + _event_case( + lambda ctx, state: ctx.on.relation_broken(_relation(state, "postgresql")), + _teardown_state, + ), id="postgresql-relation-broken", ), + pytest.param( + _event_case(_postgresql_relation_changed, _teardown_state), + id="postgresql-database-created", + ), + pytest.param( + _event_case(_postgresql_relation_changed, _teardown_endpoints_changed_state), + id="postgresql-endpoints-changed", + ), ] -@pytest.mark.parametrize("event", _TEARDOWN_EVENTS) -def test_teardown_events_skip_framework_state_and_garm_reconciliation( - ctx: Context, garm_api: _GarmApiMocks, event: typing.Callable -): - """ - arrange: No GARM units remain planned, and normal state seams fail if called. - act: Emit an event that can arrive during teardown. - assert: The event returns without reconstructing state or reconciling GARM. - """ - state = _state(debug_ssh_related=True, planned_units=0, secrets=[_TEARDOWN_SECRET]) +# These callbacks still need to be intercepted before paas-charm reconstructs state, but they +# have no cleanup-specific behavior in this change. Keep them separate from the explicit teardown +# cases so adding a guard does not accidentally expand the cleanup fallthrough. +_INHERITED_GUARDED_EVENTS = [ + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_changed(_relation(state, "secret-storage")), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + secret_storage_peers_data={1: {}}, + ), + ), + id="secret-storage-relation-changed", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_departed( + _relation(state, "secret-storage"), remote_unit=1 + ), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + secret_storage_peers_data={1: {}}, + ), + ), + id="secret-storage-relation-departed", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_changed(_relation(state, "ingress")), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + ingress_data={"ingress": json.dumps({"url": "https://garm.example"})}, + ), + ), + id="ingress-ready", + ), + pytest.param( + _event_case( + lambda ctx, state: ctx.on.relation_broken(_relation(state, "ingress")), + lambda: _state( + debug_ssh_related=True, + planned_units=0, + secrets=[_TEARDOWN_SECRET], + ingress_data={"ingress": json.dumps({"url": "https://garm.example"})}, + ), + ), + id="ingress-revoked", + ), +] + + +def _assert_teardown_event_is_gated( + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase +) -> None: + """Assert that one event cannot enter normal state or GARM reconciliation.""" + state = event_case.state_factory() with ( patch.object( @@ -1151,10 +1347,12 @@ def test_teardown_events_skip_framework_state_and_garm_reconciliation( "_get_configurator_provider_configs", side_effect=AssertionError("configurator relation was read"), ), + patch.object(GarmCharm, "_teardown", autospec=True) as teardown, ): - out = ctx.run(event(ctx, state), state) + out = ctx.run(event_case.emit(ctx, state), state) assert out is not None + teardown.assert_called_once() garm_api.client.assert_not_called() garm_api.auth.from_login.assert_not_called() garm_api.github.assert_not_called() @@ -1162,6 +1360,30 @@ def test_teardown_events_skip_framework_state_and_garm_reconciliation( garm_api.scaleset.assert_not_called() +@pytest.mark.parametrize("event_case", _TEARDOWN_EVENTS) +def test_teardown_events_skip_framework_state_and_garm_reconciliation( + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase +): + """ + arrange: No GARM units remain planned, and an explicit teardown event is emitted. + act: Emit the event. + assert: The event returns without reconstructing state or reconciling GARM. + """ + _assert_teardown_event_is_gated(ctx, garm_api, event_case) + + +@pytest.mark.parametrize("event_case", _INHERITED_GUARDED_EVENTS) +def test_inherited_events_skip_normal_reconciliation_during_teardown( + ctx: Context, garm_api: _GarmApiMocks, event_case: _EventCase +): + """ + arrange: No GARM units remain planned, and an inherited callback needs interception. + act: Emit the callback's underlying event. + assert: The pre-decorator guard returns without normal state construction or reconciliation. + """ + _assert_teardown_event_is_gated(ctx, garm_api, event_case) + + def test_teardown_handlers_skip_charm_state_creation(ctx: Context, garm_api: _GarmApiMocks): """ arrange: No GARM units remain planned. @@ -1180,6 +1402,31 @@ def test_teardown_handlers_skip_charm_state_creation(ctx: Context, garm_api: _Ga garm_api.auth.from_login.assert_not_called() +def test_paas_charm_hook_contract_matches_pinned_base(): + """ + arrange: The installed paas-charm base exposes the supported 1.12.x hook names. + act: Validate the lifecycle hook contract. + assert: The compatibility check accepts the pinned base API. + """ + charm_module._validate_paas_charm_hook_contract() + + +def test_paas_charm_hook_contract_fails_loudly_when_base_changes(monkeypatch): + """ + arrange: The base class no longer exposes the expected lifecycle hooks. + act: Validate the lifecycle hook contract. + assert: Validation fails before the base can register observers. + """ + + class IncompatibleBase: + pass + + monkeypatch.setattr(charm_module.paas_charm.go, "Charm", IncompatibleBase) + + with pytest.raises(RuntimeError, match="_on_config_changed"): + charm_module._validate_paas_charm_hook_contract() + + def test_teardown_update_status_skips_ingress_refresh(ctx: Context, garm_api: _GarmApiMocks): """ arrange: The local GARM service is tearing down. @@ -1197,3 +1444,30 @@ def test_teardown_update_status_skips_ingress_refresh(ctx: Context, garm_api: _G assert out is not None garm_api.auth.from_login.assert_not_called() + + +def test_rotate_secret_key_fails_before_state_creation_during_teardown( + ctx: Context, garm_api: _GarmApiMocks +): + """ + arrange: No GARM units remain planned and normal state construction is forbidden. + act: Run the rotate-secret-key action. + assert: The action fails before state construction, secret reset, or restart. + """ + state = _state(planned_units=0) + with ( + patch.object( + GarmCharm, + "_create_charm_state", + side_effect=AssertionError("charm state was created during teardown"), + ), + patch.object(GarmCharm, "restart") as restart, + ): + with ctx(ctx.on.action("rotate-secret-key"), state) as manager: + with patch.object(manager.charm._secret_storage, "reset_secret_key") as reset_secret: + with pytest.raises(ActionFailed, match="local teardown"): + manager.run() + + reset_secret.assert_not_called() + restart.assert_not_called() + garm_api.auth.from_login.assert_not_called() From 3e929fdb8a95bbbfd175d365411d027d41105f1d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 8 Sep 2026 02:12:32 +0000 Subject: [PATCH 7/7] refactor(garm): consolidate lifecycle hook adapters --- charms/garm/src/charm.py | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index bc54a047..ecb72009 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -137,7 +137,7 @@ def _parse_pre_install_scripts(raw: str) -> dict[str, str]: def _validate_paas_charm_hook_contract() -> None: - """Fail loudly if the pinned paas-charm lifecycle hooks have changed.""" + """Fail before startup if a paas-charm hook alias would stop guarding teardown.""" base_classes = paas_charm.go.Charm.__mro__ missing = [ hook @@ -165,21 +165,17 @@ def __init__(self, *args: typing.Any) -> None: self.on.install, self.on.leader_elected, self.on.update_status, + self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_joined, + self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_changed, + self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_departed, + self.on[GARM_CONFIGURATOR_RELATION_NAME].relation_broken, + self.on[DEBUG_SSH_INTEGRATION_NAME].relation_joined, + self.on[DEBUG_SSH_INTEGRATION_NAME].relation_changed, + self.on[DEBUG_SSH_INTEGRATION_NAME].relation_departed, + self.on[DEBUG_SSH_INTEGRATION_NAME].relation_broken, ): self.framework.observe(event, self._reconcile) - for relation_events in ( - self.on[GARM_CONFIGURATOR_RELATION_NAME], - self.on[DEBUG_SSH_INTEGRATION_NAME], - ): - for event in ( - relation_events.relation_joined, - relation_events.relation_changed, - relation_events.relation_departed, - relation_events.relation_broken, - ): - self.framework.observe(event, self._reconcile) - self.framework.observe(self.on.get_credentials_action, self._on_get_credentials_action) self.framework.observe(self.on.remove, self._on_remove) @@ -222,9 +218,13 @@ def _route_reconcile(self, event: ops.EventBase) -> None: """Route an inherited framework event through GARM's teardown gate.""" self._reconcile(event) - # PaasCharm.__init__ resolves these hook names dynamically. These aliases keep the - # teardown check before block_if_invalid_data without registering duplicate observers. - # The compatibility check above makes this adapter fail loudly when the base API changes. + def _route_reconcile_with_migrations(self, event: ops.EventBase) -> None: + """Route an inherited database event through GARM's migration gate.""" + self._reconcile_with_migrations(event) + + # PaasCharm.__init__ resolves these hook names dynamically. Keep all aliases together so + # the teardown adapters remain visible as one compatibility boundary. The contract check + # above makes this adapter fail loudly when the base API changes. _on_config_changed = _route_reconcile _on_secret_changed = _route_reconcile _on_secret_storage_relation_changed = _route_reconcile @@ -233,11 +233,6 @@ def _route_reconcile(self, event: ops.EventBase) -> None: _on_ingress_ready = _route_reconcile _on_ingress_revoked = _route_reconcile _on_pebble_ready = _route_reconcile - - def _route_reconcile_with_migrations(self, event: ops.EventBase) -> None: - """Route an inherited database event through GARM's migration gate.""" - self._reconcile_with_migrations(event) - _on_postgresql_database_database_created = _route_reconcile_with_migrations _on_postgresql_database_endpoints_changed = _route_reconcile_with_migrations