fix(garm): remove a busy runner once the drain outlives any job (ISD-7263)

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:

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)]