diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index 30f7de14..79fcddb8 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -68,6 +68,23 @@ class GarmEntityNotFoundError(GarmApiError): """Raised when a required GARM entity (org/repo/provider) cannot be found.""" +class GarmUnauthorizedError(GarmApiError): + """Raised when GARM answers a request with 401 Unauthorized. + + GARM returns 401 only for an unauthorized error, so this separates an + authorization-shaped rejection from a transport failure or a generic 500, neither + of which may be treated as one. It does not say *whose* authorization failed, and + is wider than "expired GitHub credentials" in two ways worth knowing before acting + on it: + + * GARM's own JWT middleware and its admin-only check answer 401 too, so an expired + charm token looks the same as a rejected GitHub call. + * When it is GitHub, GARM's scaleset client maps **401 and 403 alike** onto its + unauthorized error, so a 403 that is not an authorization problem at all — a + secondary rate limit, or SSO enforcement on the org — arrives here as well. + """ + + class GarmApiClient: """HTTP client for the GARM REST API. @@ -905,7 +922,10 @@ def update_scaleset(self, scaleset_id: int, params: UpdateScaleSetParams) -> Sca Updated ScaleSet model object. Raises: - GarmApiError: On API error. + GarmUnauthorizedError: If GARM answers 401. GARM only calls GitHub from this + endpoint when the name, runner group or update setting changes, so for + any other field this is GARM's own authorization rejecting the charm. + GarmApiError: On any other API error. """ with self._api_client() as client: try: @@ -993,6 +1013,27 @@ def delete_scaleset(self, scaleset_id: int) -> None: def _raise_resource_api_error(message: str, exc: ApiException) -> NoReturn: - """Raise a resource-specific wrapper error while preserving 404 semantics.""" - error_type = GarmNotFoundError if exc.status == 404 else GarmApiError + """Raise a resource-specific wrapper error, preserving 404 and 401 semantics. + + Args: + message: Human-readable description of the failed call. + exc: The generated client's exception, whose status selects the wrapper. + + Raises: + GarmNotFoundError: If the resource is already gone (404), so callers can + treat a cleanup as done rather than failed. + GarmUnauthorizedError: If GARM answers 401, which marks an authorization + rejection rather than a transport failure or a generic 500 — the + distinction the runner-removal escalation relies on before it will bypass + GitHub. See the class docstring for what that status does and does not + pin down. + GarmApiError: On any other API error. + """ + match exc.status: + case 404: + error_type: type[GarmApiError] = GarmNotFoundError + case 401: + error_type = GarmUnauthorizedError + case _: + error_type = GarmApiError raise error_type(message) from exc diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 2f616d60..0e52c42d 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -7,10 +7,17 @@ import base64 import logging from dataclasses import dataclass, field +from datetime import datetime, timedelta, timezone from charm_state import RunnerConfig -from garm_api import GarmApiError, GarmAuthenticatedClient +from garm_api import ( + GarmApiError, + GarmAuthenticatedClient, + GarmNotFoundError, + GarmUnauthorizedError, +) from garm_client.models.create_scale_set_params import CreateScaleSetParams +from garm_client.models.instance import Instance from garm_client.models.scale_set import ScaleSet from garm_client.models.template import Template from garm_client.models.update_scale_set_params import UpdateScaleSetParams @@ -27,6 +34,42 @@ # operator-supplied script runs. APROXY_SCRIPT_NAME = "00-aproxy" +# GARM's runner status for a runner that is executing a workflow job, and the +# GitHub job statuses that still hold one. GARM's job status is a closed set +# ("queued", "in_progress", "completed"), so matching the holding ones by name +# means an absent or unrecognised status frees the runner instead of pinning it +# as busy forever — the same way an unrecognised runner status is treated. +RUNNER_STATUS_ACTIVE = "active" +JOB_STATUSES_HOLDING_RUNNER = frozenset({"queued", "in_progress"}) + +# The runner statuses GARM's delete endpoint accepts. It rejects every other one with +# a 400, so a runner GARM is still creating, or already tearing down, would be refused +# on every pass; those are left for a later reconcile once GARM has moved them on +# rather than retried into the same error. +DELETABLE_RUNNER_STATUSES = frozenset( + {"running", "error", "pending_delete", "pending_force_delete"} +) + +# GARM instance statuses meaning a delete was accepted but has not completed. +# Reaching one of these is what makes a recorded provider fault mean "the teardown +# failed" rather than "the runner never came up". The transient "deleting" is not +# among them: GARM clears the fault when it enters that status and records a new one +# only on dropping back to "pending_delete", so a fault and "deleting" never coexist. +PENDING_DELETE_STATUSES = frozenset({"pending_delete", "pending_force_delete"}) + +# The status GARM parks a runner in once a *forced* delete has been accepted. Sending +# a plain delete for one of these would downgrade the escalation already in flight +# back to a plain delete, so a stuck instance could never clear. +PENDING_FORCE_DELETE_STATUS = "pending_force_delete" + +# GitHub terminates a job on a self-hosted runner at 5 days — the limit that +# applies to GARM's runners, not the 6 hours GitHub-hosted ones get. A job record +# still claiming a runner past that has to be stale (a dropped completion webhook) +# rather than a job that is genuinely still running. The bound is deliberately the +# real ceiling: undershooting it would delete a runner in the middle of a long but +# legitimate job, which costs more than leaving a scaleset around for longer. +MAX_JOB_RUNTIME = timedelta(days=5) + @dataclass class ScalesetSpec: @@ -170,31 +213,191 @@ def _reconcile_one( self._delete_custom_template(spec.name, templates) def _delete_orphaned(self, scaleset: ScaleSet) -> None: - """Disable then delete a scaleset that is no longer in the desired set.""" + """Disable, drain, then delete a scaleset that is no longer in the desired set.""" name = scaleset.name or "" logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset.id) if scaleset.id is None: return + 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 + # 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. + self._remove_runners(scaleset.id, name) try: - # Disable the scaleset first so GARM stops launching new runners. - # GARM returns 400 if the scaleset still has active runners, - # so disabling first drains it for the next reconcile to clean up. + self._client.delete_scaleset(scaleset.id) + except GarmApiError as exc: + # Runner removal is asynchronous (the provider still has to tear the + # instance down), so the scaleset is deleted on a later reconcile + # pass once GARM has finished cleaning them up. + logger.warning( + "Could not delete scaleset %s (runners may still be active; " + "will retry on next reconcile): %s", + name, + exc, + ) + + def _disable(self, scaleset_id: int, name: str) -> bool: + """Stop a scaleset launching runners, before its existing ones are removed. + + Args: + scaleset_id: Id of the scaleset being deleted. + name: Name of the scaleset being deleted, for logging. + + Returns: + Whether the scaleset is now disabled. + """ + try: + # Neither field is propagated to GitHub: GARM only calls GitHub from this + # endpoint when the name, runner group or update setting changes, so this + # is a GARM-local write and any failure here is GARM's own. self._client.update_scaleset( - scaleset.id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) + scaleset_id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) ) + return True except GarmApiError as exc: - logger.warning("Could not disable scaleset %s before delete: %s", name, exc) + logger.warning( + "Could not disable scaleset %s; deferring its delete, which GARM rejects while" + " the scaleset is still enabled (will retry on next reconcile): %s", + name, + exc, + ) + return False + + def _remove_runners(self, scaleset_id: int, name: str) -> None: + """Remove every runner belonging to a scaleset being deleted. + + Args: + scaleset_id: Id of the scaleset being deleted. + name: Name of the scaleset being deleted, for logging. + """ try: - self._client.delete_scaleset(scaleset.id) + instances = self._client.list_scale_set_instances(scaleset_id) except GarmApiError as exc: - # 400 means runners are still present; scaleset will be deleted - # on the next reconcile pass once GARM has cleaned them up. logger.warning( - "Could not delete scaleset %s (runners may still be active; " - "will retry on next reconcile): %s", + "Could not list runners of scaleset %s (will retry on next reconcile): %s", name, exc, ) + return + # GARM's delete endpoint has no atomic "delete-if-idle" precondition, so a job + # assigned to an instance between this list and its delete below is a residual + # race this loop cannot close; it relies on the next reconcile to catch it. + for instance in instances: + if not instance.name: + logger.warning( + "Skipping runner with missing name in scaleset %s (id=%s)", name, instance.id + ) + continue + if (instance.status or "").lower() not in DELETABLE_RUNNER_STATUSES: + # GARM would reject the delete with a 400, and keep rejecting it for as + # long as the runner sits in this status — one it is still creating, or + # one the provider is already tearing down. Both resolve on their own, + # so wait for GARM to move the runner on rather than retry into the + # same error every pass. + logger.info( + "Leaving runner %s of scaleset %s in place: GARM does not accept a delete" + " in status %s (will retry on the next reconcile)", + instance.name, + name, + instance.status, + ) + continue + if _is_running_job(instance): + # Deleting the runner here would fail the workflow job running on + # it, so it is left to finish and removed on a later pass along + # with the scaleset — which the disable above has already stopped + # sizing up, so no replacement is launched behind it. + logger.info( + "Leaving runner %s of scaleset %s in place: still running a job" + " (will retry on the next reconcile)", + instance.name, + name, + ) + continue + self._delete_runner(instance, name) + + def _delete_runner(self, instance: Instance, scaleset_name: str) -> None: + """Delete one runner, escalating past a stuck provider or an unauthorized GitHub. + + Args: + instance: The runner instance to delete. + scaleset_name: Name of the owning scaleset, for logging. + """ + # Both escalations below are withheld until their failure is proven, because + # each one trades a stuck runner for an orphaned resource nothing points at + # any more. force_remove makes GARM drop the runner from its database even + # when the provider teardown fails, leaving the instance running in the cloud + # with no record of it; a plain delete instead retries the teardown with a + # backoff indefinitely, so it is only forced once GARM is already sitting on a + # delete it accepted and has not managed to carry out. + instance_name = instance.name or "" + force_remove = _is_delete_stuck(instance) + logger.info( + "Removing runner %s from orphaned scaleset %s (force=%s)", + instance_name, + scaleset_name, + force_remove, + ) + try: + self._client.delete_instance(instance_name, force_remove=force_remove) + return + except GarmNotFoundError: + # The runner disappeared between the listing and here. That is the outcome + # the delete was after, so it is done, not deferred. + logger.info("Runner %s is already gone from GARM", instance_name) + return + except GarmUnauthorizedError as exc: + # Deleting a runner deregisters it in GitHub first, so this is GitHub + # rejecting that call: expired or revoked app credentials, but also a 403 + # that is not an authorization problem at all — a secondary rate limit, or + # SSO enforcement on the org — since GARM maps 401 and 403 alike onto its + # unauthorized error. (GARM's own auth answers 401 too, but the charm logs + # in as an admin on every pass, so it is not the source here.) + logger.warning( + "Could not remove runner %s: GitHub rejected the request as unauthorized;" + " retrying with the bypass (this may leave the runner registered in" + " GitHub, where it must be removed manually): %s", + instance_name, + exc, + ) + except GarmApiError as exc: + # bypass_gh_unauthorized drops the runner from the provider and GARM's + # database without deregistering it in GitHub, orphaning it there, so it is + # reserved for the branch above. Any other failure — a connection error, a + # 5xx, or a 400 for a runner that stopped being deletable between the list + # and here — is transient and left for the next reconcile instead. + self._log_deferred_runner_delete(instance_name, exc) + return + # A rate limit or an SSO block reaches the bypass too, and orphans the + # registration in GitHub. That is the accepted cost: the alternative is a + # scaleset that can never drain while GitHub answers 4xx, and a stale + # registration stays visible in GitHub and can be removed by hand. + try: + self._client.delete_instance( + instance_name, force_remove=force_remove, bypass_gh_unauthorized=True + ) + except GarmNotFoundError: + logger.info("Runner %s is already gone from GARM", instance_name) + except GarmApiError as exc: + self._log_deferred_runner_delete(instance_name, exc) + + @staticmethod + def _log_deferred_runner_delete(instance_name: str, exc: GarmApiError) -> None: + """Report a runner the next reconcile has to try again. + + Args: + instance_name: Name of the runner that could not be removed. + exc: The failure to report. + """ + logger.warning( + "Could not remove runner %s (will retry on next reconcile): %s", instance_name, exc + ) def _resolve_entity_id(self, spec: ScalesetSpec) -> str | None: """Return the GARM entity UUID for *spec*, or None if not yet registered.""" @@ -455,6 +658,83 @@ def _needs_update(observed: ScaleSet, spec: ScalesetSpec, template_id: int) -> b ) +def _is_running_job(instance: Instance) -> bool: + """Return whether a runner is currently executing a workflow job. + + GARM does not refuse to delete a busy runner, so the charm has to check before + force-removing one: tearing down a runner mid-job fails the workflow job. + + Args: + instance: The runner instance to inspect. + + Returns: + True when the runner is running a job and must be left alone. + """ + # "active" is GARM's runner status for a runner executing a job. GARM derives it + # from GitHub's live view of the runner, so it corrects itself once a job ends. + if (instance.runner_status or "").lower() == RUNNER_STATUS_ACTIVE: + return True + # The job field is a second signal, covering the window where the list endpoint + # reports an assigned job before the runner status catches up. It is only trusted + # while it is fresh: GARM reconciles stale *queued* jobs against GitHub but not + # in-progress ones, so a dropped completion webhook would otherwise leave a job + # claiming its runner forever and strand the scaleset the delete is trying to free. + job = instance.job + if job is None or (job.status or "").lower() not in JOB_STATUSES_HOLDING_RUNNER: + return False + return not _is_stale(job.updated_at) + + +def _is_delete_stuck(instance: Instance) -> bool: + """Return whether GARM has a delete for this runner that the provider keeps refusing. + + Args: + instance: The runner instance to inspect. + + Returns: + True when a delete has been accepted and the provider reported a fault + carrying it out. + """ + status = (instance.status or "").lower() + if status not in PENDING_DELETE_STATUSES: + return False + # A runner already parked in the forced-delete status keeps the escalation: it has + # been applied, whether by an earlier pass or by an operator, so the fault that + # justified it need not still be readable — GARM clears the recorded fault on every + # delete it accepts. Sending a plain delete instead would downgrade that escalation + # and leave the runner stuck for good, and re-forcing leaks nothing that is not + # already forfeit. + if status == PENDING_FORCE_DELETE_STATUS: + return True + # Both halves are needed for the rest. A delete-pending status on its own is also + # what a perfectly healthy teardown looks like while it runs, and a cloud instance + # can take minutes to disappear, so forcing on the status alone would escalate + # normal in-flight deletes and turn a retryable failure into a leaked instance. + # GARM records the provider's error against the runner when a teardown fails, which + # is what separates the two. + return bool(instance.provider_fault) + + +def _is_stale(updated_at: datetime | None) -> bool: + """Return whether a job record is too old to still describe a running job. + + Args: + updated_at: When GARM last updated the job record, if it reported one. + + Returns: + True when the record is older than the longest a job can run, so it cannot + describe a live job. An absent or unreadable timestamp is not treated as + stale: without evidence the record is old, the runner keeps its protection. + """ + if updated_at is None: + return False + # GARM serialises timestamps as RFC 3339, but a naive value would raise on + # comparison; read it as UTC rather than letting the cleanup fail on it. + if updated_at.tzinfo is None: + updated_at = updated_at.replace(tzinfo=timezone.utc) + return datetime.now(timezone.utc) - updated_at > MAX_JOB_RUNTIME + + def _effective_extra_specs(spec: ScalesetSpec) -> dict[str, object]: """Build the scaleset extra_specs a spec should produce. diff --git a/charms/garm/tests/unit/test_garm_api.py b/charms/garm/tests/unit/test_garm_api.py index 8d24e980..665c475f 100644 --- a/charms/garm/tests/unit/test_garm_api.py +++ b/charms/garm/tests/unit/test_garm_api.py @@ -14,6 +14,7 @@ GarmAuthenticatedClient, GarmConnectionError, GarmNotFoundError, + GarmUnauthorizedError, ) from garm_client.exceptions import ApiException from garm_client.models.instance import Instance @@ -293,27 +294,6 @@ def test_list_scale_set_instances_raises_not_found_on_404(): client.list_scale_set_instances(42) -def test_delete_instance_passes_force_and_bypass_flags(): - """ - arrange: An authenticated client and a generated InstancesApi stub. - act: Delete a runner with both cleanup flags enabled. - assert: The wrapper forwards the runner name, flags, and request timeout. - """ - client = GarmAuthenticatedClient(BASE_URL, "token") - with _stub_api_client(client): - with patch("garm_api.InstancesApi") as MockApi: - client.delete_instance( - "runner-1", force_remove=True, bypass_gh_unauthorized=True - ) - - MockApi.return_value.delete_instance.assert_called_once_with( - instance_name="runner-1", - force_remove=True, - bypass_gh_unauthorized=True, - _request_timeout=30, - ) - - def test_delete_instance_raises_not_found_on_404(): """ arrange: An authenticated client whose generated instance API returns HTTP 404. @@ -417,6 +397,80 @@ def test_delete_scaleset_raises_on_api_error(): client.delete_scaleset(99) +def test_list_scale_set_instances_returns_empty_on_none(): + """ + arrange: GarmAuthenticatedClient with InstancesApi returning None. + act: Call list_scale_set_instances(42). + assert: An empty list is returned so callers can iterate unconditionally. + """ + client = GarmAuthenticatedClient(BASE_URL, "token") + with _stub_api_client(client): + with patch("garm_api.InstancesApi") as MockApi: + MockApi.return_value.list_scale_set_instances.return_value = None + result = client.list_scale_set_instances(42) + assert result == [] + + +@pytest.mark.parametrize( + "kwargs, expected_force, expected_bypass", + [ + ({}, False, False), + ({"force_remove": True}, True, False), + ({"force_remove": True, "bypass_gh_unauthorized": True}, True, True), + ], + ids=["defaults", "force-remove", "force-remove-and-bypass"], +) +def test_delete_instance_forwards_removal_flags(kwargs, expected_force, expected_bypass): + """ + arrange: GarmAuthenticatedClient with InstancesApi stubbed. + act: Call delete_instance("runner-1") with the parameterised removal flags. + assert: The flags reach the API as forceRemove/bypassGHUnauthorized, and both default to + False so a plain delete never silently leaves a runner registered in GitHub. + """ + client = GarmAuthenticatedClient(BASE_URL, "token") + with _stub_api_client(client): + with patch("garm_api.InstancesApi") as MockApi: + client.delete_instance("runner-1", **kwargs) + MockApi.return_value.delete_instance.assert_called_once_with( + instance_name="runner-1", + force_remove=expected_force, + bypass_gh_unauthorized=expected_bypass, + _request_timeout=30, + ) + + +def test_delete_instance_raises_unauthorized_on_401(): + """ + arrange: GarmAuthenticatedClient with InstancesApi raising ApiException(401). + act: Call delete_instance("runner-1"). + assert: GarmUnauthorizedError is raised, so the caller can tell expired forge credentials + apart from a transient failure before escalating to the GitHub bypass. + """ + client = GarmAuthenticatedClient(BASE_URL, "token") + with _stub_api_client(client): + with patch("garm_api.InstancesApi") as MockApi: + MockApi.return_value.delete_instance.side_effect = ApiException(status=401) + with pytest.raises(GarmUnauthorizedError): + client.delete_instance("runner-1") + + +@pytest.mark.parametrize("status", [400, 404, 409, 500]) +def test_delete_instance_raises_plain_api_error_on_other_statuses(status): + """ + arrange: GarmAuthenticatedClient with InstancesApi raising the parameterised ApiException. + act: Call delete_instance("runner-1"). + assert: A plain GarmApiError (not GarmUnauthorizedError) is raised, so a non-401 failure + never escalates into a GitHub Unauthorized bypass. + """ + client = GarmAuthenticatedClient(BASE_URL, "token") + with _stub_api_client(client): + with patch("garm_api.InstancesApi") as MockApi: + MockApi.return_value.delete_instance.side_effect = ApiException(status=status) + with pytest.raises(GarmApiError) as exc_info: + client.delete_instance("runner-1") + assert not isinstance(exc_info.value, GarmUnauthorizedError) + + def test_list_credentials_returns_list(): """ arrange: GarmAuthenticatedClient with CredentialsApi returning one credential. diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 9476957d..80dfd34a 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -4,10 +4,18 @@ """Unit tests for the scaleset reconciler.""" import base64 +import logging +from datetime import datetime, timedelta, timezone import pytest from charm_state import RunnerConfig +from garm_api import ( + GarmApiError, + GarmConnectionError, + GarmNotFoundError, + GarmUnauthorizedError, +) from garm_client.models.template import Template from runner_template import build_template_data from scaleset_reconciler import ScalesetReconciler, ScalesetSpec, _effective_extra_specs @@ -51,6 +59,31 @@ def __init__( self.enabled = enabled +class _FakeJob: + def __init__(self, status, updated_at=None): + self.status = status + self.updated_at = updated_at + + +class _FakeInstance: + def __init__( + self, + name, + iid="instance-uuid", + runner_status="idle", + job_status=None, + status="running", + job_updated_at=None, + provider_fault=None, + ): + self.name = name + self.id = iid + self.runner_status = runner_status + self.status = status + self.provider_fault = provider_fault + self.job = _FakeJob(job_status, job_updated_at) if job_status is not None else None + + class FakeGarmClient: """In-memory fake for GarmAuthenticatedClient. @@ -58,7 +91,17 @@ class FakeGarmClient: tests can assert on the resulting state rather than on mock call patterns. """ - def __init__(self, providers=None, scalesets=None, org_id="org-uuid", repo_id=None): + def __init__( + self, + providers=None, + scalesets=None, + org_id="org-uuid", + repo_id=None, + instances=None, + delete_instance_error=None, + bypass_delete_error=None, + update_scaleset_error=None, + ): self._providers = [_FakeProvider(n) for n in (providers or [])] self._scalesets = [ _FakeScaleset( @@ -78,9 +121,17 @@ def __init__(self, providers=None, scalesets=None, org_id="org-uuid", repo_id=No ] self._org_id = org_id self._repo_id = repo_id + self._instances = { + sid: [_FakeInstance(**i) if isinstance(i, dict) else _FakeInstance(i) for i in names] + for sid, names in (instances or {}).items() + } + self._delete_instance_error = delete_instance_error + self._bypass_delete_error = bypass_delete_error + self._update_scaleset_error = update_scaleset_error self.created: list[tuple[str, str, object]] = [] self.updated: list[tuple[int, object]] = [] self.deleted: list[int] = [] + self.deleted_instances: list[tuple[str, bool, bool]] = [] def list_providers(self): return self._providers @@ -101,11 +152,24 @@ def create_repo_scaleset(self, repo_id, params): self.created.append(("repo", repo_id, params)) def update_scaleset(self, scaleset_id, params): + if self._update_scaleset_error is not None: + raise self._update_scaleset_error self.updated.append((scaleset_id, params)) def delete_scaleset(self, scaleset_id): self.deleted.append(scaleset_id) + def list_scale_set_instances(self, scaleset_id): + return self._instances.get(scaleset_id, []) + + def delete_instance(self, instance_name, force_remove=False, bypass_gh_unauthorized=False): + self.deleted_instances.append((instance_name, force_remove, bypass_gh_unauthorized)) + error = ( + self._bypass_delete_error if bypass_gh_unauthorized else self._delete_instance_error + ) + if error is not None: + raise error + # Template stubs: return empty results so the reconciler's template path # is a no-op when no runner config is set. def list_templates(self, partial_name=None, os_type=None): @@ -320,6 +384,400 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] +@pytest.mark.parametrize( + "error", + [GarmApiError("500 Server error"), GarmUnauthorizedError("401 Unauthorized")], + ids=["server-error", "unauthorized"], +) +def test_disable_failure_defers_the_whole_cleanup(error): + """ + arrange: FakeGarmClient whose update_scaleset (the disable-before-delete call) fails, for an + orphaned scaleset that still owns a runner. + act: Reconcile so the scaleset is orphaned. + assert: Neither the runner nor the scaleset is deleted. GARM rejects the delete of a + scaleset that is still enabled, so the delete could only 400; and removing the runners + of a scaleset that is still sized up would just have GARM launch replacements, churning + instances on every pass instead of draining. Both wait for the next reconcile. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1"]}, + update_scaleset_error=error, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [] + assert client.deleted == [] + + +@pytest.mark.parametrize( + "status, provider_fault, expected_force", + [ + ("running", None, False), + ("error", None, False), + ("pending_delete", None, False), + ("pending_delete", b"nova: instance not found", True), + ("pending_force_delete", b"nova: instance not found", True), + ("pending_force_delete", None, True), + ], + ids=[ + "healthy-runner-not-forced", + "errored-runner-not-forced", + "pending-delete-in-flight-not-forced", + "pending-delete-with-fault-forced", + "pending-force-delete-with-fault-forced", + "pending-force-delete-without-fault-stays-forced", + ], +) +def test_force_remove_only_once_the_provider_has_refused_a_delete( + status, provider_fault, expected_force +): + """ + arrange: FakeGarmClient with an orphaned scaleset owning a runner in a given delete state, + with or without a fault recorded against it by the provider. + act: Reconcile so the scaleset is orphaned. + assert: force_remove is set only once a delete has been accepted and the provider reported a + fault carrying it out. A delete-pending status alone is also what a healthy teardown + looks like while it runs, and forcing that would turn a retryable failure into an + instance leaked in the cloud; the recorded fault is what separates the two. A runner + already parked in the forced-delete status keeps the escalation regardless, since a + plain delete would downgrade a force already in flight and strand the runner for good. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: [{"name": "runner-1", "status": status, "provider_fault": provider_fault}]}, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [("runner-1", expected_force, False)] + + +def test_delete_orphaned_scaleset_removes_runners_first(): + """ + arrange: FakeGarmClient with an orphaned scaleset that still owns two runners. + act: Reconcile with a different desired scaleset name. + assert: Both runners are removed before the scaleset is deleted, so GARM does not reject + the delete for still owning runners. Neither escalation is used while the plain removal + succeeds: forcing would leak the cloud instance and the bypass would orphan the runner + in GitHub. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1", "runner-2"]}, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [ + ("runner-1", False, False), + ("runner-2", False, False), + ] + assert client.deleted == [42] + + +def test_runner_removal_falls_back_to_github_unauthorized_bypass(): + """ + arrange: FakeGarmClient whose plain runner removal fails (expired GitHub credentials). + act: Reconcile so the scaleset is orphaned. + assert: The removal is retried with the GitHub Unauthorized bypass, which drops the runner + from the provider and GARM's database so the scaleset can still be deleted. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1"]}, + delete_instance_error=GarmUnauthorizedError("401 Unauthorized"), + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [ + ("runner-1", False, False), + ("runner-1", False, True), + ] + assert client.deleted == [42] + + +def test_runner_removal_failure_does_not_abort_reconcile(): + """ + arrange: FakeGarmClient whose runner removal fails on both attempts. + act: Reconcile so the scaleset is orphaned. + assert: The scaleset delete is still attempted and the desired scaleset is still created, + so one stuck runner cannot block the rest of the pass; GARM retries next reconcile. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1"]}, + delete_instance_error=GarmUnauthorizedError("401 Unauthorized"), + bypass_delete_error=GarmApiError("provider error"), + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted == [42] + assert len(client.created) == 1 + + +@pytest.mark.parametrize( + "error", + [ + GarmApiError("500 Server error"), + GarmConnectionError("connection refused"), + GarmApiError("400 Bad Request: runner must be in one of the following states"), + ], + ids=["server-error", "connection-error", "bad-request"], +) +def test_runner_removal_does_not_bypass_github_on_non_401_errors(error): + """ + arrange: FakeGarmClient whose runner removal fails with a non-401 error. + act: Reconcile so the scaleset is orphaned. + assert: The removal is not retried with the GitHub Unauthorized bypass. A transient GARM + or provider failure is not an authorization problem, so escalating would orphan a + runner in GitHub while the credentials are still valid; it is retried next reconcile. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1"]}, + delete_instance_error=error, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [("runner-1", False, False)] + + +@pytest.mark.parametrize( + "instance_kwargs", + [ + {"runner_status": "active"}, + {"runner_status": "idle", "job_status": "in_progress"}, + {"runner_status": "idle", "job_status": "queued"}, + ], + ids=["active-status", "job-in-progress", "job-queued"], +) +def test_runner_running_a_job_is_left_alone(instance_kwargs): + """ + arrange: FakeGarmClient with an orphaned scaleset owning a runner that is running a job. + act: Reconcile so the scaleset is orphaned. + assert: The runner is not deleted, so the workflow job is not failed mid-run. The scaleset + stays disabled and is cleaned up on a later pass once the job finishes. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: [{"name": "runner-1", **instance_kwargs}]}, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [] + + +@pytest.mark.parametrize( + "age, expect_removed", + [ + (timedelta(minutes=30), False), + (timedelta(hours=8), False), + (timedelta(days=4, hours=23), False), + (timedelta(days=6), True), + ], + ids=[ + "recent-job-protects", + "long-running-job-protects", + "just-under-the-limit-protects", + "stale-job-frees-runner", + ], +) +def test_a_job_only_protects_its_runner_while_it_could_still_be_running(age, expect_removed): + """ + arrange: FakeGarmClient with an orphaned scaleset owning an idle runner whose job record is + still in progress, last updated a given time ago. + act: Reconcile so the scaleset is orphaned. + assert: The job protects its runner only while it is recent enough to describe a live job. + The bound is the five days GitHub allows a job on a self-hosted runner, not the six + hours a GitHub-hosted one gets, so a long but legitimate job keeps its runner. GARM only + reconciles stale queued jobs against the forge, so past that ceiling a dropped + completion webhook would otherwise pin the runner as busy forever and strand the + scaleset — the very failure this cleanup exists to fix. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={ + 42: [ + { + "name": "runner-1", + "runner_status": "idle", + "job_status": "in_progress", + "job_updated_at": datetime.now(timezone.utc) - age, + } + ] + }, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert bool(client.deleted_instances) is expect_removed + + +def test_a_naive_job_timestamp_is_read_as_utc(): + """ + arrange: FakeGarmClient with an orphaned scaleset owning a runner whose in-progress job + carries a long-stale timestamp with no timezone attached. + act: Reconcile so the scaleset is orphaned. + assert: The runner is still freed rather than the cleanup raising on the comparison, so a + timestamp GARM serialised without an offset cannot break the reconcile. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={ + 42: [ + { + "name": "runner-1", + "runner_status": "idle", + "job_status": "in_progress", + "job_updated_at": datetime(2020, 1, 1, 12, 0, 0), + } + ] + }, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [("runner-1", False, False)] + + +@pytest.mark.parametrize( + "instance_kwargs", + [ + {"runner_status": "idle"}, + {"runner_status": "idle", "job_status": "completed"}, + {"runner_status": "failed"}, + {"runner_status": None}, + {"runner_status": "idle", "job_status": ""}, + {"runner_status": "idle", "job_status": "some-unrecognised-status"}, + ], + ids=[ + "idle", + "job-completed", + "failed", + "unknown-status", + "job-status-empty", + "job-status-unrecognised", + ], +) +def test_idle_runner_is_removed(instance_kwargs): + """ + arrange: FakeGarmClient with an orphaned scaleset owning a runner that holds no live job. + act: Reconcile so the scaleset is orphaned. + assert: The runner is removed, so the busy-runner guard does not stall the cleanup for + runners that are safe to delete. An absent or unrecognised job status frees the runner + rather than pinning it as busy forever, since GARM's job statuses are a closed set and + a stale or unhydrated job record would otherwise strand the scaleset. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: [{"name": "runner-1", **instance_kwargs}]}, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [("runner-1", False, False)] + + +@pytest.mark.parametrize( + "status", + ["pending_create", "creating", "deleting", "deleted", "stopped", "unknown"], +) +def test_a_runner_garm_would_refuse_to_delete_is_left_alone(status): + """ + arrange: FakeGarmClient with an orphaned scaleset owning a runner in a status GARM's delete + endpoint does not accept. + act: Reconcile so the scaleset is orphaned. + assert: No delete is issued for it. GARM only accepts a delete for a runner that is running, + errored or already delete-pending, so asking for one here would 400 on every pass for as + long as the runner sat in that status; the ones GARM is still creating or already tearing + down resolve on their own and are picked up by a later reconcile instead. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: [{"name": "runner-1", "status": status}]}, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [] + + +def test_unnamed_runner_is_skipped(): + """ + arrange: FakeGarmClient with an orphaned scaleset owning a runner that has no name. + act: Reconcile so the scaleset is orphaned. + assert: No delete is attempted for the unnamed runner (it cannot be addressed by name), + and the scaleset delete still runs. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: [{"name": None}]}, + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [] + assert client.deleted == [42] + + +def test_runner_listing_failure_still_attempts_the_scaleset_delete(): + """ + arrange: FakeGarmClient whose list_scale_set_instances raises. + act: Reconcile so the scaleset is orphaned. + assert: No runner delete is attempted, but the scaleset delete still is, and the reconcile + completes. An unreadable runner list says nothing about whether the scaleset owns any: + if it does not, the delete succeeds and the scaleset is not stranded on a failure to + read it; if it does, GARM rejects the delete and the next pass retries the whole thing. + """ + + class _ListFailsClient(FakeGarmClient): + def list_scale_set_instances(self, scaleset_id): + raise GarmApiError("boom") + + client = _ListFailsClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [] + assert client.deleted == [42] + assert len(client.created) == 1 + + +@pytest.mark.parametrize("bypass_needed", [False, True], ids=["plain", "after-bypass"]) +def test_a_runner_that_is_already_gone_is_not_reported_as_deferred(bypass_needed, caplog): + """ + arrange: FakeGarmClient whose runner removal answers 404, either straight away or on the + bypass retry that follows an unauthorized rejection. + act: Reconcile so the scaleset is orphaned. + assert: Nothing is logged as awaiting a retry. A 404 is the outcome the delete was after, + so promising to try again would name a runner no later pass can act on. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1"]}, + delete_instance_error=( + GarmUnauthorizedError("401 Unauthorized") + if bypass_needed + else GarmNotFoundError("404") + ), + bypass_delete_error=GarmNotFoundError("404") if bypass_needed else None, + ) + with caplog.at_level(logging.WARNING): + _reconcile(client, [_spec(name="new-scaleset")]) + + assert "will retry on next reconcile" not in caplog.text + assert client.deleted == [42] + + def test_unnamed_observed_scaleset_is_skipped(): """ arrange: FakeGarmClient with one observed scaleset lacking a name. diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 8644f5e4..bed39437 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -378,12 +378,7 @@ def test_charm_reconciles_org_and_scaleset( # _reconcile_runners() runs now that the controller URLs are set. max-runner=10 doubles as the # scaleset assertion below. juju.config(configurator_with_image, values={"max-runner": "10"}) - juju.wait( - lambda status: jubilant.all_active(status, configurator_with_image) - and jubilant.all_agents_idle(status, configurator_garm), - timeout=3 * 60, - delay=10, - ) + _wait_for_config_applied(juju, configurator_garm, configurator_with_image) org = _wait_for_org(base_url, token, "test-org") credential = _wait_for_github_credential(base_url, token, _SYNCED_CREDENTIAL_NAME) @@ -409,6 +404,27 @@ def test_charm_reconciles_org_and_scaleset( ) +def _wait_for_config_applied( + juju: jubilant.Juju, garm_app: str, configurator_app: str +) -> None: + """Wait for a configurator config change to reach GARM's reconcile. + + A config change triggers config_changed on the configurator, then relation_changed on + GARM, whose holistic reconcile applies the new desired scaleset set. + + Args: + juju: Jubilant Juju handle. + garm_app: Name of the GARM application. + configurator_app: Name of the garm-configurator application. + """ + juju.wait( + lambda status: jubilant.all_active(status, configurator_app) + and jubilant.all_agents_idle(status, garm_app), + timeout=3 * 60, + delay=10, + ) + + def _list_templates(address: str, token: str) -> list[dict]: """List all runner install templates from the GARM API. @@ -451,6 +467,70 @@ def _get_template_body(address: str, token: str, template_id: int) -> str: return base64.b64decode(raw_b64).decode("utf-8") if raw_b64 else "" +def test_charm_disables_and_deletes_an_orphaned_scaleset( + juju: jubilant.Juju, + configurator_garm: str, + configurator_with_image: str, + fake_github_api_url: str, +): + """ + arrange: GARM and garm-configurator are integrated against the GitHub double, with the + desired scaleset created under a throwaway name. + act: Rename the desired scaleset back, so the one GARM holds is no longer desired. + assert: The charm removes the orphaned scaleset itself and creates the renamed one in its + place. GARM rejects the delete of a scaleset that is still enabled, so this covers the + charm's own disable-then-delete against a live GARM — the half of the sequence the + teardown helper below otherwise hand-rolls. The drain itself is not exercised: the + configurator's min-idle-runner defaults to 0 and the OpenStack provider is a stand-in, + so the orphaned scaleset never owns a runner. That half stays unit-tested. + """ + address = _get_garm_address(juju, configurator_garm) + base_url = _garm_api_base_url(address) + token = _garm_first_run(juju, address) + # Same arrange as the reconcile test above: unwind the charm-synced chain so github.com can + # be repointed at the mock, and restore the system templates scaleset creation needs. + _detach_synced_credential(base_url, token) + _point_github_endpoint_at_mock(base_url, token, fake_github_api_url) + _restore_system_templates(base_url, token) + + renamed = f"{_SCALESET_TEST_NAME}-renamed" + try: + juju.config(configurator_with_image, values={"name": renamed}) + _wait_for_config_applied(juju, configurator_garm, configurator_with_image) + _wait_for_scaleset(base_url, token, renamed) + finally: + # Renaming back is the act, and doubles as cleanup: every later test in this module + # asserts against the default scaleset name. + juju.config(configurator_with_image, values={"name": _SCALESET_TEST_NAME}) + _wait_for_config_applied(juju, configurator_garm, configurator_with_image) + + _wait_for_scaleset_absent(base_url, token, renamed) + _wait_for_scaleset(base_url, token, _SCALESET_TEST_NAME) + + +@retry( + retry=retry_if_exception_type( + (AssertionError, requests.exceptions.RequestException) + ), + wait=wait_exponential(multiplier=1, min=2, max=20), + stop=stop_after_attempt(30), + reraise=True, +) +def _wait_for_scaleset_absent(base_url: str, token: str, name: str) -> None: + """Wait until a named scaleset is gone from GARM. + + Args: + base_url: GARM API base URL. + token: JWT token for authentication. + name: Name of the scaleset that should disappear. + """ + scalesets = _list_scalesets(base_url, token) + assert _find_scaleset(scalesets, name) is None, ( + f"Expected scaleset {name!r} to be deleted by the charm; " + f"still present among {[scaleset.get('name') for scaleset in scalesets]}" + ) + + def test_garm_charmed_template_created_on_debug_ssh( juju: jubilant.Juju, garm_with_debug_ssh: str, @@ -705,8 +785,11 @@ def _delete_scalesets(base_url: str, token: str) -> None: scaleset_id = scaleset.get("id") if scaleset_id is None: continue - # GARM 400s a scaleset delete while the scaleset is still enabled or draining runners, so - # disable it (idle count to zero) to trigger the drain before deleting. + # GARM 400s a scaleset delete while the scaleset is still enabled, and again while it + # still owns runners, so disable it (idle count to zero) to trigger the drain before + # deleting. The charm does this for itself — see + # test_charm_disables_and_deletes_an_orphaned_scaleset; this is only teardown, unwinding + # scalesets the charm is not being asked to remove. resp = requests.put( f"{base_url}/scalesets/{scaleset_id}", json={"enabled": False, "min_idle_runners": 0}, @@ -981,12 +1064,7 @@ def test_runner_options_render_into_scaleset_template( "pre-job-script": "echo integration-marker", }, ) - juju.wait( - lambda status: jubilant.all_active(status, configurator_with_image) - and jubilant.all_agents_idle(status, configurator_garm), - timeout=3 * 60, - delay=10, - ) + _wait_for_config_applied(juju, configurator_garm, configurator_with_image) # One marker per template-delivered config option, proving each reaches GARM # via live reconcile: dockerhub-mirror (daemon.json + env var), diff --git a/docs/changelog.md b/docs/changelog.md index f2485e09..e8773705 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -8,6 +8,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Each revision is versioned by the date of the revision. +## 2026-08-24 + +- `garm`: remove a scaleset's runners before deleting the scaleset. GARM rejects deleting a scaleset that still owns runners, so removing every `garm-configurator` relation left the orphaned scalesets — and the runners they own — behind. The charm now disables an orphaned scaleset and removes its runners before deleting it, retrying on the next reconcile whatever it could not remove this time. A runner that is executing a workflow job is left to finish and cleaned up on a later pass, so the removal never fails a running job. If GitHub rejects a runner's removal as unauthorized (for example, expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which can leave the runner registered in GitHub, where it must be removed manually. + ## 2026-08-19 - Add a tutorial for deploying GARM. It walks through a first GARM deployment on Canonical Kubernetes with PostgreSQL and the `garm-configurator`, ending with a runner scale set registered on a GitHub repository. OpenStack is represented by placeholder configuration and a stand-in image provider so the tutorial runs without a cloud, and a closing section states what changes for a deployment that boots runners.