fix(webhook): preserve required affinity branch semantics#5757
fix(webhook): preserve required affinity branch semantics#5757hxrshxz wants to merge 4 commits intofluid-cloudnative:masterfrom
Conversation
…ction branch Signed-off-by: Harsh <harshmastic@gmail.com>
…s only into term[0] Signed-off-by: Harsh <harshmastic@gmail.com>
![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 adds unit tests for the RequireNodeWithFuse plugin to verify fuse node selector injection. A logical issue was identified: the implementation only appends the fuse requirement to the first NodeSelectorTerm. Because Kubernetes evaluates multiple terms using OR logic, pods could still be scheduled on nodes without the fuse if they match other terms. The reviewer suggests injecting the requirement into all existing terms to ensure strict enforcement.
pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds new Ginkgo/Gomega unit test coverage for the requirenodewithfuse mutating webhook plugin, focusing on node-affinity injection behavior used to require fuse-capable nodes for dataset-mounted pods.
Changes:
- Add a spec validating fresh-pod injection of
RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTermsbased onruntimeInfofuse selectors. - Add a spec documenting/verifying the current multi-term behavior where fuse match expressions are appended only to
NodeSelectorTerms[0].
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse_test.go
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for the RequireNodeWithFuse plugin to verify node selector injection, including a test case for pods with multiple pre-existing NodeSelectorTerms. A review comment identifies a correctness issue in the underlying logic: since NodeSelectorTerms are evaluated using a logical OR, appending the fuse requirement only to the first term fails to strictly enforce the constraint across all terms, which could allow pods to be scheduled on nodes lacking the required fuse.
pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse_test.go
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5757 +/- ##
==========================================
+ Coverage 61.22% 61.44% +0.21%
==========================================
Files 444 444
Lines 30557 30666 +109
==========================================
+ Hits 18710 18842 +132
+ Misses 10307 10278 -29
- Partials 1540 1546 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/webhook/plugins/requirenodewithfuse/require_node_with_fuse_test.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Harsh <harshmastic@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for the RequireNodeWithFuse plugin to verify node selector injection, including cases with multiple existing affinity terms. Feedback suggests refactoring repeated setup into a BeforeEach block, simplifying assertions using the ConsistOf matcher or direct struct comparison, and replacing magic strings with constants.
Signed-off-by: Harsh <harshmastic@gmail.com>
|
hxrshxz
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| existingTerms := pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms | ||
| combinedTerms := make([]corev1.NodeSelectorTerm, 0, len(existingTerms)*len(requiredSchedulingTerms)) | ||
| for i := 0; i < len(existingTerms); i++ { | ||
| if len(existingTerms[i].MatchExpressions) == 0 && len(existingTerms[i].MatchFields) == 0 { | ||
| continue | ||
| } | ||
| for j := 0; j < len(requiredSchedulingTerms); j++ { | ||
| if len(requiredSchedulingTerms[j].MatchExpressions) == 0 && len(requiredSchedulingTerms[j].MatchFields) == 0 { | ||
| continue | ||
| } | ||
| combinedTerm := corev1.NodeSelectorTerm{ | ||
| MatchExpressions: append(append([]corev1.NodeSelectorRequirement{}, existingTerms[i].MatchExpressions...), requiredSchedulingTerms[j].MatchExpressions...), | ||
| MatchFields: append(append([]corev1.NodeSelectorRequirement{}, existingTerms[i].MatchFields...), requiredSchedulingTerms[j].MatchFields...), | ||
| } | ||
| combinedTerms = append(combinedTerms, combinedTerm) | ||
| } | ||
| } | ||
| pod.Spec.Affinity.NodeAffinity.RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms = combinedTerms |
There was a problem hiding this comment.
InjectNodeSelectorTerms can set RequiredDuringSchedulingIgnoredDuringExecution.NodeSelectorTerms to an empty slice when all existing terms are empty (or all injected terms are empty and get skipped). This effectively wipes required node affinity and can make a pod unschedulable/invalid. Consider filtering out empty injected terms up front, and if no non-empty existing terms remain then fall back to setting NodeSelectorTerms to the (filtered) injected terms instead of overwriting with an empty combinedTerms (or treat empty existing term as a match-all branch and still combine).
|
[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 |
1 similar comment
|
[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 |
fluid-e2e-bot
bot
added
needs-ok-to-test
and removed
do-not-merge/work-in-progress
labels
|
Hi @hxrshxz. 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. |
Ⅰ. Describe what this PR does
Fix shared webhook required-node-affinity merging so existing OR branches are preserved correctly while adding coverage for
requirenodewithfuseandnodeaffinitywithcache.Ⅱ. Does this pull request fix one issue?
#5676
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Add helper regression tests for cross-product node selector merging, empty-term handling, and
MatchFieldsmerging, plus plugin tests covering required-affinity branch preservation forrequirenodewithfuseandnodeaffinitywithcache.Ⅳ. Describe how to verify it
Run
go test -count=1 ./pkg/utils -run TestInjectNodeSelectorTermsandgo test -count=1 ./pkg/webhook/plugins/requirenodewithfuse ./pkg/webhook/plugins/nodeaffinitywithcache.Ⅴ. Special notes for reviews
N/A