fix: resolve deadlock when maxSurge>0 rolling update on single-replica LWS#779
fix: resolve deadlock when maxSurge>0 rolling update on single-replica LWS#779veast wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for kubernetes-sigs-lws canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: veast The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
|
Welcome @veast! |
|
Hi @veast. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
ca057f0 to
8b2e727
Compare
…a LWS When a LeaderWorkerSet has replicas=1 and maxSurge=1, triggering a rolling update caused the controller to immediately emit a "deleting surge replica" event and return replicas=1 (no surge ever created), leaving the update permanently stuck with the StatefulSet at partition=1, replicas=1. Root cause: in Case 2 of rollingUpdateParameters (a new rolling update is detected) the code called wantReplicas(lwsReplicas). With replicas=1 and maxSurge=1 the condition inside wantReplicas was: unreadyReplicas(1) <= maxSurge(1) → true which jumped straight into the "release surge" branch and returned replicas=1. No surge replica was ever created, so the StatefulSet partition could never advance. Fix: Case 2 now returns burstReplicas directly instead of going through wantReplicas. At the moment a new update is detected all existing replicas are still running the old template (none are unready due to the update yet), so the correct action is to expand to lwsReplicas+maxSurge first. wantReplicas is only meaningful once stsReplicas==burstReplicas and the surge pods are being replaced. A new integration test "rolling update with maxSurge=1 and single replica creates surge before rolling" directly exercises the regression: it verifies that the leader StatefulSet expands to replicas=2 immediately after the update is triggered, then converges back to replicas=1 once all groups are ready. Fixes: kubernetes-sigs#688 Signed-off-by: veast <veast@users.noreply.github.com>
8b2e727 to
09a6e8d
Compare
|
/retest |
|
@veast: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it
Fixes a deadlock where a
LeaderWorkerSetwithreplicas=1andmaxSurge=1could never complete a rolling update.Symptom: immediately after triggering an update the controller emits
a
"deleting surge replica <name>-1"event and theStatefulSetstallsat
partition=1, replicas=1forever. The surge pod is never actuallycreated.
Root cause: in Case 2 of
rollingUpdateParameters(a new rollingupdate is detected) the code called
wantReplicas(lwsReplicas). Withreplicas=1andmaxSurge=1the condition insidewantReplicaswas:which jumped straight into the "release surge" branch and returned
replicas=1. No surge replica was ever created, so the StatefulSetpartitioncould never advance.Fix: two interrelated changes:
Case 2 returns
burstReplicasdirectly instead of going throughwantReplicas. At the moment a new update is detected all existingreplicas are still running the old template (none are unready due to
the update yet), so the correct action is to expand to
lwsReplicas + maxSurgefirst.wantReplicasis only meaningfulonce
stsReplicas == burstReplicasand the surge pods are beingreplaced.
wantReplicascondition tightened from<=to<. WhenunreadyReplicas == maxSurgeexactly (e.g.replicas=2, maxSurge=1,one replica still unready) the surge pods should be kept alive until
that last replica becomes ready. Using strict-less-than ensures we
only enter the shrink path when there is genuine headroom.
Which issue(s) this PR fixes
Fixes #688
Special notes for your reviewer
The logic change is small (two lines in
leaderworkerset_controller.go)but the comments have been expanded to make the invariants clearer.
A new integration test
"rolling update with maxSurge=1 and single replica creates surge before rolling"directly exercises the regression:it verifies that the leader StatefulSet expands to
replicas=2immediately after the update is triggered (i.e. the surge is actually
created), then converges back to
replicas=1once all groups are ready.Does this PR introduce a user-facing change?
Yes —
maxSurge-based zero-downtime rolling updates now work correctlyfor single-replica
LeaderWorkerSetobjects.