From f8613b433099667d8aeabd82ab86633e4241239d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 21 Oct 2026 10:08:26 +0000 Subject: [PATCH 01/13] fix(garm): keep a departing configurator's provider until its scaleset is gone Removing a garm-configurator relation built GARM_PROVIDERS_JSON from the relations that remained, so the removed configurator's provider was dropped and GARM replanned without it before the drain started. GARM fixes its provider map per process, so the orphaned scaleset then had no scaleset worker to purge its instances and no provider worker to tear a VM down, while GARM refuses to delete a runner in the terminal `deleted` status or a scaleset that still lists runners. The teardown could never finish. restart() now runs the provider lifecycle in one pass: read the recorded set, add every provider the relations publish, start GARM with it, reconcile scalesets, then trim and re-record what no relation and no surviving scaleset names. The record is the charm's only knowledge of a departed configurator's credentials - Juju hides its unit from relation-departed onwards - so a record that cannot be read or written raises out of the hook instead of yielding a guessed set: publishing from an incomplete record, or running a set the record does not hold, is exactly what strands a scaleset. A reconcile pass that cannot establish which scalesets GARM holds trims nothing. The trim takes effect at the following replan, so a retirement does not restart GARM twice. Refs: ISD-7262 --- charms/garm/AGENTS.md | 15 + charms/garm/src/charm.py | 253 +++++++++++- charms/garm/src/scaleset_reconciler.py | 50 ++- charms/garm/tests/unit/test_charm.py | 375 +++++++++++++++++- .../tests/unit/test_scaleset_reconciler.py | 12 +- 5 files changed, 673 insertions(+), 32 deletions(-) diff --git a/charms/garm/AGENTS.md b/charms/garm/AGENTS.md index 59cf2836..985fa23c 100644 --- a/charms/garm/AGENTS.md +++ b/charms/garm/AGENTS.md @@ -30,6 +30,21 @@ charm conventions; this file lists only what's specific to `garm`. `github_reconciler.py` base64-encodes the raw PEM private key (held as `CredentialSpec.private_key`) when building the `GithubApp` model. To fix a generated-type issue, bump `GARM_COMMIT` and re-run the script, never hand-edit the output. +- **Provider set is published, not merely derived.** `restart()` reads the `garm-providers` + secret, adds every provider the relations publish, restarts GARM with that set, reconciles + scalesets, and only then trims and re-records what no relation and no surviving scaleset + names. **DON'T** reduce the published set to the current relations: GARM fixes its provider + map per process, so dropping a provider whose scaleset still exists leaves that scaleset + with no worker that can delete its runners or the scaleset — the teardown deadlocks + (ISD-7262). The provider leaves the record on the hook after its last scaleset is gone, and + GARM's next start drops it. + - The trim takes effect at the **following** replan; that is deliberate, so a retirement + does not restart GARM twice. + - A reconcile pass that cannot establish which scalesets GARM holds returns `None`, and + `None` trims nothing. **DON'T** default it to an empty set — that deletes every provider. + - A record that cannot be read or written raises `ProviderRecordError` out of the hook and + puts the unit in error state: the charm cannot deduce the provider set, and guessing is + what strands a scaleset. - GARM serves its API and `/metrics` on one fixed port (`GARM_PORT`); the `app-port` / `metrics-port` / `metrics-path` config options have no effect (the charm logs a warning rather than blocking). The port is pinned in the `_workload_config` property. diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index a2c0ed2e..5ba4a572 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -46,6 +46,36 @@ GARM_PROVIDER_CONFIG_DIR: typing.Final[str] = "/etc/garm" GARM_SECRETS_LABEL: typing.Final[str] = "garm-secrets" GARM_ADMIN_CREDENTIALS_LABEL: typing.Final[str] = "garm-admin-credentials" +# The provider set GARM was last started with. Kept in a secret, not only in the Pebble +# plan, so it survives a pod replacement and is the same for every unit: a drain can run +# for days, and a restart with a shrunken set in the middle of one is the deadlock this +# record prevents (ISD-7262). +GARM_PROVIDERS_LABEL: typing.Final[str] = "garm-providers" +GARM_PROVIDERS_SECRET_KEY: typing.Final[str] = "providers" +# Mirrors the fields garm_entrypoint requires of a provider; a record missing one would be +# published into a workload that cannot start, so it is rejected here instead. +_PROVIDER_RECORD_FIELDS: typing.Final[tuple[str, ...]] = ( + "unit_name", + "network", + "auth_url", + "username", + "password", + "project_name", + "user_domain_name", + "project_domain_name", + "region_name", +) + + +class ProviderRecordError(Exception): + """The provider record cannot be read or written, so the provider set is unknown. + + Raised out of the hook on purpose: publishing a set derived from a record the charm + could not read, or restarting GARM with a set it could not record, would leave a + scaleset whose provider nothing can rebuild — the deadlock this record prevents. + """ + + CONTAINER_NAME: typing.Final[str] = "app" PEBBLE_SERVICE_NAME: typing.Final[str] = "app" GARM_BINARY: typing.Final[str] = "/usr/local/bin/garm" @@ -117,6 +147,52 @@ def _generate_admin_password() -> str: return "".join(chars) +def _by_provider_name(configs: typing.Iterable[dict[str, str]]) -> list[dict[str, str]]: + """Order provider configs by provider name, so a published set is deterministic. + + Args: + configs: The provider configs to order. + + Returns: + The same configs, sorted by ``unit_name``, which is the provider name. + """ + return sorted(configs, key=lambda config: config["unit_name"]) + + +def _parse_provider_record(published: str, source: str) -> list[dict[str, str]]: + """Parse a recorded provider set, rejecting anything that cannot drive GARM. + + Args: + published: The recorded JSON value. + source: Where the value came from, for the error message. + + Returns: + The provider configs. + + Raises: + ProviderRecordError: If the value is not a list of complete provider configs. A + partially read record would publish providers missing credentials, which stops + the workload from starting — the same failure, later and less legible. + """ + if not published: + return [] + try: + configs = json.loads(published) + except json.JSONDecodeError as exc: + raise ProviderRecordError(f"{source} is not valid JSON: {exc}") from exc + if not isinstance(configs, list): + raise ProviderRecordError(f"{source} is not a list of providers") + for config in configs: + if not isinstance(config, dict) or not isinstance(config.get("unit_name"), str): + raise ProviderRecordError(f"{source} holds a provider with no unit name") + missing = [field for field in _PROVIDER_RECORD_FIELDS if not config.get(field)] + if missing: + raise ProviderRecordError( + f"{source} holds provider {config['unit_name']} without {', '.join(missing)}" + ) + return configs + + def _parse_pre_install_scripts(raw: str) -> dict[str, str]: """Parse pre_install_scripts from JSON relation data string.""" if not raw: @@ -303,6 +379,18 @@ def restart(self, rerun_migrations: bool = False) -> None: install/leader_elected before pebble is ready, then gates on workload readiness before writing config and replanning. + The provider set is read, published, reconciled against GARM and only then trimmed + and recorded — in that order, once each. A provider the relations publish joins the + record; an entry leaves it only when no relation publishes it and no scaleset GARM + still holds names it. Keep that order — delete the runners, delete the scaleset, + then drop the provider — because GARM fixes its provider map per process: a + scaleset whose provider is missing gets neither the worker that purges its instances + nor the one that deletes its VMs, and its teardown can never finish. + + The record is the charm's only knowledge of a departed configurator's credentials, + so a record that cannot be read or written raises out of the hook rather than + yielding a guessed set (see ``ProviderRecordError``). + Args: rerun_migrations: Passed through to the parent restart. """ @@ -326,8 +414,8 @@ def restart(self, rerun_migrations: bool = False) -> None: self.update_app_and_unit_status(ops.WaitingStatus("Waiting for GARM secrets")) return - provider_configs = self._get_configurator_provider_configs() - if not provider_configs: + related_providers = self._get_configurator_provider_configs() + if not related_providers: if not CharmState.from_charm(self).configurator_related: self._reconcile_runners() logger.info("GARM configurator provider data not yet available; blocking") @@ -335,10 +423,13 @@ def restart(self, rerun_migrations: bool = False) -> None: ops.WaitingStatus("Waiting for garm-configurator relation") ) return + # After the gate above, so an application with no configurator left keeps its + # workload untouched rather than being replanned against a shrinking provider set. + published_providers = self._published_providers(related_providers) container = self.unit.get_container(CONTAINER_NAME) entrypoint_environment = self._entrypoint_environment( - postgresql_config, secrets_data, provider_configs + postgresql_config, secrets_data, published_providers ) super().restart(rerun_migrations=rerun_migrations) container.add_layer( @@ -358,14 +449,17 @@ def restart(self, rerun_migrations: bool = False) -> None: logger.info( "Configured GARM workload: command=%s provider_count=%d env_keys=%s", "/usr/local/bin/garm-entrypoint.py", - len(provider_configs), + len(published_providers), sorted(entrypoint_environment), ) container.replan() self._maybe_first_run() - # Reconcile last: the framework's restart finishes by reporting active unconditionally, - # which would bury the status a failed GARM sync reports here. - self._reconcile_runners() + # Reconcile before recording: the pass is what establishes which providers are still + # referenced, and it must not be buried by the framework reporting active afterward. + referenced_providers = self._reconcile_runners() + self._save_provider_record( + self._trimmed_providers(published_providers, related_providers, referenced_providers) + ) def _entrypoint_environment( self, @@ -584,6 +678,136 @@ def _get_configurator_provider_configs( ) return configs + def _published_providers(self, related: list[dict[str, str]]) -> list[dict[str, str]]: + """Return the provider set to publish: the record with the relations on top. + + A provider enters the record the first time a relation publishes it, because that is + the only moment its credentials can be read — Juju hides a departing unit from + ``relation-departed`` onwards, so a provider absent from the record at that point + could never be rebuilt. Relation data wins over the recorded copy for a provider that + is still related, so a rotated password reaches GARM. + + Args: + related: The provider configs the remaining relations publish. + + Returns: + The provider set to publish, ordered by provider name. + """ + merged = {config["unit_name"]: config for config in self._read_provider_record().values()} + merged.update({config["unit_name"]: config for config in related}) + return _by_provider_name(merged.values()) + + def _read_provider_record(self) -> dict[str, dict[str, str]]: + """Return the recorded provider set, by provider name. + + Returns: + The recorded configs, or an empty set when nothing has been recorded yet — the + first start of an application, which is a normal state rather than a failure. + + Raises: + ProviderRecordError: If the record exists but cannot be read, or is not a usable + provider set. Publishing a set derived from it would drop providers a live + scaleset still needs, so the hook must stop instead. + """ + try: + secret = self.model.get_secret(label=GARM_PROVIDERS_LABEL) + # Refresh: tracking can lag this application's own write, and the record has to + # be read at its latest revision — reading a revision behind would resurrect a + # provider that the previous hook trimmed away. + content = secret.get_content(refresh=True) + except ops.SecretNotFoundError: + logger.info("No recorded GARM provider set; publishing the relations only") + return {} + except ops.ModelError as exc: + raise ProviderRecordError(f"cannot read the GARM provider record: {exc}") from exc + recorded = _parse_provider_record( + content.get(GARM_PROVIDERS_SECRET_KEY, ""), "the GARM provider record" + ) + return {config["unit_name"]: config for config in recorded} + + @staticmethod + def _trimmed_providers( + published: list[dict[str, str]], + related: list[dict[str, str]], + referenced: set[str] | None, + ) -> list[dict[str, str]]: + """Drop providers no relation publishes and no live scaleset references. + + Args: + published: The provider set GARM was started with on this pass. + related: The provider configs the remaining relations publish. + referenced: The providers of the scalesets GARM still holds, or None when that + could not be established. + + Returns: + The set to record. ``published`` unchanged when ``referenced`` is None: "no + scaleset references it" cannot be concluded from "could not tell", and a provider + kept a hook too long costs a stale entry while a dropped one cannot be rebuilt. + """ + if referenced is None: + logger.warning( + "The scalesets GARM still holds could not be read; keeping every provider" + " this hook: %s", + sorted(config["unit_name"] for config in published), + ) + return published + + related_names = {config["unit_name"] for config in related} + kept = [ + config + for config in published + if config["unit_name"] in related_names or config["unit_name"] in referenced + ] + kept_names = {config["unit_name"] for config in kept} + for name in sorted({config["unit_name"] for config in published} - kept_names): + logger.info("Removing provider %s: no scaleset references it any more", name) + still_needed = sorted((kept_names - related_names) & referenced) + if still_needed: + logger.info("Keeping providers a live scaleset still references: %s", still_needed) + # A provider a scaleset names but no relation and no record can produce cannot be + # rebuilt, so its scaleset cannot finish tearing down. Say so, rather than leaving it + # to the reconciler's "will retry" warnings. + unrebuildable = referenced - kept_names - related_names + if unrebuildable: + logger.warning( + "Scalesets reference providers that are no longer published and cannot be" + " rebuilt from relation data: %s. Their teardown cannot complete until the" + " owning configurator is related again.", + sorted(unrebuildable), + ) + return _by_provider_name(kept) + + def _save_provider_record(self, published: list[dict[str, str]]) -> None: + """Record the provider set GARM is running with, so it outlives the pod. + + Leader-only: every unit of this application runs its own GARM against the same + database, so one record has to describe the set they should all keep. + + Args: + published: The provider configs the workload was started with. + + Raises: + ProviderRecordError: If the record cannot be written. The hook stops rather than + leave GARM running with providers the next restart could no longer retain. + """ + if not self.unit.is_leader(): + return + payload = json.dumps(published, sort_keys=True, separators=(",", ":")) + try: + try: + secret = self.model.get_secret(label=GARM_PROVIDERS_LABEL) + except ops.SecretNotFoundError: + self.app.add_secret( + {GARM_PROVIDERS_SECRET_KEY: payload}, + label=GARM_PROVIDERS_LABEL, + description="Provider set GARM was last started with (ISD-7262)", + ) + return + if secret.get_content(refresh=True).get(GARM_PROVIDERS_SECRET_KEY) != payload: + secret.set_content({GARM_PROVIDERS_SECRET_KEY: payload}) + except ops.ModelError as exc: + raise ProviderRecordError(f"cannot record the GARM provider set: {exc}") from exc + def _resolve_secret_value(self, secret_uri: str) -> str: """Resolve a secret URI and return its ``value`` content. @@ -812,7 +1036,7 @@ def _add_unit_credential( description=MANAGED_CREDENTIAL_DESCRIPTION, ) - def _reconcile_runners(self) -> None: + def _reconcile_runners(self) -> set[str] | None: """Sync GARM controller URLs, GitHub credentials, entities, then scalesets. The steps are ordered, not independent: GARM rejects every operational call with @@ -821,11 +1045,17 @@ def _reconcile_runners(self) -> None: authenticated client configures the URLs and reconciles credentials, then entities, then scalesets. The ``github_linux_charmed`` template is ensured before scalesets so each scaleset can reference it. + + Returns: + The providers the scalesets GARM still holds are created with, or None when that + could not be established — no admin credentials, a connection or authorization + failure, or a template that could not be applied. None means "unknown", and the + caller must read it as "drop nothing". """ admin_creds = self._get_admin_credentials() if not admin_creds: logger.warning("Admin credentials not yet available; deferring reconcile") - return + return None # Talk to GARM over its fixed local listener (same target as first-run), rather than # _get_garm_url() which depends on charm config that is not set for the local API. @@ -848,7 +1078,7 @@ def _reconcile_runners(self) -> None: GithubReconciler(auth_client).reconcile(self._build_desired_credentials()) EntityReconciler(auth_client, agent_mode).reconcile(charm_state.desired_entities) template_id = _apply_garm_template(auth_client, charm_state.ssh_debug_connections) - replacing = ScalesetReconciler(auth_client).reconcile( + replacing, referenced = ScalesetReconciler(auth_client).reconcile( self._build_desired_scalesets(template_id) ) # A label change recreates the scaleset and drains the old one, which @@ -863,12 +1093,15 @@ def _reconcile_runners(self) -> None: ) else: self.update_app_and_unit_status(ops.ActiveStatus()) + return referenced except CharmedTemplateError as exc: logger.warning("GARM charmed template error during reconcile: %s", exc) self.update_app_and_unit_status(ops.WaitingStatus(str(exc))) + return None except GarmApiError as exc: logger.warning("GARM API error during reconcile: %s", exc) self.update_app_and_unit_status(ops.WaitingStatus("GARM sync failed")) + return None def _ensure_controller_urls(self, auth_client: GarmAuthenticatedClient) -> None: """Configure the GARM controller URLs that gate its operational API. diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 2ad9399b..dfd08028 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -162,7 +162,9 @@ def __init__(self, client: GarmAuthenticatedClient) -> None: """ self._client = client - def reconcile(self, desired: list[ScalesetSpec]) -> list[ScalesetProgress]: + def reconcile( + self, desired: list[ScalesetSpec] + ) -> tuple[list[ScalesetProgress], set[str] | None]: """Sync GARM scalesets to match *desired*. Performs the minimum set of CREATE / UPDATE / DELETE operations, and @@ -192,7 +194,11 @@ def reconcile(self, desired: list[ScalesetSpec]) -> list[ScalesetProgress]: desired: The full desired set of scalesets. Returns: - One entry per replacement still in flight; empty once converged. + The replacements still in flight (empty once converged), and the providers the + scalesets GARM still holds are created with, so the caller can drop a provider + only once nothing references it. The provider set is ``None`` when it cannot be + established: a surviving scaleset with no provider name leaves nothing to map + it to, and "nothing is referenced" must never be guessed from a failed read. Raises: GarmApiError: If any spec failed to reconcile, re-raised once the rest @@ -231,12 +237,50 @@ def reconcile(self, desired: list[ScalesetSpec]) -> list[ScalesetProgress]: logger.warning("Failed to reconcile scaleset %s: %s", spec.name, exc) failure = failure or exc + deleted: set[str] = set() for name, scaleset in observed.items(): if name not in claimed and self._delete_orphaned(scaleset): self._delete_custom_template(name, templates) + deleted.add(name) if failure is not None: raise failure - return progress + return progress, self._referenced_providers(desired, observed, deleted) + + @staticmethod + def _referenced_providers( + desired: list[ScalesetSpec], observed: dict[str, ScaleSet], deleted: set[str] + ) -> set[str] | None: + """Return the providers the scalesets GARM still holds were created with. + + Built from what GARM answered, not from what this pass asked for: a scaleset whose + delete was refused, deferred, or never attempted is still held, and a provider + dropped while its scaleset survives can never be handed back. + + Args: + desired: The specs reconciled by this pass; their providers stay referenced even + while the scaleset that will use one is only now being created. + observed: The scalesets GARM listed before this pass, by name. + deleted: Names of scalesets GARM accepted a delete for during this pass. + + Returns: + The referenced provider names, or None when a surviving scaleset has no provider + name — GARM holds it, but nothing maps it to a provider, so the caller cannot + tell which provider is still needed and must not drop any. + """ + referenced = {spec.provider_name for spec in desired} + for name, scaleset in observed.items(): + if name in deleted: + continue + if not scaleset.provider_name: + logger.warning( + "Scaleset %s (id=%s) names no provider; the provider set cannot be" + " trimmed from this pass", + name, + scaleset.id, + ) + return None + referenced.add(scaleset.provider_name) + return referenced @staticmethod def _dedupe(desired: list[ScalesetSpec]) -> list[ScalesetSpec]: diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index e1a8f4df..b4e52b37 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -32,7 +32,14 @@ import garm_template from agent_tools import AgentToolsError -from charm import GARM_ADMIN_CREDENTIALS_LABEL, GARM_PORT, GARM_SECRETS_LABEL, GarmCharm +from charm import ( + GARM_ADMIN_CREDENTIALS_LABEL, + GARM_PORT, + GARM_PROVIDERS_LABEL, + GARM_PROVIDERS_SECRET_KEY, + GARM_SECRETS_LABEL, + GarmCharm, +) from charm_state import DEBUG_SSH_INTEGRATION_NAME, GARM_CONFIGURATOR_RELATION_NAME from garm_api import GarmApiError, GarmConnectionError from github_reconciler import ( @@ -150,8 +157,10 @@ def garm_api_fixture() -> typing.Iterator[_GarmApiMocks]: patch("charm.ensure_agent_tools", return_value=_AGENT_VERSION) as agent_tools, ): auth_client = auth_cls.from_login.return_value - # No scaleset is mid-replacement by default, so the charm reports active. - scaleset_cls.return_value.reconcile.return_value = [] + # No scaleset is mid-replacement by default, so the charm reports active. The + # referenced set is what the pass observed GARM still holds: empty means no scaleset + # references any provider. + scaleset_cls.return_value.reconcile.return_value = ([], set()) calls = MagicMock() calls.attach_mock(auth_client.update_controller, "controller") calls.attach_mock(agent_tools, "agent_tools") @@ -196,6 +205,7 @@ def _state( unit_status: ops.StatusBase | None = None, app_status: ops.StatusBase | None = None, planned_units: int = 1, + recorded_providers: list[dict[str, str]] | None = None, ) -> State: """Build a GARM charm State that is ready to serve unless a caller opts out. @@ -249,12 +259,70 @@ def _state( Container(name=CONTAINER_NAME, can_connect=can_connect, layers={"garm": _ROCK_LAYER}) ], relations=relations, - secrets=list(secrets), + secrets=[ + *secrets, + *([_recorded_secret(recorded_providers)] if recorded_providers else []), + ], planned_units=planned_units, **statuses, ) +def _recorded_secret(providers: list[dict[str, str]]) -> Secret: + """Return the ``garm-providers`` secret a previous restart recorded. + + The secret is the charm's whole knowledge of a departed configurator's credentials, + which Juju drops from relation data once its unit departs. + """ + return Secret( + label=GARM_PROVIDERS_LABEL, + owner="app", + tracked_content={ + GARM_PROVIDERS_SECRET_KEY: json.dumps(providers, sort_keys=True, separators=(",", ":")) + }, + ) + + +def _provider_config(unit_name: str) -> dict[str, str]: + """Return a provider config as the charm publishes it for one configurator unit.""" + return { + "unit_name": unit_name, + "auth_url": _PROVIDER_UNIT_DATA["openstack_auth_url"], + "username": _PROVIDER_UNIT_DATA["openstack_username"], + "password": _PROVIDER_UNIT_DATA["openstack_password"], + "project_name": _PROVIDER_UNIT_DATA["openstack_project_name"], + "user_domain_name": _PROVIDER_UNIT_DATA["openstack_user_domain_name"], + "project_domain_name": _PROVIDER_UNIT_DATA["openstack_project_domain_name"], + "region_name": _PROVIDER_UNIT_DATA["openstack_region_name"], + "network": _PROVIDER_UNIT_DATA["openstack_network"], + } + + +def _configurator_relation(app_name: str) -> Relation: + """Return a configurator relation publishing one provider and one scaleset.""" + return Relation( + endpoint=GARM_CONFIGURATOR_RELATION_NAME, + remote_app_name=app_name, + remote_units_data={ + 0: { + **_PROVIDER_UNIT_DATA, + **_SCALESET_UNIT_DATA, + "name": f"scaleset-{app_name}", + "provider_name": f"{app_name}-0", + } + }, + ) + + +def _recorded_provider_names(state: State) -> list[str]: + """Return the provider names the ``garm-providers`` record holds, latest revision.""" + secret = next(secret for secret in state.secrets if secret.label == GARM_PROVIDERS_LABEL) + return [ + config["unit_name"] + for config in json.loads(secret.latest_content[GARM_PROVIDERS_SECRET_KEY]) + ] + + def _secret_content(state: State, label: str) -> dict[str, str]: """Return the content of the state's secret with the given label.""" return next(secret for secret in state.secrets if secret.label == label).tracked_content @@ -265,6 +333,12 @@ def _service_environment(state: State) -> dict[str, str]: return state.get_container(CONTAINER_NAME).plan.services[SERVICE_NAME].environment +def _published_provider_names(state: State) -> list[str]: + """Return the provider names the state's service environment publishes, in order.""" + providers = json.loads(_service_environment(state)["GARM_PROVIDERS_JSON"]) + return [provider["unit_name"] for provider in providers] + + def _stop_workload(state: State) -> State: """Stop the app service, so that a subsequent replan is observable as a restart.""" container = state.get_container(CONTAINER_NAME) @@ -297,6 +371,7 @@ def test_leader_creates_garm_secrets(ctx: Context, garm_api: _GarmApiMocks): assert {secret.label for secret in out.secrets} == { GARM_SECRETS_LABEL, GARM_ADMIN_CREDENTIALS_LABEL, + GARM_PROVIDERS_LABEL, } @@ -730,6 +805,261 @@ def test_missing_configurator_relation_refreshes_stale_app_status( assert out.app_status == ops.WaitingStatus("Waiting for garm-configurator relation") +# --- Provider lifecycle across a departing configurator ----------------------------------- + + +def test_departing_configurator_provider_is_kept_while_its_scaleset_remains( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: Two configurators published providers, then one is removed from the model while + GARM still holds the scaleset it created — the state relation-broken leaves behind. + act: Run update-status, the hook the broken relation triggers. + assert: The removed provider is still published, credentials included, so GARM keeps the + provider worker that tears its runner down and the scaleset worker that purges the + row; the reconciler still sees the scaleset as orphaned and drains it. Dropping the + provider here would strand the scaleset: GARM rejects deleting the runner in its + ``deleted`` status and the scaleset while it still lists runners. + """ + garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + + with caplog.at_level(logging.INFO): + out = ctx.run( + ctx.on.update_status(), + _state( + configurator_relations=[_configurator_relation("configurator-b")], + recorded_providers=[ + _provider_config("configurator-b-0"), + _provider_config("garm-configurator-0"), + ], + ), + ) + + providers = json.loads(_service_environment(out)["GARM_PROVIDERS_JSON"]) + assert [provider["unit_name"] for provider in providers] == [ + "configurator-b-0", + "garm-configurator-0", + ] + # The kept config is the recorded one, not a name rebuilt from the remaining relation + # data — the departed unit's credentials are gone from Juju. + assert providers[1]["password"] == _PROVIDER_UNIT_DATA["openstack_password"] + reconciled = garm_api.scaleset.return_value.reconcile.call_args.args[0] + assert [spec.name for spec in reconciled] == ["scaleset-configurator-b"] + assert ( + "Keeping providers a live scaleset still references: ['garm-configurator-0']" + in caplog.text + ) + assert out.unit_status == ops.ActiveStatus() + + +def test_departing_configurator_provider_is_removed_once_its_scaleset_is_gone( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: The removal state above, reconciled with the departing configurator's scaleset + still in GARM — the hook where its provider has to stay published. + act: Reconcile again, with GARM no longer holding that scaleset, then once more. + assert: The record loses the provider on that hook, and the set GARM is started with + loses it on the next — the trim is recorded after the pass that justified it, so it + takes effect at the following replan rather than restarting GARM twice. + """ + garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + with caplog.at_level(logging.INFO): + fired = ctx.run( + ctx.on.update_status(), + _state( + configurator_relations=[_configurator_relation("configurator-b")], + recorded_providers=[ + _provider_config("configurator-b-0"), + _provider_config("garm-configurator-0"), + ], + ), + ) + + assert _published_provider_names(fired) == ["configurator-b-0", "garm-configurator-0"] + assert _recorded_provider_names(fired) == ["configurator-b-0", "garm-configurator-0"] + + garm_api.scaleset.return_value.reconcile.return_value = ([], set()) + with caplog.at_level(logging.INFO): + trimmed = ctx.run(ctx.on.update_status(), fired) + + # The record is trimmed on the hook that observed the scaleset gone; GARM still runs the + # set it was started with until the next replan. + assert _recorded_provider_names(trimmed) == ["configurator-b-0"] + assert _published_provider_names(trimmed) == ["configurator-b-0", "garm-configurator-0"] + assert ( + "Removing provider garm-configurator-0: no scaleset references it any more" in caplog.text + ) + + out = ctx.run(ctx.on.update_status(), trimmed) + + assert _published_provider_names(out) == ["configurator-b-0"] + assert _recorded_provider_names(out) == ["configurator-b-0"] + + +def test_provider_set_is_not_shrunk_when_the_scaleset_state_is_unknown( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: A departing configurator, and a reconcile pass that cannot establish which + scalesets GARM holds — connection failure, no credentials, or a scaleset that names + no provider. + act: Run update-status. + assert: Every published provider is kept. "Nothing references it" cannot be concluded + from "could not tell": a provider kept a hook too long only costs a stale entry, + whereas one dropped while a scaleset still needs it is the deadlock. + """ + garm_api.scaleset.return_value.reconcile.return_value = ([], None) + + with caplog.at_level(logging.INFO): + out = ctx.run( + ctx.on.update_status(), + _state( + configurator_relations=[_configurator_relation("configurator-b")], + recorded_providers=[ + _provider_config("configurator-b-0"), + _provider_config("garm-configurator-0"), + ], + ), + ) + + assert _published_provider_names(out) == ["configurator-b-0", "garm-configurator-0"] + assert "keeping every provider this hook" in caplog.text + + +def test_recorded_provider_set_survives_a_replaced_pod(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: Two configurators publishing providers, reconciled once so the set is recorded, + then a replaced pod: the same relations minus one configurator, a GARM that still + holds that configurator's scaleset, and no Pebble plan of its own. + act: Reconcile on the new pod. + assert: The removed provider is still published. The plan a drain relies on does not + survive a pod replacement, so the recorded set is what keeps a drain that is days + long from being undone by a reschedule. + """ + configurator_relations = [ + _configurator_relation("garm-configurator"), + _configurator_relation("configurator-b"), + ] + first = ctx.run(ctx.on.update_status(), _state(configurator_relations=configurator_relations)) + recorded = _recorded_provider_names(first) + assert set(recorded) == {"garm-configurator-0", "configurator-b-0"} + + garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + out = ctx.run( + ctx.on.update_status(), + _state( + configurator_relations=[_configurator_relation("configurator-b")], + recorded_providers=[_provider_config(name) for name in recorded], + ), + ) + + assert _published_provider_names(out) == ["configurator-b-0", "garm-configurator-0"] + + +def test_related_configurator_credentials_replace_the_recorded_ones( + ctx: Context, garm_api: _GarmApiMocks +): + """ + arrange: A recorded provider for a configurator that is related again with a new + OpenStack password. + act: Reconcile. + assert: The relation data wins, so a rotated password reaches GARM rather than the + recorded copy silently pinning the old one. + """ + stale = _provider_config("garm-configurator-0") + stale["password"] = "stale-password" + + out = ctx.run(ctx.on.update_status(), _state(recorded_providers=[stale])) + + providers = json.loads(_service_environment(out)["GARM_PROVIDERS_JSON"]) + assert providers[0]["password"] == _PROVIDER_UNIT_DATA["openstack_password"] + + +def test_publishing_does_not_ask_garm_for_its_scalesets(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: A configured charm whose recorded provider set matches its relations. + act: Run update-status. + assert: The publish path issues no scaleset listing of its own — the reconcile pass is + the single observation, so a hook cannot publish a set from one reading and trim it + from another. + """ + ctx.run( + ctx.on.update_status(), + _state(recorded_providers=[_provider_config("garm-configurator-0")]), + ) + + garm_api.auth_client.list_scalesets.assert_not_called() + garm_api.scaleset.return_value.reconcile.assert_called_once() + + +def test_unrebuildable_provider_is_reported( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: A departing configurator, and a scaleset whose provider is neither related nor + recorded — a stack already deadlocked before this charm, or GARM data this charm did + not create. + act: Run update-status. + assert: The provider the charm cannot rebuild is named, rather than left to the + reconciler's retry warnings, because no hook can finish that teardown unaided. + """ + garm_api.scaleset.return_value.reconcile.return_value = ( + [], + {"garm-configurator-0", "vanished-configurator-0"}, + ) + + with caplog.at_level(logging.INFO): + ctx.run( + ctx.on.update_status(), + _state( + configurator_relations=[_configurator_relation("configurator-b")], + recorded_providers=[ + _provider_config("configurator-b-0"), + _provider_config("garm-configurator-0"), + ], + ), + ) + + assert "no longer published and cannot be rebuilt" in caplog.text + assert "vanished-configurator-0" in caplog.text + + +def test_unreadable_record_fails_the_hook(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: The provider record exists but cannot be read. + act: Reconcile. + assert: The error reaches Juju, which puts the unit in error state. Carrying on would + publish a set derived from an incomplete record, dropping a provider a scaleset + still needs — and that provider's credentials are unrecoverable afterwards. + """ + get_secret = ops.Model.get_secret + + def failing_get_secret(model, *, id=None, label=None): + if label == GARM_PROVIDERS_LABEL: + raise ops.ModelError("permission denied") + return get_secret(model, id=id, label=label) + + with patch.object(ops.Model, "get_secret", failing_get_secret): + with pytest.raises(UncaughtCharmError, match="cannot read the GARM provider record"): + ctx.run(ctx.on.update_status(), _state()) + + +def test_unwritable_record_fails_the_hook(ctx: Context, garm_api: _GarmApiMocks): + """ + arrange: GARM has been reconciled, and the provider set cannot be recorded. + act: Reconcile. + assert: The error reaches Juju, which puts the unit in error state. A set GARM runs with + but the record does not hold cannot be retained on the next restart, so the failure + must be visible rather than logged and forgotten. + """ + with patch.object( + ops.Application, "add_secret", side_effect=ops.ModelError("secret limit reached") + ): + with pytest.raises(UncaughtCharmError, match="cannot record the GARM provider set"): + ctx.run(ctx.on.update_status(), _state(secrets=_owned_secrets())) + + # --- Scaleset replacement progress -------------------------------------------------------- @@ -790,7 +1120,7 @@ def test_scaleset_being_replaced_is_reported_in_the_status( throughout: every label is served during the drain, so nothing waiting on the model should block on it. """ - garm_api.scaleset.return_value.reconcile.return_value = [progress] + garm_api.scaleset.return_value.reconcile.return_value = ([progress], set()) out = ctx.run(ctx.on.update_status(), _state()) @@ -804,10 +1134,13 @@ def test_many_scalesets_being_replaced_are_summarised(ctx: Context, garm_api: _G assert: Only the first two are named and the rest are counted, so Juju does not truncate the status into uselessness. """ - garm_api.scaleset.return_value.reconcile.return_value = [ - ScalesetProgress(f"scaleset-{index}", f"old-{index}", f"new-{index}", index) - for index in range(1, 5) - ] + garm_api.scaleset.return_value.reconcile.return_value = ( + [ + ScalesetProgress(f"scaleset-{index}", f"old-{index}", f"new-{index}", index) + for index in range(1, 5) + ], + set(), + ) out = ctx.run(ctx.on.update_status(), _state()) @@ -827,11 +1160,14 @@ def test_two_generations_of_one_scaleset_are_reported_as_one_replacement( assert: The doubly-draining scaleset is named once, with its runner counts summed, so it cannot repeat itself and push the other scaleset behind "+N more". """ - garm_api.scaleset.return_value.reconcile.return_value = [ - ScalesetProgress("scaleset-a", "old-a1", "new-a", 2), - ScalesetProgress("scaleset-a", "old-a2", "new-a", 1), - ScalesetProgress("scaleset-b", "old-b", "new-b", 1), - ] + garm_api.scaleset.return_value.reconcile.return_value = ( + [ + ScalesetProgress("scaleset-a", "old-a1", "new-a", 2), + ScalesetProgress("scaleset-a", "old-a2", "new-a", 1), + ScalesetProgress("scaleset-b", "old-b", "new-b", 1), + ], + set(), + ) out = ctx.run(ctx.on.update_status(), _state()) @@ -850,10 +1186,13 @@ def test_a_scaleset_reports_its_least_advanced_generation(ctx: Context, garm_api as its least advanced generation, and a summed runner count of zero would otherwise read as fully drained. """ - garm_api.scaleset.return_value.reconcile.return_value = [ - ScalesetProgress("scaleset-a", "old-a1", "new-a", 0), - ScalesetProgress("scaleset-a", "old-a2", "new-a", 0, Handover.PENDING), - ] + garm_api.scaleset.return_value.reconcile.return_value = ( + [ + ScalesetProgress("scaleset-a", "old-a1", "new-a", 0), + ScalesetProgress("scaleset-a", "old-a2", "new-a", 0, Handover.PENDING), + ], + set(), + ) out = ctx.run(ctx.on.update_status(), _state()) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 99912ba5..a656f31c 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -54,9 +54,13 @@ def __init__( enabled=True, updated_at=None, runner_prefix=None, + provider_name="openstack-demo", ): self.name = name self.id = sid + # What the reconciler hands back so the charm can drop a provider only once no + # scaleset names it; None models a GARM row that was created without one. + self.provider_name = provider_name self.image = image self.flavor = flavor self.max_runners = max_runners @@ -280,7 +284,13 @@ def _spec( def _reconcile(client, desired): - return ScalesetReconciler(client).reconcile(desired) + progress, _ = ScalesetReconciler(client).reconcile(desired) + return progress + + +def _reconcile_referenced(client, desired): + """Reconcile and return the providers GARM still holds scalesets for.""" + return ScalesetReconciler(client).reconcile(desired)[1] @pytest.mark.parametrize( From d147d58944b9400fbd8d5b992d749f4b9964c014 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 03:39:15 +0000 Subject: [PATCH 02/13] fix(garm): address provider-record review follow-ups - Publish the recorded provider set even when no relation publishes provider data, so a pod replaced mid-drain still starts GARM with the providers that drain needs. The waiting return now needs no related providers AND an empty record; a configurator that is related but silent stays a waiting state, so the sweep cannot prune the scalesets it is about to describe. - Read the record with peek_content() (latest revision, no tracking) and remove the revision each write supersedes: the leader is the only writer and every reader peeks, so no unit depends on an older revision. - Import PROVIDER_REQUIRED_FIELDS from the entrypoint instead of duplicating it, inline the one-line ordering helper, and keep docstrings to the interface. - Say in the log that a scaleset naming no provider keeps trimming off until it is gone, and drop the internal tracker id from shipped text. - Release note: stuck deployments do not recover on upgrade; relate the same configurator again to republish the provider and resume the drain. - Tests: the last-configurator path across a replaced pod, and the record being emptied once its last scaleset is gone. --- charms/garm/AGENTS.md | 15 ++- charms/garm/src/charm.py | 136 ++++++++++++------------- charms/garm/src/garm_entrypoint.py | 4 +- charms/garm/src/scaleset_reconciler.py | 4 +- charms/garm/tests/unit/test_charm.py | 73 +++++++++++++ docs/changelog.md | 2 + 6 files changed, 157 insertions(+), 77 deletions(-) diff --git a/charms/garm/AGENTS.md b/charms/garm/AGENTS.md index 985fa23c..ff9b681f 100644 --- a/charms/garm/AGENTS.md +++ b/charms/garm/AGENTS.md @@ -35,16 +35,23 @@ charm conventions; this file lists only what's specific to `garm`. scalesets, and only then trims and re-records what no relation and no surviving scaleset names. **DON'T** reduce the published set to the current relations: GARM fixes its provider map per process, so dropping a provider whose scaleset still exists leaves that scaleset - with no worker that can delete its runners or the scaleset — the teardown deadlocks - (ISD-7262). The provider leaves the record on the hook after its last scaleset is gone, and - GARM's next start drops it. + with no worker that can delete its runners or the scaleset — the teardown deadlocks. The + provider leaves the record on the hook after its last scaleset is gone, and GARM's next + start drops it. + - The record is published even with **no** configurator left, so a pod replaced mid-drain + still starts GARM with the providers that drain needs. The hook waits for the relation + only when nothing is related **and** nothing is recorded. + - A configurator that is related but has published nothing yet is a waiting state, not a + reason to reconcile: the sweep would prune the scalesets that configurator is about to + describe. - The trim takes effect at the **following** replan; that is deliberate, so a retirement does not restart GARM twice. - A reconcile pass that cannot establish which scalesets GARM holds returns `None`, and `None` trims nothing. **DON'T** default it to an empty set — that deletes every provider. - A record that cannot be read or written raises `ProviderRecordError` out of the hook and puts the unit in error state: the charm cannot deduce the provider set, and guessing is - what strands a scaleset. + what strands a scaleset. The leader is the only writer, and readers use + `peek_content()`, so the superseded revision is removed after each write. - GARM serves its API and `/metrics` on one fixed port (`GARM_PORT`); the `app-port` / `metrics-port` / `metrics-path` config options have no effect (the charm logs a warning rather than blocking). The port is pinned in the `_workload_config` property. diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 5ba4a572..914f68ff 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -29,6 +29,7 @@ ) from entity_reconciler import EntityReconciler from garm_api import GarmApiClient, GarmApiError, GarmAuthenticatedClient, GarmConnectionError +from garm_entrypoint import PROVIDER_REQUIRED_FIELDS from garm_template import CharmedTemplateError from garm_template import apply_charmed_template as _apply_garm_template from github_reconciler import ( @@ -47,33 +48,14 @@ GARM_SECRETS_LABEL: typing.Final[str] = "garm-secrets" GARM_ADMIN_CREDENTIALS_LABEL: typing.Final[str] = "garm-admin-credentials" # The provider set GARM was last started with. Kept in a secret, not only in the Pebble -# plan, so it survives a pod replacement and is the same for every unit: a drain can run -# for days, and a restart with a shrunken set in the middle of one is the deadlock this -# record prevents (ISD-7262). +# plan, so it survives a pod replacement and is shared by every unit: a drain can run for +# days, and a restart with a shrunken set in the middle of one strands the scaleset. GARM_PROVIDERS_LABEL: typing.Final[str] = "garm-providers" GARM_PROVIDERS_SECRET_KEY: typing.Final[str] = "providers" -# Mirrors the fields garm_entrypoint requires of a provider; a record missing one would be -# published into a workload that cannot start, so it is rejected here instead. -_PROVIDER_RECORD_FIELDS: typing.Final[tuple[str, ...]] = ( - "unit_name", - "network", - "auth_url", - "username", - "password", - "project_name", - "user_domain_name", - "project_domain_name", - "region_name", -) class ProviderRecordError(Exception): - """The provider record cannot be read or written, so the provider set is unknown. - - Raised out of the hook on purpose: publishing a set derived from a record the charm - could not read, or restarting GARM with a set it could not record, would leave a - scaleset whose provider nothing can rebuild — the deadlock this record prevents. - """ + """The provider record could not be read or written, so the provider set is unknown.""" CONTAINER_NAME: typing.Final[str] = "app" @@ -185,7 +167,7 @@ def _parse_provider_record(published: str, source: str) -> list[dict[str, str]]: for config in configs: if not isinstance(config, dict) or not isinstance(config.get("unit_name"), str): raise ProviderRecordError(f"{source} holds a provider with no unit name") - missing = [field for field in _PROVIDER_RECORD_FIELDS if not config.get(field)] + missing = [field for field in PROVIDER_REQUIRED_FIELDS if not config.get(field)] if missing: raise ProviderRecordError( f"{source} holds provider {config['unit_name']} without {', '.join(missing)}" @@ -379,17 +361,11 @@ def restart(self, rerun_migrations: bool = False) -> None: install/leader_elected before pebble is ready, then gates on workload readiness before writing config and replanning. - The provider set is read, published, reconciled against GARM and only then trimmed - and recorded — in that order, once each. A provider the relations publish joins the - record; an entry leaves it only when no relation publishes it and no scaleset GARM - still holds names it. Keep that order — delete the runners, delete the scaleset, - then drop the provider — because GARM fixes its provider map per process: a - scaleset whose provider is missing gets neither the worker that purges its instances - nor the one that deletes its VMs, and its teardown can never finish. - - The record is the charm's only knowledge of a departed configurator's credentials, - so a record that cannot be read or written raises out of the hook rather than - yielding a guessed set (see ``ProviderRecordError``). + The provider set is read, published, reconciled and then trimmed and recorded, once + each, in that order. A provider joins the record when a relation publishes it and + leaves it only when no relation publishes it and no scaleset GARM still holds names + it. Keep that order: GARM fixes its provider map per process, so a scaleset whose + provider was dropped first can never be drained or deleted. Args: rerun_migrations: Passed through to the parent restart. @@ -415,17 +391,19 @@ def restart(self, rerun_migrations: bool = False) -> None: return related_providers = self._get_configurator_provider_configs() - if not related_providers: - if not CharmState.from_charm(self).configurator_related: + recorded_providers = self._read_provider_record() + configurator_related = CharmState.from_charm(self).configurator_related + if not related_providers and not recorded_providers: + if not configurator_related: self._reconcile_runners() logger.info("GARM configurator provider data not yet available; blocking") self.update_app_and_unit_status( ops.WaitingStatus("Waiting for garm-configurator relation") ) return - # After the gate above, so an application with no configurator left keeps its - # workload untouched rather than being replanned against a shrinking provider set. - published_providers = self._published_providers(related_providers) + # The record is published even with no relation left: a drain outlives the relation, + # and a replaced pod must start GARM with the providers that drain needs. + published_providers = self._published_providers(related_providers, recorded_providers) container = self.unit.get_container(CONTAINER_NAME) entrypoint_environment = self._entrypoint_environment( @@ -454,6 +432,15 @@ def restart(self, rerun_migrations: bool = False) -> None: ) container.replan() self._maybe_first_run() + if not related_providers and configurator_related: + # A configurator is related but has published nothing yet. Reconciling now would + # rebuild the desired set from that silence and prune the scalesets it is about + # to describe, so wait with the record untouched. + logger.info("GARM configurator provider data not yet available; blocking") + self.update_app_and_unit_status( + ops.WaitingStatus("Waiting for garm-configurator relation") + ) + return # Reconcile before recording: the pass is what establishes which providers are still # referenced, and it must not be buried by the framework reporting active afterward. referenced_providers = self._reconcile_runners() @@ -678,43 +665,40 @@ def _get_configurator_provider_configs( ) return configs - def _published_providers(self, related: list[dict[str, str]]) -> list[dict[str, str]]: + def _published_providers( + self, related: list[dict[str, str]], recorded: dict[str, dict[str, str]] + ) -> list[dict[str, str]]: """Return the provider set to publish: the record with the relations on top. - A provider enters the record the first time a relation publishes it, because that is - the only moment its credentials can be read — Juju hides a departing unit from - ``relation-departed`` onwards, so a provider absent from the record at that point - could never be rebuilt. Relation data wins over the recorded copy for a provider that - is still related, so a rotated password reaches GARM. - Args: related: The provider configs the remaining relations publish. + recorded: The recorded configs, by provider name. Returns: - The provider set to publish, ordered by provider name. + The provider set to publish, ordered by provider name. Relation data wins, so a + rotated password reaches GARM. """ - merged = {config["unit_name"]: config for config in self._read_provider_record().values()} + merged = dict(recorded) merged.update({config["unit_name"]: config for config in related}) - return _by_provider_name(merged.values()) + return sorted(merged.values(), key=lambda config: config["unit_name"]) def _read_provider_record(self) -> dict[str, dict[str, str]]: """Return the recorded provider set, by provider name. Returns: - The recorded configs, or an empty set when nothing has been recorded yet — the - first start of an application, which is a normal state rather than a failure. + The recorded configs, or an empty mapping when nothing has been recorded yet. Raises: - ProviderRecordError: If the record exists but cannot be read, or is not a usable - provider set. Publishing a set derived from it would drop providers a live - scaleset still needs, so the hook must stop instead. + ProviderRecordError: If the record exists but cannot be read, or does not hold a + usable provider set: publishing from it would drop providers a live scaleset + still needs. """ try: secret = self.model.get_secret(label=GARM_PROVIDERS_LABEL) - # Refresh: tracking can lag this application's own write, and the record has to - # be read at its latest revision — reading a revision behind would resurrect a - # provider that the previous hook trimmed away. - content = secret.get_content(refresh=True) + # Latest revision without tracking it: the record is written by this application + # only, and a read one revision behind would resurrect a provider the previous + # hook trimmed away. + content = secret.peek_content() except ops.SecretNotFoundError: logger.info("No recorded GARM provider set; publishing the relations only") return {} @@ -740,9 +724,8 @@ def _trimmed_providers( could not be established. Returns: - The set to record. ``published`` unchanged when ``referenced`` is None: "no - scaleset references it" cannot be concluded from "could not tell", and a provider - kept a hook too long costs a stale entry while a dropped one cannot be rebuilt. + The set to record; ``published`` unchanged when ``referenced`` is None, because + "no scaleset references it" cannot be concluded from "could not tell". """ if referenced is None: logger.warning( @@ -764,9 +747,6 @@ def _trimmed_providers( still_needed = sorted((kept_names - related_names) & referenced) if still_needed: logger.info("Keeping providers a live scaleset still references: %s", still_needed) - # A provider a scaleset names but no relation and no record can produce cannot be - # rebuilt, so its scaleset cannot finish tearing down. Say so, rather than leaving it - # to the reconciler's "will retry" warnings. unrebuildable = referenced - kept_names - related_names if unrebuildable: logger.warning( @@ -775,13 +755,12 @@ def _trimmed_providers( " owning configurator is related again.", sorted(unrebuildable), ) - return _by_provider_name(kept) + return sorted(kept, key=lambda config: config["unit_name"]) def _save_provider_record(self, published: list[dict[str, str]]) -> None: """Record the provider set GARM is running with, so it outlives the pod. - Leader-only: every unit of this application runs its own GARM against the same - database, so one record has to describe the set they should all keep. + Leader-only: this is the single writer, so the record has one revision history. Args: published: The provider configs the workload was started with. @@ -800,14 +779,33 @@ def _save_provider_record(self, published: list[dict[str, str]]) -> None: self.app.add_secret( {GARM_PROVIDERS_SECRET_KEY: payload}, label=GARM_PROVIDERS_LABEL, - description="Provider set GARM was last started with (ISD-7262)", + description="Provider set GARM was last started with", ) return - if secret.get_content(refresh=True).get(GARM_PROVIDERS_SECRET_KEY) != payload: + superseded = secret.get_info().revision + if secret.peek_content().get(GARM_PROVIDERS_SECRET_KEY) != payload: secret.set_content({GARM_PROVIDERS_SECRET_KEY: payload}) + self._remove_superseded_revision(secret, superseded) except ops.ModelError as exc: raise ProviderRecordError(f"cannot record the GARM provider set: {exc}") from exc + @staticmethod + def _remove_superseded_revision(secret: ops.Secret, superseded: int) -> None: + """Drop the revision this write replaced, so old provider passwords do not linger. + + Only this application writes the record and every reader uses ``peek_content()``, so + no unit depends on an older revision. Removal is best effort: a store that refuses + keeps it, which costs space rather than correctness. + + Args: + secret: The record secret, after the new revision was written. + superseded: The revision the write replaced. + """ + try: + secret.remove_revision(superseded) + except (ops.ModelError, ValueError) as exc: + logger.debug("Could not remove superseded provider record revision: %s", exc) + def _resolve_secret_value(self, secret_uri: str) -> str: """Resolve a secret URI and return its ``value`` content. diff --git a/charms/garm/src/garm_entrypoint.py b/charms/garm/src/garm_entrypoint.py index fd666e42..ed618a15 100755 --- a/charms/garm/src/garm_entrypoint.py +++ b/charms/garm/src/garm_entrypoint.py @@ -35,7 +35,7 @@ "GARM_PASSPHRASE", "GARM_PROVIDERS_JSON", ) -_PROVIDER_REQUIRED_FIELDS: Final[tuple[str, ...]] = ( +PROVIDER_REQUIRED_FIELDS: Final[tuple[str, ...]] = ( "unit_name", "network", "auth_url", @@ -226,7 +226,7 @@ def _build_provider( unit_name = provider.get("unit_name") if not isinstance(unit_name, str) or not unit_name or Path(unit_name).name != unit_name: raise InvalidConfigurationError(f"Invalid provider unit name: {unit_name!r}") - missing = [name for name in _PROVIDER_REQUIRED_FIELDS if name not in provider] + missing = [name for name in PROVIDER_REQUIRED_FIELDS if name not in provider] if missing: raise InvalidConfigurationError( f"GARM provider is missing required fields: {', '.join(missing)}" diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index dfd08028..3a7a9e83 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -273,8 +273,8 @@ def _referenced_providers( continue if not scaleset.provider_name: logger.warning( - "Scaleset %s (id=%s) names no provider; the provider set cannot be" - " trimmed from this pass", + "Scaleset %s (id=%s) names no provider, so the provider set cannot be" + " mapped to its scalesets and trimming stays off until it is gone", name, scaleset.id, ) diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index b4e52b37..b4af0013 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -957,6 +957,79 @@ def test_recorded_provider_set_survives_a_replaced_pod(ctx: Context, garm_api: _ assert _published_provider_names(out) == ["configurator-b-0", "garm-configurator-0"] +def test_removing_the_last_configurator_keeps_the_drain_alive_across_a_pod( + ctx: Context, garm_api: _GarmApiMocks +): + """ + arrange: The last configurator is gone, its scaleset is still in GARM, and the record + holds its provider — then the pod is replaced, so no Pebble layer survives. + act: Reconcile on the replaced pod. + assert: The recorded provider is still published and GARM is started with it. The early + return that reports "waiting for the relation" would skip the record entirely, and a + replaced pod would start with no provider at all, so the scaleset could never drain. + """ + garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + + fired = ctx.run( + ctx.on.update_status(), + _state( + configurator_related=False, + recorded_providers=[_provider_config("garm-configurator-0")], + ), + ) + + assert _published_provider_names(fired) == ["garm-configurator-0"] + assert _recorded_provider_names(fired) == ["garm-configurator-0"] + service = fired.get_container(CONTAINER_NAME).plan.services[SERVICE_NAME] + assert service.startup == "enabled" + # Nothing is related, so every scaleset is orphaned and the sweep drains it. + assert garm_api.scaleset.return_value.reconcile.call_args.args[0] == [] + + replaced = ctx.run( + ctx.on.update_status(), + _state( + configurator_related=False, + recorded_providers=[_provider_config("garm-configurator-0")], + ), + ) + + assert _published_provider_names(replaced) == ["garm-configurator-0"] + + +def test_record_is_emptied_once_its_last_scaleset_is_gone( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: No configurator left, and GARM holds no scaleset any more. + act: Reconcile, then reconcile once more. + assert: The record is emptied on the first pass — so a departed provider's credentials do + not linger with nothing left to drain — and the next pass goes back to reporting what + it waits for, with no provider published. + """ + garm_api.scaleset.return_value.reconcile.return_value = ([], set()) + + with caplog.at_level(logging.INFO): + emptied = ctx.run( + ctx.on.update_status(), + _state( + configurator_related=False, + recorded_providers=[_provider_config("garm-configurator-0")], + ), + ) + + assert ( + "Removing provider garm-configurator-0: no scaleset references it any more" in caplog.text + ) + assert _recorded_provider_names(emptied) == [] + + out = ctx.run(ctx.on.update_status(), emptied) + + assert out.unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") + # The record stays empty: nothing is republished, so a provider whose scalesets are all + # gone cannot come back when a new configurator relates. + assert _recorded_provider_names(out) == [] + + def test_related_configurator_credentials_replace_the_recorded_ones( ctx: Context, garm_api: _GarmApiMocks ): diff --git a/docs/changelog.md b/docs/changelog.md index 56f84cc5..fee25939 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -11,6 +11,8 @@ Each revision is versioned by the date of the revision. ## 2026-10-21 - `garm`: job steps now see `USER` and `LOGNAME` set to the account their UID resolves to, so `usermod -aG $USER` followed by `sg ` works on GARM runners. This works around the duplicate-UID account layout of the runner images; removal is tracked in [#438](https://github.com/canonical/github-runner-operators/issues/438). +- `garm`: keep a removed configurator's provider published until the last scale set that uses it is gone, so removing a `garm-configurator` relation finishes instead of retrying until the five-day drain deadline. The charm records the provider set it starts GARM with, adds providers as configurators relate, and removes one only once no scale set refers to it — the provider is dropped on a later reconcile, after its scale sets are deleted. +- `garm` upgrade note: deployments already stuck before this release do not recover on upgrade, because the removed configurator's credentials are no longer available to the charm. Relate the same configurator application again — its unit name reproduces the missing provider name — so the provider is republished and the drain resumes; a scale set whose runner is stuck in the `deleted` status can also be cleared by hand. ## 2026-10-16 From 3c1843aafdea07bf84528b12bb5f561099be16e2 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 04:03:37 +0000 Subject: [PATCH 03/13] style(garm): build the published provider map without mutating a copy Replaces the copied-then-updated mapping with one literal, so the published set is a new value rather than an edited one. --- charms/garm/src/charm.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 914f68ff..71408e35 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -678,8 +678,10 @@ def _published_providers( The provider set to publish, ordered by provider name. Relation data wins, so a rotated password reaches GARM. """ - merged = dict(recorded) - merged.update({config["unit_name"]: config for config in related}) + merged = { + **recorded, + **{config["unit_name"]: config for config in related}, + } return sorted(merged.values(), key=lambda config: config["unit_name"]) def _read_provider_record(self) -> dict[str, dict[str, str]]: From 7be9b8fdd095861e787ccd2c123016d6eeba4ba1 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 07:47:33 +0000 Subject: [PATCH 04/13] chore(garm): cover the referenced-provider set and drop dead code Review follow-ups: - Delete the `_by_provider_name` helper left behind after it was inlined. - Changelog: say "configurator applications" so Vale's spellcheck accepts the plural; only the singular is in the custom word list. - Test `_referenced_providers` directly: a desired spec's provider counts before its scaleset exists, a scaleset deleted in the pass drops out, a scaleset whose delete GARM refused keeps its provider, and a surviving scaleset with no provider name yields None. The fake client now forwards `provider_name`. --- charms/garm/src/charm.py | 12 ---- .../tests/unit/test_scaleset_reconciler.py | 64 +++++++++++++++++++ docs/changelog.md | 2 +- 3 files changed, 65 insertions(+), 13 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 71408e35..1076f4b8 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -129,18 +129,6 @@ def _generate_admin_password() -> str: return "".join(chars) -def _by_provider_name(configs: typing.Iterable[dict[str, str]]) -> list[dict[str, str]]: - """Order provider configs by provider name, so a published set is deterministic. - - Args: - configs: The provider configs to order. - - Returns: - The same configs, sorted by ``unit_name``, which is the provider name. - """ - return sorted(configs, key=lambda config: config["unit_name"]) - - def _parse_provider_record(published: str, source: str) -> list[dict[str, str]]: """Parse a recorded provider set, rejecting anything that cannot drive GARM. diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index a656f31c..1bbdbd65 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -160,6 +160,7 @@ def __init__( enabled=ss.get("enabled", True), updated_at=ss.get("updated_at", None), runner_prefix=ss.get("runner_prefix", None), + provider_name=ss.get("provider_name", "openstack-demo"), ) for ss in (scalesets or []) ] @@ -522,6 +523,69 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] +def test_referenced_providers_counts_a_desired_spec_before_its_scaleset_exists(): + """ + arrange: A desired spec whose scaleset does not exist yet, and an orphaned scaleset + GARM accepts a delete for. + act: Reconcile. + assert: The desired provider is referenced even though nothing names it in GARM yet, + and the deleted scaleset's provider drops out — its provider is what the charm may + now stop publishing. + """ + client = FakeGarmClient( + providers=["openstack-demo", "retired-provider"], + scalesets=[ + _existing_scaleset(name="stale-scaleset", id=42, provider_name="retired-provider") + ], + ) + + referenced = _reconcile_referenced(client, [_spec(name="new-scaleset")]) + + assert client.deleted == [42] + assert referenced == {"openstack-demo"} + + +def test_referenced_providers_keeps_a_scaleset_whose_delete_was_refused(): + """ + arrange: An orphaned scaleset whose delete GARM rejects, because it still owns runners. + act: Reconcile. + assert: Its provider stays referenced: GARM still holds the scaleset, so the provider + it needs must not be dropped. + """ + + class _DeleteFailingClient(FakeGarmClient): + def delete_scaleset(self, scaleset_id): + raise GarmApiError("scaleset still has runners") + + client = _DeleteFailingClient( + providers=["openstack-demo", "retired-provider"], + scalesets=[ + _existing_scaleset(name="stale-scaleset", id=42, provider_name="retired-provider") + ], + ) + + referenced = _reconcile_referenced(client, [_spec(name="new-scaleset")]) + + assert referenced == {"openstack-demo", "retired-provider"} + + +def test_referenced_providers_is_unknown_when_a_scaleset_names_no_provider(): + """ + arrange: A scaleset GARM holds that names no provider at all. + act: Reconcile. + assert: The answer is None, not a set: nothing maps that scaleset to a provider, so the + caller cannot tell which provider it still needs and must drop none. + """ + # A scaleset the pass keeps, so it is still in the set the caller maps: the desired + # spec's own generation, which GARM holds without saying which provider made it. + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(provider_name=None)], + ) + + assert _reconcile_referenced(client, [_spec()]) is None + + def test_orphan_template_outlives_a_failed_orphan_delete(): """ arrange: An orphaned scaleset with a custom template, whose deletion GARM rejects. diff --git a/docs/changelog.md b/docs/changelog.md index fee25939..27712c49 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -11,7 +11,7 @@ Each revision is versioned by the date of the revision. ## 2026-10-21 - `garm`: job steps now see `USER` and `LOGNAME` set to the account their UID resolves to, so `usermod -aG $USER` followed by `sg ` works on GARM runners. This works around the duplicate-UID account layout of the runner images; removal is tracked in [#438](https://github.com/canonical/github-runner-operators/issues/438). -- `garm`: keep a removed configurator's provider published until the last scale set that uses it is gone, so removing a `garm-configurator` relation finishes instead of retrying until the five-day drain deadline. The charm records the provider set it starts GARM with, adds providers as configurators relate, and removes one only once no scale set refers to it — the provider is dropped on a later reconcile, after its scale sets are deleted. +- `garm`: keep a removed configurator's provider published until the last scale set that uses it is gone, so removing a `garm-configurator` relation finishes instead of retrying until the five-day drain deadline. The charm records the provider set it starts GARM with, adds providers as configurator applications relate, and removes one only once no scale set refers to it — the provider is dropped on a later reconcile, after its scale sets are deleted. - `garm` upgrade note: deployments already stuck before this release do not recover on upgrade, because the removed configurator's credentials are no longer available to the charm. Relate the same configurator application again — its unit name reproduces the missing provider name — so the provider is republished and the drain resumes; a scale set whose runner is stuck in the `deleted` status can also be cleared by hand. ## 2026-10-16 From 24fe02fad62772b62499000d487f1d5b47bba1c8 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 14:58:16 +0000 Subject: [PATCH 05/13] fix(garm): stop pruning the provider record revisions --- charms/garm/src/charm.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 1076f4b8..61c67250 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -772,30 +772,11 @@ def _save_provider_record(self, published: list[dict[str, str]]) -> None: description="Provider set GARM was last started with", ) return - superseded = secret.get_info().revision if secret.peek_content().get(GARM_PROVIDERS_SECRET_KEY) != payload: secret.set_content({GARM_PROVIDERS_SECRET_KEY: payload}) - self._remove_superseded_revision(secret, superseded) except ops.ModelError as exc: raise ProviderRecordError(f"cannot record the GARM provider set: {exc}") from exc - @staticmethod - def _remove_superseded_revision(secret: ops.Secret, superseded: int) -> None: - """Drop the revision this write replaced, so old provider passwords do not linger. - - Only this application writes the record and every reader uses ``peek_content()``, so - no unit depends on an older revision. Removal is best effort: a store that refuses - keeps it, which costs space rather than correctness. - - Args: - secret: The record secret, after the new revision was written. - superseded: The revision the write replaced. - """ - try: - secret.remove_revision(superseded) - except (ops.ModelError, ValueError) as exc: - logger.debug("Could not remove superseded provider record revision: %s", exc) - def _resolve_secret_value(self, secret_uri: str) -> str: """Resolve a secret URI and return its ``value`` content. From 3cf2f596341ed32f5a9fa51edd14a10bf6e44e15 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 15:40:06 +0000 Subject: [PATCH 06/13] refactor(garm): reconcile an empty provider set instead of returning early restart() had two gates that skipped the publish and the reconcile when nothing was related, which pinned the last published provider set in GARM's environment forever once the last configurator left. The chain already iterates the desired state, so an empty one reconciles to zero: publish record U related (possibly empty), restart the workload with it, then reconcile credentials, entities and scalesets against that same state. One condition is left, because every reconciler deletes what it is not told to keep: a configurator relation that has not published yet must not be read as "nothing is desired", or it would lose the credentials, entities and scalesets it is about to describe. Only a relation that has published, or no configurator at all, is a complete desired state. The unit still reports "Waiting for garm-configurator relation" while nothing is related, set after the reconcile so its Active cannot bury it. --- charms/garm/src/charm.py | 40 +++++++-------- charms/garm/tests/unit/test_charm.py | 73 +++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 22 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 61c67250..a3531daf 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -381,16 +381,10 @@ def restart(self, rerun_migrations: bool = False) -> None: related_providers = self._get_configurator_provider_configs() recorded_providers = self._read_provider_record() configurator_related = CharmState.from_charm(self).configurator_related - if not related_providers and not recorded_providers: - if not configurator_related: - self._reconcile_runners() - logger.info("GARM configurator provider data not yet available; blocking") - self.update_app_and_unit_status( - ops.WaitingStatus("Waiting for garm-configurator relation") - ) - return # The record is published even with no relation left: a drain outlives the relation, - # and a replaced pod must start GARM with the providers that drain needs. + # and a replaced pod must start GARM with the providers that drain needs. An empty + # set is published the same way, so the workload converges on what the relations and + # the record justify instead of keeping a set that nothing stands behind. published_providers = self._published_providers(related_providers, recorded_providers) container = self.unit.get_container(CONTAINER_NAME) @@ -420,21 +414,27 @@ def restart(self, rerun_migrations: bool = False) -> None: ) container.replan() self._maybe_first_run() - if not related_providers and configurator_related: - # A configurator is related but has published nothing yet. Reconciling now would - # rebuild the desired set from that silence and prune the scalesets it is about - # to describe, so wait with the record untouched. + # Reconcile before recording: the pass is what establishes which providers are still + # referenced, and it must not be buried by the framework reporting active afterward. + # + # The chain iterates the desired state, so an empty one reconciles to zero. It must + # not, however, read silence as "nothing is desired": every reconciler deletes what + # it is not told to keep, so a configurator that has not published yet would lose the + # credentials, entities and scalesets it is about to describe. Only a relation that + # has published, or no configurator at all, is a complete desired state. + if related_providers or not configurator_related: + referenced_providers = self._reconcile_runners() + self._save_provider_record( + self._trimmed_providers( + published_providers, related_providers, referenced_providers + ) + ) + else: logger.info("GARM configurator provider data not yet available; blocking") + if not related_providers: self.update_app_and_unit_status( ops.WaitingStatus("Waiting for garm-configurator relation") ) - return - # Reconcile before recording: the pass is what establishes which providers are still - # referenced, and it must not be buried by the framework reporting active afterward. - referenced_providers = self._reconcile_runners() - self._save_provider_record( - self._trimmed_providers(published_providers, related_providers, referenced_providers) - ) def _entrypoint_environment( self, diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index b4af0013..9d8fad89 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -652,7 +652,8 @@ def test_missing_configurator_relation_prunes_orphaned_scalesets( arrange: A ready charm whose garm-configurator relation has been removed. act: Run update-status. assert: Scalesets are reconciled against an empty desired state, deleting the ones the - removed relation orphaned, while the entrypoint remains disabled and the unit reports + removed relation orphaned. The workload keeps running with an empty provider set, + so nothing the relations no longer justify stays published, and the unit reports what it is waiting for. """ out = ctx.run(ctx.on.update_status(), _state(configurator_related=False)) @@ -660,7 +661,8 @@ def test_missing_configurator_relation_prunes_orphaned_scalesets( garm_api.scaleset.return_value.reconcile.assert_called_once_with([]) service = out.get_container(CONTAINER_NAME).plan.services[SERVICE_NAME] assert service.command == "/usr/local/bin/garm-entrypoint.py" - assert service.startup == "disabled" + assert service.startup == "enabled" + assert json.loads(service.environment["GARM_PROVIDERS_JSON"]) == [] assert out.unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") @@ -769,6 +771,73 @@ def test_remove_refuses_without_admin_credentials( assert "Operator action: restore the labelled admin credentials secret" in caplog.text +def test_no_configurator_unpublishes_the_last_provider( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: The record still holds a provider, and no configurator relation is left — the + state in which the last trim has been recorded but not yet published. + act: Run update-status twice. + assert: The first hook publishes the record and trims it once no scaleset references it; + the next hook starts the workload with an empty provider set, so the published set can + never outlive the relation that justified it. + """ + garm_api.scaleset.return_value.reconcile.return_value = ([], set()) + + with caplog.at_level(logging.INFO): + trimmed = ctx.run( + ctx.on.update_status(), + _state( + configurator_related=False, + recorded_providers=[_provider_config("garm-configurator-0")], + ), + ) + + # The record is published for this hook — nothing says the provider is dead yet — and the + # trim that follows empties it. + assert _published_provider_names(trimmed) == ["garm-configurator-0"] + assert _recorded_provider_names(trimmed) == [] + assert ( + "Removing provider garm-configurator-0: no scaleset references it any more" in caplog.text + ) + + out = ctx.run(ctx.on.update_status(), trimmed) + + service = out.get_container(CONTAINER_NAME).plan.services[SERVICE_NAME] + assert json.loads(service.environment["GARM_PROVIDERS_JSON"]) == [] + assert out.unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") + + +def test_silent_configurator_keeps_the_record_published_and_does_not_reconcile( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: A configurator relation that exists but has published nothing yet, while the + record holds a provider. + act: Run update-status. + assert: The recorded provider is published so the running GARM keeps what a drain needs, + and the reconcile chain does NOT run: every reconciler deletes what it is not told to + keep, so an unfinished relation would otherwise cost the scalesets it is about to + describe. + """ + with caplog.at_level(logging.INFO): + out = ctx.run( + ctx.on.update_status(), + _state( + configurator_units_data={0: {}}, + recorded_providers=[_provider_config("garm-configurator-0")], + ), + ) + + service = out.get_container(CONTAINER_NAME).plan.services[SERVICE_NAME] + assert json.loads(service.environment["GARM_PROVIDERS_JSON"])[0]["unit_name"] == ( + "garm-configurator-0" + ) + garm_api.scaleset.assert_not_called() + assert out.unit_status == ops.WaitingStatus("Waiting for garm-configurator relation") + assert "provider data not yet available" in caplog.text + + def test_unpopulated_configurator_relation_does_not_prune( ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture ): From f0ece9323430ed7055a7b1ff68d6da2280367129 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 15:50:55 +0000 Subject: [PATCH 07/13] refactor(garm): report surviving scalesets instead of referenced providers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ScalesetReconciler.reconcile() now returns the scalesets GARM still holds after the pass, not a derived set of provider names. Reporting observed state keeps the policy in the charm: it maps provider_name off the objects it is given and decides what an unmappable scaleset means, instead of the reconciler deciding which providers are safe to drop. It also removes a redundant input. The reconciler used to add every desired spec's provider to the returned set, but a spec's provider is by definition one the relations publish, which the trim already keeps — only the surviving scalesets say anything the relations do not. The unmappable case moves with it: a scaleset naming no provider is returned like any other, and _trimmed_providers keeps every provider when it sees one, with the warning naming the scalesets. --- charms/garm/src/charm.py | 40 ++++++++---- charms/garm/src/scaleset_reconciler.py | 40 +++--------- charms/garm/tests/unit/test_charm.py | 64 ++++++++++++++++--- .../tests/unit/test_scaleset_reconciler.py | 42 ++++++------ 4 files changed, 114 insertions(+), 72 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index a3531daf..954cf995 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -29,6 +29,7 @@ ) from entity_reconciler import EntityReconciler from garm_api import GarmApiClient, GarmApiError, GarmAuthenticatedClient, GarmConnectionError +from garm_client.models.scale_set import ScaleSet from garm_entrypoint import PROVIDER_REQUIRED_FIELDS from garm_template import CharmedTemplateError from garm_template import apply_charmed_template as _apply_garm_template @@ -703,29 +704,40 @@ def _read_provider_record(self) -> dict[str, dict[str, str]]: def _trimmed_providers( published: list[dict[str, str]], related: list[dict[str, str]], - referenced: set[str] | None, + scalesets: list[ScaleSet] | None, ) -> list[dict[str, str]]: - """Drop providers no relation publishes and no live scaleset references. + """Drop providers no relation publishes and no live scaleset names. Args: published: The provider set GARM was started with on this pass. related: The provider configs the remaining relations publish. - referenced: The providers of the scalesets GARM still holds, or None when that - could not be established. + scalesets: The scalesets GARM still holds after the pass, or None when the pass + could not establish that. Returns: - The set to record; ``published`` unchanged when ``referenced`` is None, because - "no scaleset references it" cannot be concluded from "could not tell". + The set to record; ``published`` unchanged when the surviving scalesets are + unknown, or when one of them names no provider — "no scaleset references it" + cannot be concluded from "could not tell". """ - if referenced is None: + if scalesets is None: logger.warning( "The scalesets GARM still holds could not be read; keeping every provider" " this hook: %s", sorted(config["unit_name"] for config in published), ) return published + unmapped = [scaleset.name for scaleset in scalesets if not scaleset.provider_name] + if unmapped: + logger.warning( + "Scalesets %s name no provider, so the provider set cannot be mapped to the" + " scalesets GARM holds; keeping every provider this hook: %s", + unmapped, + sorted(config["unit_name"] for config in published), + ) + return published related_names = {config["unit_name"] for config in related} + referenced = {scaleset.provider_name for scaleset in scalesets if scaleset.provider_name} kept = [ config for config in published @@ -1005,7 +1017,7 @@ def _add_unit_credential( description=MANAGED_CREDENTIAL_DESCRIPTION, ) - def _reconcile_runners(self) -> set[str] | None: + def _reconcile_runners(self) -> list[ScaleSet] | None: """Sync GARM controller URLs, GitHub credentials, entities, then scalesets. The steps are ordered, not independent: GARM rejects every operational call with @@ -1016,10 +1028,10 @@ def _reconcile_runners(self) -> set[str] | None: scaleset can reference it. Returns: - The providers the scalesets GARM still holds are created with, or None when that - could not be established — no admin credentials, a connection or authorization - failure, or a template that could not be applied. None means "unknown", and the - caller must read it as "drop nothing". + The scalesets GARM still holds after the pass, or None when the pass could not + run — no admin credentials, a connection or authorization failure, or a template + that could not be applied. None means "unknown", and the caller must read it as + "drop nothing". """ admin_creds = self._get_admin_credentials() if not admin_creds: @@ -1047,7 +1059,7 @@ def _reconcile_runners(self) -> set[str] | None: GithubReconciler(auth_client).reconcile(self._build_desired_credentials()) EntityReconciler(auth_client, agent_mode).reconcile(charm_state.desired_entities) template_id = _apply_garm_template(auth_client, charm_state.ssh_debug_connections) - replacing, referenced = ScalesetReconciler(auth_client).reconcile( + replacing, surviving_scalesets = ScalesetReconciler(auth_client).reconcile( self._build_desired_scalesets(template_id) ) # A label change recreates the scaleset and drains the old one, which @@ -1062,7 +1074,7 @@ def _reconcile_runners(self) -> set[str] | None: ) else: self.update_app_and_unit_status(ops.ActiveStatus()) - return referenced + return surviving_scalesets except CharmedTemplateError as exc: logger.warning("GARM charmed template error during reconcile: %s", exc) self.update_app_and_unit_status(ops.WaitingStatus(str(exc))) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 3a7a9e83..9f20b2a9 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -164,7 +164,7 @@ def __init__(self, client: GarmAuthenticatedClient) -> None: def reconcile( self, desired: list[ScalesetSpec] - ) -> tuple[list[ScalesetProgress], set[str] | None]: + ) -> tuple[list[ScalesetProgress], list[ScaleSet]]: """Sync GARM scalesets to match *desired*. Performs the minimum set of CREATE / UPDATE / DELETE operations, and @@ -194,11 +194,10 @@ def reconcile( desired: The full desired set of scalesets. Returns: - The replacements still in flight (empty once converged), and the providers the - scalesets GARM still holds are created with, so the caller can drop a provider - only once nothing references it. The provider set is ``None`` when it cannot be - established: a surviving scaleset with no provider name leaves nothing to map - it to, and "nothing is referenced" must never be guessed from a failed read. + The replacements still in flight (empty once converged), and the scalesets GARM + still holds after this pass. Callers decide what those mean — the charm keeps a + provider published while a scaleset still names it — so this reports what GARM + answered rather than concluding anything about the desired state. Raises: GarmApiError: If any spec failed to reconcile, re-raised once the rest @@ -244,43 +243,24 @@ def reconcile( deleted.add(name) if failure is not None: raise failure - return progress, self._referenced_providers(desired, observed, deleted) + return progress, self._surviving_scalesets(observed, deleted) @staticmethod - def _referenced_providers( - desired: list[ScalesetSpec], observed: dict[str, ScaleSet], deleted: set[str] - ) -> set[str] | None: - """Return the providers the scalesets GARM still holds were created with. + def _surviving_scalesets(observed: dict[str, ScaleSet], deleted: set[str]) -> list[ScaleSet]: + """Return the scalesets GARM still holds after this pass. Built from what GARM answered, not from what this pass asked for: a scaleset whose delete was refused, deferred, or never attempted is still held, and a provider dropped while its scaleset survives can never be handed back. Args: - desired: The specs reconciled by this pass; their providers stay referenced even - while the scaleset that will use one is only now being created. observed: The scalesets GARM listed before this pass, by name. deleted: Names of scalesets GARM accepted a delete for during this pass. Returns: - The referenced provider names, or None when a surviving scaleset has no provider - name — GARM holds it, but nothing maps it to a provider, so the caller cannot - tell which provider is still needed and must not drop any. + The surviving scalesets, ordered by name so a caller's decisions are stable. """ - referenced = {spec.provider_name for spec in desired} - for name, scaleset in observed.items(): - if name in deleted: - continue - if not scaleset.provider_name: - logger.warning( - "Scaleset %s (id=%s) names no provider, so the provider set cannot be" - " mapped to its scalesets and trimming stays off until it is gone", - name, - scaleset.id, - ) - return None - referenced.add(scaleset.provider_name) - return referenced + return [observed[name] for name in sorted(observed) if name not in deleted] @staticmethod def _dedupe(desired: list[ScalesetSpec]) -> list[ScalesetSpec]: diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 9d8fad89..2958de2e 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -15,6 +15,7 @@ import json import logging import os +import types import typing from unittest.mock import MagicMock, PropertyMock, patch @@ -160,7 +161,7 @@ def garm_api_fixture() -> typing.Iterator[_GarmApiMocks]: # No scaleset is mid-replacement by default, so the charm reports active. The # referenced set is what the pass observed GARM still holds: empty means no scaleset # references any provider. - scaleset_cls.return_value.reconcile.return_value = ([], set()) + scaleset_cls.return_value.reconcile.return_value = ([], []) calls = MagicMock() calls.attach_mock(auth_client.update_controller, "controller") calls.attach_mock(agent_tools, "agent_tools") @@ -314,6 +315,11 @@ def _configurator_relation(app_name: str) -> Relation: ) +def _held_scaleset(provider_name: str | None, name: str = "held-scaleset-left-behind"): + """Return a scaleset as GARM reports it after a pass: only the fields the charm reads.""" + return types.SimpleNamespace(name=name, provider_name=provider_name) + + def _recorded_provider_names(state: State) -> list[str]: """Return the provider names the ``garm-providers`` record holds, latest revision.""" secret = next(secret for secret in state.secrets if secret.label == GARM_PROVIDERS_LABEL) @@ -782,7 +788,7 @@ def test_no_configurator_unpublishes_the_last_provider( the next hook starts the workload with an empty provider set, so the published set can never outlive the relation that justified it. """ - garm_api.scaleset.return_value.reconcile.return_value = ([], set()) + garm_api.scaleset.return_value.reconcile.return_value = ([], []) with caplog.at_level(logging.INFO): trimmed = ctx.run( @@ -890,7 +896,10 @@ def test_departing_configurator_provider_is_kept_while_its_scaleset_remains( provider here would strand the scaleset: GARM rejects deleting the runner in its ``deleted`` status and the scaleset while it still lists runners. """ - garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + garm_api.scaleset.return_value.reconcile.return_value = ( + [], + [_held_scaleset("garm-configurator-0")], + ) with caplog.at_level(logging.INFO): out = ctx.run( @@ -932,7 +941,10 @@ def test_departing_configurator_provider_is_removed_once_its_scaleset_is_gone( loses it on the next — the trim is recorded after the pass that justified it, so it takes effect at the following replan rather than restarting GARM twice. """ - garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + garm_api.scaleset.return_value.reconcile.return_value = ( + [], + [_held_scaleset("garm-configurator-0")], + ) with caplog.at_level(logging.INFO): fired = ctx.run( ctx.on.update_status(), @@ -948,7 +960,7 @@ def test_departing_configurator_provider_is_removed_once_its_scaleset_is_gone( assert _published_provider_names(fired) == ["configurator-b-0", "garm-configurator-0"] assert _recorded_provider_names(fired) == ["configurator-b-0", "garm-configurator-0"] - garm_api.scaleset.return_value.reconcile.return_value = ([], set()) + garm_api.scaleset.return_value.reconcile.return_value = ([], []) with caplog.at_level(logging.INFO): trimmed = ctx.run(ctx.on.update_status(), fired) @@ -996,6 +1008,34 @@ def test_provider_set_is_not_shrunk_when_the_scaleset_state_is_unknown( assert "keeping every provider this hook" in caplog.text +def test_provider_set_is_not_shrunk_when_a_scaleset_names_no_provider( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + arrange: A departing configurator, and a scaleset GARM holds that names no provider. + act: Run update-status. + assert: Every published provider is kept: nothing maps that scaleset to a provider, so + the charm cannot tell which provider it still needs — and dropping one it cannot rule + out is the deadlock. + """ + garm_api.scaleset.return_value.reconcile.return_value = ([], [_held_scaleset(None)]) + + with caplog.at_level(logging.INFO): + out = ctx.run( + ctx.on.update_status(), + _state( + configurator_relations=[_configurator_relation("configurator-b")], + recorded_providers=[ + _provider_config("configurator-b-0"), + _provider_config("garm-configurator-0"), + ], + ), + ) + + assert _published_provider_names(out) == ["configurator-b-0", "garm-configurator-0"] + assert "name no provider" in caplog.text + + def test_recorded_provider_set_survives_a_replaced_pod(ctx: Context, garm_api: _GarmApiMocks): """ arrange: Two configurators publishing providers, reconciled once so the set is recorded, @@ -1014,7 +1054,10 @@ def test_recorded_provider_set_survives_a_replaced_pod(ctx: Context, garm_api: _ recorded = _recorded_provider_names(first) assert set(recorded) == {"garm-configurator-0", "configurator-b-0"} - garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + garm_api.scaleset.return_value.reconcile.return_value = ( + [], + [_held_scaleset("garm-configurator-0")], + ) out = ctx.run( ctx.on.update_status(), _state( @@ -1037,7 +1080,10 @@ def test_removing_the_last_configurator_keeps_the_drain_alive_across_a_pod( return that reports "waiting for the relation" would skip the record entirely, and a replaced pod would start with no provider at all, so the scaleset could never drain. """ - garm_api.scaleset.return_value.reconcile.return_value = ([], {"garm-configurator-0"}) + garm_api.scaleset.return_value.reconcile.return_value = ( + [], + [_held_scaleset("garm-configurator-0")], + ) fired = ctx.run( ctx.on.update_status(), @@ -1075,7 +1121,7 @@ def test_record_is_emptied_once_its_last_scaleset_is_gone( not linger with nothing left to drain — and the next pass goes back to reporting what it waits for, with no provider published. """ - garm_api.scaleset.return_value.reconcile.return_value = ([], set()) + garm_api.scaleset.return_value.reconcile.return_value = ([], []) with caplog.at_level(logging.INFO): emptied = ctx.run( @@ -1148,7 +1194,7 @@ def test_unrebuildable_provider_is_reported( """ garm_api.scaleset.return_value.reconcile.return_value = ( [], - {"garm-configurator-0", "vanished-configurator-0"}, + [_held_scaleset("garm-configurator-0"), _held_scaleset("vanished-configurator-0")], ) with caplog.at_level(logging.INFO): diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 1bbdbd65..40457e63 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -523,14 +523,14 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] -def test_referenced_providers_counts_a_desired_spec_before_its_scaleset_exists(): +def test_surviving_scalesets_exclude_the_ones_deleted_in_the_pass(): """ - arrange: A desired spec whose scaleset does not exist yet, and an orphaned scaleset - GARM accepts a delete for. + arrange: An orphaned scaleset GARM accepts a delete for, and a desired spec whose + scaleset does not exist yet. act: Reconcile. - assert: The desired provider is referenced even though nothing names it in GARM yet, - and the deleted scaleset's provider drops out — its provider is what the charm may - now stop publishing. + assert: Only the scalesets GARM still holds are returned: the deleted one drops out, and + a scaleset the pass only created is not reported, because the caller derives what is + still needed from what GARM answered, not from what the pass asked for. """ client = FakeGarmClient( providers=["openstack-demo", "retired-provider"], @@ -539,18 +539,19 @@ def test_referenced_providers_counts_a_desired_spec_before_its_scaleset_exists() ], ) - referenced = _reconcile_referenced(client, [_spec(name="new-scaleset")]) + surviving = _reconcile_referenced(client, [_spec(name="new-scaleset")]) assert client.deleted == [42] - assert referenced == {"openstack-demo"} + assert [scaleset.name for scaleset in surviving] == [] -def test_referenced_providers_keeps_a_scaleset_whose_delete_was_refused(): +def test_surviving_scalesets_keep_a_scaleset_whose_delete_was_refused(): """ arrange: An orphaned scaleset whose delete GARM rejects, because it still owns runners. act: Reconcile. - assert: Its provider stays referenced: GARM still holds the scaleset, so the provider - it needs must not be dropped. + assert: The scaleset is reported as surviving, so the caller keeps the provider it names: + GARM still holds it, and a provider dropped while its scaleset survives can never be + handed back. """ class _DeleteFailingClient(FakeGarmClient): @@ -564,26 +565,29 @@ def delete_scaleset(self, scaleset_id): ], ) - referenced = _reconcile_referenced(client, [_spec(name="new-scaleset")]) + surviving = _reconcile_referenced(client, [_spec(name="new-scaleset")]) - assert referenced == {"openstack-demo", "retired-provider"} + assert [scaleset.name for scaleset in surviving] == ["stale-scaleset"] + assert surviving[0].provider_name == "retired-provider" -def test_referenced_providers_is_unknown_when_a_scaleset_names_no_provider(): +def test_surviving_scalesets_are_reported_even_with_no_provider_name(): """ arrange: A scaleset GARM holds that names no provider at all. act: Reconcile. - assert: The answer is None, not a set: nothing maps that scaleset to a provider, so the - caller cannot tell which provider it still needs and must drop none. + assert: The scaleset is still reported as observed, and the caller decides what an + unmappable scaleset means — the reconciler states what GARM answered, it does not + conclude which providers are safe to drop. """ - # A scaleset the pass keeps, so it is still in the set the caller maps: the desired - # spec's own generation, which GARM holds without saying which provider made it. client = FakeGarmClient( providers=["openstack-demo"], scalesets=[_existing_scaleset(provider_name=None)], ) - assert _reconcile_referenced(client, [_spec()]) is None + surviving = _reconcile_referenced(client, [_spec()]) + + assert [scaleset.name for scaleset in surviving] == [_existing_scaleset()["name"]] + assert surviving[0].provider_name is None def test_orphan_template_outlives_a_failed_orphan_delete(): From d9b64994a2123f61fbe58f99ed8cbcccad5e6007 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 15:58:00 +0000 Subject: [PATCH 08/13] refactor(garm): report every live scaleset after a pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reconciler now returns the state of the world after the pass — what GARM listed and kept, plus what this pass created, minus what it deleted — instead of the survivors of the pre-pass observation. A caller never has to reason about which half of a pass a scaleset came from, and a scaleset created in the pass is no longer invisible to it. _create() returns the scaleset GARM answers with, and the pass accumulates those so the live set can be assembled in one place. `_live_scalesets()` replaces `_surviving_scalesets()`. The fake client now returns a scaleset from its create methods, as the real one does; a fake that returned None hid the created half of the live set. --- charms/garm/src/scaleset_reconciler.py | 67 ++++++++++++++----- .../tests/unit/test_scaleset_reconciler.py | 62 ++++++++++++----- 2 files changed, 95 insertions(+), 34 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 9f20b2a9..65ce71af 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -194,10 +194,11 @@ def reconcile( desired: The full desired set of scalesets. Returns: - The replacements still in flight (empty once converged), and the scalesets GARM - still holds after this pass. Callers decide what those mean — the charm keeps a - provider published while a scaleset still names it — so this reports what GARM - answered rather than concluding anything about the desired state. + The replacements still in flight (empty once converged), and every scaleset GARM + holds after this pass — listed and kept, newly created, or not deleted. Callers + decide what those mean: the charm keeps a provider published while a scaleset + still names it, so this reports live state rather than concluding anything about + the desired state. Raises: GarmApiError: If any spec failed to reconcile, re-raised once the rest @@ -222,12 +223,18 @@ def reconcile( claimed.update(family) progress: list[ScalesetProgress] = [] + created: dict[str, ScaleSet] = {} failure: GarmApiError | None = None for spec in desired: try: progress.extend( self._reconcile_one( - spec, providers, observed, templates, families.get(spec.name, []) + spec, + providers, + observed, + templates, + families.get(spec.name, []), + created, ) ) except (GarmConnectionError, GarmUnauthorizedError): @@ -243,24 +250,33 @@ def reconcile( deleted.add(name) if failure is not None: raise failure - return progress, self._surviving_scalesets(observed, deleted) + return progress, self._live_scalesets(observed, deleted, created) @staticmethod - def _surviving_scalesets(observed: dict[str, ScaleSet], deleted: set[str]) -> list[ScaleSet]: - """Return the scalesets GARM still holds after this pass. + def _live_scalesets( + observed: dict[str, ScaleSet], deleted: set[str], created: dict[str, ScaleSet] + ) -> list[ScaleSet]: + """Return every scaleset GARM holds after this pass. - Built from what GARM answered, not from what this pass asked for: a scaleset whose - delete was refused, deferred, or never attempted is still held, and a provider - dropped while its scaleset survives can never be handed back. + The live set is what GARM answered, adjusted by what this pass did to it: the + scalesets it listed and kept, the ones it created, minus the ones whose delete GARM + accepted. Reporting the state after the pass rather than the observation before it + means a caller never has to reason about which half of a pass a scaleset came from. + + A scaleset whose delete was refused, deferred, or never attempted is still live, and + a provider dropped while its scaleset survives can never be handed back. Args: observed: The scalesets GARM listed before this pass, by name. deleted: Names of scalesets GARM accepted a delete for during this pass. + created: Scalesets this pass created, by name. Returns: - The surviving scalesets, ordered by name so a caller's decisions are stable. + The live scalesets, ordered by name so a caller's decisions are stable. """ - return [observed[name] for name in sorted(observed) if name not in deleted] + live = {name: scaleset for name, scaleset in observed.items() if name not in deleted} + live.update(created) + return [live[name] for name in sorted(live)] @staticmethod def _dedupe(desired: list[ScalesetSpec]) -> list[ScalesetSpec]: @@ -348,6 +364,7 @@ def _reconcile_one( observed: dict[str, ScaleSet], templates: dict[str, Template], family: list[str], + created: dict[str, ScaleSet], ) -> list[ScalesetProgress]: """Reconcile a single desired scaleset: validate, create or update, and sync its template. @@ -357,6 +374,7 @@ def _reconcile_one( observed: Observed scalesets keyed by name. templates: Observed templates keyed by name. family: Every live generation of this spec, plus its target name. + created: Scalesets this pass created, filled in so the caller sees them as live. Returns: One entry per replaced generation of this spec still draining. @@ -394,7 +412,9 @@ def _reconcile_one( if active_name in observed: self._maybe_update(observed[active_name], spec, template_id) else: - self._create(spec, active_name, entity_id, create_params, template_id) + created[active_name] = self._create( + spec, active_name, entity_id, create_params, template_id + ) if not spec.runner_config.has_config(): # Runner options were cleared (or the system template is @@ -1089,14 +1109,25 @@ def _create( entity_id: str, params: CreateScaleSetParams, template_id: int, - ) -> None: + ) -> ScaleSet: + """Create the scaleset and return it as GARM reports it. + + Args: + spec: The desired scaleset. + scaleset_name: The generation name to create. + entity_id: The GARM entity UUID to create it under. + params: Creation parameters. + template_id: The runner template to reference, if any. + + Returns: + The created scaleset. + """ if template_id: params.template_id = template_id logger.info("Creating scaleset %s under %s %s", scaleset_name, spec.entity_type, entity_id) if spec.entity_type == "organization": - self._client.create_org_scaleset(entity_id, params) - else: - self._client.create_repo_scaleset(entity_id, params) + return self._client.create_org_scaleset(entity_id, params) + return self._client.create_repo_scaleset(entity_id, params) def _maybe_update(self, observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> None: observed_labels = _observed_labels(observed) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 40457e63..5a11a511 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -190,11 +190,35 @@ def find_org_id(self, _name): def find_repo_id(self, _name): return self._repo_id + def _created_scaleset(self, params, sid): + """Model GARM's answer to a create: the scaleset it now holds. + + The reconciler reports the live set after a pass, so it keeps what the API returns + for a create — a fake that returns None would hide that half of the pass. + """ + return _FakeScaleset( + name=params.name, + sid=sid, + provider_name=params.provider_name, + image=params.image, + flavor=params.flavor, + max_runners=params.max_runners, + min_idle_runners=params.min_idle_runners, + github_runner_group=getattr(params, "github_runner_group", None), + enable_shell=getattr(params, "enable_shell", None), + extra_specs=getattr(params, "extra_specs", None) or {}, + tags=getattr(params, "tags", None), + template_id=getattr(params, "template_id", None), + runner_prefix=getattr(params, "runner_prefix", None), + ) + def create_org_scaleset(self, org_id, params): self.created.append(("org", org_id, params)) + return self._created_scaleset(params, sid=1000 + len(self.created)) def create_repo_scaleset(self, repo_id, params): self.created.append(("repo", repo_id, params)) + return self._created_scaleset(params, sid=2000 + len(self.created)) def update_scaleset(self, scaleset_id, params): # Models a GARM that rejects max_runners=0 on update. Today's GARM does not @@ -289,8 +313,8 @@ def _reconcile(client, desired): return progress -def _reconcile_referenced(client, desired): - """Reconcile and return the providers GARM still holds scalesets for.""" +def _reconcile_live(client, desired): + """Reconcile and return every scaleset GARM holds afterwards.""" return ScalesetReconciler(client).reconcile(desired)[1] @@ -523,14 +547,14 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] -def test_surviving_scalesets_exclude_the_ones_deleted_in_the_pass(): +def test_live_scalesets_exclude_the_ones_deleted_in_the_pass(): """ arrange: An orphaned scaleset GARM accepts a delete for, and a desired spec whose scaleset does not exist yet. act: Reconcile. - assert: Only the scalesets GARM still holds are returned: the deleted one drops out, and - a scaleset the pass only created is not reported, because the caller derives what is - still needed from what GARM answered, not from what the pass asked for. + assert: The live set is the state after the pass: the deleted orphan drops out and the + scaleset this pass created is reported, so a caller never has to reason about which + half of the pass a scaleset came from. """ client = FakeGarmClient( providers=["openstack-demo", "retired-provider"], @@ -539,13 +563,14 @@ def test_surviving_scalesets_exclude_the_ones_deleted_in_the_pass(): ], ) - surviving = _reconcile_referenced(client, [_spec(name="new-scaleset")]) + live = _reconcile_live(client, [_spec(name="new-scaleset")]) assert client.deleted == [42] - assert [scaleset.name for scaleset in surviving] == [] + assert [scaleset.name for scaleset in live] == [target_scaleset_name("new-scaleset", [])] + assert live[0].provider_name == "openstack-demo" -def test_surviving_scalesets_keep_a_scaleset_whose_delete_was_refused(): +def test_live_scalesets_keep_a_scaleset_whose_delete_was_refused(): """ arrange: An orphaned scaleset whose delete GARM rejects, because it still owns runners. act: Reconcile. @@ -565,13 +590,18 @@ def delete_scaleset(self, scaleset_id): ], ) - surviving = _reconcile_referenced(client, [_spec(name="new-scaleset")]) + live = _reconcile_live(client, [_spec(name="new-scaleset")]) - assert [scaleset.name for scaleset in surviving] == ["stale-scaleset"] - assert surviving[0].provider_name == "retired-provider" + # Both are live: the orphan whose delete GARM refused, and the scaleset this pass created. + assert [scaleset.name for scaleset in live] == [ + target_scaleset_name("new-scaleset", []), + "stale-scaleset", + ] + by_name = {scaleset.name: scaleset for scaleset in live} + assert by_name["stale-scaleset"].provider_name == "retired-provider" -def test_surviving_scalesets_are_reported_even_with_no_provider_name(): +def test_live_scalesets_are_reported_even_with_no_provider_name(): """ arrange: A scaleset GARM holds that names no provider at all. act: Reconcile. @@ -584,10 +614,10 @@ def test_surviving_scalesets_are_reported_even_with_no_provider_name(): scalesets=[_existing_scaleset(provider_name=None)], ) - surviving = _reconcile_referenced(client, [_spec()]) + live = _reconcile_live(client, [_spec()]) - assert [scaleset.name for scaleset in surviving] == [_existing_scaleset()["name"]] - assert surviving[0].provider_name is None + assert [scaleset.name for scaleset in live] == [_existing_scaleset()["name"]] + assert live[0].provider_name is None def test_orphan_template_outlives_a_failed_orphan_delete(): From 8b515fc6f4e0a81c46e2d955d0a91ae6f738ded3 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 16:10:06 +0000 Subject: [PATCH 09/13] refactor(garm): model a provider config as a value object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The provider config travelled as dict[str, str] with string keys repeated across charm.py — built from relation data, merged, trimmed, serialised and parsed back. ProviderConfig (charm_state.py, per the relation-derived value object rule) names the fields, keeps the wire contract in one place via as_dict(), and makes the merge, trim and log lines read as config.unit_name instead of config["unit_name"]. The published form is unchanged — same keys, same JSON — so the entrypoint and the record keep reading what they read before. A test pins the dataclass fields against the entrypoint's required-field tuple, so a field added to one without the other fails here rather than in a workload that cannot start. --- charms/garm/src/charm.py | 85 ++++++++++++---------- charms/garm/src/charm_state.py | 40 ++++++++++ charms/garm/tests/unit/test_charm_state.py | 34 ++++++++- 3 files changed, 120 insertions(+), 39 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 954cf995..c204f0e5 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -23,6 +23,7 @@ DEBUG_SSH_INTEGRATION_NAME, GARM_CONFIGURATOR_RELATION_NAME, CharmState, + ProviderConfig, RunnerConfig, credential_name, resolve_entity, @@ -130,7 +131,7 @@ def _generate_admin_password() -> str: return "".join(chars) -def _parse_provider_record(published: str, source: str) -> list[dict[str, str]]: +def _parse_provider_record(published: str, source: str) -> list[ProviderConfig]: """Parse a recorded provider set, rejecting anything that cannot drive GARM. Args: @@ -153,6 +154,7 @@ def _parse_provider_record(published: str, source: str) -> list[dict[str, str]]: raise ProviderRecordError(f"{source} is not valid JSON: {exc}") from exc if not isinstance(configs, list): raise ProviderRecordError(f"{source} is not a list of providers") + parsed: list[ProviderConfig] = [] for config in configs: if not isinstance(config, dict) or not isinstance(config.get("unit_name"), str): raise ProviderRecordError(f"{source} holds a provider with no unit name") @@ -161,7 +163,10 @@ def _parse_provider_record(published: str, source: str) -> list[dict[str, str]]: raise ProviderRecordError( f"{source} holds provider {config['unit_name']} without {', '.join(missing)}" ) - return configs + parsed.append( + ProviderConfig(**{field: str(config[field]) for field in PROVIDER_REQUIRED_FIELDS}) + ) + return parsed def _parse_pre_install_scripts(raw: str) -> dict[str, str]: @@ -441,7 +446,7 @@ def _entrypoint_environment( self, postgresql_config: dict[str, typing.Any], secrets_data: dict[str, str], - provider_configs: list[dict[str, str]], + provider_configs: list[ProviderConfig], ) -> dict[str, str]: """Build the environment consumed by the GARM entrypoint.""" environment = { @@ -453,7 +458,9 @@ def _entrypoint_environment( "GARM_JWT_SECRET": secrets_data["jwt-secret"], "GARM_PASSPHRASE": secrets_data["db-passphrase"], "GARM_PROVIDERS_JSON": json.dumps( - provider_configs, sort_keys=True, separators=(",", ":") + [config.as_dict() for config in provider_configs], + sort_keys=True, + separators=(",", ":"), ), "APP_BASE_URL": self._base_url, } @@ -576,7 +583,7 @@ def _get_postgresql_config(self) -> dict[str, typing.Any] | None: def _get_configurator_provider_configs( self, - ) -> list[dict[str, str]]: + ) -> list[ProviderConfig]: """Read OpenStack provider configs from all Configurator applications. Each Configurator application is expected to have one unit, which @@ -590,9 +597,7 @@ def _get_configurator_provider_configs( clouds.yaml file. Returns: - A list of dicts, each containing the provider config fields - (auth_url, username, password, project_name, etc.) plus a - ``unit_name`` key for the provider's TOML name. + One ``ProviderConfig`` per configurator application. """ relations = sorted( self.model.relations.get(GARM_CONFIGURATOR_RELATION_NAME, []), @@ -602,7 +607,7 @@ def _get_configurator_provider_configs( logger.info("GARM configurator relation not available; provider_count=0") return [] - configs: list[dict[str, str]] = [] + configs: list[ProviderConfig] = [] for relation in relations: units = sorted(relation.units, key=lambda unit: unit.name) if not units: @@ -634,17 +639,17 @@ def _get_configurator_provider_configs( password = self._resolve_secret_value(str(password_secret_uri)) configs.append( - { - "unit_name": unit_name, - "auth_url": data.get("openstack_auth_url", ""), - "username": data.get("openstack_username", ""), - "password": password, - "project_name": data.get("openstack_project_name", ""), - "user_domain_name": data.get("openstack_user_domain_name", ""), - "project_domain_name": data.get("openstack_project_domain_name", ""), - "region_name": data.get("openstack_region_name", ""), - "network": data.get("openstack_network", ""), - } + ProviderConfig( + unit_name=unit_name, + auth_url=data.get("openstack_auth_url", ""), + username=data.get("openstack_username", ""), + password=password, + project_name=data.get("openstack_project_name", ""), + user_domain_name=data.get("openstack_user_domain_name", ""), + project_domain_name=data.get("openstack_project_domain_name", ""), + region_name=data.get("openstack_region_name", ""), + network=data.get("openstack_network", ""), + ) ) logger.info( @@ -655,8 +660,8 @@ def _get_configurator_provider_configs( return configs def _published_providers( - self, related: list[dict[str, str]], recorded: dict[str, dict[str, str]] - ) -> list[dict[str, str]]: + self, related: list[ProviderConfig], recorded: dict[str, ProviderConfig] + ) -> list[ProviderConfig]: """Return the provider set to publish: the record with the relations on top. Args: @@ -669,11 +674,11 @@ def _published_providers( """ merged = { **recorded, - **{config["unit_name"]: config for config in related}, + **{config.unit_name: config for config in related}, } - return sorted(merged.values(), key=lambda config: config["unit_name"]) + return sorted(merged.values(), key=lambda config: config.unit_name) - def _read_provider_record(self) -> dict[str, dict[str, str]]: + def _read_provider_record(self) -> dict[str, ProviderConfig]: """Return the recorded provider set, by provider name. Returns: @@ -698,14 +703,14 @@ def _read_provider_record(self) -> dict[str, dict[str, str]]: recorded = _parse_provider_record( content.get(GARM_PROVIDERS_SECRET_KEY, ""), "the GARM provider record" ) - return {config["unit_name"]: config for config in recorded} + return {config.unit_name: config for config in recorded} @staticmethod def _trimmed_providers( - published: list[dict[str, str]], - related: list[dict[str, str]], + published: list[ProviderConfig], + related: list[ProviderConfig], scalesets: list[ScaleSet] | None, - ) -> list[dict[str, str]]: + ) -> list[ProviderConfig]: """Drop providers no relation publishes and no live scaleset names. Args: @@ -723,7 +728,7 @@ def _trimmed_providers( logger.warning( "The scalesets GARM still holds could not be read; keeping every provider" " this hook: %s", - sorted(config["unit_name"] for config in published), + sorted(config.unit_name for config in published), ) return published unmapped = [scaleset.name for scaleset in scalesets if not scaleset.provider_name] @@ -732,19 +737,19 @@ def _trimmed_providers( "Scalesets %s name no provider, so the provider set cannot be mapped to the" " scalesets GARM holds; keeping every provider this hook: %s", unmapped, - sorted(config["unit_name"] for config in published), + sorted(config.unit_name for config in published), ) return published - related_names = {config["unit_name"] for config in related} + related_names = {config.unit_name for config in related} referenced = {scaleset.provider_name for scaleset in scalesets if scaleset.provider_name} kept = [ config for config in published - if config["unit_name"] in related_names or config["unit_name"] in referenced + if config.unit_name in related_names or config.unit_name in referenced ] - kept_names = {config["unit_name"] for config in kept} - for name in sorted({config["unit_name"] for config in published} - kept_names): + kept_names = {config.unit_name for config in kept} + for name in sorted({config.unit_name for config in published} - kept_names): logger.info("Removing provider %s: no scaleset references it any more", name) still_needed = sorted((kept_names - related_names) & referenced) if still_needed: @@ -757,9 +762,9 @@ def _trimmed_providers( " owning configurator is related again.", sorted(unrebuildable), ) - return sorted(kept, key=lambda config: config["unit_name"]) + return sorted(kept, key=lambda config: config.unit_name) - def _save_provider_record(self, published: list[dict[str, str]]) -> None: + def _save_provider_record(self, published: list[ProviderConfig]) -> None: """Record the provider set GARM is running with, so it outlives the pod. Leader-only: this is the single writer, so the record has one revision history. @@ -773,7 +778,11 @@ def _save_provider_record(self, published: list[dict[str, str]]) -> None: """ if not self.unit.is_leader(): return - payload = json.dumps(published, sort_keys=True, separators=(",", ":")) + payload = json.dumps( + [config.as_dict() for config in published], + sort_keys=True, + separators=(",", ":"), + ) try: try: secret = self.model.get_secret(label=GARM_PROVIDERS_LABEL) diff --git a/charms/garm/src/charm_state.py b/charms/garm/src/charm_state.py index ff1ed075..b91fa026 100644 --- a/charms/garm/src/charm_state.py +++ b/charms/garm/src/charm_state.py @@ -385,3 +385,43 @@ def from_charm(cls, charm: ops.CharmBase) -> "CharmState": charm.model.relations.get(GARM_CONFIGURATOR_RELATION_NAME, []) ), ) + + +@dataclasses.dataclass(frozen=True) +class ProviderConfig: + """One OpenStack provider, as the charm publishes it to GARM. + + The provider's name is its configurator's unit name with ``/`` replaced by ``-``, which is + also the key GARM's provider registry and the scale sets' ``provider_name`` use. The field + names are the wire contract: ``as_dict()`` is what the workload entrypoint reads out of + ``GARM_PROVIDERS_JSON`` and what the record keeps, so renaming a field changes both. + + Attributes: + unit_name: The provider name. + auth_url: Keystone authentication URL. + username: OpenStack username. + password: OpenStack password, in plaintext — it goes into the provider's clouds.yaml. + project_name: OpenStack project (tenant). + user_domain_name: Domain of the user. + project_domain_name: Domain of the project. + region_name: OpenStack region. + network: Network name or ID instances are attached to. + """ + + unit_name: str + auth_url: str + username: str + password: str + project_name: str + user_domain_name: str + project_domain_name: str + region_name: str + network: str + + def as_dict(self) -> dict[str, str]: + """Return the wire form the entrypoint and the record read. + + Returns: + The provider's fields, keyed by the names in ``GARM_PROVIDERS_JSON``. + """ + return dataclasses.asdict(self) diff --git a/charms/garm/tests/unit/test_charm_state.py b/charms/garm/tests/unit/test_charm_state.py index 447b7b77..19b56b88 100644 --- a/charms/garm/tests/unit/test_charm_state.py +++ b/charms/garm/tests/unit/test_charm_state.py @@ -7,7 +7,15 @@ import pytest -from charm_state import CharmState, RunnerConfig, SSHDebugInfo, _get_ssh_debug_connections +from garm_entrypoint import PROVIDER_REQUIRED_FIELDS + +from charm_state import ( + CharmState, + ProviderConfig, + RunnerConfig, + SSHDebugInfo, + _get_ssh_debug_connections, +) _COMPLETE = { "host": "10.0.0.1", @@ -329,3 +337,27 @@ def test_runner_config_from_databag_preserves_multiline_pre_job_script(): config = RunnerConfig.from_databag({"pre_job_script": "echo one\necho two"}) assert config.pre_job_script == "echo one\necho two" + + +def test_provider_config_wire_form_matches_the_entrypoint_contract(): + """ + arrange: A provider config for one configurator unit. + act: Take its published form. + assert: It carries exactly the fields the workload entrypoint requires — the dataclass and + the entrypoint's tuple are the same contract expressed twice, so this catches drift in + either direction before a workload is started with a config it cannot read. + """ + config = ProviderConfig( + unit_name="garm-configurator-0", + auth_url="https://ks.example.com:5000/v3", + username="admin", + password="pass", + project_name="proj", + user_domain_name="Default", + project_domain_name="Default", + region_name="RegionOne", + network="net1", + ) + + assert set(config.as_dict()) == set(PROVIDER_REQUIRED_FIELDS) + assert config.as_dict()["unit_name"] == "garm-configurator-0" From 744fd434be7930b496bf4ee495bde0f38a269b93 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 16:17:54 +0000 Subject: [PATCH 10/13] fix(garm): reconcile providers past a scaleset nothing can attribute MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live scaleset that names no provider made the trim give up and keep every provider. That protects nothing: GARM resolves a scaleset's provider from the same field, so an unattributable row cannot become attributable because a provider stays published — it simply cannot be serviced either way. What it did do is disable trimming for the whole application: the record never shrank and a departed configurator's credentials stayed published for as long as the corrupt row existed. The unmapped scaleset is now reported as an orphan and skipped, and the trim carries on with the providers that can be accounted for — the ones the relations publish and the ones live scalesets name. The only remaining reason to keep everything is an unknown observation (the reconcile pass could not run). --- charms/garm/src/charm.py | 19 +++++++++++++------ charms/garm/tests/unit/test_charm.py | 26 +++++++++++++++++++------- 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index c204f0e5..0294020d 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -721,8 +721,9 @@ def _trimmed_providers( Returns: The set to record; ``published`` unchanged when the surviving scalesets are - unknown, or when one of them names no provider — "no scaleset references it" - cannot be concluded from "could not tell". + unknown, because "no scaleset references it" cannot be concluded from "could not + tell". A scaleset that names no provider is reported and skipped: it cannot be + attributed to a provider, so it neither keeps one nor blocks the rest. """ if scalesets is None: logger.warning( @@ -731,15 +732,21 @@ def _trimmed_providers( sorted(config.unit_name for config in published), ) return published - unmapped = [scaleset.name for scaleset in scalesets if not scaleset.provider_name] + # A scaleset with no provider name cannot be attributed to one, so it can neither + # justify retaining a provider nor be rescued by retaining them: GARM resolves a + # scaleset's provider from that same field. Report it as an orphan and reconcile the + # providers that can be accounted for, rather than letting one corrupt row disable + # trimming for the whole application. + unmapped = sorted( + str(scaleset.name) for scaleset in scalesets if not scaleset.provider_name + ) if unmapped: logger.warning( - "Scalesets %s name no provider, so the provider set cannot be mapped to the" - " scalesets GARM holds; keeping every provider this hook: %s", + "Live scalesets %s name no provider, so nothing attributes them to one;" + " leaving them to GARM and reconciling the remaining providers: %s", unmapped, sorted(config.unit_name for config in published), ) - return published related_names = {config.unit_name for config in related} referenced = {scaleset.provider_name for scaleset in scalesets if scaleset.provider_name} diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 2958de2e..98f9640c 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1008,17 +1008,23 @@ def test_provider_set_is_not_shrunk_when_the_scaleset_state_is_unknown( assert "keeping every provider this hook" in caplog.text -def test_provider_set_is_not_shrunk_when_a_scaleset_names_no_provider( +def test_unattributable_scaleset_is_reported_and_does_not_block_the_trim( ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture ): """ - arrange: A departing configurator, and a scaleset GARM holds that names no provider. + arrange: A departing configurator, a live scaleset that names no provider, and one that + names the surviving configurator's provider. act: Run update-status. - assert: Every published provider is kept: nothing maps that scaleset to a provider, so - the charm cannot tell which provider it still needs — and dropping one it cannot rule - out is the deadlock. + assert: The unmapped scaleset is reported as an orphan and the trim carries on: the + departed provider goes, the surviving one stays. Keeping every provider for a row + nothing can attribute to one would disable trimming for the whole application without + making that row any more deletable — GARM resolves a scaleset's provider from the same + field the charm cannot read. """ - garm_api.scaleset.return_value.reconcile.return_value = ([], [_held_scaleset(None)]) + garm_api.scaleset.return_value.reconcile.return_value = ( + [], + [_held_scaleset(None), _held_scaleset("configurator-b-0")], + ) with caplog.at_level(logging.INFO): out = ctx.run( @@ -1032,8 +1038,14 @@ def test_provider_set_is_not_shrunk_when_a_scaleset_names_no_provider( ), ) - assert _published_provider_names(out) == ["configurator-b-0", "garm-configurator-0"] + # The trim lands in the record on this hook; the published set follows on the next one. + assert _recorded_provider_names(out) == ["configurator-b-0"] assert "name no provider" in caplog.text + assert "Removing provider garm-configurator-0" in caplog.text + + later = ctx.run(ctx.on.update_status(), out) + + assert _published_provider_names(later) == ["configurator-b-0"] def test_recorded_provider_set_survives_a_replaced_pod(ctx: Context, garm_api: _GarmApiMocks): From 4f9a9ac17ae025e455a7f750bea2460888e141f0 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 16:32:51 +0000 Subject: [PATCH 11/13] refactor(garm): name the GARM pass for what it reconciles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _reconcile_runners reconciled far more than runners: the controller URLs GARM needs before it will answer any operational call, the garm-agent binaries, the GitHub App credentials, the org/repo entities, the runner template, and finally the scalesets that own the runners. _reconcile_garm_state names the object it brings into line — GARM's state, as described by CharmState — and matches the vocabulary the rest of the module already uses for this pass. --- charms/garm/src/charm.py | 10 ++++------ charms/garm/tests/unit/test_charm.py | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 0294020d..fa4229c4 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -429,11 +429,9 @@ def restart(self, rerun_migrations: bool = False) -> None: # credentials, entities and scalesets it is about to describe. Only a relation that # has published, or no configurator at all, is a complete desired state. if related_providers or not configurator_related: - referenced_providers = self._reconcile_runners() + live_scalesets = self._reconcile_garm_state() self._save_provider_record( - self._trimmed_providers( - published_providers, related_providers, referenced_providers - ) + self._trimmed_providers(published_providers, related_providers, live_scalesets) ) else: logger.info("GARM configurator provider data not yet available; blocking") @@ -1033,8 +1031,8 @@ def _add_unit_credential( description=MANAGED_CREDENTIAL_DESCRIPTION, ) - def _reconcile_runners(self) -> list[ScaleSet] | None: - """Sync GARM controller URLs, GitHub credentials, entities, then scalesets. + def _reconcile_garm_state(self) -> list[ScaleSet] | None: + """Make GARM match the charm's desired state: its own config, then its resources. The steps are ordered, not independent: GARM rejects every operational call with 409 ``urls_required`` until the controller URLs are set; org/repo entities reference a diff --git a/charms/garm/tests/unit/test_charm.py b/charms/garm/tests/unit/test_charm.py index 98f9640c..8f40fcba 100644 --- a/charms/garm/tests/unit/test_charm.py +++ b/charms/garm/tests/unit/test_charm.py @@ -1472,7 +1472,7 @@ def test_first_run_skipped_when_credentials_are_incomplete(ctx: Context, garm_ap """ secrets = _owned_secrets(admin_content={"username": "admin"}) - with patch.object(GarmCharm, "_reconcile_runners"): + with patch.object(GarmCharm, "_reconcile_garm_state"): ctx.run(ctx.on.update_status(), _state(secrets=secrets)) garm_api.client.assert_not_called() From 3d067a0fea1e612141058f22a7e9552b793e8270 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Thu, 22 Oct 2026 16:40:47 +0000 Subject: [PATCH 12/13] refactor(garm): leave the silent-relation case early MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit restart()'s tail read as two conditions in a row — one deciding whether to reconcile, one deciding what to report — with an else branch holding a single log line. Guarding the abnormal case and returning makes the happy path read top to bottom: publish, reconcile, record, report. The waiting status had to be reported from both exits, so it moves into `_report_waiting_for_configurator()`, which also keeps the message in one place instead of two inline statuses with the same text. --- charms/garm/src/charm.py | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index fa4229c4..71dbf6bc 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -420,25 +420,31 @@ def restart(self, rerun_migrations: bool = False) -> None: ) container.replan() self._maybe_first_run() + # A configurator that is related but has not published yet must not be reconciled: + # the chain iterates the desired state, and every reconciler deletes what it is not + # told to keep, so that silence would cost the credentials, entities and scalesets it + # is about to describe. + if not related_providers and configurator_related: + logger.info("GARM configurator provider data not yet available; blocking") + self._report_waiting_for_configurator() + return + # Reconcile before recording: the pass is what establishes which providers are still # referenced, and it must not be buried by the framework reporting active afterward. - # - # The chain iterates the desired state, so an empty one reconciles to zero. It must - # not, however, read silence as "nothing is desired": every reconciler deletes what - # it is not told to keep, so a configurator that has not published yet would lose the - # credentials, entities and scalesets it is about to describe. Only a relation that - # has published, or no configurator at all, is a complete desired state. - if related_providers or not configurator_related: - live_scalesets = self._reconcile_garm_state() - self._save_provider_record( - self._trimmed_providers(published_providers, related_providers, live_scalesets) - ) - else: - logger.info("GARM configurator provider data not yet available; blocking") + # An empty desired state is a real one — with no configurator left the workload runs + # with no providers, and the pass reconciles that to zero. + live_scalesets = self._reconcile_garm_state() + self._save_provider_record( + self._trimmed_providers(published_providers, related_providers, live_scalesets) + ) if not related_providers: - self.update_app_and_unit_status( - ops.WaitingStatus("Waiting for garm-configurator relation") - ) + self._report_waiting_for_configurator() + + def _report_waiting_for_configurator(self) -> None: + """Report that the unit is waiting for a configurator that publishes providers.""" + self.update_app_and_unit_status( + ops.WaitingStatus("Waiting for garm-configurator relation") + ) def _entrypoint_environment( self, From bbd6a39f103a8e3321af2e5b88baeae4df1ca3e7 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Fri, 23 Oct 2026 02:21:27 +0000 Subject: [PATCH 13/13] ci: trigger ci