diff --git a/charms/garm/AGENTS.md b/charms/garm/AGENTS.md index 59cf2836..ff9b681f 100644 --- a/charms/garm/AGENTS.md +++ b/charms/garm/AGENTS.md @@ -30,6 +30,28 @@ 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. 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. 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 a2c0ed2e..71dbf6bc 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -23,12 +23,15 @@ DEBUG_SSH_INTEGRATION_NAME, GARM_CONFIGURATOR_RELATION_NAME, CharmState, + ProviderConfig, RunnerConfig, credential_name, resolve_entity, ) 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 from github_reconciler import ( @@ -46,6 +49,17 @@ 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 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" + + +class ProviderRecordError(Exception): + """The provider record could not be read or written, so the provider set is unknown.""" + + CONTAINER_NAME: typing.Final[str] = "app" PEBBLE_SERVICE_NAME: typing.Final[str] = "app" GARM_BINARY: typing.Final[str] = "/usr/local/bin/garm" @@ -117,6 +131,44 @@ def _generate_admin_password() -> str: return "".join(chars) +def _parse_provider_record(published: str, source: str) -> list[ProviderConfig]: + """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") + 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") + 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)}" + ) + 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]: """Parse pre_install_scripts from JSON relation data string.""" if not raw: @@ -303,6 +355,12 @@ 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 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. """ @@ -326,19 +384,18 @@ 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: - if not CharmState.from_charm(self).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 + related_providers = self._get_configurator_provider_configs() + recorded_providers = self._read_provider_record() + configurator_related = CharmState.from_charm(self).configurator_related + # 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. 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) 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,20 +415,42 @@ 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() + # 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. + # 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._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, 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 = { @@ -383,7 +462,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, } @@ -506,7 +587,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 @@ -520,9 +601,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, []), @@ -532,7 +611,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: @@ -564,17 +643,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( @@ -584,6 +663,152 @@ def _get_configurator_provider_configs( ) return configs + def _published_providers( + self, related: list[ProviderConfig], recorded: dict[str, ProviderConfig] + ) -> list[ProviderConfig]: + """Return the provider set to publish: the record with the relations on top. + + 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. Relation data wins, so a + rotated password reaches GARM. + """ + 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, ProviderConfig]: + """Return the recorded provider set, by provider name. + + Returns: + The recorded configs, or an empty mapping when nothing has been recorded yet. + + Raises: + 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) + # 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 {} + 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[ProviderConfig], + related: list[ProviderConfig], + scalesets: list[ScaleSet] | None, + ) -> list[ProviderConfig]: + """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. + 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 the surviving scalesets are + 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( + "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 + # 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( + "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), + ) + + 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 + ] + 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) + 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 sorted(kept, key=lambda config: config.unit_name) + + 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. + + 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( + [config.as_dict() for config in 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", + ) + return + if secret.peek_content().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,8 +1037,8 @@ def _add_unit_credential( description=MANAGED_CREDENTIAL_DESCRIPTION, ) - def _reconcile_runners(self) -> 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 @@ -821,11 +1046,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 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: 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 +1079,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, surviving_scalesets = ScalesetReconciler(auth_client).reconcile( self._build_desired_scalesets(template_id) ) # A label change recreates the scaleset and drains the old one, which @@ -863,12 +1094,15 @@ def _reconcile_runners(self) -> None: ) else: self.update_app_and_unit_status(ops.ActiveStatus()) + 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))) + 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/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/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 2ad9399b..65ce71af 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], list[ScaleSet]]: """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 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 @@ -217,12 +223,18 @@ def reconcile(self, desired: list[ScalesetSpec]) -> list[ScalesetProgress]: 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): @@ -231,12 +243,40 @@ 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._live_scalesets(observed, deleted, created) + + @staticmethod + def _live_scalesets( + observed: dict[str, ScaleSet], deleted: set[str], created: dict[str, ScaleSet] + ) -> list[ScaleSet]: + """Return every scaleset GARM holds after this pass. + + 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 live scalesets, ordered by name so a caller's decisions are stable. + """ + 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]: @@ -324,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. @@ -333,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. @@ -370,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 @@ -1065,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_charm.py b/charms/garm/tests/unit/test_charm.py index e1a8f4df..8f40fcba 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 @@ -32,7 +33,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 +158,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 = ([], []) calls = MagicMock() calls.attach_mock(auth_client.update_controller, "controller") calls.attach_mock(agent_tools, "agent_tools") @@ -196,6 +206,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 +260,75 @@ 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 _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) + 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 +339,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 +377,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, } @@ -577,7 +658,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)) @@ -585,7 +667,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") @@ -694,6 +777,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 = ([], []) + + 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 ): @@ -730,6 +880,386 @@ 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 = ( + [], + [_held_scaleset("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 = ( + [], + [_held_scaleset("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 = ([], []) + 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_unattributable_scaleset_is_reported_and_does_not_block_the_trim( + ctx: Context, garm_api: _GarmApiMocks, caplog: pytest.LogCaptureFixture +): + """ + 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: 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), _held_scaleset("configurator-b-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"), + ], + ), + ) + + # 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): + """ + 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 = ( + [], + [_held_scaleset("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_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 = ( + [], + [_held_scaleset("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 = ([], []) + + 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 +): + """ + 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 = ( + [], + [_held_scaleset("garm-configurator-0"), _held_scaleset("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 +1320,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 +1334,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 +1360,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 +1386,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()) @@ -933,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() 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" diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 99912ba5..5a11a511 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 @@ -156,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 []) ] @@ -185,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 @@ -280,7 +309,13 @@ def _spec( def _reconcile(client, desired): - return ScalesetReconciler(client).reconcile(desired) + progress, _ = ScalesetReconciler(client).reconcile(desired) + return progress + + +def _reconcile_live(client, desired): + """Reconcile and return every scaleset GARM holds afterwards.""" + return ScalesetReconciler(client).reconcile(desired)[1] @pytest.mark.parametrize( @@ -512,6 +547,79 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] +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: 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"], + scalesets=[ + _existing_scaleset(name="stale-scaleset", id=42, provider_name="retired-provider") + ], + ) + + live = _reconcile_live(client, [_spec(name="new-scaleset")]) + + assert client.deleted == [42] + assert [scaleset.name for scaleset in live] == [target_scaleset_name("new-scaleset", [])] + assert live[0].provider_name == "openstack-demo" + + +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. + 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): + 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") + ], + ) + + live = _reconcile_live(client, [_spec(name="new-scaleset")]) + + # 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_live_scalesets_are_reported_even_with_no_provider_name(): + """ + arrange: A scaleset GARM holds that names no provider at all. + act: Reconcile. + 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. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(provider_name=None)], + ) + + live = _reconcile_live(client, [_spec()]) + + 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(): """ arrange: An orphaned scaleset with a custom template, whose deletion GARM rejects. diff --git a/docs/changelog.md b/docs/changelog.md index 56f84cc5..27712c49 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 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