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