From a6b55372f1d06d8d1d45c191209602a74ec3ac2d Mon Sep 17 00:00:00 2001 From: charlie4284 Date: Fri, 23 Oct 2026 03:35:14 +0000 Subject: [PATCH] fix(garm): remove a busy runner once the drain outlives any job MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An orphaned scaleset never drained when GARM reported one of its runners as running a job: `_remove_runners` skipped that runner on every pass, whatever the drain's age, so the scaleset, its template and its provider stayed behind forever while the charm logged the same line every five minutes. Past DRAIN_DEADLINE the record cannot describe a job that is still running — a self-hosted job cannot outlive MAX_JOB_RUNTIME, and the deadline is that plus a margin — so the completion that would have cleared the runner was lost. The busy protection is now time-bounded: past the deadline the runner is removed with the existing escalation (a plain delete, forcing only when GARM is already sitting on a delete it could not carry out), and before it the runner is left alone exactly as now. `_advance_retirement` already only calls `_remove_runners(past_deadline=True)` once `_drain_deadline_passed()` is true, so the retirement path inherits the same rule rather than deleting a busy runner early. Also documents, at `_drain_deadline_passed`, why re-disabling a scaleset does not reset the clock: GARM's `updateScaleSet` writes only the fields that differ and issues no statement when none do, so an idempotent disable leaves `updated_at` at the moment it was first disabled. Refs: ISD-7263 --- charms/garm/src/scaleset_reconciler.py | 33 ++++++++++++++--- .../tests/unit/test_scaleset_reconciler.py | 35 ++++++++++--------- 2 files changed, 47 insertions(+), 21 deletions(-) diff --git a/charms/garm/src/scaleset_reconciler.py b/charms/garm/src/scaleset_reconciler.py index 65ce71af..e8cee205 100644 --- a/charms/garm/src/scaleset_reconciler.py +++ b/charms/garm/src/scaleset_reconciler.py @@ -523,8 +523,9 @@ def _advance_retirement( # not only an active one — is still listed, so whatever is left needs the # same guarded cleanup the orphan sweep uses: a plain delete for anything # removable, escalating to forced removal only once GARM itself is stuck - # carrying one out, and still leaving a genuinely running job's runner - # alone, though past MAX_JOB_RUNTIME none legitimately can be one. + # carrying one out. A runner still marked busy is removed too: this branch only + # runs once the drain has outlived MAX_JOB_RUNTIME, so no legitimate job can be + # running on it. logger.warning( "Scaleset %s still reports %d runner(s) after %s of draining;" " removing them directly rather than continuing to wait for GARM's" @@ -752,9 +753,10 @@ def _remove_runners(self, scaleset_id: int, name: str, *, past_deadline: bool) - Args: scaleset_id: Id of the scaleset being deleted. name: Name of the scaleset being deleted, for logging. - past_deadline: Whether the scaleset has been draining longer than any job - can last, which turns a runner GARM will not accept a delete for from - something to wait out into something to report. + past_deadline: Whether the scaleset has been draining longer than any job can + last. It turns a runner GARM will not accept a delete for from something to + wait out into something to report, and it expires the protection a busy + runner would otherwise have indefinitely. Returns: Whether the scaleset owns no runners, so GARM will accept its delete. @@ -790,6 +792,21 @@ def _remove_runners(self, scaleset_id: int, name: str, *, past_deadline: bool) - # same error every pass. self._log_undeletable_runner(instance, name, past_deadline) continue + if past_deadline: + # The scaleset has been draining longer than any job can last, so a runner + # this record still calls busy cannot be running one: the completion that + # would have cleared it was lost. The protection has expired rather than + # become true, so removing it is what lets the scaleset finish, and it is a + # plain delete unless GARM is already sitting on one it could not carry out. + logger.warning( + "Removing runner %s of scaleset %s: it still reports a job after %s of" + " draining, which no legitimate job outlives", + instance.name, + name, + DRAIN_DEADLINE, + ) + self._delete_runner(instance, name) + 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 @@ -1289,6 +1306,12 @@ def _drain_deadline_passed(scaleset: ScaleSet) -> bool: stopped the listener. ``handleAutoScale`` keeps running but writes instances, a separate table. False when GARM reports no timestamp: an unknown drain age must never read as an expired one. + + Re-disabling a scaleset does not move this clock: GARM's ``updateScaleSet`` builds + its update from the fields that differ and issues no statement when none do, so an + idempotent disable leaves ``updated_at`` at the moment it was first disabled. That + is what lets the drain deadline survive the orphan sweep re-disabling the scaleset + on every pass. """ retired_at = scaleset.updated_at if retired_at is None: diff --git a/charms/garm/tests/unit/test_scaleset_reconciler.py b/charms/garm/tests/unit/test_scaleset_reconciler.py index 5a11a511..5ec8703d 100644 --- a/charms/garm/tests/unit/test_scaleset_reconciler.py +++ b/charms/garm/tests/unit/test_scaleset_reconciler.py @@ -2288,14 +2288,16 @@ def test_drain_past_the_deadline_deletes_the_scaleset_once_removal_completes(): assert progress == [] -def test_drain_past_the_deadline_still_leaves_a_running_job_alone(): +def test_drain_past_the_deadline_removes_a_runner_still_marked_busy(): """ - arrange: The old generation was disabled longer ago than any job can run, but still - reports a runner GARM's job-status stream marks as running one. + arrange: The scaleset has been draining longer than any job can last, and still reports + a runner GARM's job-status stream marks as running one. act: Reconcile. - assert: Direct removal past the deadline reuses the same job protection as the - orphan sweep, so a genuinely running job is not cut short just because the - scaleset around it has been draining a long time. + assert: The runner is handed a plain delete. Past MAX_JOB_RUNTIME the record cannot + describe a job that is still running — a self-hosted job cannot outlive it — so the + completion that would have cleared the runner was lost. Without this the scaleset + never drains: the runner is reported busy on every pass for as long as GARM keeps + refreshing its row. """ long_ago = datetime.now(timezone.utc) - DRAIN_DEADLINE * 2 client = FakeGarmClient( @@ -2309,31 +2311,32 @@ def test_drain_past_the_deadline_still_leaves_a_running_job_alone(): progress = _reconcile(client, [_spec(labels=_LABELS_NEW)]) - assert client.deleted_instances == [] - assert client.deleted == [] + # Plain, not forced: forcing is reserved for a delete GARM accepted and could not carry + # out, and would drop the record while the VM may still exist. + assert client.deleted_instances == [("runner-1", False, False)] assert progress == [ScalesetProgress("my-scaleset", _OLD_NAME, _NEW_NAME, 1)] -def test_drain_within_the_deadline_still_waits_for_the_runners(): +def test_drain_within_the_deadline_leaves_a_runner_marked_busy_alone(): """ - arrange: The old generation was disabled recently and still has a runner. + arrange: A replaced scaleset whose runner reports a job, with the drain still inside + DRAIN_DEADLINE. act: Reconcile. - assert: It is not deleted — the deadline is a backstop for stuck instances, and must - not shorten a normal drain and cut a running job short. + assert: No delete is issued: a genuinely running job is never cut short, which is the + whole reason the deadline exists. """ - recently = datetime.now(timezone.utc) - DRAIN_DEADLINE / 2 client = FakeGarmClient( providers=["openstack-demo"], scalesets=[ - _generation(_LABELS_OLD, id=1, enabled=False, updated_at=recently), + _generation(_LABELS_OLD, id=1, enabled=False), _generation(_LABELS_NEW, id=2), ], - instances={1: 1}, + instances={1: [{"name": "runner-1", "runner_status": "active"}]}, ) progress = _reconcile(client, [_spec(labels=_LABELS_NEW)]) - assert client.deleted == [] + assert client.deleted_instances == [] assert progress == [ScalesetProgress("my-scaleset", _OLD_NAME, _NEW_NAME, 1)]