From cecbcd40aedc486119a83e5427f290e98ec754b6 Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Tue, 18 Aug 2026 06:11:44 +0000 Subject: [PATCH 1/2] feat(garm): add idempotent resource cleanup --- charms/garm/src/resource_cleanup.py | 131 ++++++++++++++++++ .../garm/tests/unit/test_resource_cleanup.py | 87 ++++++++++++ 2 files changed, 218 insertions(+) create mode 100644 charms/garm/src/resource_cleanup.py create mode 100644 charms/garm/tests/unit/test_resource_cleanup.py diff --git a/charms/garm/src/resource_cleanup.py b/charms/garm/src/resource_cleanup.py new file mode 100644 index 00000000..6694f6f6 --- /dev/null +++ b/charms/garm/src/resource_cleanup.py @@ -0,0 +1,131 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Idempotent cleanup of GARM resources before charm removal.""" + +from __future__ import annotations + +import time +from collections.abc import Callable +from typing import Any + +from garm_api import GarmApiError +from garm_client.models.update_scale_set_params import UpdateScaleSetParams + +# GARM only accepts runner deletion for these states. Runners in creation or +# already being deleted are observed again on a later cleanup pass. +_DELETABLE_RUNNER_STATES = frozenset({ + "running", + "error", +}) + + +class GarmCleanupError(GarmApiError): + """Raised when GARM resources could not be drained before the deadline.""" + + +class GarmResourceCleanup: + """Drain all GARM scalesets and runners using observed state. + + The operation is deliberately retryable: runner deletion is asynchronous, + so a scaleset is deleted only after a later poll observes no instances. + """ + + def __init__( + self, + client: Any, + *, + timeout: float = 120.0, + poll_interval: float = 5.0, + sleep: Callable[[float], None] = time.sleep, + monotonic: Callable[[], float] = time.monotonic, + force_remove: bool = False, + bypass_gh_unauthorized: bool = False, + ) -> None: + self._client = client + self._timeout = timeout + self._poll_interval = poll_interval + self._sleep = sleep + self._monotonic = monotonic + self._force_remove = force_remove + self._bypass_gh_unauthorized = bypass_gh_unauthorized + + def run(self) -> None: + """Drain resources, or raise if the cleanup deadline is reached.""" + deadline = self._monotonic() + self._timeout + last_errors: list[str] = [] + + while True: + pending, errors = self._drain_pass() + if not pending: + return + + last_errors = errors or last_errors + if self._monotonic() >= deadline: + details = "; ".join(last_errors) if last_errors else "resources remain" + raise GarmCleanupError( + f"GARM cleanup did not complete before the deadline: {details}" + ) + self._sleep(self._poll_interval) + + def _drain_pass(self) -> tuple[bool, list[str]]: + """Perform one observed-state cleanup pass.""" + try: + scalesets = list(self._client.list_scalesets() or []) + except GarmApiError as exc: + return True, [f"listing scalesets: {exc}"] + + pending = False + errors: list[str] = [] + for scaleset in scalesets: + scaleset_pending, scaleset_errors = self._drain_scaleset(scaleset) + pending = pending or scaleset_pending + errors.extend(scaleset_errors) + return pending, errors + + def _drain_scaleset(self, scaleset: Any) -> tuple[bool, list[str]]: + """Disable and drain one scaleset, returning whether another pass is needed.""" + scaleset_id = getattr(scaleset, "id", None) + if scaleset_id is None: + return True, ["observed scaleset has no id"] + + try: + # Disabling before instance deletion prevents new runners from + # appearing while this drain is in progress. + self._client.update_scaleset( + scaleset_id, + UpdateScaleSetParams(enabled=False, min_idle_runners=0), + ) + instances = list(self._client.list_scale_set_instances(scaleset_id) or []) + except GarmApiError as exc: + return True, [f"scaleset {scaleset_id}: {exc}"] + + if instances: + errors = self._delete_eligible_instances(instances) + # Runner deletion is asynchronous; always wait for the next pass + # before attempting to delete the scaleset. + return True, errors + + try: + self._client.delete_scaleset(scaleset_id) + except GarmApiError as exc: + return True, [f"scaleset {scaleset_id}: {exc}"] + return False, [] + + def _delete_eligible_instances(self, instances: list[Any]) -> list[str]: + """Request deletion for eligible instances without aborting the pass.""" + errors: list[str] = [] + for instance in instances: + name = getattr(instance, "name", None) + state = getattr(instance, "status", None) + if not name or state not in _DELETABLE_RUNNER_STATES: + continue + try: + self._client.delete_instance( + name, + force_remove=self._force_remove, + bypass_gh_unauthorized=self._bypass_gh_unauthorized, + ) + except GarmApiError as exc: + errors.append(f"runner {name}: {exc}") + return errors diff --git a/charms/garm/tests/unit/test_resource_cleanup.py b/charms/garm/tests/unit/test_resource_cleanup.py new file mode 100644 index 00000000..e062ca71 --- /dev/null +++ b/charms/garm/tests/unit/test_resource_cleanup.py @@ -0,0 +1,87 @@ +# Copyright 2026 Canonical Ltd. +# See LICENSE file for licensing details. + +"""Tests for pre-remove GARM resource cleanup.""" + +from types import SimpleNamespace + +from resource_cleanup import GarmResourceCleanup + + +class _FakeClient: + def __init__(self, instance_lists): + self.scalesets = [SimpleNamespace(id=7, name="stale", enabled=True)] + self.instance_lists = iter(instance_lists) + self.events = [] + + def list_scalesets(self): + return self.scalesets + + def update_scaleset(self, scaleset_id, params): + self.events.append(("disable", scaleset_id, params.enabled, params.min_idle_runners)) + + def list_scale_set_instances(self, scaleset_id): + self.events.append(("list", scaleset_id)) + return next(self.instance_lists) + + def delete_instance(self, instance_name, *, force_remove=False, bypass_gh_unauthorized=False): + self.events.append( + ("delete-instance", instance_name, force_remove, bypass_gh_unauthorized) + ) + + def delete_scaleset(self, scaleset_id): + self.events.append(("delete-scaleset", scaleset_id)) + self.scalesets = [] + + +def test_cleanup_drains_runners_before_deleting_scaleset(): + """A runner deletion is asynchronous, so scaleset deletion waits for an empty poll.""" + client = _FakeClient( + [ + [SimpleNamespace(name="runner-1", status="running")], + [], + ] + ) + + GarmResourceCleanup( + client, + timeout=1, + poll_interval=0, + sleep=lambda _: None, + monotonic=lambda: 0, + ).run() + + assert client.events == [ + ("disable", 7, False, 0), + ("list", 7), + ("delete-instance", "runner-1", False, False), + ("disable", 7, False, 0), + ("list", 7), + ("delete-scaleset", 7), + ] + + +def test_cleanup_does_not_redelete_pending_runner(): + """Already pending runner deletion is observed until the row disappears.""" + client = _FakeClient( + [ + [SimpleNamespace(name="runner-1", status="pending_delete")], + [], + ] + ) + + GarmResourceCleanup( + client, + timeout=1, + poll_interval=0, + sleep=lambda _: None, + monotonic=lambda: 0, + ).run() + + assert client.events == [ + ("disable", 7, False, 0), + ("list", 7), + ("disable", 7, False, 0), + ("list", 7), + ("delete-scaleset", 7), + ] From a91719d7766a24d1002633ca76f5fe9ae547007d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Wed, 19 Aug 2026 03:51:32 +0000 Subject: [PATCH 2/2] test(garm): document cleanup tests with AAA --- charms/garm/tests/unit/test_resource_cleanup.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/charms/garm/tests/unit/test_resource_cleanup.py b/charms/garm/tests/unit/test_resource_cleanup.py index e062ca71..f6072b55 100644 --- a/charms/garm/tests/unit/test_resource_cleanup.py +++ b/charms/garm/tests/unit/test_resource_cleanup.py @@ -35,7 +35,11 @@ def delete_scaleset(self, scaleset_id): def test_cleanup_drains_runners_before_deleting_scaleset(): - """A runner deletion is asynchronous, so scaleset deletion waits for an empty poll.""" + """ + arrange: A scaleset whose runner list is populated, then empty on the next poll. + act: Run cleanup. + assert: The runner is requested for deletion before the now-empty scaleset is deleted. + """ client = _FakeClient( [ [SimpleNamespace(name="runner-1", status="running")], @@ -62,7 +66,11 @@ def test_cleanup_drains_runners_before_deleting_scaleset(): def test_cleanup_does_not_redelete_pending_runner(): - """Already pending runner deletion is observed until the row disappears.""" + """ + arrange: A scaleset whose runner is already pending deletion, then disappears. + act: Run cleanup. + assert: Cleanup polls without reissuing runner deletion, then deletes the scaleset. + """ client = _FakeClient( [ [SimpleNamespace(name="runner-1", status="pending_delete")],