DRA pod reconciler and node labeling#1289
Conversation
|
Skipping CI for Draft Pull Request. |
✅ Deploy Preview for kubernetes-sigs-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 |
|
/cc @yevgeny-shnaidman @ybettan |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1289 +/- ##
==========================================
- Coverage 79.09% 73.52% -5.58%
==========================================
Files 51 67 +16
Lines 5109 5004 -105
==========================================
- Hits 4041 3679 -362
- Misses 882 1156 +274
+ Partials 186 169 -17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
999f13a to
d6eaa21
Compare
d6eaa21 to
6417d60
Compare
Why is d/s related here? |
| return ctrl.Result{}, fmt.Errorf("pod %s/%s has no %q label", pod.Namespace, pod.Name, constants.ModuleNameLabel) | ||
| } | ||
|
|
||
| labelName := utils.GetDRANodeLabel(pod.Namespace, moduleName) |
There was a problem hiding this comment.
GetDRANodeLabel is a it confusing here since we are constructing/building the label pattern, not getting it from any object.
Can we rename this function while still staying consistent with how we construct version-ready/ready labels?
There was a problem hiding this comment.
Also, the labelName receiver have a too generic name IMO.
There was a problem hiding this comment.
This follows the same naming convention as the existing GetDevicePluginNodeLabel, GetKernelModuleReadyNodeLabel, GetDevicePluginTargetNodeLabel, etc. in kmmlabels.go. Renaming just this one would be inconsistent
There was a problem hiding this comment.
labelName is the same variable name used in DevicePluginPodReconciler for the same concept. Changing it only here would be inconsistent.
There was a problem hiding this comment.
labelName is the same variable name used in DevicePluginPodReconciler for the same concept. Changing it only here would be inconsistent.
Seems like this controller doesn't exist anymore. So can we change it only in one place in the converged controller?
| ) | ||
|
|
||
| if !podutils.IsPodReady(pod) || !pod.DeletionTimestamp.IsZero() { | ||
| if nodeName != "" { |
There was a problem hiding this comment.
I would add a funcion here or anything else to make it clearer we are targeting pods that weren't scheduled yet.
There was a problem hiding this comment.
Done — added the same comment as in DevicePluginPodReconciler.
6417d60 to
740df9d
Compare
|
[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 |
"Downstream consumers" here doesn't refer to hub-spoke — it means anything that reads node labels to make scheduling or readiness decisions (other controllers, custom schedulers, or user workloads with node selectors). Poor wording on my part — I've updated the PR description to say "provides a node-level readiness signal" instead. |
|
|
||
| WorkerPodVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-worker-pod" | ||
| DevicePluginVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-device-plugin" | ||
| DRAVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-dra" |
There was a problem hiding this comment.
I don't think this should be in this PR. This is related to ordered uprade.
There was a problem hiding this comment.
Done — removed.
|
General question: dra_pod_reconciler.go is a copy of device_plugin_pod_reconciler.go, except for the label it uses. Since device plugin and dra are mutually exclusive, can't we just use the device-pod-reconcile (and rename it) for both device plugin and dra |
740df9d to
cb93dc3
Compare
cb93dc3 to
fba8829
Compare
| labelName := utils.GetDevicePluginNodeLabel(pod.Namespace, moduleName) | ||
| isDRA := pod.Labels[constants.DaemonSetRole] == constants.DRARoleLabelValue | ||
|
|
||
| var labelName string |
There was a problem hiding this comment.
| var labelName string | |
| labelName := utils.GetDevicePluginNodeLabel(pod.Namespace, moduleName) | |
| if isDRA { | |
| labelName = utils.GetDRANodeLabel(pod.Namespace, moduleName) | |
| } |
| // Make sure we don't already have a new running pod before unlabeling the node | ||
| // Making sure there is no other pod of the same role already running | ||
| labelSelector := client.MatchingLabels{constants.ModuleNameLabel: moduleName} | ||
| if isDRA { |
There was a problem hiding this comment.
have not we also added a role label for devicePlugin?
There was a problem hiding this comment.
Existing device-plugin pods from older KMM versions may not have the role label. Adding it to the selector would miss those pods during the replacement check and incorrectly unlabel the node. That's why we only narrow the selector for DRA (which is new — all DRA pods will always have the label) and use a post-filter skip for device-plugin instead.
| var foundRunningPod bool | ||
| for _, p := range modulePodsList.Items { | ||
| for _, p := range podsList.Items { | ||
| if !isDRA && p.Labels[constants.DaemonSetRole] == constants.DRARoleLabelValue { |
There was a problem hiding this comment.
don't we need to make the same check for DRA pod?
There was a problem hiding this comment.
No — when isDRA is true, lines 69-71 already add DaemonSetRole=dra to the label selector, so the List only returns DRA pods. Device-plugin pods won't appear in the results. This skip (line 81) is only needed for the device-plugin path, where the query doesn't filter by role (for backwards compatibility), so DRA pods could appear in the list and need to be excluded.
| ) | ||
| } | ||
|
|
||
| func (dppr *DevicePluginPodReconciler) addLabel(ctx context.Context, nodeName string, labelName string) error { |
There was a problem hiding this comment.
why not keep this function here? this is the only code that is using it
There was a problem hiding this comment.
Done — moved back as private methods on the reconciler.
fba8829 to
d2ff271
Compare
|
/retest |
1 similar comment
|
/retest |
44e8e24 to
f4c9bd9
Compare
|
I am happy with this PR. @yevgeny-shnaidman can lgtm when he is happy too. |
f4c9bd9 to
4e001cb
Compare
| // Make sure we don't already have a new running pod before unlabeling the node | ||
| // Making sure there is no other pod of the same role already running | ||
| labelSelector := client.MatchingLabels{constants.ModuleNameLabel: moduleName} | ||
| if isDRA { |
There was a problem hiding this comment.
we already have a code in line 42-43 that check if this DRA or not. Lets define a variable there that will hold either constants.DRARoleLabelValue or constants.DevicePluginRoleLabelValue and here we just set it , without needing to check again if it is DRA
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.
4e001cb to
d250135
Compare
|
/lgtm |
|
/retest |
Add DRA node labeling via unified PodNodeLabel reconciler
Summary
Merge
DevicePluginPodReconcilerand the new DRA pod labeling logic into a singlePodNodeLabelReconciler. The reconciler watches DaemonSet pods with aModuleNameLabel, determines the role from theDaemonSetRolelabel, and manages the corresponding node label (device-plugin-readyordra-ready) based on pod readiness. This provides a node-level readiness signal indicating which nodes have a functioning device-plugin or DRA driver for a Module.Changes
Unified controller:
PodNodeLabelReconcilerininternal/controllers/pod_node_label_reconciler.go— replacesDevicePluginPodReconcilercmd/manager/main.goDaemonSetRolelabel:dra→dra-ready, otherwise →device-plugin-readyLabel utilities:
GetDRANodeLabelininternal/utils/kmmlabels.go— generates thedra-readynode labelGetDRANodeLabelexposed in public API viapkg/labels/labels.goConstants:
DRARoleLabelValueandDRAReadySuffixininternal/constants/constants.goTesting
Acceptance Criteria
PodNodeLabelReconcilerwatches pods and setsdra-readyordevice-plugin-readylabel on node when readyModuleNameLabel+ DaemonSet ownerNodeLabelerFinalizerPodNodeLabelReconcilerregistered incmd/manager/main.go