[frenemy17]

Ⅰ. Describe what this PR does

Direct follow-up to #6217 and fixes #6220.

In #6216 and #6217, MergeNodeSelectorAndNodeAffinity was updated to sort nodeSelector keys alphabetically to ensure deterministic output order and prevent spurious status updates. During review of #6217, several other sites were identified where map iteration without key sorting caused non-deterministic ordering:

1. Volume creation sites:
   • pkg/utils/dataset/volume/create.go: CreatePersistentVolumeForRuntime iterated over nodeSelector map directly when building PV node affinity MatchExpressions.
   • pkg/ddc/efc/create_volume.go: createPersistentVolumeForRuntime iterated over nodeSelector map directly when building PV node affinity MatchExpressions.
2. Webhook sites:
   • pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse.go: getRequiredSchedulingTerm iterated over selectors map directly to append MatchExpressions.
   • pkg/webhook/plugins/nodeaffinitywithcache/node_affinity_with_cache.go: getTieredLocalityPreferredSchedulingTerms and getTieredLocalityNodeSelectorTerms iterated over preferredRuntimes and runtimeInfos maps directly, producing non-deterministic term order in mutated pod specs for pods bound to multiple datasets/runtimes.

This PR addresses all these sites:

• Introduces utils.ToNodeSelectorRequirements(selectors map[string]string) []corev1.NodeSelectorRequirement in pkg/utils/affinity.go, which converts selector maps into []corev1.NodeSelectorRequirement sorted alphabetically by key.
• Refactors MergeNodeSelectorAndNodeAffinity (pkg/utils/kubeclient/pod.go), CreatePersistentVolumeForRuntime (pkg/utils/dataset/volume/create.go), createPersistentVolumeForRuntime (pkg/ddc/efc/create_volume.go), and RequireNodeWithFuse webhook (pkg/webhook/plugins/requirenodewithfuse) to use utils.ToNodeSelectorRequirements.
• Refactors NodeAffinityWithCache webhook (pkg/webhook/plugins/nodeaffinitywithcache) to iterate over utils.OrderedKeys(preferredRuntimes) and utils.OrderedKeys(runtimeInfos), guaranteeing deterministic term order.

Ⅱ. Does this pull request fix one issue?

fixes #6220

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

• pkg/utils/affinity_test.go:
  • Added unit test TestToNodeSelectorRequirements covering nil map, empty map, single entry, multi-entry sorted, and 100 repeated iterations asserting deterministic ordering.
• pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse_test.go:
  • Added test case for multi-label fuse node selector verifying sorted MatchExpressions across 50 iterations.
• pkg/utils/dataset/volume/create_test.go:
  • Added test case verifying PV node affinity match expressions are sorted alphabetically for multi-label selectors.
• pkg/ddc/efc/create_volume_test.go:
  • Added test TestEFCEngine_createFusePersistentVolumeWithNodeSelector verifying PV node affinity match expressions are sorted alphabetically.
• pkg/webhook/plugins/nodeaffinitywithcache/node_affinity_with_cache_test.go:
  • Added test TestTieredLocalityDeterministicOrdering verifying that both preferred and required scheduling terms for multi-runtime pods are strictly sorted and deterministic across 50 iterations.

Ⅳ. Describe how to verify it

go test -v -gcflags="all=-N -l" ./pkg/utils ./pkg/utils/kubeclient ./pkg/webhook/plugins/requirenodewithfuse ./pkg/utils/dataset/volume ./pkg/ddc/efc ./pkg/webhook/plugins/nodeaffinitywithcache
go vet ./pkg/utils ./pkg/utils/kubeclient ./pkg/webhook/plugins/requirenodewithfuse ./pkg/utils/dataset/volume ./pkg/ddc/efc ./pkg/webhook/plugins/nodeaffinitywithcache
gofmt -l pkg/utils/ pkg/ddc/efc/ pkg/webhook/plugins/

Ⅴ. Special notes for reviews

None.

