Re-register the runner scale set when it is gone from the Actions service#4571
Open
diogotorres97 wants to merge 1 commit into
Open
Re-register the runner scale set when it is gone from the Actions service#4571diogotorres97 wants to merge 1 commit into
diogotorres97 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the AutoscalingRunnerSet controller against out-of-band deletion/recreation of runner scale sets in the Actions service by re-validating the recorded scale set ID before creating a listener, and by treating “already deleted” as success during finalization. This prevents listener crash-loops on stale IDs and avoids stuck Terminating resources when the scale set is missing.
Changes:
- Add a pre-listener existence check (
GetRunnerScaleSetByID) and re-register the scale set when the recorded ID is definitively not found. - Treat
scaleset.NotFoundErrorfromDeleteRunnerScaleSetas desired state to allow finalizers to complete. - Add fake client plumbing + Ginkgo specs covering stale ID recovery, transient failure behavior, and deletion when the scale set is already gone.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Bumps github.com/actions/scaleset to a pseudo-version to gain a not-found sentinel; indirect dependency bumps. |
| go.sum | Updates module checksums for the dependency changes. |
| controllers/actions.github.com/multiclient/fake/client.go | Adds WithGetRunnerScaleSetByIDFunc and dispatch logic to support per-ID behavior in tests. |
| controllers/actions.github.com/autoscalingrunnerset_controller.go | Adds runnerScaleSetGone check before listener creation; handles not-found on delete as success. |
| controllers/actions.github.com/autoscalingrunnerset_controller_test.go | Adds test coverage for stale scale set IDs, transient failures, and deletion when scale set is already missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5781b14 to
a1a8aba
Compare
…vice A runner scale set can disappear from the Actions service out of band: it can be deleted from the organization settings, or dropped and registered again with a fresh ID. The ID recorded in the runner-scale-set-id annotation then points at nothing. Nothing notices. shouldCreateScaleSet only registers a scale set again when the annotation is missing or unparseable, so a valid-but-dead ID survives every reconcile. The listener reads that ID once, when it is created, and exits on any error, so every listener pod fails with RunnerScaleSetNotFoundException and the listener crash-loops with no way back. Restarting the pods does not help, and neither does correcting the annotation by hand. Today the only way out is to delete the AutoscalingRunnerSet and let it be created again. Confirm the recorded scale set still exists before creating a listener with its ID, and register it again when it does not. createRunnerScaleSet already reuses an existing scale set with the same name, so a scale set that came back with a new ID is adopted rather than duplicated. Only a definitive not-found triggers this. Any other failure is returned, so a transient Actions service outage cannot cause a second scale set to be registered. Deleting an AutoscalingRunnerSet whose scale set is already gone gets stuck the same way: DeleteRunnerScaleSet returns not-found, the finalizer never completes, and the resource sits in Terminating until the finalizer is removed by hand. Treat an already-deleted scale set as the desired state instead. Both paths need the not-found sentinel from github.com/actions/scaleset, which is not in v0.4.0, so the dependency moves to a pseudo-version of main. Fixes actions#4370
a1a8aba to
0a22a74
Compare
Author
|
@nikola-jokic @Okabe-Junya can you take a look? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partially addresses #4370.
New scale sets — and any whose listener has not been created yet — recover automatically. An
AutoscalingListenerthat is already crash-looping on a dead ID is not healed on its own; deleting it withkubectl delete autoscalinglistener <name> -n <controller-namespace>makes the controller rebuild it against a revalidated ID. Before this change the only recovery was deleting the wholeAutoscalingRunnerSet. See "Follow-up worth considering" below for why, and what would close the gap.The problem
A runner scale set can disappear from the Actions service out of band: deleted from the organization settings, or dropped and registered again with a fresh ID. The ID recorded in the
runner-scale-set-idannotation then points at nothing.Nothing notices:
shouldCreateScaleSetregisters a scale set again only when the annotation is missing, unparseable, or non-positive (autoscalingrunnerset_controller.go:798-805). A valid-but-dead ID survives every reconcile.AutoscalingListenerspec once, when it is built (resourcebuilder.go:137), and each listener pod reads it on startup and exits on any error. Every pod fails withRunnerScaleSetNotFoundExceptionand the listener crash-loops.Restarting pods does not help, because the ID is baked into the
AutoscalingListenerspec. Correcting the annotation by hand does not help either. The only way out today is to delete theAutoscalingRunnerSetand let it be created again, which several people in #4370 report is not viable in production.Deleting it gets stuck too:
DeleteRunnerScaleSetreturns not-found for a scale set that is already gone,deleteRunnerScaleSetpropagates that error, the finalizer never completes, and the resource sits inTerminatinguntil the finalizer is removed by hand.The fix
Confirm the recorded scale set still exists before creating a listener with its ID, and register it again when it does not.
createRunnerScaleSetalready reuses an existing scale set with the same name, so a scale set that came back with a new ID is adopted rather than duplicated.Only a definitive not-found triggers this. Any other failure is returned to the caller, so a transient Actions service outage cannot cause a second scale set to be registered — the failure mode reported in #3942. There is a test for exactly this.
The lookup runs only when a listener is about to be created, not on every reconcile, so steady state costs no extra API calls.
The delete path now treats an already-deleted scale set as the desired state, so an
AutoscalingRunnerSetwhose scale set is gone no longer needs its finalizer stripped by hand.A note on the dependency bump
Both paths need a not-found sentinel from
github.com/actions/scaleset.wrapResponseErrorTypeonmainmaps HTTP 404 toscaleset.NotFoundError, which is exactly what is needed, but it is not in v0.4.0 — the latest tag — so this PR moves the dependency to a pseudo-version ofmain.Happy to re-point it at a tag if you cut a
scalesetrelease, or to rework this if you would rather not carry a pseudo-version. Without the sentinel the alternative is matching on the error string, which is out of step with how the rest of the controllers detect these conditions (errors.Is(err, scaleset.JobStillRunningError)and friends).Tests
Three specs in
autoscalingrunnerset_controller_test.go:main, where the listener is created with the dead IDAutoscalingRunnerSetwhose scale set is already gone removes the finalizer instead of hanging inTerminating— fails onmain, where deletion never completesAlso adds
WithGetRunnerScaleSetByIDFuncto the fake client, mirroring the existingFuncoptions, so a test can answer per-ID.The full
TestAPIssuite passes locally, including the three new specs.Follow-up worth considering
This heals a listener at creation time. A listener that is already crash-looping is not healed automatically, because its spec never changes and so it is never rebuilt — deleting the
AutoscalingListeneris now enough to recover, where before the wholeAutoscalingRunnerSethad to go. Making the controller notice a persistently failing listener and revalidate would close that gap, but it is a larger change and I left it out of this PR.#4561 looks like the same shape with a different field, and may already be resolved on
mainnow that theEphemeralRunnerSetis patched in place under a deterministic name.