fix(opensearch-operator): use RollingUpdate maxSurge=0 instead of Recreate - #3319
Conversation
github-actions
Bot
added
area/uncategorized
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe OpenSearch operator now uses ChangesReplica-aware OpenSearch operator rollout
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant ControllerDeployment
participant ControllerManager
HelmValues->>ControllerDeployment: Set manager.replicaCount
ControllerDeployment->>ControllerManager: Omit --leader-elect when count <= 1
ControllerDeployment->>ControllerDeployment: Set RollingUpdate constraints when count <= 1
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/M
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a Helm upgrade failure in the opensearch-operator chart caused by a conflict between the 'Recreate' deployment strategy and leftover 'rollingUpdate' configurations from previous chart versions. By switching to a 'RollingUpdate' strategy with specific surge and unavailability settings, the operator maintains the desired single-replica rollout behavior while ensuring compatibility with existing Kubernetes deployments. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
dosubot
Bot
added
the
area/platform
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the leader election test suite for the opensearch-operator package. Specifically, it changes the expected deployment strategy type from Recreate to RollingUpdate and adds assertions to verify that maxSurge is set to 0 and maxUnavailable is set to 1. This configuration ensures that the old manager is torn down before the new one starts during a single-replica rollout, preventing uncoordinated managers from overlapping while avoiding invalid transition errors during upgrades. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
NOT LGTM — the fix itself is directionally right, but the root cause it documents is empirically false, and the regression coverage it claims does not execute.
Business context: a v1.5.x -> v1.6.0-rc upgrade of opensearch-operator fails repeatedly until the HelmRelease times out, because the Deployment ends up with strategy.type: Recreate alongside a leftover rollingUpdate block, which the apiserver rejects.
The direction is right and I want to say so up front: maxSurge: 0 is a sound choice, and re-enabling leader election is correctly off the table (#3040 gated it off because lease-renewal blips crashloop a single-replica manager). The fix works under both the client-side and server-side apply paths. The blockers below are about the reasoning that ships with it, not the strategy value.
Blockers
B1: The documented root cause is disproven, and the wrong mechanism is baked into four places
The PR states that Helm's 3-way strategic merge "never owned rollingUpdate (it was server-defaulted), so Helm emits no delete directive for it — the patch only flips type to Recreate". That is not what happens: DeploymentSpec.Strategy carries patchStrategy:"retainKeys", the exact Kubernetes mechanism added for this transition.
Evidence: k8s.io/[email protected]/apps/v1/types.go:395 declares Strategy DeploymentStrategy \json:"strategy,omitempty" patchStrategy:"retainKeys"`. Running strategicpatch.CreateThreeWayMergePatchwith this repo's own apimachinery, usingoriginal= the v1.5.x manifest (no strategy),modified = the #3040 manifest (type: Recreate), current= the live object (server-defaulted25%/25%`):
patch : {"spec":{"strategy":{"$retainKeys":["type"],"type":"Recreate"}}}
merged: {"spec":{"strategy":{"type":"Recreate"}}} <- rollingUpdate IS cleared, object is valid
So the client-side path cannot produce the observed error. Since the error is real, the path that produced it is server-side apply, which has no $retainKeys and leaves the server-defaulted, unowned rollingUpdate in place. That path is the default here: helm-controller v1.5.0 documents install.serverSideApply as "Defaults to true" and upgrade.serverSideApply as "Defaults to auto ... based on the release's previous usage", and this repo sets neither (internal/operator/package_reconciler.go populates only Timeout, Strategy, CRDs). It also explains why the failure shows up through a HelmRelease rather than a local helm upgrade.
Impact: this is not a wording nit — it changes the scope of the bug, and it is recorded permanently in the commit message plus three comments (template, patch, test). The PR's own theory is self-refuting: if a client-side merge could not clear rollingUpdate, then packages/system/ouroboros would be failing today for the same reason (see follow-up 1) — it is not. Under the server-side apply mechanism, ouroboros is latently broken. Please re-derive the mechanism and correct the commit message and the comments; the class of bug is "adding Recreate over a server-defaulted rollingUpdate under SSA", not "Helm 3-way merge".
B2: tests/leader_election_test.yaml never runs in CI, so the claimed regression coverage is a no-op
The PR states "a revert to Recreate fails the unit suite". It does not — this suite has never executed.
Evidence: hack/helm-unit-tests.sh runs a package's tests only when the package's own Makefile defines a test: target (guarded by make -C "$dir" -n test >/dev/null 2>&1), and hack/package.mk provides no such target. packages/system/opensearch-operator/Makefile has no test: target — make -C packages/system/opensearch-operator -n test returns "No rule to make target 'test'". 64 other packages define it; packages/system/etcd-operator/Makefile is the idiom:
test:
helm unittest .Impact: the only chart-layer guard this PR relies on is dead. Recreate can be reintroduced with fully green CI. This is a pre-existing gap inherited from #3040, but the PR touches this exact file and makes a load-bearing claim about it, so it belongs here — and the two-line fix revives the suite for free. This is a recurring shape in this repo (packages/system/linstor had the same dead suite until #2987 accidentally revived it).
B3: maxSurge: 0 does not provide Recreate's exclusivity, and the comment says it does
The comment claims maxSurge: 0 "tears the old manager down before the new one starts" and the PR body calls it "the exact guarantee #3040 wanted". The guarantee is weaker than that.
Evidence: rolloutRecreate gates scale-up on oldPodsRunning(newRS, oldRSs, podMap), which counts pods by non-terminal Status.Phase — a Terminating pod is still Running, so Recreate waits for the old pod to actually be gone. rolloutRolling has no such gate; NewRSNewReplicas computes currentPodCount from GetReplicaCountForReplicaSets, which sums rs.Spec.Replicas (desired), not live pods. So with replicas: 1, maxSurge: 0, maxUnavailable: 1 the old RS is scaled to 0, and on the next sync currentPodCount is already 0 and the new pod is created while the old pod is still terminating — bounded here by terminationGracePeriodSeconds: 10, during which the old manager drains in-flight reconciles with no lease coordination.
Impact: small in practice, and not a reason to change the approach — but it is the claim the whole design rests on, so it should state what is actually guaranteed. What maxSurge: 0 buys is that the old manager is sent SIGTERM before the new pod is created, rather than the pre-#3040 default where a surge pod started while the old manager was still fully live. That is a real improvement over RollingUpdate 25%, just not equivalence with Recreate.
Fix: reword to something like "maxSurge: 0 means the old manager is torn down before the new pod is created, rather than overlapping with it for a full startup as the default 25% surge would; a terminating old manager can still briefly overlap the new one within terminationGracePeriodSeconds."
Non-blocking follow-ups
packages/system/ouroboros/charts/ouroboros/templates/controller-deployment.yaml:16-23has the identical latent transition: it gatesstrategy: {type: Recreate}oncontroller.mode == "external-dns"and ships no strategy otherwise, whilecontroller.modeis a documented, user-settable value defaulting tocoredns. Togglingcoredns -> external-dnson a live release addsRecreateover a server-defaultedrollingUpdate— the same shape this PR fixes. Out of scope here; worth its own issue.make updatedoes not currently reproduce the committed vendored template. The patch's own hunks apply cleanly to pristine upstream 2.8.0, but the committed template also carries hand-edits not captured inpatches/(manager.extraEnvnindent 10vs upstream8,manager.imagePullSecretsnindent 8vs6, and asecurityContextspacing difference), so a re-vendor would silently revert them. Pre-existing onmainand not this PR's doing — but it makes the body's "the regenerated patch reproduces the committed template exactly" inaccurate.- The body cites cert-manager as an in-repo precedent for
maxSurge: 0 / maxUnavailable: 1. Its chart actually shipsstrategy: {};maxSurge: 0appears only in a commented-out example invalues.yaml. The kubeovn precedent (central-deploy.yaml:13-15) is real — that one carries the point on its own.
| # an upgrade; with leader election gated off they would both reconcile without lease | ||
| # coordination. Recreate closes that window at zero availability cost on one replica. | ||
| # Leader election is gated off on a single replica, so a rollout must never run two | ||
| # managers at once. maxSurge: 0 tears the old manager down before the new one starts |
There was a problem hiding this comment.
maxSurge: 0 does not give this guarantee. rolloutRecreate gates scale-up on oldPodsRunning(), which counts pods by non-terminal Status.Phase — so Recreate waits for the old pod to actually be gone. rolloutRolling has no such gate, and NewRSNewReplicas derives currentPodCount from GetReplicaCountForReplicaSets, which sums rs.Spec.Replicas (desired), not live pods. Once the old RS is scaled to 0, the next sync creates the new pod while the old one is still terminating (up to terminationGracePeriodSeconds: 10 here), draining in-flight reconciles without lease coordination.
The strategy value is still the right call — please just state the real guarantee: the old manager is torn down before the new pod is created (unlike the default 25% surge, which starts a second pod while the old manager is fully live), with a brief terminating overlap still possible.
There was a problem hiding this comment.
Correct, and I A/B'd it on kind rather than argue: with maxSurge: 0, two pods coexisted within 1s of a rollout (old Terminating, new ContainerCreating); with Recreate, it held at one pod for the entire drain. Your reading of oldPodsRunning vs NewRSNewReplicas is exactly what the behaviour shows.
One refinement in your direction, though: I think "sent SIGTERM before the new pod is created" still overstates it, so I did not use that phrasing. The deployment controller only orders the ReplicaSet desired counts — it scales the old RS to 0 before it can scale the new one up (rolloutRolling tries scale-up first, and maxSurge: 0 structurally blocks it). Pod deletion, kubelet SIGTERM delivery and new-pod creation are all downstream and asynchronous, so the real-time ordering is incidental rather than guaranteed.
The comment now claims only the guaranteed part:
# maxSurge: 0 caps total pods at replicas, so the old manager is
# scaled down before the new one is scaled up (maxUnavailable: 1 permits the gap),
# unlike the default 25% surge which starts a second manager while the first is fully
# live. This is not Recreate's exclusivity: the old manager may still be draining
# in-flight reconciles (up to terminationGracePeriodSeconds) as the new one starts.
| # managers at once. maxSurge: 0 tears the old manager down before the new one starts | ||
| # (maxUnavailable: 1 permits the brief gap) — Recreate-like exclusivity while keeping | ||
| # type RollingUpdate, so upgrades from a chart that shipped the default RollingUpdate | ||
| # never hit "spec.strategy.rollingUpdate: Forbidden ... when strategy type is Recreate". |
There was a problem hiding this comment.
This attributes the failure to Helm's client-side 3-way merge, which is disproven. DeploymentSpec.Strategy carries patchStrategy:"retainKeys" (k8s.io/[email protected]/apps/v1/types.go:395), and CreateThreeWayMergePatch over (v1.5.x manifest, #3040 manifest, live 25%/25% object) emits {"spec":{"strategy":{"$retainKeys":["type"],"type":"Recreate"}}}, which merges to a valid {"strategy":{"type":"Recreate"}} — rollingUpdate is cleared.
The path that actually produces the error is server-side apply (no $retainKeys, and the server-defaulted rollingUpdate is unowned), which helm-controller v1.5.0 uses by default here. Worth correcting in the commit message too, since it is what a future reader will find.
There was a problem hiding this comment.
You're right, and this was my error. I checked it both ways before rewriting rather than take it on faith, and both agree with you.
CreateThreeWayMergePatch with this repo's apimachinery emits exactly what you quoted, and on kind a client-side kubectl apply of the v1.5.x manifest followed by the Recreate manifest succeeds, leaving {"type":"Recreate"} — rollingUpdate is cleared, so the client-side path cannot produce the error.
The SSA path reproduces it verbatim. After kubectl apply --server-side --field-manager=helm-controller of the strategy-less manifest, the apiserver defaults rollingUpdate to 25%/25% and managedFields shows the applier owning f:replicas, f:selector and f:template but not f:strategy; the same-manager apply of type: Recreate then fails with the exact production error. So the mechanism is the one you named, confirmed rather than inferred.
The commit message and all three comments now describe the SSA field-ownership mechanism, and the PR body carries a note crediting the correction. Your self-refutation argument was what made it airtight — ouroboros would have to be broken today under my theory, and it isn't. It is latently broken under the real one: filed as #3324.
| - equal: | ||
| path: spec.strategy.type | ||
| value: Recreate | ||
| value: RollingUpdate |
There was a problem hiding this comment.
This assertion never runs. hack/helm-unit-tests.sh executes a package's suite only when the package Makefile defines a test: target, and packages/system/opensearch-operator/Makefile does not (hack/package.mk does not supply one either) — so a revert to Recreate would not fail the unit suite, contrary to the PR body.
Two lines in the package Makefile revive it, matching the idiom in packages/system/etcd-operator/Makefile:
test:
helm unittest .There was a problem hiding this comment.
Confirmed and fixed. make -C packages/system/opensearch-operator -n test returned No rule to make target 'test', exactly as you said — the suite had never executed, so the coverage claim in the body was empty.
Added the target using your snippet (matching etcd-operator). The repo-wide runner now picks the package up, and I mutation-checked it rather than assume: reverting the template to Recreate fails the suite with unknown path spec.strategy.rollingUpdate.maxUnavailable. So the guard is real now.
I left .PHONY off to match the dominant idiom here (66 packages define a bare test:, 16 declare .PHONY). Unrelated, but noticed while checking: hack/package.mk:2 writes .PHONY=help show diff ... — an assignment, not the .PHONY: special target, so it is a no-op today.
74caf9e to
518e227
Compare
|
Thanks — this was a genuinely good review. I verified all three blockers and all three follow-ups independently instead of taking them on faith, and every one of them held. Details are in the inline replies; summary of what changed: B1 — my root cause was wrong. Confirmed both ways: the client-side 3-way merge emits B2 — the suite had never run. Added the B3 — you were right that Follow-ups:
The strategy value itself is unchanged, since your review agreed it was the right call. What changed is the reasoning shipped with it — which is the part that would have misled the next reader. The PR body now has a Verification table with the full kind reproduction (SSA fails / client-side succeeds / fix succeeds / rc-recovery succeeds / rollout A/B). |
…reate #3040 set the single-replica operator Deployment to strategy.type: Recreate to avoid two uncoordinated managers overlapping during a rollout while leader election is gated off. That breaks Helm upgrades of existing releases: the upgrade fails validation immediately, helm-controller retries it (x19 over 11m in Actions run 29480724982), and the HelmRelease never becomes Ready: Deployment.apps "opensearch-operator-controller-manager" is invalid: spec.strategy.rollingUpdate: Forbidden: may not be specified when strategy type is Recreate The trigger is server-side apply. A v1.5.x release shipped no strategy at all, so the apiserver defaulted spec.strategy.rollingUpdate to 25%/25%, and the helm-controller field manager never owned that block. helm-controller applies server-side by default (Install.ServerSideApply defaults to true, Upgrade.ServerSideApply to "auto"; the operator's package_reconciler.go sets neither), and SSA does not remove a field the applier never owned merely because the new intent omits it -- so sending type: Recreate leaves the defaulted rollingUpdate in place and the apiserver rejects the merged object. The client-side path is unaffected: DeploymentSpec.Strategy carries patchStrategy:"retainKeys", so a 3-way merge emits {"$retainKeys":["type"],"type":"Recreate"} and does clear rollingUpdate. That is why this surfaces through helm-controller's server-side apply but not a local 'helm upgrade'. Keep type: RollingUpdate and set rollingUpdate.maxSurge: 0 / maxUnavailable: 1 on the single-replica path instead. maxSurge: 0 caps total pods at replicas, so the old manager is scaled down before the new one is scaled up, rather than starting a surge pod while the old manager is still fully live as the default 25% would. It is not Recreate's exclusivity -- the deployment controller orders the ReplicaSet desired counts, not the pod lifecycles, so a draining old manager can still overlap the new pod -- but it keeps the type transition legal on every apply path, and recovers releases already on an rc build with Recreate. This matches the existing kube-ovn idiom (central-deploy.yaml). Also add a 'test' target to the package Makefile: hack/helm-unit-tests.sh runs a package's suite only when its Makefile defines one, so tests/leader_election_test.yaml had never executed. Multi-replica installs keep the default RollingUpdate (leader election on), unchanged. The change is carried in patches/leaderElection.diff so it survives 'make update'. Verified on kind v1.33.1: SSA of the v1.5.x manifest followed by SSA of the Recreate manifest reproduces the error, while the same pair applied client-side succeeds; SSA of the maxSurge:0 manifest succeeds both from v1.5.x and from a live Recreate Deployment. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
518e227 to
955f7cc
Compare
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
The change replaces strategy.type: Recreate with RollingUpdate / maxSurge:0 / maxUnavailable:1 on the single-replica path; the direction is correct, the render corners are valid, the regression test is now wired in and non-vacuous, and the only residual items are verification that cannot be executed in a hermetic static review.
Caveats
- SSA upgrade merge is the crux of the fix and is not statically verifiable. The bug (
spec.strategy.rollingUpdate: Forbiddenwhentype: Recreatemerges onto an apiserver-defaulted, helm-controller-unownedrollingUpdateblock under server-side apply) and the two recovery paths (upgrade from no-strategy v1.5.x, and recovery from an rc build already onRecreate) require a live apiserver. This review is hermetic (no cluster).helm templaterenders a valid object on every corner and the SSA field-ownership mechanism described in the PR body is internally sound, but the actualRecreate → RollingUpdateandno-strategy → RollingUpdate maxSurge:0SSA merges are relied upon from the author's kind v1.33.1 reproduction, not independently executed here. Recommend the release-upgrade E2E lane (#3276) as the authoritative gate. - Residual overlap window is a documented, accepted trade-off.
maxSurge:0does not giveRecreate's exclusivity: a terminating old manager can overlap the new pod for up toterminationGracePeriodSeconds(10s here) with no lease coordination, since leader election is gated off on one replica. This is now correctly stated in both the template comment and the PR body. Impact is a ~10s double-reconcile risk on a single-replica operator, far less severe than the permanently-failing upgrade it replaces. Non-blocking. - charts-direct-edit invariant is mitigated. The edit under
packages/system/opensearch-operator/charts/.../opensearch-operator-controller-manager-deployment.yamlis captured inpatches/leaderElection.diffand theMakefileupdatetarget re-applies it. Template and patch were edited symmetrically in this diff, so the change survivesmake update.
Verification performed
helm unittest .green baseline (2 tests). Non-vacuity confirmed by mutation: reverting the template totype: Recreateturns the suite RED (spec.strategy.rollingUpdate.maxSurge/maxUnavailableunknown path). The render-layer guard genuinely protects against a naive revert.- Config corners rendered: default and
replicaCount=1→RollingUpdate maxSurge:0/maxUnavailable:1;replicaCount=2/3→ strategy block entirely absent (apiserver default) with--leader-electpresent. Both branches valid. - Precedent
packages/system/kubeovn/charts/kube-ovn/templates/central-deploy.yaml:10-14confirmed to use the identicalRollingUpdate maxSurge:0/maxUnavailable:1idiom. - Round-trip check: #3040 set
Recreate; this PR reverts it. The reversal premise is re-verified, not blindly flipped — the overlap window #3040 cared about is narrowed viamaxSurge:0rather than fully reverting to the 25% default, andRecreateis left because of a hard SSA constraint. Justified.
Recommended follow-ups
- Land the fix behind the release-upgrade E2E lane (#3276) which reproduces the SSA merge on a real apiserver — that is the authoritative behavioural regression test, since helm-unittest cannot exercise SSA.
- Author already filed #3324 (ouroboros has the same latent
Recreate-on-mode-toggle shape) and #3325 (make updatedoes not fully reproduce this package's vendored template). Both are correctly scoped out of this PR.
lexfrei can't update his review and I addressed his findings
3 participants
What this PR does
Fixes a Helm-upgrade failure in the
opensearch-operatorchart that blocks the new release-upgrade E2E lane (added in #3276).Upgrading from a chart that shipped no explicit strategy (v1.5.x) to a v1.6.0-rc build fails validation immediately, helm-controller retries it, and the HelmRelease never becomes Ready:
Observed in the "Upgrade E2E Test" job of run 29480724982 —
UpgradeFailed ... (x19 over 11m), with the HelmRelease never reaching Ready.Root cause
#3040 (commit db797b1) set the single-replica operator Deployment to
strategy.type: Recreate— a reasonable goal: with leader election gated off on one replica, a defaultRollingUpdatebriefly runs two uncoordinated managers (maxSurge) during a rollout, andRecreatecloses that window.The problem surfaces only on upgrade from an older chart, under server-side apply:
spec.strategy.rollingUpdateto25%/25%, and thehelm-controllerfield manager never owned that block. ItsmanagedFieldsentry coversf:replicas,f:selectorandf:template, but notf:strategy.Install.ServerSideApplydefaults to true,Upgrade.ServerSideApplytoauto;internal/operator/package_reconciler.gosets neither), and SSA does not remove a field the applier never owned merely because the new intent omits it.type: Recreatemerges onto a live object that still carries the defaultedrollingUpdate, and the apiserver rejects the result withFieldValueForbidden.The client-side path is unaffected, which is why this surfaces through helm-controller's SSA but not a local
helm upgrade:DeploymentSpec.StrategycarriespatchStrategy:"retainKeys"(k8s.io/[email protected]/apps/v1/types.go:395), so a 3-way merge emits{"spec":{"strategy":{"$retainKeys":["type"],"type":"Recreate"}}}, which merges to a valid{"strategy":{"type":"Recreate"}}and does clear the block.This is a genuine product upgrade bug, not a test-harness issue — #3276 only adds the upgrade lane that exposes it; it does not touch
opensearch-operator.The fix
Keep
type: RollingUpdateon the single-replica path and setrollingUpdate.maxSurge: 0 / maxUnavailable: 1instead of switching toRecreate:maxSurge: 0caps total pods atreplicas, so the deployment controller must scale the old manager down before it can scale the new one up — unlike the default 25% surge, which starts a second manager while the first is still fully live.Recreate's exclusivity, and the PR no longer claims it is.rolloutRecreategates scale-up onoldPodsRunning(), which counts non-terminalStatus.Phase, soRecreatewaits for the old pod to be gone.rolloutRollinghas no such gate:NewRSNewReplicasderivescurrentPodCountfromGetReplicaCountForReplicaSets, which sumsrs.Spec.Replicas(desired, not live). The ordering that is guaranteed is over ReplicaSet desired counts, not pod lifecycles, so a draining old manager can still overlap the new pod withinterminationGracePeriodSeconds(10s here).strategy.typenever becomesRecreate, the forbidden merge cannot occur on any apply path. It also recovers anyone already on an rc build withRecreate, sinceRecreate -> RollingUpdateis permitted.packages/system/kubeovn/charts/kube-ovn/templates/central-deploy.yaml:11-15usesRollingUpdatewithmaxSurge: 0 / maxUnavailable: 1.Multi-replica installs (
manager.replicaCount > 1) are unchanged: they keep the defaultRollingUpdatewith leader election enabled.The change is carried in
patches/leaderElection.diffso it survives a chart re-vendor viamake update; the patch applies to pristine upstream 2.8.0 with no fuzz and reproduces the committed strategy block exactly.Alternatives considered
Recreate, but make the chart ownrollingUpdatefirst (ship an explicit block, then switchtypein a later release)patch/deleteDeployments, and hook ordering; runs on every upgrade even when unneeded; adds standing attack surface — disproportionate for a scalar field transition.helm.sh/resource-policy/ force-replaceresource-policy: keeponly governs deletion on uninstall (irrelevant). Helm has no per-resource "force recreate" annotation;--force/ HelmReleasespec.upgrade.forceis global and disruptive and not controllable from within the chart.RollingUpdate(25%/25%)maxSurge: 0narrows it to a draining old pod without leavingRollingUpdate.Verification
The mechanism and the fix were reproduced on kind v1.33.1 (a bare apiserver is enough — the failure is defaulting + SSA + validation):
rollingUpdate: 25%/25%;managedFieldsshowshelm-controllerdoes not ownf:strategytype: RecreaterollingUpdateis cleared, live strategy becomes{"type":"Recreate"}maxSurge: 0(this PR)maxSurge: 0onto a liveRecreateDeployment (rc recovery)maxSurge: 0vsRecreatemaxSurge: 0: two pods coexisted within 1s (old Terminating, new ContainerCreating).Recreate: one pod, waited the full drain. This is the evidence for the "not exclusivity" wording above.Note on regression coverage
tests/leader_election_test.yamlwas never executed:hack/helm-unit-tests.shruns a package's suite only when the package Makefile defines atest:target, and this one did not — so the coverage the previous revision of this PR claimed did not exist. This PR adds the target (matchingpackages/system/etcd-operator/Makefile), which revives the suite; the repo-wide runner now picks the package up and a revert toRecreatefails it. Thanks to Aleksei Sviridkin (@lexfrei) for catching that.The apiserver-side merge that produces the invalid object still needs a live cluster, so the behavioural regression test remains the release-upgrade E2E lane in #3276 (which this fix unblocks).
Follow-ups filed
packages/system/ouroboroshas the same latent shape (Recreategated oncontroller.mode == "external-dns", whilecontroller.modedefaults tocoredns). Not fixed here, and it likely needs a different fix, since that mode genuinely depends on exclusivity.make updatedoes not reproduce this package's vendored template: three hand-edits are not captured inpatches/, and one of them breaks rendering whenmanager.extraEnvis set. Pre-existing onmain; an earlier revision of this body claimed the regeneration was exact, which was wrong.Downstream repositories
Release note
Summary by CodeRabbit
make testtarget to run Helm unit tests.