From 0f3830a4dbbaf6b3d799f8e948b20e70a9d3e0ee Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Mon, 24 Aug 2026 10:18:50 +0800 Subject: [PATCH 01/10] fix(garm): remove a scaleset's runners before deleting it GARM rejects deleting a scaleset that still owns runners, so an orphaned scaleset with active runners was never removed and its runners were left behind. Remove each runner first, ignoring provider errors so an instance GARM can no longer reach does not block the cleanup. A runner executing a workflow job is left in place and removed on a later reconcile once the job finishes, so an in-flight job is never failed. On a 401 (expired forge credentials) the removal is retried with GARM's GitHub Unauthorized bypass; any other failure is left for the next reconcile rather than escalated into a runner orphaned in GitHub. Co-Authored-By: Claude Opus 5 --- charms/garm/src/garm_api.py | 77 ++++++ charms/garm/src/scaleset_reconciler.py | 126 +++++++++- charms/garm/tests/unit/test_garm_api.py | 115 ++++++++- .../tests/unit/test_scaleset_reconciler.py | 228 +++++++++++++++++- docs/changelog.md | 4 + 5 files changed, 541 insertions(+), 9 deletions(-) diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index d321864b..57a16d1b 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -14,6 +14,7 @@ from garm_client.api.controller_info_api import ControllerInfoApi from garm_client.api.credentials_api import CredentialsApi from garm_client.api.first_run_api import FirstRunApi +from garm_client.api.instances_api import InstancesApi from garm_client.api.login_api import LoginApi from garm_client.api.organizations_api import OrganizationsApi from garm_client.api.providers_api import ProvidersApi @@ -29,6 +30,7 @@ from garm_client.models.create_scale_set_params import CreateScaleSetParams from garm_client.models.create_template_params import CreateTemplateParams from garm_client.models.forge_credentials import ForgeCredentials +from garm_client.models.instance import Instance from garm_client.models.new_user_params import NewUserParams from garm_client.models.organization import Organization from garm_client.models.password_login_params import PasswordLoginParams @@ -61,6 +63,15 @@ 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's error handler returns 401 only for an unauthorized error, so this + distinguishes expired forge credentials from a transport failure or a generic + 500, neither of which may be treated as an authorization problem. + """ + + class GarmApiClient: """HTTP client for the GARM REST API. @@ -940,3 +951,69 @@ def delete_scaleset(self, scaleset_id: int) -> None: ) from exc except urllib3.exceptions.HTTPError as exc: raise GarmConnectionError(f"GARM connection error: {exc}") from exc + + def list_scaleset_instances(self, scaleset_id: int) -> list[Instance]: + """List the runner instances belonging to a scaleset. + + Args: + scaleset_id: Integer scaleset ID. + + Returns: + List of Instance model objects. + + Raises: + GarmApiError: On API error. + """ + with self._api_client() as client: + try: + return ( + InstancesApi(api_client=client).list_scale_set_instances( + scaleset_id=str(scaleset_id), + _request_timeout=_REQUEST_TIMEOUT, + ) + or [] + ) + except ApiException as exc: + raise GarmApiError( + f"Failed to list instances of scaleset {scaleset_id} " + f"({exc.status}): {exc.body}" + ) from exc + except urllib3.exceptions.HTTPError as exc: + raise GarmConnectionError(f"GARM connection error: {exc}") from exc + + def delete_instance( + self, + instance_name: str, + force_remove: bool = False, + bypass_gh_unauthorized: bool = False, + ) -> None: + """Delete a runner instance. + + Args: + instance_name: Name of the runner instance to delete. + force_remove: Ignore provider errors and still remove the runner from + GitHub and the GARM database (the API's ``forceRemove``). + bypass_gh_unauthorized: Ignore GitHub Unauthorized errors and remove the + runner from the provider and the GARM database (the API's + ``bypassGHUnauthorized``). This can leave a runner registered in + GitHub when the credentials are no longer valid. + + Raises: + GarmUnauthorizedError: If GARM answers 401 (expired forge credentials). + GarmApiError: On any other API error. + """ + with self._api_client() as client: + try: + InstancesApi(api_client=client).delete_instance( + instance_name=instance_name, + force_remove=force_remove, + bypass_gh_unauthorized=bypass_gh_unauthorized, + _request_timeout=_REQUEST_TIMEOUT, + ) + except ApiException as exc: + message = f"Failed to delete instance {instance_name} ({exc.status}): {exc.body}" + if exc.status == 401: + raise GarmUnauthorizedError(message) from exc + raise GarmApiError(message) from exc + except urllib3.exceptions.HTTPError as exc: + raise GarmConnectionError(f"GARM connection error: {exc}") from exc diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 2f616d60..d3f43f8f 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -9,8 +9,9 @@ from dataclasses import dataclass, field from charm_state import RunnerConfig -from garm_api import GarmApiError, GarmAuthenticatedClient +from garm_api import GarmApiError, GarmAuthenticatedClient, 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 +28,11 @@ # 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 status for one that has finished. +RUNNER_STATUS_ACTIVE = "active" +JOB_STATUS_COMPLETED = "completed" + @dataclass class ScalesetSpec: @@ -170,25 +176,28 @@ 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 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. + # Disable the scaleset first so GARM stops launching new runners + # while its existing ones are being removed. self._client.update_scaleset( scaleset.id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) ) except GarmApiError as exc: logger.warning("Could not disable scaleset %s before delete: %s", name, exc) + # GARM returns 400 while the scaleset still owns runners, so the runners + # have to go first; anything left behind is retried on the next pass. + self._remove_runners(scaleset.id, name) try: self._client.delete_scaleset(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. + # 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", @@ -196,6 +205,88 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: exc, ) + 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: + instances = self._client.list_scaleset_instances(scaleset_id) + except GarmApiError as exc: + logger.warning( + "Could not list runners of scaleset %s (will retry on next reconcile): %s", + name, + exc, + ) + return + 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 _is_running_job(instance): + # The scaleset is already disabled with min_idle_runners=0, so GARM + # launches no replacement; the runner is left to finish its job and + # is removed on a later pass, along with the scaleset. Deleting it + # here would fail the workflow job that is currently running on 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, name) + + def _delete_runner(self, instance_name: str, scaleset_name: str) -> None: + """Delete one runner, escalating to a GitHub-unauthorized bypass if needed. + + Provider errors are always ignored (``force_remove``): the scaleset is going + away, so a runner GARM can no longer reach in the provider must not block the + removal. The bypass is reserved for a 401 — the only status GARM returns for + an unauthorized forge error — because it drops the runner from the provider + and GARM's database without deregistering it in GitHub, orphaning it there. + Any other failure (a connection error, a 5xx, or a 400 for a runner that is + not yet in a deletable state) is transient, so it is left for the next + reconcile rather than escalated into a GitHub orphan. + + Args: + instance_name: Name of the runner instance to delete. + scaleset_name: Name of the owning scaleset, for logging. + """ + logger.info("Removing runner %s from orphaned scaleset %s", instance_name, scaleset_name) + try: + self._client.delete_instance(instance_name, force_remove=True) + return + except GarmUnauthorizedError as exc: + 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: + logger.warning( + "Could not remove runner %s (will retry on next reconcile): %s", + instance_name, + exc, + ) + return + try: + self._client.delete_instance( + instance_name, force_remove=True, bypass_gh_unauthorized=True + ) + except GarmApiError as exc: + 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.""" if spec.entity_type == "organization": @@ -455,6 +546,27 @@ 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. The job field + # is a second signal: the list endpoint may report an assigned job before the + # status catches up, and a completed job no longer holds the runner. + if (instance.runner_status or "").lower() == RUNNER_STATUS_ACTIVE: + return True + job = instance.job + return job is not None and (job.status or "").lower() != JOB_STATUS_COMPLETED + + 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 09df38c2..e5502296 100644 --- a/charms/garm/tests/unit/test_garm_api.py +++ b/charms/garm/tests/unit/test_garm_api.py @@ -8,7 +8,13 @@ import pytest -from garm_api import GarmApiClient, GarmApiError, GarmAuthenticatedClient, GarmConnectionError +from garm_api import ( + GarmApiClient, + GarmApiError, + GarmAuthenticatedClient, + GarmConnectionError, + GarmUnauthorizedError, +) from garm_client.exceptions import ApiException from garm_client.models.instance import Instance @@ -343,6 +349,113 @@ def test_delete_scaleset_raises_on_api_error(): client.delete_scaleset(99) +def test_list_scaleset_instances_returns_list(): + """ + arrange: GarmAuthenticatedClient with InstancesApi returning two instances. + act: Call list_scaleset_instances(42). + assert: The scaleset id is passed as a string and the instances are returned. + """ + client = GarmAuthenticatedClient(BASE_URL, "token") + first, second = MagicMock(), MagicMock() + first.name, second.name = "runner-1", "runner-2" + with _stub_api_client(client): + with patch("garm_api.InstancesApi") as MockApi: + MockApi.return_value.list_scale_set_instances.return_value = [first, second] + result = client.list_scaleset_instances(42) + MockApi.return_value.list_scale_set_instances.assert_called_once_with( + scaleset_id="42", _request_timeout=30 + ) + assert [instance.name for instance in result] == ["runner-1", "runner-2"] + + +def test_list_scaleset_instances_returns_empty_on_none(): + """ + arrange: GarmAuthenticatedClient with InstancesApi returning None. + act: Call list_scaleset_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_scaleset_instances(42) + assert result == [] + + +def test_list_scaleset_instances_raises_on_api_error(): + """ + arrange: GarmAuthenticatedClient with InstancesApi raising ApiException(404). + act: Call list_scaleset_instances(99). + assert: GarmApiError is raised. + """ + client = GarmAuthenticatedClient(BASE_URL, "token") + with _stub_api_client(client): + with patch("garm_api.InstancesApi") as MockApi: + MockApi.return_value.list_scale_set_instances.side_effect = ApiException(status=404) + with pytest.raises(GarmApiError): + client.list_scaleset_instances(99) + + +@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..558af00c 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -8,6 +8,7 @@ import pytest from charm_state import RunnerConfig +from garm_api import GarmApiError, GarmConnectionError, 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 +52,19 @@ def __init__( self.enabled = enabled +class _FakeJob: + def __init__(self, status): + self.status = status + + +class _FakeInstance: + def __init__(self, name, iid="instance-uuid", runner_status="idle", job_status=None): + self.name = name + self.id = iid + self.runner_status = runner_status + self.job = _FakeJob(job_status) if job_status is not None else None + + class FakeGarmClient: """In-memory fake for GarmAuthenticatedClient. @@ -58,7 +72,16 @@ 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, + ): self._providers = [_FakeProvider(n) for n in (providers or [])] self._scalesets = [ _FakeScaleset( @@ -78,9 +101,16 @@ 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.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 @@ -106,6 +136,17 @@ def update_scaleset(self, scaleset_id, params): def delete_scaleset(self, scaleset_id): self.deleted.append(scaleset_id) + def list_scaleset_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 +361,191 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] +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 force-removed before the scaleset is deleted, so GARM does not + reject the delete for still owning runners; the GitHub bypass is not used while the + plain removal succeeds. + """ + 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", True, False), + ("runner-2", True, 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", True, False), + ("runner-1", True, 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", True, 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( + "instance_kwargs", + [ + {"runner_status": "idle"}, + {"runner_status": "idle", "job_status": "completed"}, + {"runner_status": "failed"}, + {"runner_status": None}, + ], + ids=["idle", "job-completed", "failed", "unknown-status"], +) +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 force-removed, so the busy-runner guard does not stall the cleanup + for runners that are safe to delete. + """ + 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", True, False)] + + +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_defers_scaleset_delete_attempt(): + """ + arrange: FakeGarmClient whose list_scaleset_instances raises. + act: Reconcile so the scaleset is orphaned. + assert: No runner delete is attempted and the reconcile completes, leaving the cleanup to + the next pass rather than aborting on an unreadable runner list. + """ + + class _ListFailsClient(FakeGarmClient): + def list_scaleset_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 len(client.created) == 1 + + def test_unnamed_observed_scaleset_is_skipped(): """ arrange: FakeGarmClient with one observed scaleset lacking a name. diff --git a/docs/changelog.md b/docs/changelog.md index b2e314de..0e12965e 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-21 + +- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first, ignoring provider errors so an instance GARM can no longer reach in the provider does not block the cleanup. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. + ## 2026-07-13 - `garm`: fix the SSH debug (tmate) connection details so the runner actually reads them. The `TMATE_SERVER_*` variables injected into the shared runner template were written to `/home/runner/.env`, but the GitHub Actions runner sources a `.env` file from its install directory (`/home/runner/actions-runner/.env`), so the debug-SSH server host, port, and fingerprints were silently ignored. They are now written to the `.env` file the runner reads, matching the per-scaleset runner options. From 0de4356c963121dd0f47ef89a338709cec54b2ea Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Mon, 24 Aug 2026 16:01:10 +0800 Subject: [PATCH 02/10] fix(garm): withhold destructive runner-removal escalations until proven Follow-up to the runner-removal change, addressing three issues found in review against GARM's own source at the pinned commit. Force-remove is no longer unconditional. GARM's provider worker retries a plain delete's teardown with a backoff indefinitely, but a forced one logs the provider error and marks the instance deleted anyway, leaving the VM running in the cloud with nothing pointing at it. It is now applied only once the instance already sits in a pending-delete state, so a transient provider error gets a retry cycle while a permanently unreachable instance still cannot block the scaleset delete forever. A failed disable no longer aborts the removal. Disabling a scaleset goes through the forge, so expired credentials fail it -- exactly the case the unauthorized-bypass escalation exists for -- and treating it as a hard precondition stranded the scaleset instead. Leaving the scaleset enabled only risks GARM replacing a removed runner for one pass, which it cannot do when the forge is what is broken. The busy-runner guard now matches the job statuses that actually hold a runner. GARM's job status is a closed set, so treating anything other than "completed" as running meant a stale or unhydrated job record pinned the runner as busy on every reconcile and the scaleset was never deleted. Also deduplicates the deferred-delete warning and records the residual list-then-delete race, which GARM's API offers no way to close. Co-Authored-By: Claude Opus 5 --- charms/garm/src/scaleset_reconciler.py | 98 ++++++++++++------- .../tests/unit/test_scaleset_reconciler.py | 87 ++++++++++++++-- docs/changelog.md | 2 +- 3 files changed, 142 insertions(+), 45 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index d3f43f8f..08bf714f 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -29,9 +29,16 @@ APROXY_SCRIPT_NAME = "00-aproxy" # GARM's runner status for a runner that is executing a workflow job, and the -# GitHub job status for one that has finished. +# 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_STATUS_COMPLETED = "completed" +JOB_STATUSES_HOLDING_RUNNER = frozenset({"queued", "in_progress"}) + +# GARM instance statuses meaning a delete was accepted but has not completed: +# the provider teardown is failing and being retried with a backoff. +PENDING_DELETE_STATUSES = frozenset({"pending_delete", "deleting"}) @dataclass @@ -188,6 +195,11 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: scaleset.id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) ) except GarmApiError as exc: + # Carry on rather than deferring the whole removal: disabling goes + # through the forge, so expired credentials fail here — the very case + # the runner removal below is built to escalate past. Leaving the + # scaleset enabled only risks GARM replacing a removed runner for one + # pass, and it cannot even do that when the forge is what is broken. logger.warning("Could not disable scaleset %s before delete: %s", name, exc) # GARM returns 400 while the scaleset still owns runners, so the runners # have to go first; anything left behind is retried on the next pass. @@ -221,6 +233,9 @@ def _remove_runners(self, scaleset_id: int, name: str) -> None: 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( @@ -228,10 +243,10 @@ def _remove_runners(self, scaleset_id: int, name: str) -> None: ) continue if _is_running_job(instance): - # The scaleset is already disabled with min_idle_runners=0, so GARM - # launches no replacement; the runner is left to finish its job and - # is removed on a later pass, along with the scaleset. Deleting it - # here would fail the workflow job that is currently running on it. + # 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)", @@ -239,27 +254,44 @@ def _remove_runners(self, scaleset_id: int, name: str) -> None: name, ) continue - self._delete_runner(instance.name, name) - - def _delete_runner(self, instance_name: str, scaleset_name: str) -> None: - """Delete one runner, escalating to a GitHub-unauthorized bypass if needed. - - Provider errors are always ignored (``force_remove``): the scaleset is going - away, so a runner GARM can no longer reach in the provider must not block the - removal. The bypass is reserved for a 401 — the only status GARM returns for - an unauthorized forge error — because it drops the runner from the provider - and GARM's database without deregistering it in GitHub, orphaning it there. - Any other failure (a connection error, a 5xx, or a 400 for a runner that is - not yet in a deletable state) is transient, so it is left for the next - reconcile rather than escalated into a GitHub orphan. + 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 forge. + + Both escalations 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 the instance is already sitting in + a pending-delete state — GARM accepted an earlier delete and has not managed + to carry it out. + * ``bypass_gh_unauthorized`` drops the runner from the provider and GARM's + database without deregistering it in GitHub, orphaning it there. It is + reserved for a 401 — the only status GARM returns for an unauthorized forge + error. Any other failure (a connection error, a 5xx, or a 400 for a runner + that is not yet in a deletable state) is transient, so it is left for the + next reconcile rather than escalated into a GitHub orphan. Args: - instance_name: Name of the runner instance to delete. + instance: The runner instance to delete. scaleset_name: Name of the owning scaleset, for logging. """ - logger.info("Removing runner %s from orphaned scaleset %s", instance_name, scaleset_name) + instance_name = instance.name or "" + # A delete GARM has already accepted but not completed means the provider + # teardown is failing and backing off; forcing it is what finally lets the + # scaleset go, at the cost of leaking the instance in the cloud. + force_remove = (instance.status or "").lower() in PENDING_DELETE_STATUSES + 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=True) + self._client.delete_instance(instance_name, force_remove=force_remove) return except GarmUnauthorizedError as exc: logger.warning( @@ -270,22 +302,20 @@ def _delete_runner(self, instance_name: str, scaleset_name: str) -> None: exc, ) except GarmApiError as exc: - logger.warning( - "Could not remove runner %s (will retry on next reconcile): %s", - instance_name, - exc, - ) + self._log_deferred_runner_delete(instance_name, exc) return try: self._client.delete_instance( - instance_name, force_remove=True, bypass_gh_unauthorized=True + instance_name, force_remove=force_remove, bypass_gh_unauthorized=True ) except GarmApiError as exc: - logger.warning( - "Could not remove runner %s (will retry on next reconcile): %s", - instance_name, - exc, - ) + self._log_deferred_runner_delete(instance_name, exc) + + @staticmethod + def _log_deferred_runner_delete(instance_name: str, exc: GarmApiError) -> None: + 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.""" @@ -564,7 +594,7 @@ def _is_running_job(instance: Instance) -> bool: if (instance.runner_status or "").lower() == RUNNER_STATUS_ACTIVE: return True job = instance.job - return job is not None and (job.status or "").lower() != JOB_STATUS_COMPLETED + return job is not None and (job.status or "").lower() in JOB_STATUSES_HOLDING_RUNNER def _effective_extra_specs(spec: ScalesetSpec) -> dict[str, object]: diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 558af00c..84a5633e 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -58,10 +58,13 @@ def __init__(self, status): class _FakeInstance: - def __init__(self, name, iid="instance-uuid", runner_status="idle", job_status=None): + def __init__( + self, name, iid="instance-uuid", runner_status="idle", job_status=None, status="running" + ): self.name = name self.id = iid self.runner_status = runner_status + self.status = status self.job = _FakeJob(job_status) if job_status is not None else None @@ -81,6 +84,7 @@ def __init__( 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 = [ @@ -107,6 +111,7 @@ def __init__( } 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] = [] @@ -131,6 +136,8 @@ 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): @@ -361,6 +368,55 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] +def test_disable_failure_does_not_block_removal(): + """ + arrange: FakeGarmClient whose update_scaleset (the disable-before-delete call) raises, for + an orphaned scaleset that still owns a runner. + act: Reconcile so the scaleset is orphaned. + assert: The runner and the scaleset are still removed. Disabling goes through the forge, so + expired credentials fail it — the very case runner removal is built to escalate past; + treating it as a precondition would strand the scaleset forever. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1"]}, + update_scaleset_error=GarmApiError("500 Server error"), + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [("runner-1", False, False)] + assert client.deleted == [42] + + +@pytest.mark.parametrize( + "status, expected_force", + [ + ("running", False), + ("pending_delete", True), + ("deleting", True), + ], + ids=["running-not-forced", "pending-delete-forced", "deleting-forced"], +) +def test_force_remove_only_after_a_delete_is_already_stuck(status, expected_force): + """ + arrange: FakeGarmClient with an orphaned scaleset owning a runner in a given instance state. + act: Reconcile so the scaleset is orphaned. + assert: force_remove is set only once GARM has already accepted a delete it could not carry + out. Forcing makes GARM drop the runner from its database even when the provider teardown + fails, leaking a live cloud instance nothing points at, so a first attempt is left plain + and retried with a backoff 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 == [("runner-1", expected_force, False)] + + def test_delete_orphaned_scaleset_removes_runners_first(): """ arrange: FakeGarmClient with an orphaned scaleset that still owns two runners. @@ -377,8 +433,8 @@ def test_delete_orphaned_scaleset_removes_runners_first(): _reconcile(client, [_spec(name="new-scaleset")]) assert client.deleted_instances == [ - ("runner-1", True, False), - ("runner-2", True, False), + ("runner-1", False, False), + ("runner-2", False, False), ] assert client.deleted == [42] @@ -399,8 +455,8 @@ def test_runner_removal_falls_back_to_github_unauthorized_bypass(): _reconcile(client, [_spec(name="new-scaleset")]) assert client.deleted_instances == [ - ("runner-1", True, False), - ("runner-1", True, True), + ("runner-1", False, False), + ("runner-1", False, True), ] assert client.deleted == [42] @@ -450,7 +506,7 @@ def test_runner_removal_does_not_bypass_github_on_non_401_errors(error): ) _reconcile(client, [_spec(name="new-scaleset")]) - assert client.deleted_instances == [("runner-1", True, False)] + assert client.deleted_instances == [("runner-1", False, False)] @pytest.mark.parametrize( @@ -486,15 +542,26 @@ def test_runner_running_a_job_is_left_alone(instance_kwargs): {"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", ], - ids=["idle", "job-completed", "failed", "unknown-status"], ) 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 force-removed, so the busy-runner guard does not stall the cleanup - for runners that are safe to delete. + 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"], @@ -503,7 +570,7 @@ def test_idle_runner_is_removed(instance_kwargs): ) _reconcile(client, [_spec(name="new-scaleset")]) - assert client.deleted_instances == [("runner-1", True, False)] + assert client.deleted_instances == [("runner-1", False, False)] def test_unnamed_runner_is_skipped(): diff --git a/docs/changelog.md b/docs/changelog.md index 0e12965e..e742b48c 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,7 +10,7 @@ Each revision is versioned by the date of the revision. ## 2026-08-21 -- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first, ignoring provider errors so an instance GARM can no longer reach in the provider does not block the cleanup. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. +- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is already sitting on a delete it has accepted but could not carry out, so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. ## 2026-07-13 From ba7d0ae801a13b1450dde680c93f48d537bcf007 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Mon, 24 Aug 2026 16:22:45 +0800 Subject: [PATCH 03/10] fix(garm): close three gaps in the scaleset drain escalations A forced delete leaves the instance in "pending_force_delete" until the provider worker picks it up, which the pending-delete set did not cover: the next pass read it as a fresh instance and issued a plain delete, downgrading an escalation already in flight so a stuck instance could never clear. A failed disable is now split by cause. Continuing unconditionally was only justified for a forge rejection -- GARM cannot launch replacements when it cannot reach the forge either -- but a GARM-side failure leaves the scaleset enabled and still sized up, so removing its runners just had GARM re-provision them, churning instances on every pass. Runner removal now goes ahead only on an unauthorized disable; any other failure leaves the runners alone and still attempts the scaleset delete, so an already empty scaleset is not stranded either. update_scaleset classifies a 401 into GarmUnauthorizedError to make that distinction available. A job record now only protects its runner while it could still describe a live job. GARM reconciles stale queued jobs against the forge but not in-progress ones, so a dropped completion webhook pinned the runner as busy forever and stranded the scaleset -- the same symptom this branch exists to fix. Records older than GitHub's six-hour job ceiling are read as stale, and a naive timestamp is read as UTC rather than raising. Also corrects a test docstring that described the opposite of the force-remove policy its assertion checks. Co-Authored-By: Claude Opus 5 --- charms/garm/src/garm_api.py | 10 +- charms/garm/src/scaleset_reconciler.py | 105 +++++++++++--- .../tests/unit/test_scaleset_reconciler.py | 132 ++++++++++++++++-- docs/changelog.md | 2 +- 4 files changed, 208 insertions(+), 41 deletions(-) diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index 57a16d1b..1c002f0c 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -866,7 +866,8 @@ def update_scaleset(self, scaleset_id: int, params: UpdateScaleSetParams) -> Sca Updated ScaleSet model object. Raises: - GarmApiError: On API error. + GarmUnauthorizedError: If GARM answers 401 (expired forge credentials). + GarmApiError: On any other API error. """ with self._api_client() as client: try: @@ -876,9 +877,10 @@ def update_scaleset(self, scaleset_id: int, params: UpdateScaleSetParams) -> Sca _request_timeout=_REQUEST_TIMEOUT, ) except ApiException as exc: - raise GarmApiError( - f"Failed to update scaleset {scaleset_id} ({exc.status}): {exc.body}" - ) from exc + message = f"Failed to update scaleset {scaleset_id} ({exc.status}): {exc.body}" + if exc.status == 401: + raise GarmUnauthorizedError(message) from exc + raise GarmApiError(message) from exc except urllib3.exceptions.HTTPError as exc: raise GarmConnectionError(f"GARM connection error: {exc}") from exc diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 08bf714f..d8525a21 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -7,6 +7,7 @@ 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, GarmUnauthorizedError @@ -37,8 +38,15 @@ JOB_STATUSES_HOLDING_RUNNER = frozenset({"queued", "in_progress"}) # GARM instance statuses meaning a delete was accepted but has not completed: -# the provider teardown is failing and being retried with a backoff. -PENDING_DELETE_STATUSES = frozenset({"pending_delete", "deleting"}) +# the provider teardown is failing and being retried with a backoff. The forced +# variant belongs here too — dropping it would downgrade an escalation already in +# flight back to a plain delete, so a stuck instance could never clear. +PENDING_DELETE_STATUSES = frozenset({"pending_delete", "pending_force_delete", "deleting"}) + +# GitHub terminates a workflow job at 6 hours, so 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. +MAX_JOB_RUNTIME = timedelta(hours=6) @dataclass @@ -188,22 +196,10 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset.id) if scaleset.id is None: return - try: - # Disable the scaleset first so GARM stops launching new runners - # while its existing ones are being removed. - self._client.update_scaleset( - scaleset.id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) - ) - except GarmApiError as exc: - # Carry on rather than deferring the whole removal: disabling goes - # through the forge, so expired credentials fail here — the very case - # the runner removal below is built to escalate past. Leaving the - # scaleset enabled only risks GARM replacing a removed runner for one - # pass, and it cannot even do that when the forge is what is broken. - logger.warning("Could not disable scaleset %s before delete: %s", name, exc) - # GARM returns 400 while the scaleset still owns runners, so the runners - # have to go first; anything left behind is retried on the next pass. - self._remove_runners(scaleset.id, name) + if self._disable(scaleset.id, name): + # GARM returns 400 while the scaleset still owns runners, so the runners + # have to go first; anything left behind is retried on the next pass. + self._remove_runners(scaleset.id, name) try: self._client.delete_scaleset(scaleset.id) except GarmApiError as exc: @@ -217,6 +213,45 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: 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 removing its runners should go ahead. A failure here is only + survivable when the forge is what rejected it: GARM cannot replace the + runners about to be removed if it cannot reach the forge either, and + that is the very case the removal below is built to escalate past. Any + other failure leaves a scaleset that is still enabled and still sized + up, so removing its runners would just have GARM launch replacements — + churning instances on every pass instead of draining. + """ + try: + self._client.update_scaleset( + scaleset_id, UpdateScaleSetParams(enabled=False, min_idle_runners=0) + ) + return True + except GarmUnauthorizedError as exc: + logger.warning( + "Could not disable scaleset %s before delete: the forge rejected the request" + " as unauthorized. Removing its runners anyway, since GARM cannot launch" + " replacements while the forge is unreachable: %s", + name, + exc, + ) + return True + except GarmApiError as exc: + logger.warning( + "Could not disable scaleset %s; leaving its runners in place so GARM does not" + " replace them while it 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. @@ -588,13 +623,39 @@ def _is_running_job(instance: Instance) -> bool: 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. The job field - # is a second signal: the list endpoint may report an assigned job before the - # status catches up, and a completed job no longer holds the runner. + # "active" is GARM's runner status for a runner executing a job. GARM derives it + # from the forge'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 the forge 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 - return job is not None and (job.status or "").lower() in JOB_STATUSES_HOLDING_RUNNER + 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_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]: diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 84a5633e..5a8b04eb 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -4,6 +4,7 @@ """Unit tests for the scaleset reconciler.""" import base64 +from datetime import datetime, timedelta, timezone import pytest @@ -53,19 +54,26 @@ def __init__( class _FakeJob: - def __init__(self, status): + 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" + self, + name, + iid="instance-uuid", + runner_status="idle", + job_status=None, + status="running", + job_updated_at=None, ): self.name = name self.id = iid self.runner_status = runner_status self.status = status - self.job = _FakeJob(job_status) if job_status is not None else None + self.job = _FakeJob(job_status, job_updated_at) if job_status is not None else None class FakeGarmClient: @@ -368,20 +376,21 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] -def test_disable_failure_does_not_block_removal(): +def test_unauthorized_disable_failure_does_not_block_removal(): """ - arrange: FakeGarmClient whose update_scaleset (the disable-before-delete call) raises, for - an orphaned scaleset that still owns a runner. + arrange: FakeGarmClient whose update_scaleset (the disable-before-delete call) fails as + unauthorized, for an orphaned scaleset that still owns a runner. act: Reconcile so the scaleset is orphaned. assert: The runner and the scaleset are still removed. Disabling goes through the forge, so - expired credentials fail it — the very case runner removal is built to escalate past; - treating it as a precondition would strand the scaleset forever. + expired credentials fail it — the very case runner removal is built to escalate past, + and GARM cannot launch replacements while the forge is unreachable either. Treating it + as a precondition would strand the scaleset forever. """ client = FakeGarmClient( providers=["openstack-demo"], scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], instances={42: ["runner-1"]}, - update_scaleset_error=GarmApiError("500 Server error"), + update_scaleset_error=GarmUnauthorizedError("401 Unauthorized"), ) _reconcile(client, [_spec(name="new-scaleset")]) @@ -389,14 +398,42 @@ def test_disable_failure_does_not_block_removal(): assert client.deleted == [42] +def test_garm_side_disable_failure_leaves_runners_in_place(): + """ + arrange: FakeGarmClient whose update_scaleset fails with a GARM-side error, for an orphaned + scaleset that still owns a runner. + act: Reconcile so the scaleset is orphaned. + assert: The runner is left alone, since the scaleset is still enabled and sized up and GARM + would simply launch a replacement — churning instances on every pass instead of + draining. The scaleset delete is still attempted, so an already-empty scaleset is not + stranded by a failing disable. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: ["runner-1"]}, + update_scaleset_error=GarmApiError("500 Server error"), + ) + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [] + assert client.deleted == [42] + + @pytest.mark.parametrize( "status, expected_force", [ ("running", False), ("pending_delete", True), + ("pending_force_delete", True), ("deleting", True), ], - ids=["running-not-forced", "pending-delete-forced", "deleting-forced"], + ids=[ + "running-not-forced", + "pending-delete-forced", + "pending-force-delete-forced", + "deleting-forced", + ], ) def test_force_remove_only_after_a_delete_is_already_stuck(status, expected_force): """ @@ -405,7 +442,8 @@ def test_force_remove_only_after_a_delete_is_already_stuck(status, expected_forc assert: force_remove is set only once GARM has already accepted a delete it could not carry out. Forcing makes GARM drop the runner from its database even when the provider teardown fails, leaking a live cloud instance nothing points at, so a first attempt is left plain - and retried with a backoff instead. + and retried with a backoff instead. An already-forced delete keeps its force, so an + escalation in flight is never downgraded back to a plain delete. """ client = FakeGarmClient( providers=["openstack-demo"], @@ -421,9 +459,10 @@ 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 force-removed before the scaleset is deleted, so GARM does not - reject the delete for still owning runners; the GitHub bypass is not used while the - plain removal succeeds. + 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"], @@ -535,6 +574,71 @@ def test_runner_running_a_job_is_left_alone(instance_kwargs): assert client.deleted_instances == [] +@pytest.mark.parametrize( + "age, expect_removed", + [ + (timedelta(minutes=30), False), + (timedelta(hours=5, minutes=55), False), + (timedelta(hours=7), True), + ], + ids=["recent-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. + GitHub terminates a job at six hours and GARM only reconciles stale queued jobs against + the forge, so 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", [ diff --git a/docs/changelog.md b/docs/changelog.md index e742b48c..ee1ff86d 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,7 +10,7 @@ Each revision is versioned by the date of the revision. ## 2026-08-21 -- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is already sitting on a delete it has accepted but could not carry out, so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. +- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is already sitting on a delete it has accepted but could not carry out, so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the six hours GitHub allows a job to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. ## 2026-07-13 From d4ed3f00b901441476a7e5e3ad9e804c6765d986 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Mon, 24 Aug 2026 16:39:11 +0800 Subject: [PATCH 04/10] fix(garm): base the force escalation on a recorded provider fault The force escalation keyed off the instance's delete-pending status alone, but that is also what a healthy teardown looks like while it runs, and a cloud instance takes minutes to disappear. A reconcile landing in that window escalated a normal in-flight delete, turning a retryable failure into a leaked instance -- the outcome the plain-delete-first design exists to avoid. GARM records the provider's error against the runner when a teardown fails, so the escalation now requires both a delete-pending status and that fault. The stale-job ceiling was the GitHub-hosted job limit, but GARM provisions self-hosted runners, which GitHub allows to run for five days. A legitimate job past six hours whose runner status had not caught up was classified stale and its runner deleted mid-job. The bound is now the real ceiling: overshooting leaves a scaleset around longer, undershooting fails someone's running job. An unreachable forge no longer stalls silently. GARM learns what a runner is doing from the forge, so expired credentials freeze a mid-job runner as busy on every pass and the scaleset can never drain. The runner's own job reports to GitHub with its registration token rather than GARM's credential, so it may still be working and deleting it on state GARM cannot confirm would fail a live job -- the cleanup now names the stuck scaleset and points at the credentials instead of retrying in silence. Co-Authored-By: Claude Opus 5 --- charms/garm/src/scaleset_reconciler.py | 97 +++++++++++++------ .../tests/unit/test_scaleset_reconciler.py | 88 ++++++++++++----- docs/changelog.md | 2 +- 3 files changed, 136 insertions(+), 51 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index d8525a21..4e1f49a9 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -43,10 +43,13 @@ # flight back to a plain delete, so a stuck instance could never clear. PENDING_DELETE_STATUSES = frozenset({"pending_delete", "pending_force_delete", "deleting"}) -# GitHub terminates a workflow job at 6 hours, so 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. -MAX_JOB_RUNTIME = timedelta(hours=6) +# 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 @@ -196,10 +199,11 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset.id) if scaleset.id is None: return - if self._disable(scaleset.id, name): + forge_reachable = self._disable(scaleset.id, name) + if forge_reachable is not None: # GARM returns 400 while the scaleset still owns runners, so the runners # have to go first; anything left behind is retried on the next pass. - self._remove_runners(scaleset.id, name) + self._remove_runners(scaleset.id, name, forge_reachable=forge_reachable) try: self._client.delete_scaleset(scaleset.id) except GarmApiError as exc: @@ -213,7 +217,7 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: exc, ) - def _disable(self, scaleset_id: int, name: str) -> bool: + def _disable(self, scaleset_id: int, name: str) -> bool | None: """Stop a scaleset launching runners, before its existing ones are removed. Args: @@ -221,13 +225,16 @@ def _disable(self, scaleset_id: int, name: str) -> bool: name: Name of the scaleset being deleted, for logging. Returns: - Whether removing its runners should go ahead. A failure here is only - survivable when the forge is what rejected it: GARM cannot replace the - runners about to be removed if it cannot reach the forge either, and - that is the very case the removal below is built to escalate past. Any - other failure leaves a scaleset that is still enabled and still sized - up, so removing its runners would just have GARM launch replacements — - churning instances on every pass instead of draining. + None when removing the scaleset's runners should be skipped this pass, + otherwise whether the forge answered — which decides how far the runner + state GARM reports can be trusted. + + A failure here is only survivable when the forge is what rejected it: + GARM cannot replace the runners about to be removed if it cannot reach + the forge either, and that is the very case the removal is built to + escalate past. Any other failure leaves a scaleset that is still enabled + and still sized up, so removing its runners would just have GARM launch + replacements — churning instances on every pass instead of draining. """ try: self._client.update_scaleset( @@ -242,7 +249,7 @@ def _disable(self, scaleset_id: int, name: str) -> bool: name, exc, ) - return True + return False except GarmApiError as exc: logger.warning( "Could not disable scaleset %s; leaving its runners in place so GARM does not" @@ -250,14 +257,17 @@ def _disable(self, scaleset_id: int, name: str) -> bool: name, exc, ) - return False + return None - def _remove_runners(self, scaleset_id: int, name: str) -> None: + def _remove_runners(self, scaleset_id: int, name: str, forge_reachable: bool) -> 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. + forge_reachable: Whether the forge answered when the scaleset was + disabled. When it did not, GARM cannot refresh what its runners are + doing, so a runner that looks busy may simply be frozen that way. """ try: instances = self._client.list_scaleset_instances(scaleset_id) @@ -282,12 +292,28 @@ def _remove_runners(self, scaleset_id: int, name: str) -> None: # 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, - ) + if forge_reachable: + logger.info( + "Leaving runner %s of scaleset %s in place: still running a job" + " (will retry on the next reconcile)", + instance.name, + name, + ) + else: + # GARM learns what a runner is doing from the forge, so with the + # forge unreachable this runner stays "busy" on every pass and the + # scaleset can never be drained. The runner's own job reports to + # GitHub with its registration token rather than GARM's credential, + # so it may well still be working — deleting it on a state GARM + # cannot confirm risks failing a live job. Say what is stuck and + # why instead, since restoring the credentials is what clears it. + logger.warning( + "Cannot drain scaleset %s: runner %s last reported running a job and" + " the forge is unreachable, so GARM cannot confirm whether it still" + " is. The scaleset stays until its credentials are valid again.", + name, + instance.name, + ) continue self._delete_runner(instance, name) @@ -315,10 +341,7 @@ def _delete_runner(self, instance: Instance, scaleset_name: str) -> None: scaleset_name: Name of the owning scaleset, for logging. """ instance_name = instance.name or "" - # A delete GARM has already accepted but not completed means the provider - # teardown is failing and backing off; forcing it is what finally lets the - # scaleset go, at the cost of leaking the instance in the cloud. - force_remove = (instance.status or "").lower() in PENDING_DELETE_STATUSES + force_remove = _is_delete_stuck(instance) logger.info( "Removing runner %s from orphaned scaleset %s (force=%s)", instance_name, @@ -638,6 +661,26 @@ def _is_running_job(instance: Instance) -> bool: 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. Both halves are needed: 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. + """ + if (instance.status or "").lower() not in PENDING_DELETE_STATUSES: + return False + 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. diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 5a8b04eb..c2dcfbc9 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -4,6 +4,7 @@ """Unit tests for the scaleset reconciler.""" import base64 +import logging from datetime import datetime, timedelta, timezone import pytest @@ -68,11 +69,13 @@ def __init__( 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 @@ -420,35 +423,66 @@ def test_garm_side_disable_failure_leaves_runners_in_place(): assert client.deleted == [42] +def test_busy_runner_survives_an_unreachable_forge(caplog): + """ + arrange: FakeGarmClient whose disable fails as unauthorized, for an orphaned scaleset owning + a runner that last reported running a job. + act: Reconcile so the scaleset is orphaned. + assert: The runner is not deleted and the stall is reported. GARM learns what a runner is + doing from the forge, so with the forge unreachable that runner reads as busy on every + pass — but its job reports to GitHub with its own registration token, not GARM's + credential, so it may still be working and deleting it would fail a live job. The + warning names the scaleset and the remedy instead of stalling silently. + """ + client = FakeGarmClient( + providers=["openstack-demo"], + scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], + instances={42: [{"name": "runner-1", "runner_status": "active"}]}, + update_scaleset_error=GarmUnauthorizedError("401 Unauthorized"), + ) + with caplog.at_level(logging.WARNING): + _reconcile(client, [_spec(name="new-scaleset")]) + + assert client.deleted_instances == [] + assert "Cannot drain scaleset stale-scaleset" in caplog.text + assert "credentials" in caplog.text + + @pytest.mark.parametrize( - "status, expected_force", + "status, provider_fault, expected_force", [ - ("running", False), - ("pending_delete", True), - ("pending_force_delete", True), - ("deleting", True), + ("running", None, False), + ("pending_delete", None, False), + ("deleting", None, False), + ("pending_delete", b"nova: instance not found", True), + ("pending_force_delete", b"nova: instance not found", True), + ("deleting", b"nova: instance not found", True), ], ids=[ - "running-not-forced", - "pending-delete-forced", - "pending-force-delete-forced", - "deleting-forced", + "healthy-runner-not-forced", + "pending-delete-in-flight-not-forced", + "deleting-in-flight-not-forced", + "pending-delete-with-fault-forced", + "pending-force-delete-with-fault-forced", + "deleting-with-fault-forced", ], ) -def test_force_remove_only_after_a_delete_is_already_stuck(status, expected_force): +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 instance state. + 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 GARM has already accepted a delete it could not carry - out. Forcing makes GARM drop the runner from its database even when the provider teardown - fails, leaking a live cloud instance nothing points at, so a first attempt is left plain - and retried with a backoff instead. An already-forced delete keeps its force, so an - escalation in flight is never downgraded back to a plain delete. + 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. """ client = FakeGarmClient( providers=["openstack-demo"], scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], - instances={42: [{"name": "runner-1", "status": status}]}, + instances={42: [{"name": "runner-1", "status": status, "provider_fault": provider_fault}]}, ) _reconcile(client, [_spec(name="new-scaleset")]) @@ -578,10 +612,16 @@ def test_runner_running_a_job_is_left_alone(instance_kwargs): "age, expect_removed", [ (timedelta(minutes=30), False), - (timedelta(hours=5, minutes=55), False), - (timedelta(hours=7), True), + (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", ], - ids=["recent-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): """ @@ -589,9 +629,11 @@ def test_a_job_only_protects_its_runner_while_it_could_still_be_running(age, exp 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. - GitHub terminates a job at six hours and GARM only reconciles stale queued jobs against - the forge, so a dropped completion webhook would otherwise pin the runner as busy - forever and strand the scaleset — the very failure this cleanup exists to fix. + 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"], diff --git a/docs/changelog.md b/docs/changelog.md index ee1ff86d..1f30f5b3 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,7 +10,7 @@ Each revision is versioned by the date of the revision. ## 2026-08-21 -- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is already sitting on a delete it has accepted but could not carry out, so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the six hours GitHub allows a job to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. +- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is sitting on a delete it has accepted and the provider has recorded a fault carrying out — not merely one still in flight, which is what a healthy teardown looks like too — so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the five days GitHub allows a job on a self-hosted runner to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. ## 2026-07-13 From 51d3c5283b0aa9ac3ce0ca4c541ffe26bf9ec003 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Mon, 24 Aug 2026 16:50:57 +0800 Subject: [PATCH 05/10] docs(changelog): correct and complete the scaleset drain entry The heading was dated 2026-08-21 but every commit on this branch is dated 2026-08-24, which the file's own "versioned by the date of the revision" convention asks for. Fixes a typo in the force-escalation clause, and documents two user-visible behaviours the entry had not caught up with: what happens when disabling the scaleset itself fails (an unauthorized rejection still drains, anything else defers), and the case the charm cannot resolve on its own -- an unreachable forge while a runner is mid-job, where restoring credentials is the remedy. Co-Authored-By: Claude Opus 5 --- docs/changelog.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 1f30f5b3..76a18a96 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -8,9 +8,9 @@ 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-21 +## 2026-08-24 -- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is sitting on a delete it has accepted and the provider has recorded a fault carrying out — not merely one still in flight, which is what a healthy teardown looks like too — so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the five days GitHub allows a job on a self-hosted runner to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. +- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is sitting on a delete it has accepted and the provider has recorded a fault carrying it out — not merely one still in flight, which is what a healthy teardown looks like too — so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the five days GitHub allows a job on a self-hosted runner to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. Disabling the scaleset before the drain can itself fail: when the forge rejects it as unauthorized the drain still goes ahead, since GARM cannot launch replacement runners while the forge is unreachable either, but any other failure leaves the runners in place for the next reconcile rather than having GARM immediately replace them. One case the charm cannot resolve on its own: if the forge is unreachable while a runner is mid-job, GARM cannot refresh what that runner is doing, so it reads as busy on every pass and the scaleset is not removed. The charm logs a warning naming the scaleset rather than retrying silently — restoring valid credentials is what clears it. ## 2026-07-13 From 30cfeee8c68040acf64941bd8b5eec35f0c72e39 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Wed, 26 Aug 2026 08:32:14 +0800 Subject: [PATCH 06/10] fix(garm): keep the force escalation on an already-forced runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _is_delete_stuck required a recorded provider fault before escalating to a forced removal, so a runner GARM had already parked in pending_force_delete but with no readable fault fell back to a plain delete. That downgrades an escalation already in flight — whether from an earlier reconcile or applied by an operator — and the runner can never clear. Treat pending_force_delete as stuck on its own: the escalation has already been applied, so the fault that justified it need not still be readable, and re-forcing leaks nothing that is not already forfeit. The other two pending-delete statuses still require a fault, since forcing those would turn a retryable teardown into a leaked instance. Co-Authored-By: Claude Opus 5 --- charms/garm/src/scaleset_reconciler.py | 24 +++++++++++++++---- .../tests/unit/test_scaleset_reconciler.py | 6 ++++- docs/changelog.md | 1 + 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index ea324adc..07110975 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -37,12 +37,16 @@ RUNNER_STATUS_ACTIVE = "active" JOB_STATUSES_HOLDING_RUNNER = frozenset({"queued", "in_progress"}) -# GARM instance statuses meaning a delete was accepted but has not completed: -# the provider teardown is failing and being retried with a backoff. The forced -# variant belongs here too — dropping it would downgrade an escalation already in -# flight back to a plain delete, so a stuck instance could never clear. +# 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". PENDING_DELETE_STATUSES = frozenset({"pending_delete", "pending_force_delete", "deleting"}) +# 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. +FORCED_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) @@ -675,9 +679,19 @@ def _is_delete_stuck(instance: Instance) -> bool: 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. + + A runner already parked in the forced-delete status is the exception: the + escalation has been applied to it, whether by an earlier pass or by an + operator, so the fault that justified it need not still be readable here. + Sending a plain delete for it would downgrade that escalation and leave the + runner stuck for good, and re-forcing leaks nothing that is not already + forfeit. """ - if (instance.status or "").lower() not in PENDING_DELETE_STATUSES: + status = (instance.status or "").lower() + if status not in PENDING_DELETE_STATUSES: return False + if status == FORCED_DELETE_STATUS: + return True return bool(instance.provider_fault) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 692085c0..e642345f 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -457,6 +457,7 @@ def test_busy_runner_survives_an_unreachable_forge(caplog): ("pending_delete", b"nova: instance not found", True), ("pending_force_delete", b"nova: instance not found", True), ("deleting", b"nova: instance not found", True), + ("pending_force_delete", None, True), ], ids=[ "healthy-runner-not-forced", @@ -465,6 +466,7 @@ def test_busy_runner_survives_an_unreachable_forge(caplog): "pending-delete-with-fault-forced", "pending-force-delete-with-fault-forced", "deleting-with-fault-forced", + "pending-force-delete-without-fault-stays-forced", ], ) def test_force_remove_only_once_the_provider_has_refused_a_delete( @@ -477,7 +479,9 @@ def test_force_remove_only_once_the_provider_has_refused_a_delete( 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. + 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"], diff --git a/docs/changelog.md b/docs/changelog.md index eb5c7e92..f54556a6 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -11,6 +11,7 @@ Each revision is versioned by the date of the revision. ## 2026-08-24 - `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is sitting on a delete it has accepted and the provider has recorded a fault carrying it out — not merely one still in flight, which is what a healthy teardown looks like too — so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the five days GitHub allows a job on a self-hosted runner to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. Disabling the scaleset before the drain can itself fail: when the forge rejects it as unauthorized the drain still goes ahead, since GARM cannot launch replacement runners while the forge is unreachable either, but any other failure leaves the runners in place for the next reconcile rather than having GARM immediately replace them. One case the charm cannot resolve on its own: if the forge is unreachable while a runner is mid-job, GARM cannot refresh what that runner is doing, so it reads as busy on every pass and the scaleset is not removed. The charm logs a warning naming the scaleset rather than retrying silently — restoring valid credentials is what clears it. + ## 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. From 3c703f1ea1eb814fb04c91b54ce7300062a90989 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Thu, 27 Aug 2026 09:50:58 +0800 Subject: [PATCH 07/10] fix(garm): drop the unreachable-GitHub branch from the scaleset drain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review against GARM at the pinned commit (47811d0) settled three claims the branch had inferred rather than checked, and each one removes code. The disable never reaches GitHub. UpdateScaleSetByID only calls the forge from its update callback, and only when the name, runner group or update setting changes; enabled and min_idle_runners do not trigger it, and neither the github.Client() construction nor its RateLimit probe propagates an error. So the unauthorized branch was unreachable, and with it the bool | None tri-state, the forge_reachable parameter, and the warn-and-stall path for a runner frozen mid-job. A failed disable now defers the whole cleanup: DeleteScaleSetByID rejects an enabled scaleset as well as one that still owns runners, so the delete that used to be attempted anyway could only 400. A delete is only issued for a status GARM accepts. DeleteRunner takes running, error, pending_delete and pending_force_delete and 400s on everything else, so a runner still being created, or already being torn down, was retried into the same error on every pass. "deleting" also leaves the pending-delete set: the provider worker sets it with a nil fault immediately before the teardown and records the fault only on dropping back to pending_delete, so the two can never coexist and the case the parametrize covered was unproducible. Names GitHub rather than "forge" in the new code, and writes down what a 401 actually pins down: GARM's own JWT and admin checks answer it too, and its scaleset client folds 403 into the same error, so a secondary rate limit or SSO enforcement reaches the unauthorized bypass and orphans the runner in GitHub. That trade is now stated as it is rather than as "the only status GARM returns for an unauthorized forge error". Adds the integration coverage the branch was missing — renaming the desired scaleset makes the charm drain and delete the orphan against a live GARM, the sequence the test teardown otherwise hand-rolls — moves the docstring rationale that argued against alternatives no caller can select into the bodies it defends, and cuts the changelog entry to the size of its neighbours. Co-Authored-By: Claude Opus 5 --- charms/garm/src/garm_api.py | 27 ++- charms/garm/src/scaleset_reconciler.py | 176 +++++++++--------- .../tests/unit/test_scaleset_reconciler.py | 101 ++++------ charms/tests/integration/test_garm.py | 90 ++++++++- docs/changelog.md | 2 +- 5 files changed, 236 insertions(+), 160 deletions(-) diff --git a/charms/garm/src/garm_api.py b/charms/garm/src/garm_api.py index c143361a..79fcddb8 100644 --- a/charms/garm/src/garm_api.py +++ b/charms/garm/src/garm_api.py @@ -71,9 +71,17 @@ class GarmEntityNotFoundError(GarmApiError): class GarmUnauthorizedError(GarmApiError): """Raised when GARM answers a request with 401 Unauthorized. - GARM's error handler returns 401 only for an unauthorized error, so this - distinguishes expired forge credentials from a transport failure or a generic - 500, neither of which may be treated as an authorization problem. + 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. """ @@ -914,7 +922,9 @@ def update_scaleset(self, scaleset_id: int, params: UpdateScaleSetParams) -> Sca Updated ScaleSet model object. Raises: - GarmUnauthorizedError: If GARM answers 401 (expired forge credentials). + 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: @@ -1012,10 +1022,11 @@ def _raise_resource_api_error(message: str, exc: ApiException) -> NoReturn: 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. GARM returns it only for an - unauthorized forge error, so it marks expired credentials specifically - rather than a transport failure or a generic 500 — the distinction the - runner-removal escalation relies on before it will bypass the forge. + 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: diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 07110975..823bb042 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -37,10 +37,18 @@ RUNNER_STATUS_ACTIVE = "active" JOB_STATUSES_HOLDING_RUNNER = frozenset({"queued", "in_progress"}) +# The instance 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_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". -PENDING_DELETE_STATUSES = frozenset({"pending_delete", "pending_force_delete", "deleting"}) +# 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 @@ -203,11 +211,17 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: logger.info("Deleting orphaned scaleset %s (id=%s)", name, scaleset.id) if scaleset.id is None: return - forge_reachable = self._disable(scaleset.id, name) - if forge_reachable is not None: - # GARM returns 400 while the scaleset still owns runners, so the runners - # have to go first; anything left behind is retried on the next pass. - self._remove_runners(scaleset.id, name, forge_reachable=forge_reachable) + 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: self._client.delete_scaleset(scaleset.id) except GarmApiError as exc: @@ -221,7 +235,7 @@ def _delete_orphaned(self, scaleset: ScaleSet) -> None: exc, ) - def _disable(self, scaleset_id: int, name: str) -> bool | None: + def _disable(self, scaleset_id: int, name: str) -> bool: """Stop a scaleset launching runners, before its existing ones are removed. Args: @@ -229,49 +243,31 @@ def _disable(self, scaleset_id: int, name: str) -> bool | None: name: Name of the scaleset being deleted, for logging. Returns: - None when removing the scaleset's runners should be skipped this pass, - otherwise whether the forge answered — which decides how far the runner - state GARM reports can be trusted. - - A failure here is only survivable when the forge is what rejected it: - GARM cannot replace the runners about to be removed if it cannot reach - the forge either, and that is the very case the removal is built to - escalate past. Any other failure leaves a scaleset that is still enabled - and still sized up, so removing its runners would just have GARM launch - replacements — churning instances on every pass instead of draining. + 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) ) return True - except GarmUnauthorizedError as exc: - logger.warning( - "Could not disable scaleset %s before delete: the forge rejected the request" - " as unauthorized. Removing its runners anyway, since GARM cannot launch" - " replacements while the forge is unreachable: %s", - name, - exc, - ) - return False except GarmApiError as exc: logger.warning( - "Could not disable scaleset %s; leaving its runners in place so GARM does not" - " replace them while it is still enabled (will retry on next reconcile): %s", + "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 None + return False - def _remove_runners(self, scaleset_id: int, name: str, forge_reachable: bool) -> None: + 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. - forge_reachable: Whether the forge answered when the scaleset was - disabled. When it did not, GARM cannot refresh what its runners are - doing, so a runner that looks busy may simply be frozen that way. """ try: instances = self._client.list_scale_set_instances(scaleset_id) @@ -291,59 +287,48 @@ def _remove_runners(self, scaleset_id: int, name: str, forge_reachable: bool) -> "Skipping runner with missing name in scaleset %s (id=%s)", name, instance.id ) continue + if (instance.status or "").lower() not in DELETABLE_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. - if forge_reachable: - logger.info( - "Leaving runner %s of scaleset %s in place: still running a job" - " (will retry on the next reconcile)", - instance.name, - name, - ) - else: - # GARM learns what a runner is doing from the forge, so with the - # forge unreachable this runner stays "busy" on every pass and the - # scaleset can never be drained. The runner's own job reports to - # GitHub with its registration token rather than GARM's credential, - # so it may well still be working — deleting it on a state GARM - # cannot confirm risks failing a live job. Say what is stuck and - # why instead, since restoring the credentials is what clears it. - logger.warning( - "Cannot drain scaleset %s: runner %s last reported running a job and" - " the forge is unreachable, so GARM cannot confirm whether it still" - " is. The scaleset stays until its credentials are valid again.", - name, - instance.name, - ) + 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 forge. - - Both escalations 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 the instance is already sitting in - a pending-delete state — GARM accepted an earlier delete and has not managed - to carry it out. - * ``bypass_gh_unauthorized`` drops the runner from the provider and GARM's - database without deregistering it in GitHub, orphaning it there. It is - reserved for a 401 — the only status GARM returns for an unauthorized forge - error. Any other failure (a connection error, a 5xx, or a 400 for a runner - that is not yet in a deletable state) is transient, so it is left for the - next reconcile rather than escalated into a GitHub orphan. + """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( @@ -356,6 +341,12 @@ def _delete_runner(self, instance: Instance, scaleset_name: str) -> None: self._client.delete_instance(instance_name, force_remove=force_remove) 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" @@ -364,8 +355,17 @@ def _delete_runner(self, instance: Instance, scaleset_name: str) -> None: 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 @@ -651,12 +651,12 @@ def _is_running_job(instance: Instance) -> bool: 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 the forge's live view of the runner, so it corrects itself once a job ends. + # 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 the forge but not + # 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 @@ -673,25 +673,25 @@ def _is_delete_stuck(instance: Instance) -> bool: Returns: True when a delete has been accepted and the provider reported a fault - carrying it out. Both halves are needed: 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. - - A runner already parked in the forced-delete status is the exception: the - escalation has been applied to it, whether by an earlier pass or by an - operator, so the fault that justified it need not still be readable here. - Sending a plain delete for it would downgrade that escalation and leave the - runner stuck for good, and re-forcing leaks nothing that is not already - forfeit. + 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 == FORCED_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) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index e642345f..f509f0ad 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -4,7 +4,6 @@ """Unit tests for the scaleset reconciler.""" import base64 -import logging from datetime import datetime, timedelta, timezone import pytest @@ -379,93 +378,49 @@ def test_delete_orphaned_scaleset(): assert client.deleted == [42] -def test_unauthorized_disable_failure_does_not_block_removal(): - """ - arrange: FakeGarmClient whose update_scaleset (the disable-before-delete call) fails as - unauthorized, for an orphaned scaleset that still owns a runner. - act: Reconcile so the scaleset is orphaned. - assert: The runner and the scaleset are still removed. Disabling goes through the forge, so - expired credentials fail it — the very case runner removal is built to escalate past, - and GARM cannot launch replacements while the forge is unreachable either. Treating it - as a precondition would strand the scaleset forever. - """ - client = FakeGarmClient( - providers=["openstack-demo"], - scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], - instances={42: ["runner-1"]}, - update_scaleset_error=GarmUnauthorizedError("401 Unauthorized"), - ) - _reconcile(client, [_spec(name="new-scaleset")]) - - assert client.deleted_instances == [("runner-1", False, False)] - assert client.deleted == [42] - - -def test_garm_side_disable_failure_leaves_runners_in_place(): +@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 fails with a GARM-side error, for an orphaned - scaleset that still owns a runner. + 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: The runner is left alone, since the scaleset is still enabled and sized up and GARM - would simply launch a replacement — churning instances on every pass instead of - draining. The scaleset delete is still attempted, so an already-empty scaleset is not - stranded by a failing disable. + 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=GarmApiError("500 Server error"), + update_scaleset_error=error, ) _reconcile(client, [_spec(name="new-scaleset")]) assert client.deleted_instances == [] - assert client.deleted == [42] - - -def test_busy_runner_survives_an_unreachable_forge(caplog): - """ - arrange: FakeGarmClient whose disable fails as unauthorized, for an orphaned scaleset owning - a runner that last reported running a job. - act: Reconcile so the scaleset is orphaned. - assert: The runner is not deleted and the stall is reported. GARM learns what a runner is - doing from the forge, so with the forge unreachable that runner reads as busy on every - pass — but its job reports to GitHub with its own registration token, not GARM's - credential, so it may still be working and deleting it would fail a live job. The - warning names the scaleset and the remedy instead of stalling silently. - """ - client = FakeGarmClient( - providers=["openstack-demo"], - scalesets=[_existing_scaleset(name="stale-scaleset", id=42)], - instances={42: [{"name": "runner-1", "runner_status": "active"}]}, - update_scaleset_error=GarmUnauthorizedError("401 Unauthorized"), - ) - with caplog.at_level(logging.WARNING): - _reconcile(client, [_spec(name="new-scaleset")]) - - assert client.deleted_instances == [] - assert "Cannot drain scaleset stale-scaleset" in caplog.text - assert "credentials" in caplog.text + assert client.deleted == [] @pytest.mark.parametrize( "status, provider_fault, expected_force", [ ("running", None, False), + ("error", None, False), ("pending_delete", None, False), - ("deleting", None, False), ("pending_delete", b"nova: instance not found", True), ("pending_force_delete", b"nova: instance not found", True), - ("deleting", 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", - "deleting-in-flight-not-forced", "pending-delete-with-fault-forced", "pending-force-delete-with-fault-forced", - "deleting-with-fault-forced", "pending-force-delete-without-fault-stays-forced", ], ) @@ -723,6 +678,30 @@ def test_idle_runner_is_removed(instance_kwargs): assert client.deleted_instances == [("runner-1", False, False)] +@pytest.mark.parametrize( + "status", + ["pending_create", "creating", "deleting", "deleted", "stopped", "unknown"], +) +def test_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. diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index 8644f5e4..e7adb842 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -451,6 +451,89 @@ 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_drains_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, and again while it + still owns runners, so this covers the charm's own disable-drain-delete against a live + GARM — the sequence the teardown helper below otherwise hand-rolls. + """ + 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_reconcile(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_reconcile(juju, configurator_garm, configurator_with_image) + + _wait_for_scaleset_absent(base_url, token, renamed) + _wait_for_scaleset(base_url, token, _SCALESET_TEST_NAME) + + +def _wait_for_reconcile( + 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, + ) + + +@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 +788,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_drains_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}, diff --git a/docs/changelog.md b/docs/changelog.md index f54556a6..e8773705 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -10,7 +10,7 @@ Each revision is versioned by the date of the revision. ## 2026-08-24 -- `garm`: remove a scaleset's runners before deleting the scaleset. Removing every `garm-configurator` relation disabled and deleted the orphaned scalesets, but GARM rejects deleting a scaleset that still owns runners, so a scaleset with active runners was never removed and its runners were left behind. The charm now removes each runner of an orphaned scaleset first. A plain removal is tried first, so GARM keeps retrying a failing provider teardown rather than dropping the runner from its database and leaving the instance running in the cloud with no record of it; the removal is only forced once GARM is sitting on a delete it has accepted and the provider has recorded a fault carrying it out — not merely one still in flight, which is what a healthy teardown looks like too — so a permanently unreachable instance still cannot block the cleanup forever. A runner that is currently executing a workflow job is left alone and removed on a later reconcile once the job finishes, so an in-flight job is never failed by the cleanup; a job record older than the five days GitHub allows a job on a self-hosted runner to run is treated as stale rather than as a runner still working, so a dropped completion webhook cannot strand the scaleset. If GARM reports that the forge rejected the removal as unauthorized (expired credentials), the charm retries with GARM's GitHub Unauthorized bypass, which drops the runner from the provider and GARM's database — note that this can leave the runner registered in GitHub, where it must be removed manually. Any other failure is retried on the next reconcile instead, so a transient error never causes a runner to be orphaned in GitHub. Disabling the scaleset before the drain can itself fail: when the forge rejects it as unauthorized the drain still goes ahead, since GARM cannot launch replacement runners while the forge is unreachable either, but any other failure leaves the runners in place for the next reconcile rather than having GARM immediately replace them. One case the charm cannot resolve on its own: if the forge is unreachable while a runner is mid-job, GARM cannot refresh what that runner is doing, so it reads as busy on every pass and the scaleset is not removed. The charm logs a warning naming the scaleset rather than retrying silently — restoring valid credentials is what clears it. +- `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 From 7237ae10edc97e5a6bc4192abf5347ab17f78b44 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Thu, 27 Aug 2026 10:07:53 +0800 Subject: [PATCH 08/10] refactor(garm): align the new names with the vocabulary around them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DELETABLE_STATUSES said "statuses" where the file names both scaleset and runner state, and where the sibling guard for the same GARM constraint — resource_cleanup._DELETABLE_RUNNER_STATES — already says "runner". It is now DELETABLE_RUNNER_STATUSES. (The two sets still disagree on content, which is a real gap in resource_cleanup rather than a naming one: it omits pending_delete and pending_force_delete, both of which DeleteRunner accepts.) FORCED_DELETE_STATUS dropped the "pending" its own value carries, sitting directly beneath PENDING_DELETE_STATUSES, which keeps it. Renamed to PENDING_FORCE_DELETE_STATUS so the pair reads as the pair it is. The integration helper introduced with the drain test was a third copy of a wait its two neighbours had already inlined, under a name — _wait_for_reconcile — that described neither what it waits on nor the _wait_for_ shape of every other helper in the file. It is now _wait_for_config_applied, used at all three call sites, and sits below the first of them. Also adds the article the surrounding test names carry. Co-Authored-By: Claude Opus 5 --- charms/garm/src/scaleset_reconciler.py | 18 +++--- .../tests/unit/test_scaleset_reconciler.py | 2 +- charms/tests/integration/test_garm.py | 60 ++++++++----------- 3 files changed, 36 insertions(+), 44 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 823bb042..e7d00dac 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -37,11 +37,13 @@ RUNNER_STATUS_ACTIVE = "active" JOB_STATUSES_HOLDING_RUNNER = frozenset({"queued", "in_progress"}) -# The instance 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_STATUSES = frozenset({"running", "error", "pending_delete", "pending_force_delete"}) +# 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 @@ -53,7 +55,7 @@ # 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. -FORCED_DELETE_STATUS = "pending_force_delete" +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 @@ -287,7 +289,7 @@ def _remove_runners(self, scaleset_id: int, name: str) -> None: "Skipping runner with missing name in scaleset %s (id=%s)", name, instance.id ) continue - if (instance.status or "").lower() not in DELETABLE_STATUSES: + 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, @@ -684,7 +686,7 @@ def _is_delete_stuck(instance: Instance) -> bool: # 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 == FORCED_DELETE_STATUS: + 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 diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index f509f0ad..5221e074 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -682,7 +682,7 @@ def test_idle_runner_is_removed(instance_kwargs): "status", ["pending_create", "creating", "deleting", "deleted", "stopped", "unknown"], ) -def test_runner_garm_would_refuse_to_delete_is_left_alone(status): +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. diff --git a/charms/tests/integration/test_garm.py b/charms/tests/integration/test_garm.py index e7adb842..adc00088 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. @@ -478,39 +494,18 @@ def test_charm_drains_and_deletes_an_orphaned_scaleset( renamed = f"{_SCALESET_TEST_NAME}-renamed" try: juju.config(configurator_with_image, values={"name": renamed}) - _wait_for_reconcile(juju, configurator_garm, configurator_with_image) + _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_reconcile(juju, configurator_garm, configurator_with_image) + _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) -def _wait_for_reconcile( - 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, - ) - - @retry( retry=retry_if_exception_type( (AssertionError, requests.exceptions.RequestException) @@ -1067,12 +1062,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), From 068b4012f9709113988fef4f0e14be479b3c3c9e Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Thu, 27 Aug 2026 13:35:54 +0800 Subject: [PATCH 09/10] fix(garm): stop promising a retry for a runner that is already gone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A 404 from delete_instance means the runner GARM was asked to remove is no longer there — the outcome the delete was after, not a failure. It fell into the generic handler and logged "will retry on next reconcile" against a name no later pass can act on. Both delete attempts now treat GarmNotFoundError as done, the way resource_cleanup already does. Two test corrections found alongside it. The listing-failure test asserted neither half of its own name: the scaleset delete is still attempted after an unreadable runner list, deliberately, since the list says nothing about whether the scaleset owns runners and an empty one should not be stranded on a failure to read it. It now asserts that and says why. The integration test claimed to cover the drain, which it cannot: 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. It covers disable and delete, and is now named and documented as that; the drain stays unit-tested. Also gives _log_deferred_runner_delete the docstring the rest of the file carries. Co-Authored-By: Claude Opus 5 --- charms/garm/src/scaleset_reconciler.py | 20 ++++++++- .../tests/unit/test_scaleset_reconciler.py | 44 +++++++++++++++++-- charms/tests/integration/test_garm.py | 12 ++--- 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index e7d00dac..0e52c42d 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -10,7 +10,12 @@ from datetime import datetime, timedelta, timezone from charm_state import RunnerConfig -from garm_api import GarmApiError, GarmAuthenticatedClient, GarmUnauthorizedError +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 @@ -342,6 +347,11 @@ def _delete_runner(self, instance: Instance, scaleset_name: str) -> None: 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 @@ -372,11 +382,19 @@ def _delete_runner(self, instance: Instance, scaleset_name: str) -> None: 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 ) diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 5221e074..80dfd34a 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -4,12 +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, GarmUnauthorizedError +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 @@ -720,12 +726,14 @@ def test_unnamed_runner_is_skipped(): assert client.deleted == [42] -def test_runner_listing_failure_defers_scaleset_delete_attempt(): +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 and the reconcile completes, leaving the cleanup to - the next pass rather than aborting on an unreadable runner list. + 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): @@ -739,9 +747,37 @@ def list_scale_set_instances(self, scaleset_id): _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 adc00088..bed39437 100644 --- a/charms/tests/integration/test_garm.py +++ b/charms/tests/integration/test_garm.py @@ -467,7 +467,7 @@ 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_drains_and_deletes_an_orphaned_scaleset( +def test_charm_disables_and_deletes_an_orphaned_scaleset( juju: jubilant.Juju, configurator_garm: str, configurator_with_image: str, @@ -478,9 +478,11 @@ def test_charm_drains_and_deletes_an_orphaned_scaleset( 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, and again while it - still owns runners, so this covers the charm's own disable-drain-delete against a live - GARM — the sequence the teardown helper below otherwise hand-rolls. + 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) @@ -786,7 +788,7 @@ def _delete_scalesets(base_url: str, token: str) -> None: # 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_drains_and_deletes_an_orphaned_scaleset; this is only teardown, unwinding + # 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}", From c8415990ae3e2282976705998b0a0bdedde2c1e2 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Mon, 31 Aug 2026 12:04:22 +0800 Subject: [PATCH 10/10] docs(garm): finish the GitHub rename in the drain tests 7237ae1 moved the new scaleset-drain code off GARM's "forge" vocabulary, but two test docstrings kept it. Scalesets are a GitHub concept and the rest of the charm says GitHub, so say GitHub here too. Co-Authored-By: Claude Opus 5 --- charms/garm/tests/unit/test_garm_api.py | 2 +- charms/garm/tests/unit/test_scaleset_reconciler.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/charms/garm/tests/unit/test_garm_api.py b/charms/garm/tests/unit/test_garm_api.py index 665c475f..960df664 100644 --- a/charms/garm/tests/unit/test_garm_api.py +++ b/charms/garm/tests/unit/test_garm_api.py @@ -443,7 +443,7 @@ 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 + assert: GarmUnauthorizedError is raised, so the caller can tell expired GitHub credentials apart from a transient failure before escalating to the GitHub bypass. """ client = GarmAuthenticatedClient(BASE_URL, "token") diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 80dfd34a..e57d886d 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -596,7 +596,7 @@ def test_a_job_only_protects_its_runner_while_it_could_still_be_running(age, exp 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 + reconciles stale queued jobs against GitHub, 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. """