Fix runner records stranded in pending_delete wedging scale set autoscaling - #855
Fix runner records stranded in pending_delete wedging scale set autoscaling#855KT-Doan wants to merge 4 commits into
Conversation
|
Added an end-to-end regression test that replays the full interleave from #854 through a real store, the watcher, and a running provider worker (slow provider delete, deleting event parked on the manager mutex, forced pending_delete regressing the row mid-delete, follow-up event dropped by the 10s send timeout). On main the test times out with the record stuck in pending_delete; with this series it converges to deleted within a couple of ticks. |
When a runner reports terminated, SetInstanceToPendingDelete force-updated the instance to pending_delete, bypassing the status transition table. If the provider worker had already moved the instance to deleting and was mid-way through the provider delete call, the forced write moved the record backwards. The provider worker then failed its final transition (pending_delete -> deleted is not a valid transition), and the record was stranded in pending_delete forever while the compute resource was already gone. Such phantom records count toward the scale set's runner count and can permanently wedge autoscaling into scale-down. Use a validated update instead, and treat a rejected transition from a deletion-lane status as success: the intent of the call, the runner goes away, is already being fulfilled. Signed-off-by: Kevin <235441252+KT-Doan@users.noreply.github.com>
The instance manager's consolidateState had no case for InstanceDeleting. A manager whose cached state landed on deleting (for example when the status update from its own transition was delivered only after a long provider delete call finished, because handleUpdate serializes on the same mutex the delete path holds) ticked every 5 seconds doing nothing, forever. Combined with a racing forced pending_delete write from another worker, this stranded database records in pending_delete with the compute resource already gone: phantom runners that count toward the scale set runner count and wedge autoscaling (cloudbase#854). Three changes: - Treat InstanceDeleting as a resumable delete. Deletes are idempotent; providers report a missing instance as success. - Requeue on provider errors for every non-forced path, so a resumed delete that fails does not fall through and mark a live resource deleted. - If the final transition to deleted is refused because the row slid back onto the deletion lane (a racing pending_delete write), force the write: the provider resource is confirmed gone at that point. Signed-off-by: Kevin <235441252+KT-Doan@users.noreply.github.com>
runnerCount() was len(w.runners), which counts records in pending_delete, pending_force_delete, deleting and deleted. Those records cannot pick up jobs. Counting them suppresses scale-up, and a record stuck on the deletion lane (see the phantom pending_delete wedge, cloudbase#854) pinned the autoscaler in permanent scale-down: with min_idle_runners 0 and desired usually 1 on a quiet set, two stuck records were enough to starve any scale set regardless of its max_runners. handleScaleDown's deletion-lane case no longer consumes the removal delta for the same reason: those records are no longer part of the count the delta was derived from. Signed-off-by: Kevin <235441252+KT-Doan@users.noreply.github.com>
…te wedge Replays the full incident interleave from cloudbase#854 through a real store, the real watcher, and a running provider worker: a running instance moves to pending_delete, the manager starts a slow provider delete holding its mutex, the deleting event parks inside handleUpdate, a forced pending_delete write regresses the row mid-delete, and the follow-up event is dropped by Update()'s 10s send timeout. Without the fixes in this series the manager's cached state lands on deleting, a state consolidateState did not handle, and the record stays in pending_delete forever (this test times out on main). With them the manager re-drives the delete and the record converges to deleted within a couple of ticks. Signed-off-by: Kevin <235441252+KT-Doan@users.noreply.github.com>
9968fb8 to
bd0c265
Compare
|
Hi @KT-Doan We need to fix the root cause of this, rather than work around it. It's clear that there is a flaw in the way I designed state handling here, that is easily triggered under load. I am on vacation for a the next couple of weeks, but once I get back this will be first on the list of priorities. We need to iron this out and formalize a better way to keep track of state across workers. Otherwise we'll keep plugging holes as they sprout. This PR and the ones bellow do a great job at explaining what's wrong. The long term solution may be making a step back and redesigning some things. I will be away until the 31st of the month. After that I'll be back at it. Apologies for the delay on this. related: |
|
No worries! This PR is just the implementation we did in our organization and its working pretty smoothly so far, maybe it will help you in the future! Thank you for taking the time to look at this. |
|
It definitely helps. It demonstrates the failure mode. The other 2 PRs I linked to add more data points. I will try to tackle this as soon as I get back into the office. |
2 participants
Fixes #854.
Runner records could get stranded in
pending_deleteforever while the provider instance was already gone; stranded records count toward the scale set's runner count, so a handful of them pin the autoscaler in permanent scale-down and jobs queue on an empty scale set. The full verified chain (source references plus controller log excerpts) is in the issue.Three independent commits, each with tests; any one of them breaks the chain, and together they remove the corruption, the dead state, and the starvation:
runner:SetInstanceToPendingDelete(the agent terminated path) usedForceUpdateInstance, which can move a record backwards fromdeletingtopending_deletemid-delete, behind the provider worker's back. It now uses a validated update and treats a rejected transition from a deletion-lane status as success, since the runner is already on its way out.workers/provider:consolidateStatehad no case forInstanceDeleting, so a manager whose cached state landed there (a status update delivered late, after a long provider call, or a restart mid-delete) ticked forever doing nothing.InstanceDeletingis now a resumable delete (providers report a missing instance as success, so re-driving is safe), provider errors requeue for every non-forced path instead of falling through todeleted, and adeletedwrite refused because the row regressed onto the deletion lane is retried with force, because the provider resource is confirmed gone at that point.workers/scaleset:runnerCount()counted deletion-lane records, so stuck records suppressed scale-up regardless ofmax_runners, and withmin_idle_runners: 0two of them wedge a quiet scale set permanently. It now counts live records only, andhandleScaleDown's deletion-lane case no longer consumes the removal delta those records are no longer part of.Testing: new unit tests in
runner(real sqlite store),workers/provider(fake helper plus the generated provider mock), andworkers/scaleset; the new tests fail on main exactly where the incident behavior lives. Full suite green with the changes (go test -tags testing ./...). The remediation path for an already-wedged deployment (garm-cli runner delete -f) was verified on two live occurrences before writing these fixes.Not addressed here, flagged at the end of #854:
instanceManager.Update()discards a timed-out send with no requeue, andconsolidateStateholdsi.muxacross provider calls, which is what starveshandleUpdateinto those timeouts. Both are worth their own change; with this PR the dropped updates become harmless.