[fluid-e2e-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cheyang for approval by writing /assign @cheyang in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[fluid-e2e-bot]

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions 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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.79%. Comparing base (a01c038) to head (5648784).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6234      +/-   ##
==========================================
+ Coverage   65.75%   65.79%   +0.03%     
==========================================
  Files         486      486              
  Lines       34459    34444      -15     
==========================================
+ Hits        22660    22663       +3     
+ Misses      10044    10027      -17     
+ Partials     1755     1754       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟢 Approval recommended

No active review issues remain for the deterministic affinity-ordering changes.

No open issues 🎉

What changed in this PR

Extends the deterministic affinity ordering introduced in #6217 to the webhook and persistent-volume creation sites tracked in #6220, reducing ordering-only differences in generated Kubernetes objects.

Changes:

• Adds a shared helper that converts selector maps into requirements sorted by key.
• Reuses the helper across affinity generation and sorts runtime-map iteration in the cache-locality webhook.
• Adds regression tests for sorted output, repeated-call consistency, and empty selectors.

File	Description
pkg/​webhook/​plugins/​requirenodewithfuse/​require_node_with_fuse.go	Uses the shared helper for sorted FUSE selector requirements.
pkg/​webhook/​plugins/​requirenodewithfuse/​require_node_with_fuse_test.go	Tests stable ordering with multiple FUSE labels.
pkg/​webhook/​plugins/​nodeaffinitywithcache/​node_affinity_with_cache.go	Sorts runtime keys before generating locality terms.
pkg/​webhook/​plugins/​nodeaffinitywithcache/​node_affinity_with_cache_test.go	Tests deterministic preferred and required term ordering.
pkg/​utils/​kubeclient/​pod.go	Replaces inline sorting with the shared helper.
pkg/​utils/​dataset/​volume/​create.go	Uses sorted selector requirements for PV affinity.
pkg/​utils/​dataset/​volume/​create_test.go	Tests multi-label PV affinity ordering.
pkg/​utils/​affinity.go	Adds ToNodeSelectorRequirements.
pkg/​utils/​affinity_test.go	Covers empty inputs, sorted requirements, and repeated calls.
pkg/​ddc/​efc/​create_volume.go	Uses the shared helper for EFC PV affinity.
pkg/​ddc/​efc/​create_volume_test.go	Tests sorted EFC PV selector requirements.

_{Review effort: Balanced}

[cheyang]

Both reviewers reproduced the #6220 nondeterminism on the merge-base and verified this head fixes all four enumerated sites: contract tests that were red on base (4–6 distinct orderings per site) are green at head in two independent harnesses, and a control test run on both revisions proves the MergeNodeSelectorAndNodeAffinity refactor is behavior-preserving. Required CI is green at head (all checks SUCCESS, PR mergeable — the round-start snapshot predated the runs completing). Two pre-existing gaps remain outside this PR's scope: RequireNodeWithFuse.Mutate still iterates its runtimeInfos map unsorted, so per-runtime term order stays random for multi-dataset pods (cosmetic — terms are OR-ed; a verified one-line utils.OrderedKeys fix would close it), and pkg/dataflow/helper.go has the same unsorted-map class in dataflow job affinity. Two nits on the new code: the determinism subtest derives its expectation from the function under test, and the len(selectors) > 0 guard at require_node_with_fuse.go:93 is redundant. Agreement between the reviewers was complete — every finding was independently confirmed in cross-critique with zero disputes; Copilot (Reviewer C) reviewed this head and raised no findings.

Next round

[MUST] none — nothing blocks merge; required CI is green at head
[SHOULD] fold in the one-line utils.OrderedKeys fix for the Mutate term order (F1), or track it as a follow-up issue
[SHOULD] file a follow-up issue for the unsorted annotation iteration in pkg/dataflow/helper.go (F2)
[SHOULD] make deterministic_order_over_repeated_calls assert a literal sorted expectation instead of the function's own output (F3)
[SHOULD] drop the redundant len(selectors) > 0 guard at require_node_with_fuse.go:93 (F4)
[Filed] none this round — F1 and F2 are proposed as follow-up issues, not yet opened

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/requirenodewithfuse/require_node_with_fuse.go:61 [minor] — pre-existing, not introduced by this PR — Per-runtime NodeSelectorTerm order in RequireNodeWithFuse.Mutate is still map-random (consensus: both reviewers, independently found and cross-confirmed)
  Mutate() still iterates for _, runtime := range runtimeInfos — a map[string]base.RuntimeInfoInterface — appending one NodeSelectorTerm per runtime, so for a pod bound to multiple datasets with global-fuse node selectors the ORDER of the injected NodeSelectorTerms in the mutated pod spec remains non-deterministic between admissions. The PR sorted the MatchExpressions inside each term (utils.ToNodeSelectorRequirements) and gave the sibling nodeaffinitywithcache plugin the full utils.OrderedKeys treatment over its runtime-map loops, but left this outer loop unordered — the same class of non-determinism this PR exists to eliminate, in a file it already touches. Impact is cosmetic: required NodeSelectorTerms are OR-ed, so scheduling semantics are unchanged; only byte-determinism of the mutated spec is affected. Pre-existing loop (untouched by the diff, byte-identical on merge-base), so it does not gate this merge. A verified one-line fix exists: for _, name := range utils.OrderedKeys(runtimeInfos) + runtimeInfos[name], mirroring the sibling plugin.
• pkg/dataflow/helper.go:50 [minor] — pre-existing, not introduced by this PR — pkg/dataflow/helper.go builds MatchExpressions from unsorted annotation iteration (Reviewer A finding, CONFIRMed by Reviewer B in cross-critique)
  for key, value := range annotations appends one NodeSelectorRequirement per AnnotationDataFlowCustomizedAffinityPrefix annotation in map order — the same nondeterminism class as #6220, at a site the issue does not enumerate (dataflow job affinity, opt-in via AnnotationDataFlowAffinityInject: "true"). The PR fully fixes the enumerated list, so merging legitimately closes #6220, but the broader 'unify ... sites' ask stays incomplete here. The requirements land AND-ed in a single NodeSelectorTerm, so order is purely cosmetic in the generated job spec. Recommend a follow-up issue rather than expanding this PR.
• pkg/utils/affinity_test.go:146 [nit] — determinism subtest derives its expectation from the function under test (PR-introduced nit)
  The deterministic_order_over_repeated_calls subtest sets expected := ToNodeSelectorRequirements(multi) (line 154) and compares 100 further calls against that self-derived value, so it detects cross-call nondeterminism but would stay green under a consistently-unsorted implementation. The multiple_entries_sorted table case pins a literal sorted expectation (arch, instance, region, zone), so real coverage exists; consider asserting a literal sorted expectation in the subtest too. Cross-critique: Reviewer B CONFIRMed (verified at head, including the table-case mitigation).
• pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse.go:93 [nit] — redundant len(selectors) > 0 guard around the ToNodeSelectorRequirements append (PR-introduced nit; Reviewer B finding, CONFIRMed by Reviewer A)
  The if len(selectors) > 0 guard around append(..., utils.ToNodeSelectorRequirements(selectors)...) is redundant: ToNodeSelectorRequirements returns nil for an empty map and append(x, nil...) is a no-op. Harmless (and note it does NOT mirror the PV call sites, where the len>0 guard legitimately protects construction of the whole pv.Spec.NodeAffinity block — only here, wrapping a bare append onto a pre-initialized slice, is it dead weight). Drop it or keep it; either way it is a nit only.

Verified review delta

• F1-term-order-requirenodewithfuse: open (preexisting)
• F2-dataflow-helper-unsorted: open (preexisting)
• F3-self-referential-determinism-test: open (initial-pr)
• F4-redundant-empty-selector-guard: open (initial-pr)

Code: clear. CI: success; attribution: unknown. Required CI checks still apply.

[frenemy17]

@cheyang Thanks for the review — addressed all four [SHOULD] items:

• F1 (RequireNodeWithFuse.Mutate): Sorted runtimeInfos iteration via utils.OrderedKeys(runtimeInfos) in pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse.go and added a multi-runtime determinism test in require_node_with_fuse_test.go.
• F2 (pkg/dataflow/helper.go): Filed follow-up issue [FEATURE] Sort map iteration in dataflow GenerateNodeAffinity when building MatchExpressions #6238 to keep this PR scoped to [FEATURE] Unify and sort map iteration in NodeSelector and affinity generation sites across webhooks and PV volume creation #6220.
• F3 (affinity_test.go): Replaced expected := ToNodeSelectorRequirements(multi) in deterministic_order_over_repeated_calls with an explicit literal sorted slice.
• F4 (getRequiredSchedulingTerm): Removed the redundant if len(selectors) > 0 guard around utils.ToNodeSelectorRequirements(selectors).

Re-verified with go test -gcflags="all=-N -l", go vet, and gofmt -l.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[cheyang]

Re-checked the delta since the 11-08 review (c9dfc9f -> 5648784); the four SHOULDs are handled. RequireNodeWithFuse.Mutate walks runtimeInfos via utils.OrderedKeys, with a spec pinning the term order across repeated calls; TestToNodeSelectorRequirements compares against a literal sorted slice; the redundant len(selectors) guard is gone (appending an empty result is a no-op); and the pkg/dataflow/helper.go iteration is tracked in #6238. CI is green on this head, and all touched packages build, vet and pass locally on top of current master. Thanks @frenemy17!