fix(controller): Keep pod-template-hash stable across k8s library upgrade by aligning with the upstream Deployment controller.#4755
Conversation
Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
Published E2E Test Results 4 files 4 suites 3h 53m 51s ⏱️ For more details on these failures, see this check. Results for commit dd62288. ♻️ This comment has been updated with latest results. |
Published Unit Test Results2 482 tests 2 482 ✅ 3m 20s ⏱️ Results for commit dd62288. ♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4755 +/- ##
=======================================
Coverage 85.10% 85.11%
=======================================
Files 164 164
Lines 18998 18987 -11
=======================================
- Hits 16169 16160 -9
+ Misses 1987 1986 -1
+ Partials 842 841 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
…ash-k8s-1.34-changes
Signed-off-by: Zach Aller <zachaller@users.noreply.github.com>
…ash-k8s-1.34-changes
|
|
It seems that Flagger has a similar problem fluxcd/flagger#1673 |
|
It would be great if we also include testing scenarios. At the very least
|



Fixes #4696
Summary
Upgrading the controller across a Kubernetes library bump (v1.8.x → v1.9.0, i.e. k8s
v0.29→v0.34) causes every Rollout to recompute a differentrollouts-pod-template-hash. That spuriously creates a new revision which progresses through the canary steps and parks at the first indefinite pause, even though nothing in the user'sspec.templateactually changed.This PR removes the correctness dependency on byte-stable hashing (the hash-label lookup remains as a fast path) and aligns Argo Rollouts with how the upstream Kubernetes Deployment controller decides ReplicaSet identity:
pod-template-hashwith the upstreamcontroller.ComputeHash(spew/reflect based) instead ofjson.Marshal, so the hash is immune to JSON-serialization changes.PodTemplateEqualIgnoreHash) when no ReplicaSet carries the computed hash label, exactly likedeploymentutil.FindNewReplicaSet. The hash-label lookup remains the first (fast) tier: the label was written by this controller from the same desired template, so a match is authoritative even when the live template has since diverged (apiserver defaulting from a newer server, or mutating admission on ReplicaSets).The net effect: a controller/library upgrade is a no-op for existing Rollouts — the existing ReplicaSet is adopted (keeping its original hash label), so no new revision and no redeploy.
Background / root cause
hash.ComputePodTemplateHashpreviously computed an FNV-32a sum overjson.Marshal(template). The hash function itself didn't change between releases — the Kubernetes libraries did:In apimachinery
v0.34, the JSON struct tag onmetav1.ObjectMeta.CreationTimestampgainedomitzero(kubernetes/kubernetes#130989):A pod template's
creationTimestampis always the zero value. Previously it serialized as"creationTimestamp":null; withomitzero(honored by Go 1.24+) it is omitted entirely. That single byte change flips the JSON-based hash for 100% of Rollouts on upgrade. This is a recurrence of the long-standing class of bug in #88.How the upstream Deployment controller avoids this
The Deployment controller never had this problem because:
controller.ComputeHashfeeds the template throughhashutil.DeepHashObject(thespewlibrary, reflect-based). JSON struct tags such asomitzeroare irrelevant to it.deploymentutil.FindNewReplicaSetiterates ReplicaSets and compares withEqualIgnoreHash(a semanticapiequality.Semantic.DeepEqualof the pod templates). Even if the hashing algorithm changes, an existing ReplicaSet is still recognized as current.This PR brings Argo Rollouts in line with both behaviors, but deliberately keeps the hash-label lookup as a fast first tier. Unlike the in-tree Deployment controller, Argo Rollouts is not version-locked to the apiserver: a newer apiserver can default pod-template fields our vendored libraries don't know, and mutating admission can rewrite ReplicaSet templates. In both cases semantic comparison would fail forever; the hash label — written by this controller from the same desired template — still matches and prevents an unwarranted redeploy (and
collisionCountchurn).Changes
Production code
utils/hash/hash.go—ComputePodTemplateHashnow delegates to the upstream Deployment controller's hash:This makes the hash spew-based (matching Deployment/ReplicaSet labeling going forward) and immune to JSON-tag changes like
omitzero.utils/replicaset/replicaset.goFindNewReplicaSetis reduced from three tiers to two: (1) lookup bypod-template-hashlabel computed with the current algorithm, then (2) semantic pod-template equality, mirroringdeploymentutil.FindNewReplicaSet. The "deprecated hash" tier is gone (our hash iscontroller.ComputeHashnow, so the two label searches collapsed into one). When the fallback adopts a ReplicaSet whose label doesn't match the computed hash, anInfofis logged so operators can observe the one-time transition.status.currentPodHashis preferred when it still matches the desired template; otherwise the oldest match is reused (upstream behavior). This matters when several retained ReplicaSets carry identical templates with different hash labels — the fleet state v1.9.0 created by spuriously duplicating revisions (Upgrading controller to v1.9.0 triggers spurious new revisions and Canary pause-for-promotion on all Rollouts due to pod template hash change from k8s library bump (#4497) #4696). Without the preference, oldest-first would resurrect the scaled-down pre-1.9.0 twin and trigger a second redeploy for rollouts whose spurious revision was already promoted. On a genuine template change the current ReplicaSet no longer matches semantically, so the preference is a no-op and rollback-reuse behaves exactly like the upstream Deployment controller.ReplicaSetTemplateMatchesRolloutencapsulates the rollouts-specific normalization before the semantic compare: strips injected ephemeral canary/stable metadata and the injected anti-affinity rule from the live template. Used by bothFindNewReplicaSetand the hash-collision check (seerollout/sync.gobelow).PodTemplateEqualIgnoreHashnow runs both the live and desired templates through library defaulting (defaultPodTemplateSpec) beforeSemantic.DeepEqual. Previously only the desired side was defaulted; defaulting both keeps the comparison stable when a live ReplicaSet was created by an older library that defaulted fewer fields. Thespec.serviceAccountcarve-out from #1356 is unchanged.CheckPodSpecChangeis unchanged in behavior. Its control flow is identical to before (comparestatus.CurrentPodHashagainst the adopted RS's label, or against the freshly computed hash when no RS is found); only the underlying hash implementation changed. Doc comment expanded to explain why adopting via label keeps detection stable across the upgrade.GenerateReplicaSetAffinityandIfInjectedAntiAffinityRuleNeedsUpdatenow take the ReplicaSet'spod-template-hashlabel (podHash) and compare it againststatus.StableRS, instead of recomputing a hash from the desired template. Both are stored label values of the same provenance, so the "are we mid-rollout?" decision is not skewed when the stored stable hash predates the hashing change. See "Edge cases handled" below.rollout/sync.goreplicasetutil.GetPodTemplateHash(c.newRS)podTemplateSpecHash(computation reordered to occur before the call)AlreadyExistshash-collision check increateDesiredReplicaSetnow usesReplicaSetTemplateMatchesRolloutinstead of comparing the raw live template against the rollout template. Canary/blue-green ReplicaSets are created with ephemeral metadata injected, so the raw comparison could misread the controller's own ReplicaSet as a hash collision and bumpcollisionCount(a pre-existing bug, fixed here because this path is adjacent to the matching changes). Hash matching is deliberately not used in this path — a genuine collision has a matching hash by definition, so only the semantic check is valid there.rollout/experiment.go—GetExperimentFromTemplatenow prefers the new ReplicaSet's storedpod-template-hashlabel for the experiment's name and hash label, falling back to the computed hash only when there is no ReplicaSet yet. This keeps experiments created mid-transition consistent with the adopted ReplicaSet they run against (matching howrollout/analysis.goalready derives its labels).Edge cases handled
FindNewReplicaSetadopts the existing ReplicaSet via the semantic fallback; the status keeps the adopted RS's original (json-era) label;CheckPodSpecChangeseesstatus.CurrentPodHash == newRS.label→ no change → no new revision.FindNewReplicaSetreturns nil →CheckPodSpecChangereports a change → a new ReplicaSet is created and labeled with the new spew hash.collisionCountchurn.status.currentPodHash) is adopted rather than its older identical-template twin, so upgrading to this version is a no-op instead of a second redeploy. Whenstatus.currentPodHashmatches no retained ReplicaSet, the oldest semantic match is reused, as upstream does.status.StableRS(equal for the stable RS) rather than a recomputed hash.Tests
utils/hash/hash_test.go— replaced the brittle golden-value assertion (which had to be updated on every k8s bump) withMatchesDeploymentControllerComputeHash, asserting our hash equalscontroller.ComputeHash.utils/replicaset/replicaset_test.goTestFindNewReplicaSetnow covers both tiers: lookup by current hash label, and semantic adoption when the label is stale.TestFindNewReplicaSetAcrossHashChange(new) — an unchanged template whose RS carries a stale hash label is adopted; a real spec change is not; with twin identical-template ReplicaSets, the one recorded instatus.currentPodHashwins over an older twin, with oldest-first as the fallback. Stable across future k8s upgrades (no hashes to update).TestFindNewReplicaSetWithDivergedLiveTemplate(new) — pins the tier-1 protection: an RS whose live template was mutated after creation (simulated webhook-injected annotation) is still found via its matching hash label.TestPodTemplateEqualIgnoreHashAcrossLibraryDefaulting(new) — an under-defaulted live template (older library) still matches the semantically identical desired template.TestGenerateReplicaSetAffinity/TestIfInjectedAntiAffinityRuleNeedsUpdate/TestRemoveInjectedAntiAffinityRuleupdated for the newpodHashparameter.TestGenerateReplicaSetAffinityStableAcrossHashChange(new) — pins the anti-affinity upgrade edge case: a stable Rollout whoseStableRSis an old-format label (deliberately!=the recomputed hash) gets no injection.rollout/controller_test.go—TestPodTemplateHashEquivalencerewritten to assert the runtime-relevant property (two equivalent quantity formats compare equal viaPodTemplateEqualIgnoreHash, so they are adopted, not redeployed) plus a self-consistent RS-name sanity check, instead of a hardcoded hash.rollout/ephemeralmetadata_test.go— the "ReplicaSet already patched" fixtures now set the ephemeral-metadata annotation and preserve realistic template labels, so semantic adoption works (these previously relied on label-based lookup).utils/conditions/rollouts_test.goandutils/experiment/experiment_test.go— compute expected hashes dynamically instead of pinning literal values that drift on every k8s bump.test/e2e/canary_test.go— the three hardcodedpod-template-hashselector assertions (which carried// NOTE: This must be updated for every k8s version upgrade) are replaced withassertCanarySelectorPointsToStable, a helper that reads the expected hash from the live stable (revision 1) ReplicaSet. Never needs updating on a hashing/library change.Behavior / compatibility
spec.templateis unchanged. Existing ReplicaSets keep their original hash labels; services, selectors,status.stableRS, andstatus.currentPodHashcontinue to use those values. The new spew hash is only used when a genuinely new ReplicaSet is created (a real template change). Old and new ReplicaSets can coexist with different hash-label formats until the old ones are scaled down — adoption is by template, not by label format.CheckPodSpecChangemid-rollout step-reset semantics are preserved.2000mvs2) is treated as equal bySemantic.DeepEqual(quantities compare by value), so it does not cause churn. Note the spew-based hash itself is not format-stable for quantities — adoption relies on the label/semantic matching above, not on hash stability.Upgrading from v1.9.0 specifically
v1.9.0 already shipped the broken hash, so fleets are not in a clean pre-bug state — the spurious revisions it created leave twin ReplicaSets (identical templates, different hash labels) in revision history. What this version does for each state:
status.currentPodHashrecords.status.currentPodHashpoints at the spurious ReplicaSet, which is adopted — the rollout stays exactly as v1.9.0 left it (paused awaiting promote/abort). No automatic state change on controller upgrade.status.currentPodHashpoints at the running ReplicaSet, which is adopted — the upgrade is a no-op. The scaled-down pre-v1.9.0 twin remains in revision history until it ages out viarevisionHistoryLimit.In all cases adopted ReplicaSets keep their existing (json-era) hash labels permanently; only genuinely new revisions get spew-format names/labels. Services, traffic routing, and analysis key off stored labels, so mixed label formats coexist safely — but external tooling that pattern-matches hash values (dashboards, log queries) will see both formats during the transition.
Notes / tradeoffs
DeepEqualfallback (defaulting + deep compare per ReplicaSet) only runs when the O(1) hash-label lookup misses — i.e. once per genuine template change or hashing transition — so steady-state reconcile cost is unchanged.GenerateReplicaSetAffinityandIfInjectedAntiAffinityRuleNeedsUpdategained apodHashparameter — a signature change to two exported functions inutils/replicaset. All in-tree callers are updated; external importers (unlikely) would need to pass the ReplicaSet's hash.pod-template-hashvalues in tests no longer match; affected unit fixtures now compute hashes dynamically and the e2e selector assertions read the live value. There are intentionally no new golden-hash assertions to maintain.Validation
go build ./...— clean.go vet -tags e2e ./test/e2e/— clean.utils/hash,utils/replicaset,utils/conditions,utils/experiment,rollout,experiments.Checklist
"fix(controller): Updates such and such. Fixes #1234".