Add DRA node labeling via unified PodNodeLabel reconciler#1832
Conversation
Merge DevicePluginPodReconciler and the new DRA pod labeling logic into a single PodNodeLabelReconciler that handles both roles. The reconciler watches DaemonSet pods with a ModuleNameLabel, determines the role from the DaemonSetRole label, and manages the corresponding node label (device-plugin-ready or dra-ready) based on pod readiness.
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TomerNewman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughRenames ChangesPodNodeLabelReconciler unification and DRA labeling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/controllers/pod_node_label_reconciler_test.go (1)
98-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd mixed-role regression coverage for the replacement-pod lookup.
Lines 107-122 and Lines 311-326 only prove the happy path where the same role keeps the label. This PR’s key contract is that device-plugin and DRA pods must not keep each other’s node labels alive, so please add the inverse cases too: a ready DRA pod must not block removal of
device-plugin-ready, and a ready device-plugin pod must not block removal ofdra-ready. Otherwise a future change that dropsconstants.DaemonSetRolefrom theListselector will still pass this suite.Also applies to: 302-341
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controllers/pod_node_label_reconciler_test.go` around lines 98 - 134, Add inverse mixed-role regression tests around PodNodeLabelReconciler’s replacement-pod lookup so the suite covers cross-role behavior, not just the same-role happy path. In the test blocks around Reconcile and the kubeClient.List expectation, add cases where a ready DRA pod exists while checking removal of device-plugin-ready, and where a ready device-plugin pod exists while checking removal of dra-ready; assert the node label is still removed in both cases. Use the existing constants.DaemonSetRole, constants.DevicePluginRoleLabelValue, and the Reconcile flow in pod_node_label_reconciler_test.go to locate the right spots.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controllers/pod_node_label_reconciler.go`:
- Around line 69-79: The sibling-pod lookup in PodNodeLabelReconciler is too
broad because r.client.List only filters by module name, role, and node, so pods
from other namespaces can block label cleanup for the current pod. Update the
lookup in the pod node label reconciler to scope the List call to the current
pod’s namespace by adding a namespace filter such as
client.InNamespace(pod.Namespace), alongside the existing MatchingLabels and
MatchingFields selectors.
- Around line 40-47: The role selection logic in PodNodeLabelReconciler
currently treats any non-"dra" DaemonSetRole value as device-plugin, which can
incorrectly reconcile pods with missing or invalid labels. Update the label
handling in the reconciler flow to explicitly validate
pod.Labels[constants.DaemonSetRole] before choosing roleLabelValue and
labelName, and return early from the reconcile path for missing or unknown
values instead of defaulting to constants.DevicePluginRoleLabelValue. Keep the
existing DRA/device-plugin branching, but only after the role label has been
confirmed valid.
---
Nitpick comments:
In `@internal/controllers/pod_node_label_reconciler_test.go`:
- Around line 98-134: Add inverse mixed-role regression tests around
PodNodeLabelReconciler’s replacement-pod lookup so the suite covers cross-role
behavior, not just the same-role happy path. In the test blocks around Reconcile
and the kubeClient.List expectation, add cases where a ready DRA pod exists
while checking removal of device-plugin-ready, and where a ready device-plugin
pod exists while checking removal of dra-ready; assert the node label is still
removed in both cases. Use the existing constants.DaemonSetRole,
constants.DevicePluginRoleLabelValue, and the Reconcile flow in
pod_node_label_reconciler_test.go to locate the right spots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06ca9e8a-ebe5-47d8-9800-37b0998d49dc
📒 Files selected for processing (8)
cmd/manager/main.gointernal/controllers/device_plugin_pod_reconciler_test.gointernal/controllers/pod_node_label_reconciler.gointernal/controllers/pod_node_label_reconciler_test.gointernal/utils/kmmlabels.gointernal/utils/kmmlabels_test.gopkg/labels/labels.gopkg/labels/labels_test.go
💤 Files with no reviewable changes (1)
- internal/controllers/device_plugin_pod_reconciler_test.go
| isDRA := pod.Labels[constants.DaemonSetRole] == constants.DRARoleLabelValue | ||
|
|
||
| roleLabelValue := constants.DevicePluginRoleLabelValue | ||
| labelName := utils.GetDevicePluginNodeLabel(pod.Namespace, moduleName) | ||
| if isDRA { | ||
| roleLabelValue = constants.DRARoleLabelValue | ||
| labelName = utils.GetDRANodeLabel(pod.Namespace, moduleName) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject missing or unknown DaemonSetRole values instead of defaulting to device-plugin.
Anything other than "dra" currently falls through to the device-plugin path. Because the controller only filters on ModuleNameLabel, a DaemonSet pod with a missing or typoed role will still reconcile and toggle the device-plugin node label incorrectly. Validate the label explicitly and return early on unknown values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controllers/pod_node_label_reconciler.go` around lines 40 - 47, The
role selection logic in PodNodeLabelReconciler currently treats any non-"dra"
DaemonSetRole value as device-plugin, which can incorrectly reconcile pods with
missing or invalid labels. Update the label handling in the reconciler flow to
explicitly validate pod.Labels[constants.DaemonSetRole] before choosing
roleLabelValue and labelName, and return early from the reconcile path for
missing or unknown values instead of defaulting to
constants.DevicePluginRoleLabelValue. Keep the existing DRA/device-plugin
branching, but only after the role label has been confirmed valid.
| // Making sure there is no other pod of the same role already running | ||
| labelSelector := client.MatchingLabels{ | ||
| constants.ModuleNameLabel: moduleName, | ||
| constants.DaemonSetRole: roleLabelValue, | ||
| } | ||
| fieldSelector := client.MatchingFields{"spec.nodeName": nodeName} | ||
| var modulePodsList v1.PodList | ||
| err := dppr.client.List(ctx, &modulePodsList, labelSelector, fieldSelector) | ||
| if err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to get list of all pods for module %s on node %s: %v", moduleName, nodeName, err) | ||
|
|
||
| var podsList v1.PodList | ||
| if err := r.client.List(ctx, &podsList, labelSelector, fieldSelector); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("failed to get list of pods for module %s on node %s: %v", moduleName, nodeName, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope the sibling-pod lookup to the pod namespace.
The node label key is namespace-qualified, but this List is cluster-wide and only filters by module name + role. If ns-a/foo and ns-b/foo land on the same node, a ready pod in ns-b will prevent removing the stale label for ns-a. Add client.InNamespace(pod.Namespace) (or an equivalent namespace filter) to the lookup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controllers/pod_node_label_reconciler.go` around lines 69 - 79, The
sibling-pod lookup in PodNodeLabelReconciler is too broad because r.client.List
only filters by module name, role, and node, so pods from other namespaces can
block label cleanup for the current pod. Update the lookup in the pod node label
reconciler to scope the List call to the current pod’s namespace by adding a
namespace filter such as client.InNamespace(pod.Namespace), alongside the
existing MatchingLabels and MatchingFields selectors.
|
/lgtm |
openshift-merge-bot
Bot
merged commit 89084d7
into
rh-ecosystem-edge:main
3 participants
Merge DevicePluginPodReconciler and the new DRA pod labeling logic into a single PodNodeLabelReconciler that handles both roles. The reconciler watches DaemonSet pods with a ModuleNameLabel, determines the role from the DaemonSetRole label, and manages the corresponding node label (device-plugin-ready or dra-ready) based on pod readiness.
/cc @ybettan @yevgeny-shnaidman
/assign @ybettan @yevgeny-shnaidman
fix #1826
Summary by CodeRabbit
New Features
Bug Fixes
Tests