diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 8edcac2a..359d330a 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -130,49 +130,99 @@ 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: + """Return whether Juju plans no remaining units for the local application.""" + return self.app.planned_units() == 0 + + def _reconcile(self, event: ops.EventBase) -> None: + """Reconcile GARM, or handle local teardown before normal state construction.""" + if self._is_tearing_down(): + self._teardown(event) + return + self._normal_reconcile(event) + + def _reconcile_with_migrations(self, event: ops.EventBase) -> None: + """Reconcile a database event, preserving the teardown gate.""" + if self._is_tearing_down(): + 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 _reconcile(self, _: ops.EventBase) -> None: - """Reconcile charm state.""" + def _normal_reconcile(self, _: ops.EventBase) -> None: + """Reconcile active GARM charm state.""" self.restart() + @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 _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. + _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) + + _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.""" + if self._is_tearing_down(): + logger.info("Skipping update-status handling during local teardown") + 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(): @@ -287,6 +337,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 4fc90d95..c7242bd3 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -508,9 +508,7 @@ def test_remove_runs_garm_cleanup_before_charm_termination(ctx: Context, garm_ap assert: Cleanup runs with the authenticated client before removal completes. """ with patch("charm.GarmResourceCleanup") as cleanup_cls: - out = ctx.run( - ctx.on.remove(), _state(secrets=_owned_secrets(), planned_units=0) - ) + out = ctx.run(ctx.on.remove(), _state(secrets=_owned_secrets(), planned_units=0)) cleanup_cls.assert_called_once_with(garm_api.auth_client) cleanup_cls.return_value.run.assert_called_once_with() @@ -1072,3 +1070,130 @@ 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.object( + GarmCharm, + "_create_charm_state", + side_effect=AssertionError("charm state was created during teardown"), + ), + 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_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. + 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()