Bound how long a wedged Update can hold a reconcile worker - #1296
Open
bpalermo wants to merge 1 commit into
Open
Bound how long a wedged Update can hold a reconcile worker#1296bpalermo wants to merge 1 commit into
bpalermo wants to merge 1 commit into
Conversation
An update controller reconcile blocks for the entire Pulumi operation, so each one occupies a MaxConcurrentReconciles slot for as long as the operation runs. Nothing bounded that: the reconcile context had no deadline, all four operation RPCs use grpc.WaitForReady(true), and an agent that is alive but wedged inside a provider call keeps its connection perfectly healthy while never sending another message. Recv() then blocks forever. Because the reconcile never returns, Progressing=True is never revised and the deferred activeReconciles cleanup never runs, so the workspace watch also declines to re-enqueue it. - Add an idle timeout to the operation stream, defaulting to 30m and settable per update via Update.spec.idleTimeout (reachable from a Stack through spec.updateTemplate). It measures silence rather than total duration and is reset on every message, so a slow but progressing update is never cut off. The operation runs on a context derived from the reconcile's rather than the reconcile context itself. That distinction is the point: abandoning the stream must not also cancel the status write that records the outcome, or the Update would be left Progressing forever -- the very state being fixed. When the reconcile context is cancelled instead, as on manager shutdown, the write is skipped deliberately so the restarted operator's guard marks it Aborted. - Tighten TCP keepalive on connections to workspace pods. net.Dialer already enables keepalive with a 15s idle, but leaves interval and count to the system -- 75s x 9 on Linux, so a black-holed peer takes ~11 minutes to notice. This brings that to about a minute. Deliberately TCP-level: gRPC pings are policed by the server's EnforcementPolicy, whose 5m default MinTime the operator cannot know for an agent image pinned per Stack, and exceeding it earns a GOAWAY that would kill healthy long-running updates. - Add server-side keepalive to the agent so it notices a departed operator and can unwind the orphaned operation. Server pings are not subject to any enforcement policy, so this needs no client cooperation. - Give the update controller its own concurrency budget. Sharing the manager-wide default means hour-long operations crowd out the stack and workspace controllers, and queue the update controller's own cheap terminal transitions behind them -- which is why a deleted Update could sit in Terminating, unable to reach Complete so its finalizer could be removed. - Honour GRACEFUL_SHUTDOWN_TIMEOUT_DURATION. It is set to 5m in the deploy template but was read nowhere, so shutdown used the 30s default and cut short in-flight updates that the pod's 300s grace period was sized to allow. Also add a --max-concurrent-reconciles flag and stop discarding the environment overrides' parse errors. A malformed MAX_CONCURRENT_RECONCILES yielded 0, which controller-runtime treats as 1: a silent drop from 25 workers to one, with nothing logged. Fixes pulumi#1293 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting. |
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.
Proposed changes
An update controller reconcile blocks for the entire Pulumi operation, so each one occupies a
MaxConcurrentReconcilesslot for as long as that operation runs. Nothing bounded it: the reconcile context has no deadline (the onlycontext.WithTimeoutcoversConnect), all four operation RPCs usegrpc.WaitForReady(true), and an agent that is alive but wedged inside a provider call keeps its connection perfectly healthy while never sending another message — soRecv()blocks indefinitely. Because the reconcile never returns,Progressing=Trueis never revised and the deferredactiveReconcilescleanup never runs, somapWorkspaceToUpdatealso declines to re-enqueue it.Idle timeout on the operation stream
New
--update-idle-timeout(default 30m), overridable per update viaUpdate.spec.idleTimeout. BecauseapplyUpdateTemplatemerge-patches the whole Update, this is reachable from a Stack throughspec.updateTemplate.spec.idleTimeoutwith no Stack-side plumbing.It measures silence rather than total duration and is reset on every message, so a slow but progressing update is never interrupted — which matters given #1293 reports legitimate runs reaching 2h22m and 13.8h.
0disables it.The operation runs on a context derived from the reconcile's rather than the reconcile context itself. That distinction is the point:
setStatusBlockFromGRPCErrsetsComplete=Truein memory and the caller then persists it withupdateStatus(ctx, obj). Cancelling the reconcile context to stop the stream would fail that write, leaving the UpdateProgressingforever — the very state being fixed. When the reconcile context is cancelled instead, as on manager shutdown, the write is skipped deliberately so the restarted operator's existingProgressingguard marks itAborted.Tightened TCP keepalive to workspace pods
Worth being precise about what this does and does not buy, because my first reading of the problem was wrong.
net.Dialeralready enables keepalive with a 15s idle, so these connections were never unprobed — I verifiedSO_KEEPALIVE=1and a 15s idle on a plain(&net.Dialer{}).Dial. What the defaults leave to the system is the probe interval and count: 75s × 9 on Linux, so a black-holed peer takes roughly 11 minutes to notice. This brings that to about a minute.So it is hardening, not the fix — a lost node or partition is bounded sooner. It cannot see a connection that is healthy while the operation is wedged; that is what the idle timeout is for. It also benefits the workspace controller, which has the same unbounded-RPC exposure over the same dial path.
Deliberately TCP-level rather than gRPC-level. gRPC pings are policed by the server's
keepalive.EnforcementPolicy, whose defaultMinTimeis 5 minutes; since the agent image is pinned per Stack, the operator routinely talks to older agents whose policy it cannot know, and exceeding it earns aGOAWAY / ENHANCE_YOUR_CALMthat would kill healthy long-running updates. TCP probes are invisible to HTTP/2, so they are safe against every agent version ever shipped.Agent-side server keepalive
keepalive.ServerParameterson the agent, so it notices a departed operator, closes the transport, and lets the orphaned operation unwind via the existingCancel()path. Server-initiated pings are not subject to any enforcement policy, so this needs no client cooperation. NoEnforcementPolicyis set, on purpose: tightening itsMinTimewould let a new agent reject pings from a client behaving correctly for the default policy it was written against.A separate worker budget for the update controller
MaxConcurrentReconcileswas set once, manager-wide. The update controller is the only one whose reconciles block for hours, so sharing a pool means long operations crowd out the stack and workspace controllers — and queue the update controller's own cheap terminal transitions (deleting → Canceled) behind hour-long ones. That priority inversion is why a deleted Update could sit inTerminatingunable to reachComplete, which is what the finalizer needs.New
--update-max-concurrent-reconciles(0= use the shared value).Honour
GRACEFUL_SHUTDOWN_TIMEOUT_DURATIONIt is set to
5mindeploy/operator_template.yaml, and read nowhere — so shutdown used controller-runtime's 30s default and cut short exactly the in-flight updates the pod's 300sterminationGracePeriodSecondswas sized to allow. Now wired, with a matching flag.Tuning-surface fixes called out in the issue
The discarded error made a malformed
MAX_CONCURRENT_RECONCILESsilently0, which controller-runtime treats as 1 — a silent drop from 25 workers to one with nothing in the logs. Adds--max-concurrent-reconciles, rejects non-positive values, and applies the same non-silent parsing to the adjacentLEADER_ELECTION_*/KUBE_API_TIMEOUToverrides. Also drops a misleading emptycontroller.Options{}in the stack controller's setup.Docs
docs/metrics.mdgains a section on reading the update controller's workqueue, since resource usage cannot distinguish idle from saturated here. Per the issue's own correction, it states plainly whatworkqueue_longest_running_processor_secondscannot show — it tracks only the longest actively-running reconcile, so it is blind to Updates stranded by a killed operator, and a large value is not proof of a problem — and gives the detection method that actually worked: groupProgressing=TrueUpdates bylastTransitionTime, and gate any deletion on the workspace'spulumicontainer start time, not the pod's. Also corrects the statedMaxConcurrentReconcilesdefault from 10 to 25.Testing
TestUpdateIdleTimeout— a silent stream is abandoned and the terminal status actually reaches the API server (the point of the derived context); assertsComplete=True,Failed=True / IdleTimeout.TestUpdateIdleTimeoutDisabled— a zero timeout arms nothing, and a long gap is not cut off.TestIdleTimeoutFor— precedence between the per-Update value and the operator default, including that an explicit zero disables rather than falls back.connect_test.go— the dialer applies the tuned probe interval and count, read back off the socket. Note it asserts the tuned values rather thanSO_KEEPALIVE, because that is on by default and would pass regardless; I checked the test fails when the tuning is removed.main_test.go— table tests over valid / malformed / empty / zero / negative for the env overrides and the concurrency validation.make test(operator + agent) andgolangci-lintare clean, with no codegen drift.make test-e2ehas not been run, and the failure mode has not been reproduced end-to-end against a live cluster.Related issues
Fixes #1293. The
Terminating-with-finalizer half of that issue is handled in #1295, which touches the same stack-controller function as its own fix.🤖 Generated with Claude Code