fix: Implement Taint and Toleration Handling for NicClusterPolicy #1544
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 15584574385Details
💛 - Coveralls |
|
/retest-nic_operator_helm |
|
/retest-nic_operator_kind |
There was a problem hiding this comment.
Pull Request Overview
This PR implements taint-aware node filtering and enhances OFED deployment by handling GPU tolerations and adding new predicates for Node taint changes.
- Introduces a new NodeTaintChangedPredicate for filtering Node events based on taint changes.
- Adds GPU toleration support in state_ofed.go via the addGPUToleration function and updates manifest rendering to use processed tolerations.
- Expands test coverage for node filter behavior and controller responses to taint/toleration changes.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/state/state_ofed_test.go | Updates tests to validate GPU toleration and node filtering logic |
| pkg/state/state_ofed.go | Adds GPU toleration helper, adjusts manifest object generation |
| pkg/nodeinfo/filter_test.go | Introduces comprehensive tests for composite node filtering |
| pkg/nodeinfo/filter.go | Implements NodeTaintFilter and composite filters |
| controllers/predicate.go | Adds NodeTaintChangedPredicate for Node event filtering |
| controllers/nicclusterpolicy_controller_test.go | Enhances tests for taint and toleration interactions in NCP |
| controllers/nicclusterpolicy_controller.go | Updates controller setup to include new Node predicates |
Comments suppressed due to low confidence (1)
pkg/nodeinfo/filter.go:175
- The unschedulable taint key is hard-coded. Consider defining it as a constant to improve readability and maintainability.
if taint.Key == "node.kubernetes.io/unschedulable" && taint.Effect == corev1.TaintEffectNoSchedule {
|
|
||
| if len(objs) == 0 { | ||
| // GetManifestObjects returned no objects, this means that no objects need to be applied to the cluster | ||
| // as (most likely) no Mellanox hardware is found (No mellanox labels where found). |
There was a problem hiding this comment.
There is a duplicated empty check for objs after waitForStaleObjectsRemoval. Please add a clarifying comment to explain the positioning of this check and confirm that this ordering is intentional.
Suggested change
| // as (most likely) no Mellanox hardware is found (No mellanox labels where found). | |
| // as (most likely) no Mellanox hardware is found (No mellanox labels where found). | |
| // This check is intentionally placed after waitForStaleObjectsRemoval to ensure that we handle cases | |
| // where no objects are returned but stale objects might still exist and need to be removed. |
Copilot uses AI. Check for mistakes.
pkg/state/state_ofed.go
Outdated
| Key: "nvidia.com/gpu", | ||
| Effect: v1.TaintEffectNoSchedule, | ||
| Operator: v1.TolerationOpExists, // Operator can be Exists if Value is not specified, or Equal if Value is specified. | ||
| // Value: "true", // Often GPU taints have a value, but Exists is fine if the taint is just by key and effect. |
There was a problem hiding this comment.
[nitpick] When comparing tolerations in addGPUToleration, consider also comparing the Value field if GPU taints might include a value. This will ensure that the toleration check remains robust in all scenarios.
Suggested change
| // Value: "true", // Often GPU taints have a value, but Exists is fine if the taint is just by key and effect. | |
| Value: "", // Default to an empty value; update if specific taints require a value. |
Copilot uses AI. Check for mistakes.
dahalperin
force-pushed
the
fix_ofed_not_ready
branch
2 times, most recently
from
823cffc to
1a9c1a8
Compare
rollandf
previously approved these changes
Jun 9, 2025
heyvister1
reviewed
Jun 10, 2025
e0ne
reviewed
Jun 10, 2025
|
@dahalperin you need to sign-off your commits to pass the DCO check |
Introduces taint-aware node filtering and OFED deployment. - Controller now watches Node taint changes. - New `NodeTaintChangedPredicate` and composite node filter (labels & taints) for OFED node selection. - `state_ofed` uses this filter and adds a default `nvidia.com/gpu` toleration to OFED DaemonSets. - NCP tolerations are respected. - Extensive new tests for controller, filtering, and OFED state. Signed-off-by: Dana Halperin <[email protected]>
dahalperin
force-pushed
the
fix_ofed_not_ready
branch
from
75bab8f to
988f748
Compare
|
/retest-nic_operator_helm |
|
/retest-nic_operator_helm |
rollandf
approved these changes
Jun 12, 2025
|
/retest-nic_operator_helm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduces taint-aware node filtering and OFED deployment.
NodeTaintChangedPredicateand composite node filter (labels & taints) for OFED node selection.state_ofeduses this filter and adds a defaultnvidia.com/gputoleration to OFED DaemonSets.