diff --git a/charms/garm-configurator/tests/unit/test_charm.py b/charms/garm-configurator/tests/unit/test_charm.py index 9b253dd2..cdb518d8 100644 --- a/charms/garm-configurator/tests/unit/test_charm.py +++ b/charms/garm-configurator/tests/unit/test_charm.py @@ -658,6 +658,7 @@ def test_reconcile_writes_optional_scaleset_fields_to_garm_relation(): ) assert "repo" not in garm_out.local_unit_data + @pytest.mark.parametrize( "config_key, bad_value, expected_fragment", [ diff --git a/charms/garm-configurator/tox.toml b/charms/garm-configurator/tox.toml index 869a0eee..bea7ce20 100644 --- a/charms/garm-configurator/tox.toml +++ b/charms/garm-configurator/tox.toml @@ -40,7 +40,7 @@ commands = [ [env.unit] description = "Run unit tests" -deps = ["pytest", "coverage[toml]", "ops-scenario==8.8.1", "-r requirements.txt"] +deps = ["pytest", "coverage[toml]", "ops-scenario==8.8.2", "-r requirements.txt"] commands = [ [ "coverage", diff --git a/charms/garm/src/charm.py b/charms/garm/src/charm.py index 8edcac2a..bdcef044 100755 --- a/charms/garm/src/charm.py +++ b/charms/garm/src/charm.py @@ -37,7 +37,7 @@ GithubReconciler, ) from resource_cleanup import GarmCleanupError, GarmResourceCleanup -from scaleset_reconciler import ScalesetReconciler, ScalesetSpec +from scaleset_reconciler import Handover, ScalesetProgress, ScalesetReconciler, ScalesetSpec logger = logging.getLogger(__name__) @@ -52,6 +52,14 @@ GARM_LOCAL_API_BASE_URL: typing.Final[str] = f"http://127.0.0.1:{GARM_PORT}/api/v1" GARM_LISTEN_ADDRESS: typing.Final[str] = "0.0.0.0" _DB_PASSPHRASE_LENGTH: typing.Final[int] = 32 +# Juju truncates long statuses; name a couple of scalesets and count the rest. +_MAX_DRAINING_IN_STATUS: typing.Final[int] = 2 +# The phases a scaleset replacement passes through, in order. Named here rather than +# spelled inline so the status vocabulary is greppable; not an enum, because the +# draining phase carries the runner count and so is formatted, not selected. +_PHASE_CREATING: typing.Final[str] = "creating replacement" +_PHASE_RETIRING: typing.Final[str] = "retiring predecessor" +_PHASE_AWAITING_DELETION: typing.Final[str] = "awaiting deletion" GARM_CONFIG_VERSION: typing.Final[str] = "1" @@ -792,8 +800,21 @@ def _reconcile_runners(self) -> None: GithubReconciler(auth_client).reconcile(self._build_desired_credentials()) EntityReconciler(auth_client).reconcile(charm_state.desired_entities) template_id = _apply_garm_template(auth_client, charm_state.ssh_debug_connections) - ScalesetReconciler(auth_client).reconcile(self._build_desired_scalesets(template_id)) - self.update_app_and_unit_status(ops.ActiveStatus()) + replacing = ScalesetReconciler(auth_client).reconcile( + self._build_desired_scalesets(template_id) + ) + # A label change recreates the scaleset and drains the old one, which + # outlives this hook: report progress and let update-status converge it. + # Active, not maintenance: every label is served throughout the drain, by + # the replacement or the predecessor, so nothing is degraded. A drain can + # run to DRAIN_DEADLINE, and blocking `juju wait-for` on hours of healthy + # background convergence would be wrong. + if replacing: + self.update_app_and_unit_status( + ops.ActiveStatus(_scaleset_replacement_status(replacing)) + ) + else: + self.update_app_and_unit_status(ops.ActiveStatus()) except CharmedTemplateError as exc: logger.warning("GARM charmed template error during reconcile: %s", exc) self.update_app_and_unit_status(ops.WaitingStatus(str(exc))) @@ -819,5 +840,59 @@ def _ensure_controller_urls(self, auth_client: GarmAuthenticatedClient) -> None: ) +def _scaleset_replacement_status(replacing: list[ScalesetProgress]) -> str: + """Summarise in-progress scaleset replacements for the unit status. + + Args: + replacing: Scaleset replacements still in flight, one entry per generation + being replaced. + + Returns: + A status message naming at most two scalesets, so it stays readable when + several are replaced at once. Each is named by the logical name the operator + configured — the live names carry a label hash the operator never chose — and + the live names are logged in full by the reconciler. A scaleset draining two + generations at once is named once, so it cannot push the others out of the + message with a repeated line. + """ + by_scaleset: dict[str, list[ScalesetProgress]] = {} + for progress in replacing: + by_scaleset.setdefault(progress.logical_name, []).append(progress) + shown = [ + f"{logical_name} -> {entries[0].replacement_name} ({_scaleset_replacement_phase(entries)})" + for logical_name, entries in list(by_scaleset.items())[:_MAX_DRAINING_IN_STATUS] + ] + remainder = len(by_scaleset) - len(shown) + if remainder > 0: + shown.append(f"+{remainder} more") + return f"Replacing scaleset {', '.join(shown)}" + + +def _scaleset_replacement_phase(entries: list[ScalesetProgress]) -> str: + """Describe where one scaleset's replacement has got to. + + Args: + entries: Every generation of one scaleset still being replaced. A label change + during an earlier drain leaves several predecessors handing over to the same + replacement; to the operator that is one changeover, so they collapse into + one phase — the least advanced, with the runner counts summed. + + Returns: + The phase, in the operator's terms. The runner count is only reported once + the labels have been handed over, since before that nothing is draining yet; + with the count at zero the scaleset has drained and GARM has yet to accept + its deletion, which is a distinct thing to be waiting on. + """ + handovers = {entry.handover for entry in entries} + if Handover.PENDING in handovers: + return _PHASE_CREATING + if Handover.FAILED in handovers: + return _PHASE_RETIRING + runners = sum(entry.remaining_runners for entry in entries) + if not runners: + return _PHASE_AWAITING_DELETION + return f"draining {runners} runner{'' if runners == 1 else 's'}" + + if __name__ == "__main__": ops.main(GarmCharm) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 73b4f96d..84450ac9 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -5,7 +5,10 @@ """Scaleset reconciler: diffs desired vs observed GARM scalesets and applies changes.""" import base64 +import enum +import hashlib import logging +import re from dataclasses import dataclass, field from datetime import datetime, timedelta, timezone @@ -13,6 +16,7 @@ from garm_api import ( GarmApiError, GarmAuthenticatedClient, + GarmConnectionError, GarmNotFoundError, GarmUnauthorizedError, ) @@ -70,6 +74,47 @@ # legitimate job, which costs more than leaving a scaleset around for longer. MAX_JOB_RUNTIME = timedelta(days=5) +LABEL_HASH_LENGTH = 8 + +# A conservative bound on the generated name: the operator-supplied part is truncated to +# fit, never the hash. GARM validates only that the name is non-empty, and the tighter +# ceiling that used to apply is gone — GARM derived the OpenStack `garm-pool-id` tag from +# the scale set name, so anything over 10 characters overran Nova's 60-character tag limit +# until the pinned GARM bump replaced it with a fixed-length UUID. What remains is +# GitHub's own limit on the System label GARM registers the scale set under. +MAX_SCALESET_NAME_LENGTH = 64 + +# Past GitHub's 6h job cap a remaining runner is stuck, not busy: stop gating on the +# count and retry the delete, which GARM rejects while runners are active. +DRAIN_DEADLINE = timedelta(hours=7) + + +class Handover(enum.Enum): + """How far the routing hand-over from a replaced generation has got. + + Attributes: + PENDING: The replacement is not up yet, so the predecessor must stay enabled. + FAILED: GARM refused to disable the predecessor; the next reconcile retries. + DONE: The predecessor is disabled and the replacement holds their shared labels. + """ + + # Both unfinished states leave the predecessor enabled and serving jobs, so nothing + # is draining yet and the runner count reported alongside them is 0. + PENDING = "pending" + FAILED = "failed" + DONE = "done" + + +@dataclass(frozen=True) +class ScalesetProgress: + """A scaleset replacement still in flight: the generation it replaced is not gone yet.""" + + logical_name: str + retiring_name: str + replacement_name: str + remaining_runners: int + handover: Handover = Handover.DONE + @dataclass class ScalesetSpec: @@ -103,7 +148,7 @@ def __init__(self, client: GarmAuthenticatedClient) -> None: """ self._client = client - def reconcile(self, desired: list[ScalesetSpec]) -> None: + def reconcile(self, desired: list[ScalesetSpec]) -> list[ScalesetProgress]: """Sync GARM scalesets to match *desired*. Performs the minimum set of CREATE / UPDATE / DELETE operations, and @@ -112,9 +157,26 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: (org/repo) is not registered in GARM, that spec is skipped silently (deferred creation) — no error state is set. + A label change is applied by creating a replacement scaleset and draining + the old one (see ``_retire_replaced``), which spans several reconciles. + + A spec that fails against the GARM API does not abort the pass: the + remaining specs and the orphan sweep still run, and the first failure is + re-raised at the end so the charm still reports the sync as failed. A + connection error is not contained that way — GARM is down, so retrying + every remaining spec would only stall the hook. + Args: desired: The full desired set of scalesets. + + Returns: + One entry per replacement still in flight; empty once converged. + + Raises: + GarmApiError: If any spec failed to reconcile, re-raised once the rest + of the pass has completed. """ + desired = self._dedupe(desired) providers = {provider.name for provider in self._client.list_providers()} observed: dict[str, ScaleSet] = {} for scaleset in self._client.list_scalesets(): @@ -124,15 +186,92 @@ def reconcile(self, desired: list[ScalesetSpec]) -> None: observed[scaleset.name] = scaleset templates = self._load_templates(desired, observed) - all_desired_names: set[str] = {spec.name for spec in desired} - + families = self._resolve_families(desired, observed) + # Covers in-flight replacements and draining predecessors, so the orphan pass + # can't delete one mid-changeover. Built before the provider/entity gates. + claimed: set[str] = set() + for family in families.values(): + claimed.update(family) + + progress: list[ScalesetProgress] = [] + failure: GarmApiError | None = None for spec in desired: - self._reconcile_one(spec, providers, observed, templates) + try: + progress.extend( + self._reconcile_one( + spec, providers, observed, templates, families.get(spec.name, []) + ) + ) + except GarmConnectionError: + raise + except GarmApiError as exc: + logger.warning("Failed to reconcile scaleset %s: %s", spec.name, exc) + failure = failure or exc for name, scaleset in observed.items(): - if name not in all_desired_names: - self._delete_orphaned(scaleset) + if name not in claimed and self._delete_orphaned(scaleset): self._delete_custom_template(name, templates) + if failure is not None: + raise failure + return progress + + @staticmethod + def _dedupe(desired: list[ScalesetSpec]) -> list[ScalesetSpec]: + """Drop specs repeating a logical name already claimed by an earlier one. + + Args: + desired: The full desired set of scalesets. + + Returns: + The specs with duplicate names removed, first occurrence winning. Two + specs sharing a name would each own the other's live scaleset and retire + it on every reconcile, so they would replace each other forever. + """ + unique: dict[str, ScalesetSpec] = {} + for spec in desired: + existing = unique.get(spec.name) + if existing is not None: + # Only a conflicting duplicate is worth a warning: the scaleset config is + # app-level, so every configurator unit's databag yields the same spec and + # a multi-unit configurator would otherwise warn on every hook. + if existing != spec: + logger.warning( + "Ignoring duplicate desired scaleset %s: a logical name must" + " identify exactly one scaleset", + spec.name, + ) + continue + unique[spec.name] = spec + return list(unique.values()) + + @staticmethod + def _resolve_families( + desired: list[ScalesetSpec], observed: dict[str, ScaleSet] + ) -> dict[str, list[str]]: + """Group the live scalesets by the desired spec that owns them. + + Args: + desired: The full desired set of scalesets. + observed: Observed scalesets keyed by name. + + Returns: + Per logical name, every live generation it owns plus its target name + (which may not exist yet). A live name that is another spec's own or + target name is never claimed, so ``foo`` can't swallow a separate + scaleset that happens to be named ``foo-1a2b3c4d``. + """ + targets = {spec.name: target_scaleset_name(spec.name, spec.labels) for spec in desired} + reserved = set(targets) | set(targets.values()) + families: dict[str, list[str]] = {} + for spec in desired: + others = reserved - {spec.name, targets[spec.name]} + family = [ + name + for name in observed + if name not in others and _is_family_member(name, spec.name) + ] + families[spec.name] = sorted({*family, targets[spec.name]}) + return families def _load_templates( self, desired: list[ScalesetSpec], observed: dict[str, ScaleSet] @@ -166,7 +305,8 @@ def _reconcile_one( providers: set[str | None], observed: dict[str, ScaleSet], templates: dict[str, Template], - ) -> None: + family: list[str], + ) -> list[ScalesetProgress]: """Reconcile a single desired scaleset: validate, create or update, and sync its template. Args: @@ -174,12 +314,20 @@ def _reconcile_one( providers: Names of providers currently registered in GARM. observed: Observed scalesets keyed by name. templates: Observed templates keyed by name. + family: Every live generation of this spec, plus its target name. + + Returns: + One entry per replaced generation of this spec still draining. """ + active_name = _resolve_active_name(spec, observed) try: - create_params = self._to_create_params(spec) + create_params = self._to_create_params(spec, active_name) except Exception as exc: - logger.warning("Skipping scaleset %s: spec validation failed: %s", spec.name, exc) - return + # Error, not warning: unlike the provider/entity gates below this does not + # resolve on its own — the spec is malformed and will fail every pass until + # the operator changes it. + logger.error("Skipping scaleset %s: spec validation failed: %s", spec.name, exc) + return [] if spec.provider_name not in providers: logger.warning( @@ -187,7 +335,7 @@ def _reconcile_one( spec.name, spec.provider_name, ) - return + return [] entity_id = self._resolve_entity_id(spec) if entity_id is None: @@ -197,34 +345,263 @@ def _reconcile_one( spec.entity_type, spec.entity_name, ) - return + return [] - template_id = self._ensure_template(spec, templates) + template_id = self._ensure_template(spec, active_name, templates) - if spec.name in observed: - self._maybe_update(observed[spec.name], spec, template_id) + if active_name in observed: + self._maybe_update(observed[active_name], spec, template_id) else: - self._create(spec, entity_id, create_params, template_id) + 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 # unavailable): the scaleset has been reverted to the default # template above, so drop any now-unreferenced custom template. - self._delete_custom_template(spec.name, templates) + self._delete_custom_template(active_name, templates) + + return self._retire_replaced(spec, active_name, observed, templates, family) + + def _retire_replaced( + self, + spec: ScalesetSpec, + active_name: str, + observed: dict[str, ScaleSet], + templates: dict[str, Template], + family: list[str], + ) -> list[ScalesetProgress]: + """Hand routing over to the current generation and drain the ones it replaced. + + Workloads route by label, not by scaleset name, and GitHub only assigns jobs to + a scaleset holding a live listener session. So the replacement is brought up + first and only then is the old one disabled, which closes its session and leaves + the replacement as the sole holder of the labels they share. Any label carried by + both is served throughout — by one scaleset or the other, both fully functional — + so the changeover has no queue gap, and runners already mid-job are left to + finish rather than being killed. + + Args: + spec: The desired scaleset. + active_name: The generation that should serve *spec*. + observed: Observed scalesets keyed by name. + templates: Observed templates keyed by name. + family: Every live generation of this spec, plus its target name. + + Returns: + One entry per replaced generation not yet gone. + """ + # Close the old session only once the replacement exists: disabling any + # earlier leaves the labels unserved if the charm dies in between. + replacement_live = active_name in observed and observed[active_name].enabled is True + + progress: list[ScalesetProgress] = [] + for name in family: + old = observed.get(name) + if name == active_name or old is None: + continue + if not replacement_live: + progress.append( + ScalesetProgress(spec.name, name, active_name, 0, Handover.PENDING) + ) + continue + entry = self._advance_retirement(spec, active_name, name, old, templates) + if entry is not None: + progress.append(entry) + return progress + + def _advance_retirement( + self, + spec: ScalesetSpec, + active_name: str, + name: str, + old: ScaleSet, + templates: dict[str, Template], + ) -> ScalesetProgress | None: + """Move one replaced generation a step closer to being gone. + + Args: + spec: The desired scaleset. + active_name: The generation that should serve *spec*. + name: Name of the replaced generation. + old: The replaced generation, as GARM reports it. + templates: Observed templates keyed by name. + + Returns: + What to report for this generation, or None once it is gone and there is + nothing left to report. + + Raises: + GarmConnectionError: If GARM is unreachable — see ``_remaining_runners``. + """ + # Truthy, not `is not False`: GARM tags Enabled `omitempty`, so a disabled + # scaleset comes back with no `enabled` key at all and the client reads None. + if old.enabled: + if not self._retire(old): + # It is still enabled, so its session is still open and nothing is + # draining: report the stalled hand-over rather than a runner count, + # which would read as a changeover that is quietly making progress. + return ScalesetProgress(spec.name, name, active_name, 0, Handover.FAILED) + return ScalesetProgress(spec.name, name, active_name, self._remaining_runners(old)) + remaining = self._remaining_runners(old) + if remaining and not _drain_deadline_passed(old): + return ScalesetProgress(spec.name, name, active_name, remaining) + if remaining: + logger.warning( + "Scaleset %s still reports %d runner(s) after %s of draining;" + " attempting deletion anyway. GARM rejects the delete while runners" + " are genuinely active, so no in-flight job is cut short.", + name, + remaining, + DRAIN_DEADLINE, + ) + if not self._delete_drained(old, templates): + # Keep reporting it: a scaleset GARM refused to delete is still on + # GitHub, so the replacement has not actually converged yet. + return ScalesetProgress(spec.name, name, active_name, remaining) + return None + + def _retire(self, scaleset: ScaleSet) -> bool: + """Disable a replaced scaleset and stop it launching runners. + + Disabling closes its listener session, handing the shared labels to the + replacement. GARM's handleScaleDown then reaps idle runners but skips + RunnerActive, so a runner mid-job finishes instead of being killed. The + second call is defensive: GARM rejects max_runners=0 on create only. + + Args: + scaleset: The replaced scaleset. + + Returns: + Whether the scaleset is now disabled. False leaves both generations + enabled and their min_idle_runners doubled, so the caller must report the + changeover as stalled rather than as a drain in progress. + + Raises: + GarmConnectionError: If GARM is unreachable — see ``_remaining_runners``. + """ + if scaleset.id is None: + logger.warning("Scaleset %s has no id; cannot retire it", scaleset.name) + return False + logger.info("Retiring replaced scaleset %s (id=%s)", scaleset.name, scaleset.id) + try: + self._client.update_scaleset( + scaleset.id, + UpdateScaleSetParams(enabled=False, min_idle_runners=0, max_runners=0), + ) + return True + except GarmConnectionError: + raise + except GarmApiError as exc: + logger.warning( + "Could not zero runner counts on scaleset %s; disabling only: %s", + scaleset.name, + exc, + ) + try: + self._client.update_scaleset( + scaleset.id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) + ) + except GarmConnectionError: + raise + except GarmApiError as exc: + logger.warning( + "Could not disable scaleset %s (will retry on next reconcile): %s", + scaleset.name, + exc, + ) + return False + return True + + def _remaining_runners(self, scaleset: ScaleSet) -> int: + """Return how many runners a scaleset still has. + + Args: + scaleset: The scaleset to inspect. + + Returns: + The instance count, or 1 when it cannot be read — an unknown count must + never be mistaken for a drained scaleset and delete live runners. + + Raises: + GarmConnectionError: If GARM is unreachable. Contained failures are + per-scaleset; an outage is not, and must reach the charm's status + rather than be reported as drain progress. + """ + if scaleset.id is None: + return 0 + try: + return len(self._client.list_scale_set_instances(scaleset.id)) + except GarmConnectionError: + raise + except GarmApiError as exc: + logger.warning( + "Could not count runners of scaleset %s; assuming still draining: %s", + scaleset.name, + exc, + ) + return 1 + + def _delete_drained(self, scaleset: ScaleSet, templates: dict[str, Template]) -> bool: + """Delete a replaced scaleset that has finished draining, and its template. + + Args: + scaleset: The drained scaleset. + templates: Observed templates keyed by name. + + Returns: + Whether the caller can stop tracking it. False means GARM still holds the + scaleset, so the replacement has not converged yet. An id-less scaleset is + untouchable rather than gone, but reporting it forever would wedge the + status, so it reads as done — matching ``_remaining_runners``. + + Raises: + GarmConnectionError: If GARM is unreachable — see ``_remaining_runners``. + """ + if scaleset.id is None: + return True + logger.info("Deleting drained scaleset %s (id=%s)", scaleset.name, scaleset.id) + try: + self._client.delete_scaleset(scaleset.id) + except GarmConnectionError: + raise + except GarmApiError as exc: + logger.warning( + "Could not delete drained scaleset %s (will retry on next reconcile): %s", + scaleset.name, + exc, + ) + return False + self._delete_custom_template(scaleset.name or "", templates) + return True + + def _delete_orphaned(self, scaleset: ScaleSet) -> bool: + """Disable, drain, then delete a scaleset that is no longer in the desired set. + + Args: + scaleset: The orphaned scaleset. - def _delete_orphaned(self, scaleset: ScaleSet) -> None: - """Disable, drain, then delete a scaleset that is no longer in the desired set.""" + Returns: + Whether GARM no longer holds it, and so whether its runner template can go + too. A template is only ever deleted after the scaleset referencing it, so + a scaleset awaiting a retry keeps the runner config that retry relies on. + + Raises: + GarmConnectionError: If GARM is unreachable — an outage is not a per-scaleset + failure and must reach the charm's status rather than read as a sweep + that found nothing left to do. + """ name = scaleset.name or "" logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset.id) if scaleset.id is None: - return + logger.warning("Scaleset %s has no id; skipping delete", name) + return False if not self._disable(scaleset.id, name): # GARM rejects the delete of a scaleset that is still enabled, so nothing # below can succeed this pass. Removing the runners of a scaleset that is # still enabled and sized up would only have GARM launch replacements — # churning instances on every pass instead of draining — so both the drain # and the delete wait for the next reconcile. - return + return False # GARM rejects the delete while the scaleset still owns runners (and, above, # while it is enabled), so the runners have to go first; anything left behind # is retried on the next pass. @@ -234,9 +611,11 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: # delete issued just above and still in flight, a runner left mid-job, or # one in a status GARM will not delete — makes this pass's delete a # certain 400, and it waits for the next reconcile instead. - return + return False try: self._client.delete_scaleset(scaleset.id) + except GarmConnectionError: + raise except GarmApiError as exc: # The scaleset was empty when its runners were listed, so this is either a # runner registered in the window since (the scaleset is disabled, so only @@ -246,6 +625,8 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: name, exc, ) + return False + return True def _disable(self, scaleset_id: int, name: str) -> bool: """Stop a scaleset launching runners, before its existing ones are removed. @@ -256,6 +637,9 @@ def _disable(self, scaleset_id: int, name: str) -> bool: Returns: Whether the scaleset is now disabled. + + Raises: + GarmConnectionError: If GARM is unreachable — see ``_delete_orphaned``. """ try: # Neither field is propagated to GitHub: GARM only calls GitHub from this @@ -265,6 +649,8 @@ def _disable(self, scaleset_id: int, name: str) -> bool: scaleset_id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) ) return True + except GarmConnectionError: + raise except GarmApiError as exc: logger.warning( "Could not disable scaleset %s; deferring its delete, which GARM rejects while" @@ -283,9 +669,14 @@ def _remove_runners(self, scaleset_id: int, name: str) -> bool: Returns: Whether the scaleset owns no runners, so GARM will accept its delete. + + Raises: + GarmConnectionError: If GARM is unreachable — see ``_delete_orphaned``. """ try: instances = self._client.list_scale_set_instances(scaleset_id) + except GarmConnectionError: + raise except GarmApiError as exc: logger.warning( "Could not list runners of scaleset %s (will retry on next reconcile): %s", @@ -422,7 +813,9 @@ def _resolve_entity_id(self, spec: ScalesetSpec) -> str | None: logger.warning("Unknown entity_type %r for scaleset %s", spec.entity_type, spec.name) return None - def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) -> int: + def _ensure_template( + self, spec: ScalesetSpec, scaleset_name: str, templates: dict[str, Template] + ) -> int: """Ensure the scaleset's runner template reflects its runner options. Copies the system ``github_linux`` template, injects the runner options, @@ -432,6 +825,9 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) - Args: spec: The desired scaleset. + scaleset_name: The live name of the scaleset the template belongs to. Each + generation owns its own template, so a draining predecessor keeps the + template its runners were built from until it is deleted. templates: Observed templates keyed by name. Returns: @@ -440,7 +836,7 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) - is unavailable and no custom template already exists). Returning ``0`` for a scaleset that previously had a custom template detaches it. """ - custom_name = f"{SYSTEM_TEMPLATE_NAME}-{spec.name}" + custom_name = f"{SYSTEM_TEMPLATE_NAME}-{scaleset_name}" existing = templates.get(custom_name) if not spec.runner_config.has_config(): @@ -458,13 +854,13 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) - logger.warning( "System template %s not found; keeping existing custom template for %s", SYSTEM_TEMPLATE_NAME, - spec.name, + scaleset_name, ) return existing.id or 0 logger.warning( "System template %s not found; scaleset %s will use the default template", SYSTEM_TEMPLATE_NAME, - spec.name, + scaleset_name, ) return 0 @@ -474,7 +870,7 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) - logger.warning( "Runner template %s has no id; scaleset %s will use the default template", custom_name, - spec.name, + scaleset_name, ) return 0 if self._template_bytes(existing) != new_data: @@ -486,7 +882,7 @@ def _ensure_template(self, spec: ScalesetSpec, templates: dict[str, Template]) - created = self._client.create_template( name=custom_name, data=new_data, - description=f"Runner template for scaleset {spec.name}", + description=f"Runner template for scaleset {scaleset_name}", ) return created.id or 0 @@ -555,11 +951,12 @@ def _delete_custom_template(self, scaleset_name: str, templates: dict[str, Templ ) @staticmethod - def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: + def _to_create_params(spec: ScalesetSpec, scaleset_name: str) -> CreateScaleSetParams: """Build and validate CreateScaleSetParams from a ScalesetSpec. Args: spec: The desired scaleset specification. + scaleset_name: The live name to create the scaleset under. Returns: Validated CreateScaleSetParams ready for the GARM API. @@ -569,7 +966,7 @@ def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: """ return CreateScaleSetParams.model_validate( { - "name": spec.name, + "name": scaleset_name, "provider_name": spec.provider_name, "image": spec.image, "flavor": spec.flavor, @@ -588,29 +985,27 @@ def _to_create_params(spec: ScalesetSpec) -> CreateScaleSetParams: def _create( self, spec: ScalesetSpec, + scaleset_name: str, entity_id: str, params: CreateScaleSetParams, template_id: int, ) -> None: if template_id: params.template_id = template_id - logger.info("Creating scaleset %s under %s %s", spec.name, spec.entity_type, entity_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) def _maybe_update(self, observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> None: - observed_labels = sorted(t.name for t in (observed.tags or []) if t.name) + observed_labels = _observed_labels(observed) if observed_labels != sorted(spec.labels): - # UpdateScaleSetParams has no labels field; label changes require - # recreating the scaleset. To delete a scaleset, remove the - # garm-configurator relation for the corresponding unit. logger.warning( - "Scaleset %s labels changed (%s -> %s) but cannot be updated in place;" - " to apply label changes, remove and re-add the garm-configurator relation" - " for this unit", - spec.name, + "Scaleset %s carries labels %s but %s were expected; updating its other" + " fields anyway. Labels are immutable in GitHub — delete this scaleset" + " in GARM to let the charm recreate it with the expected labels.", + observed.name, observed_labels, sorted(spec.labels), ) @@ -620,12 +1015,12 @@ def _maybe_update(self, observed: ScaleSet, spec: ScalesetSpec, template_id: int # _needs_update already covers the template id (its last clause), so an # id change alone forces an update here. if not self._needs_update(observed, spec, template_id): - logger.debug("Scaleset %s is up to date", spec.name) + logger.debug("Scaleset %s is up to date", observed.name) return # UpdateScaleSetParams omits None fields (exclude_none), so None can only # leave extra_specs untouched, never clear them. Send an explicit empty - # dict when the desired specs are empty but the scaleset still carries + # dict when the desired extra specs are empty but the scaleset still carries # some (e.g. a proxy was unset) — otherwise a stale aproxy script would # persist and _needs_update would loop forever trying to converge. desired_extra = _effective_extra_specs(spec) @@ -645,9 +1040,9 @@ def _maybe_update(self, observed: ScaleSet, spec: ScalesetSpec, template_id: int # unrelated update never spuriously sets the field. if template_id or observed_template_id: params.template_id = template_id - logger.info("Updating scaleset %s (id=%s)", spec.name, observed.id) + logger.info("Updating scaleset %s (id=%s)", observed.name, observed.id) if observed.id is None: - logger.warning("Scaleset %s has no id; skipping update", spec.name) + logger.warning("Scaleset %s has no id; skipping update", observed.name) return self._client.update_scaleset(observed.id, params) @@ -672,6 +1067,110 @@ def _needs_update(observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> b ) +def target_scaleset_name(logical_name: str, labels: list[str]) -> str: + """Return the live GARM name a spec's scaleset should have. + + Args: + logical_name: The scaleset name the operator configured. + labels: The desired labels. + + Returns: + ``-