fix: prevent permanent reconcile hang when a runner pod dies mid-reconcile#1838
Open
agentydragon wants to merge 2 commits into
Open
fix: prevent permanent reconcile hang when a runner pod dies mid-reconcile#1838agentydragon wants to merge 2 commits into
agentydragon wants to merge 2 commits into
Conversation
When a runner pod dies mid-reconcile (e.g. its node reboots), the reconcile goroutine hangs forever inside a runner RPC. The runner gRPC client is created with waitForReady:true (getRunnerConnection), so an RPC on a channel that can no longer connect blocks until its context is done -- and setupTerraform's RPCs (UploadAndExtract ... Init ... SelectWorkspace) use the raw reconcile context, which has no deadline. controller-runtime will not start a new Reconcile for a key while the previous one is still running, so the affected Terraform object is never reconciled again and one of the --concurrent worker slots is leaked permanently. Wrap the setup RPCs in a context bounded by a new --runner-rpc-timeout flag (default 30m -- generous enough for provider downloads and long inits, finite so a dead runner surfaces as an error and requeues instead of hanging). This also supersedes the long-commented-out ctx30s TODO in setupTerraform. Signed-off-by: Rai <agentydragon@gmail.com>
A runner pod in Failed phase (e.g. killed by a node reboot) matches none of the branches in reconcileRunnerPod: it has the tls-secret-name label, the label matches, it has no DeletionTimestamp, and its phase is not Running -- so podState stays stateUnknown and the switch is a no-op. The dead pod is never cleaned up, so Error runner pods accumulate and connections to them hang under waitForReady. Add a stateUnknown case that force-deletes the stale pod and creates a fresh one, mirroring the stateMustBeDeleted path. Signed-off-by: Rai <agentydragon@gmail.com>
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.
Problem
When a node hosting
tf-runnerpods reboots (or the pods are evicted / OOM-killed) while the controller is mid-reconcile, the affectedTerraformreconciles hang permanently. They freeze inReady=Unknown / Initializing, stop logging, and never retry or self-heal. Only a controller restart clears it.We hit this in a ~26-
Terraformcluster after a worker node reboot: everyTerraformwhose runner was on that node wedged simultaneously and stayed wedged for 8h+ until we restarted the controller.Root cause
Two things combine into an unbounded hang:
The runner gRPC client uses
waitForReady: true—controllers/tf_controller_runner.go(getRunnerConnection, theretryPolicyservice config). An RPC on a channel inTRANSIENT_FAILURE(backend pod gone) blocks waiting for READY instead of failing fast withUNAVAILABLE.The runner RPCs carry no context deadline — e.g.
runnerClient.Init(ctx, initRequest)atcontrollers/tf_controller_backend.go:361uses the raw reconcilectx. controller-runtime imposes no per-reconcile timeout by default, and the controller never wrapsctx. (RunnerCreationTimeoutbounds only pod creation inreconcileRunnerPod, not the RPCs; the 30sdialCtxinLookupOrCreateRunneris cancelled before any RPC runs, andgrpc.NewClientis lazy.)waitForReady:true+ a deadline-less context = an RPC to a vanished runner blocks the reconcile goroutine forever.Because controller-runtime won't start a new
Reconcilefor a key while the previous one is still running, that object is never reconciled again, and the parked goroutine permanently consumes one of the--concurrentworker slots. Enough such events deadlock the whole controller.Log signature: the wedged object's last line is
"generated template"(tf_controller_backend.go), then silence — the next RPC (Init) never returns. A healthy reconcile logs"init reply:"or"error running Init:"next; a wedged one logs neither.Secondary bug (dead runner pods pile up)
In
reconcileRunnerPod, a pod inphase=Failedmatches none of the state branches (label present, label matches, noDeletionTimestamp, phase ≠Running) →podState = stateUnknown, and theswitchhas nocase stateUnknown→ silent no-op. The dead pod is never cleaned up, soErrorrunner pods accumulate.Reproduction
Terraformwith a runner pod.kubectl delete pod <name>-tf-runner) — or reboot/drain its node — so the pod vanishes mid-Init.Terraformfreezes atReady=Unknown/Initializingand never recovers; controller logs go silent for that object; aFailedrunner pod is left behind.Fix
--runner-rpc-timeoutflag (default30m— generous enough for provider downloads / long inits, finite enough that a dead runner surfaces as an error and requeues instead of hanging). Applied to thesetupTerraformRPC batch, where the hang occurs. This also supersedes the long-commented-outctx30sTODO insetupTerraform(introduced already-commented inb98b632, 2022, and never enabled).Failedrunner pods. Addcase stateUnknowninreconcileRunnerPodto force-delete a pod that exists but is neitherRunningnorTerminating, then recreate it, mirroring thestateMustBeDeletedpath.Split into two commits so they can be reviewed independently.
Alternative considered
Dropping
waitForReady:true(so a broken channel returnsUNAVAILABLEimmediately) is smaller, but risks spurious failures during normal runner cold-start, when the pod isRunningbut its gRPC server isn't listening yet. A bounded context keeps the startup-smoothing benefit ofwaitForReadywhile removing the infinite-hang failure mode. Happy to go the other direction if maintainers prefer.Scope note
The timeout is applied to
setupTerraform(upload → backend config → init → workspace select), which is where the observed hang occurs.plan/applyare invoked separately and can legitimately run much longer, so they are intentionally left unbounded here; a similar (larger) budget could be added for them as a follow-up if desired.Testing
go build ./...andgo vet ./controllers/ ./cmd/manager/pass.stateUnknowncase falls through to the existing "wait for pod IP" path, consistent with the other create branches.controllers/tc0002xx) exercises real runner pods and is intended to run in CI; I was not able to run it in my local sandbox (envtest apiserver instability, reproducible on unmodifiedmain). Happy to add a focusedtc000260-style case for thestateUnknownreap if you'd like it in this PR.I'm running these patches on my own cluster to confirm the wedge no longer recurs on node reboots. Feedback on the approach (flag name/default,
setupTerraform-only scope, or preferring thewaitForReadyremoval) very welcome.