Repository navigation
fix(utils): sort nodeSelector keys in MergeNodeSelectorAndNodeAffinity to guarantee deterministic order - #6217
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Hi @frenemy17. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with 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/test-infra repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6217 +/- ##
=======================================
Coverage 65.61% 65.62%
=======================================
Files 486 486
Lines 34361 34367 +6
=======================================
+ Hits 22546 22552 +6
Misses 10061 10061
Partials 1754 1754 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/assign @TrafalgarZZZ |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, preserves selector semantics, and includes focused regression coverage with no unresolved findings.
No open issues 🎉
What changed in this PR
This PR fixes #6216 by making node-selector expression ordering deterministic in the shared affinity helper, preventing unnecessary status updates caused by fluctuating slice order.
Changes:
- Sort node-selector keys before building match expressions, preserving scheduling semantics.
- Add sorted-order coverage and a regression test checking consistency across 100 repeated calls.
| File | Description |
|---|---|
pkg/utils/kubeclient/pod.go |
Sorts selector keys to produce deterministic match expressions. |
pkg/utils/kubeclient/pod_test.go |
Tests alphabetical ordering and repeated-call consistency. |
Review effort: Balanced
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Verified from the ground up: the premise behind this fix is real. On the merge-base, a multi-label nodeSelector makes MergeNodeSelectorAndNodeAffinity emit MatchExpressions in Go's randomized map order, so the reflect.DeepEqual status gate in pkg/ddc/*/status.go sees a phantom diff every cycle — we reproduced it independently with two harnesses (6 distinct key orderings per 1000 calls on base; 172/200 flapped comparison cycles and 26/30 real status writes per 30 reconcile cycles against an envtest API server). On this head both harnesses converge: sorted, byte-stable output on both generation paths, and exactly one status write before convergence. The sort is the right minimal fix — expression order carries no scheduling meaning inside a NodeSelectorTerm — the PR ships its own regression tests, caller packages show no new failures versus base, and CI is fully green at the reviewed head.
Two immaterial nits and one small coverage suggestion, none blocking: the PR body names a nonexistent status.WorkerNodeAffinity field (only status.CacheAffinity exists); the new tests pin the fresh-terms path but not the append-to-existing-affinity branch; and the same unsorted map-range pattern survives pre-existing in the cache-affinity webhook and two PV-creation sites — worth a tracking issue rather than changes here.
Next round
[MUST] (none — merge is not gated on any finding)
[SHOULD] Add one multi-label + existing-NodeSelectorTerms table entry to pin the append path (pod_test.go, mirroring the 'exist node affinity' entry)
[SHOULD] Correct status.WorkerNodeAffinity → status.CacheAffinity in the PR body
[SHOULD] File a tracking issue for the pre-existing unsorted map-range sites (node_affinity_with_cache.go:138,174; require_node_with_fuse.go:93; dataset/volume/create.go:119; efc/create_volume.go:150) and consider a shared sorted-keys helper
[Filed] none
Reviewer disagreement: none — every finding from both reviews was confirmed in cross-critique, and Reviewer C (Copilot) reviewed this head without raising any findings.
Non-blocking observations (4) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/webhook/plugins/nodeaffinitywithcache/node_affinity_with_cache.go:174 [minor] — pre-existing, not introduced by this PR — Pre-existing: cache-affinity webhook composes injected terms by iterating maps, so multi-dataset pods can get NodeSelectorTerms in varying order across admissions
Same class of nondeterminism as issue #6216, one layer down in the consumer of status.CacheAffinity: getTieredLocalityNodeSelectorTerms iteratesfor name, runtimeInfo := range runtimeInfos(line 174, required path) and getTieredLocalityPreferredSchedulingTerms iteratesrange preferredRuntimes(line 138, preferred path), so for pods bound to multiple datasets the order of injected NodeSelectorTerms varies between admissions even after this PR. NodeSelectorTerms are OR-ed and the inner MatchExpressions now come from the sorted status.CacheAffinity, so scheduling semantics are unchanged and no reconcile loop exists — but the emitted pod spec term order is nondeterministic. Present on the merge-base (pkg/webhook/ untouched by this PR); not introduced by this PR. Codex confirmed in cross-critique and independently corroborated the pre-existing attribution (empty base..head diff on pkg/webhook/). Tracking-issue material, not a hold on this PR. - pkg/utils/dataset/volume/create.go:119 [nit] — pre-existing, not introduced by this PR — Pre-existing: same unsorted map-range pattern survives in PV node affinity creation (producer side)
pkg/utils/dataset/volume/create.go:119 and pkg/ddc/efc/create_volume.go:150 build pv.Spec.NodeAffinity.Required.NodeSelectorTerms[0].MatchExpressions by ranging over runtime.GetFuseNodeSelector() without sorting — the exact pattern this PR fixes in MergeNodeSelectorAndNodeAffinity. Benign today: those PVs are create-only (guarded by IsPersistentVolumeExistWithContext, no compare-then-update path in either file — verified by both reviewers) and scheduler semantics are order-insensitive. Not this PR's scope; a shared sorted-keys helper would prevent further copies. Claude's cross-critique addendum: at least two more pre-existing sites exist (pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse.go:93 and the nodeaffinitywithcache webhook above), so this is really the fourth and fifth copy of the pattern — strengthening the shared-helper recommendation without changing the verdict. - pkg/utils/kubeclient/pod_test.go:416 [minor] — PR's own suite lacks a multi-label + pre-existing-affinity (append path) case
The two new tests cover only the podAffinity==nil generation path (table entry at line 416 and the 100-invocation determinism It at line 441); the only pre-existing entry reaching the append branch ("exist node affinity") uses a single-label map, structurally incapable of detecting an ordering regression. Determinism is equally load-bearing on the append branch (pod.go:148) where sorted expressions are appended to NodeSelectorTerms already present in the worker spec's Affinity — the realistic case when the Runtime CR specifies affinity that propagates into the worker StatefulSet template, and exactly what all 8 callers pass (workers.Spec.Template.Spec.Affinity). A future regression limited to that branch (e.g. a per-branch restructure of the key collection) would not be caught by this suite; the shared sort before the branch means a wholesale sort removal is still caught. Confirmed by Codex in cross-critique with independent corroboration (TestVerifyMergeIntoExistingTermsDeterministic fails on base, passes on head, unpinned by the PR's suite). Behavior itself is verified correct on head by both harnesses. Suggested fix: one more Entry with a multi-label nodeSelector plus a podAffinity carrying an existing NodeSelectorTerm, mirroring the existing 'exist node affinity' entry. - [nit] — nit: PR body mentions nonexistent status.WorkerNodeAffinity
The PR description says the fluctuation affected 'status.CacheAffinity or status.WorkerNodeAffinity', but no field named WorkerNodeAffinity exists in api/ or pkg/ (grep finds zero hits); the local Go variables named workerNodeAffinity in pkg/ddc/{alluxio,jindo,jindocache,jindofsx,juicefs}/status.go are assigned into runtimeToUpdate.Status.CacheAffinity, and the #6200 CacheEngine path likewise writes status.CacheAffinity (pkg/ddc/cache/engine/status.go:73) — almost certainly where the name came from. Cosmetic doc inaccuracy only; the linked issue #6216 correctly names CacheAffinity, so the auto-close target is unaffected. Confirmed by Claude in cross-critique (missed in Claude's own round).
Verified review delta
- F2-webhook-term-order-nondeterministic: open (preexisting)
- pv-nodeaffinity-map-order-siblings: open (preexisting)
- F1-coverage-append-path: open (initial-pr)
- pr-body-workernodeaffinity-field: open (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.
…y to guarantee deterministic order (fluid-cloudnative#6216) - In MergeNodeSelectorAndNodeAffinity, sort the keys of nodeSelector before building expressions to prevent Go map iteration randomization from producing matchExpressions in non-deterministic order. - This eliminates spurious status updates and controller re-reconciles for multi-label workers. - Add multi-label test entry and determinism regression tests over 100 repeated invocations in pod_test.go. Fixes fluid-cloudnative#6216 Signed-off-by: Siddhanth Sadashiv Raikar <raikarsiddhanth@gmail.com>
4fd9701 to
f5bb49a
Compare
|
|
Thank you for the review! All review findings and recommendations have been addressed:
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Round 2 on the force-pushed head f5bb49a3 (previous head 4fd9701c): the delta is test-only — +84 lines in pkg/utils/kubeclient/pod_test.go, with pod.go byte-identical — and it closes every actionable item from the last review. The base moved 86e3adf3 → e5251b39, but that is a workflows-only dependency bump (no changes under pkg/, api/, or cmd/; the PR was not rebased), so the effective merge-base and every reviewed contract are unchanged.
What changed for the better, with evidence:
- Append-path coverage added (the round-1 SHOULD): a multi-label + existing-NodeSelectorTerms table entry and a 100-invocation determinism spec now pin the append branch that all 8 callers actually exercise. The tests are real, not vacuous — grafted onto the unfixed base they fail (both new specs red; 4/10 focused), on head they pass 10/10. Two independent graft differentials agree, and the new test code was checked against the implementation (the
DeepCopyatpod.go:101makes the 101-call pointer reuse safe). - PR body corrected: the nonexistent
status.WorkerNodeAffinitymention is gone — the body now namesstatus.CacheAffinityonly. - Pre-existing sites tracked: issue #6220 (opened by the author, verified open) covers the remaining unsorted map-range sites in the cache-affinity webhook and PV creation, so they stay out of this PR's scope by design rather than by omission.
Because the base moved, the premise was re-verified rather than carried: the bug still reproduces on the new base (169/200 simulated reconcile cycles flap; 20/30 cycles issue real status writes against an envtest API server), and the head is clean (0/200 flap; exactly one status write before convergence). No new findings in the delta, no caller changes, and all required CI is green at the frozen head (only the merge tide itself pending).
Next round
[MUST] (none — merge is not gated on any finding)
[SHOULD] (none outstanding — all round-1 SHOULD items are resolved)
[Filed] #6220 (pre-existing unsorted map-range sites: node_affinity_with_cache.go:138,174; require_node_with_fuse.go:93; dataset/volume/create.go:119; efc/create_volume.go:150 — filed by the author)
Reviewer disagreement: none — both reviews converged on the same four ledger entries with the same dispositions, and every cross-critique claim was independently reproduced. Reviewer C (Copilot) reviewed this head and raised no findings.
Non-blocking observations (2) — not part of this round's ask; fix now, follow up, or file an issue as you see fit
- pkg/webhook/plugins/nodeaffinitywithcache/node_affinity_with_cache.go:174 [minor] — pre-existing, not introduced by this PR — Pre-existing: cache-affinity webhook composes injected terms by iterating maps, so multi-dataset pods can get NodeSelectorTerms in varying order across admissions
Pre-existing (introducedByPR: false; git diff base..head -- pkg/webhook/ is empty on both bases, and the base advance touched no Go code): getTieredLocalityNodeSelectorTerms iteratesfor name, runtimeInfo := range runtimeInfos(line 174, required path) and getTieredLocalityPreferredSchedulingTerms iteratesrange preferredRuntimes(line 138, preferred path), so for pods bound to multiple datasets the order of injected NodeSelectorTerms varies between admissions even after this PR. NodeSelectorTerms are OR-ed and the inner MatchExpressions now come from sorted status.CacheAffinity, so scheduling semantics are unchanged and no reconcile loop exists — but the emitted pod spec term order is nondeterministic. Confirmed by both reviewers in round 1 and unchanged this round. Deferred, not a hold on this PR. - pkg/utils/dataset/volume/create.go:119 [nit] — pre-existing, not introduced by this PR — Pre-existing: same unsorted map-range pattern survives in PV node affinity creation (producer side)
Pre-existing: pkg/utils/dataset/volume/create.go:119, pkg/ddc/efc/create_volume.go:150 and pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse.go:93 build MatchExpressions by ranging over runtime.GetFuseNodeSelector() without sorting — the exact pattern this PR fixes in MergeNodeSelectorAndNodeAffinity. Benign today: create-only paths (create.go guarded by IsPersistentVolumeExistWithContext; efc/create_volume.go uses plain Client.Create), no compare-then-update path in either file (verified by both reviewers), scheduler semantics order-insensitive. Not this PR's scope. Deferred, not a hold.
Verified review delta
- F2-webhook-term-order-nondeterministic: deferred (preexisting)
- pv-nodeaffinity-map-order-siblings: deferred (preexisting)
- F1-coverage-append-path: fixed (initial-pr)
- pr-body-workernodeaffinity-field: fixed (initial-pr)
Code: clear. CI: success; attribution: baseline. Required CI checks still apply.
Ⅰ. Describe what this PR does
In
pkg/utils/kubeclient/pod.go,MergeNodeSelectorAndNodeAffinityconvertsnodeSelectormap entries intocorev1.NodeSelectorRequirementmatch expressions. Because Go map iteration order is non-deterministic, multi-labelnodeSelectormaps resulted inMatchExpressionsslices in arbitrary order across reconciliation cycles.When controllers construct or compare status (such as
status.CacheAffinity), the fluctuating slice order caused status equality checks (reflect.DeepEqual) to detect spurious differences, triggering unnecessary API server status writes and re-reconciliation loops on every cycle.This PR sorts the keys of
nodeSelectoralphabetically before buildingMatchExpressions, guaranteeing deterministic output order across all cycles and runtimes (CacheRuntime, Alluxio, Jindo, JuiceFS, Thin, etc.).Ⅱ. Does this pull request fix one issue?
fixes #6216
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
pkg/utils/kubeclient/pod_test.go:DescribeTablefor multi-label node selector (both with and without existingNodeSelectorTerms) verifying sortedMatchExpressionsorder.Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews