Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
b7c20ed to
3cd2c28
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
3cd2c28 to
4d4b829
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: Rahul Sharma <rahulsharm@nvidia.com>
4d4b829 to
3518aac
Compare
|
|
||
| // mergeService preserves fields allocated by the API server. Clearing these fields in a full | ||
| // update is rejected because their values are immutable or must remain allocated. | ||
| func (s *stateSkel) mergeService(updated, current *unstructured.Unstructured) error { |
There was a problem hiding this comment.
Question -- why is this needed now when it wasn't needed before? Is it because this commit is introducing a diff in the nvidia-dcgm-exporter-dra Service object?
There was a problem hiding this comment.
This was flagged by LLMs. I believe we had this bug introduced recently but never caught it.
non-dra path uses:
gpu-operator/controllers/object_controls.go
Lines 4952 to 4956 in 93c0ff5
It sets resourceVersion and clusterip allocated before calling update().
However, GPUCluster path uses generic unstructured reconciler which only preserves resourceVersion but not the clusterIP. Kubernetes can reject the update if clusterIP is omitted. This might be an existing bug in v26.7.0 not yet reported. Once service is created and ip is assigned, future reconcile updates might be failing but its not getting affected as it exists, just not getting updated.
📝 WalkthroughWalkthroughThe change adds reserved DCGM and DCGM Exporter labels to DaemonSets and pod templates. It prevents user labels from overriding those labels. GPUCluster manifests add stable DCGM and exporter Services while retaining DRA-specific Services. Service merging now preserves API-assigned networking fields, node ports, and health-check node ports. Tests cover labels, Services, selectors, annotations, overrides, and merge behavior. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to A GPUCluster upgrade can remove legacy DCGM or exporter endpoints before DRA pods are ready, temporarily interrupting hostengine or metrics access. Preserve current selectors for existing Services before merging. 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 6c4f18da-82df-4dc8-89bc-2805492a802d
⛔ Files ignored due to path filters (4)
internal/state/testdata/golden/gpucluster-dcgm-exporter-embedded.yamlis excluded by!**/testdata/**internal/state/testdata/golden/gpucluster-dcgm-exporter-pod-metadata.yamlis excluded by!**/testdata/**internal/state/testdata/golden/gpucluster-dcgm-exporter-remote-engine.yamlis excluded by!**/testdata/**internal/state/testdata/golden/gpucluster-dcgm.yamlis excluded by!**/testdata/**
📒 Files selected for processing (13)
assets/state-dcgm-exporter/0800_daemonset.yamlassets/state-dcgm/0400_dcgm.ymlcontrollers/object_controls.gocontrollers/object_controls_test.gointernal/state/dcgm_exporter_test.gointernal/state/dcgm_test.gointernal/state/state_skel.gointernal/state/state_skel_test.gomanifests/state-dcgm-exporter/0490_service.yamlmanifests/state-dcgm-exporter/0500_service.yamlmanifests/state-dcgm-exporter/0700_daemonset.yamlmanifests/state-dcgm/0500_daemonset.yamlmanifests/state-dcgm/0550_service.yaml
💤 Files with no reviewable changes (1)
- manifests/state-dcgm-exporter/0500_service.yaml
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| if gvk.Group == "" && gvk.Kind == "ServiceAccount" { | ||
| return s.mergeServiceAccount(updated, current) | ||
| } | ||
| if gvk.Group == "" && gvk.Kind == "Service" { | ||
| return s.mergeService(updated, current) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // mergeService preserves fields allocated by the API server. Clearing these fields in a full | ||
| // update is rejected because their values are immutable or must remain allocated. | ||
| func (s *stateSkel) mergeService(updated, current *unstructured.Unstructured) error { | ||
| updatedService := &corev1.Service{} | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(updated.Object, updatedService); err != nil { | ||
| return fmt.Errorf("failed to convert updated Service: %w", err) | ||
| } | ||
| currentService := &corev1.Service{} | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(current.Object, currentService); err != nil { | ||
| return fmt.Errorf("failed to convert current Service: %w", err) | ||
| } | ||
|
|
||
| if updatedService.Spec.ClusterIP == "" && len(updatedService.Spec.ClusterIPs) == 0 { | ||
| updatedService.Spec.ClusterIP = currentService.Spec.ClusterIP | ||
| updatedService.Spec.ClusterIPs = currentService.Spec.ClusterIPs | ||
| } | ||
| if len(updatedService.Spec.IPFamilies) == 0 { | ||
| updatedService.Spec.IPFamilies = currentService.Spec.IPFamilies | ||
| } | ||
| if updatedService.Spec.IPFamilyPolicy == nil { | ||
| updatedService.Spec.IPFamilyPolicy = currentService.Spec.IPFamilyPolicy | ||
| } | ||
|
|
||
| if serviceAllocatesNodePorts(updatedService) { | ||
| for i := range updatedService.Spec.Ports { | ||
| if updatedService.Spec.Ports[i].NodePort != 0 { | ||
| continue | ||
| } | ||
| if currentPort := findMatchingServicePort(updatedService.Spec.Ports[i], currentService.Spec.Ports); currentPort != nil { | ||
| updatedService.Spec.Ports[i].NodePort = currentPort.NodePort | ||
| } | ||
| } | ||
| } | ||
| if updatedService.Spec.Type == corev1.ServiceTypeLoadBalancer && | ||
| updatedService.Spec.ExternalTrafficPolicy == corev1.ServiceExternalTrafficPolicyLocal && | ||
| updatedService.Spec.HealthCheckNodePort == 0 { | ||
| updatedService.Spec.HealthCheckNodePort = currentService.Spec.HealthCheckNodePort | ||
| } | ||
|
|
||
| merged, err := runtime.DefaultUnstructuredConverter.ToUnstructured(updatedService) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to convert merged Service: %w", err) | ||
| } | ||
| updated.Object = merged | ||
| return nil | ||
| } | ||
|
|
||
| func serviceAllocatesNodePorts(service *corev1.Service) bool { | ||
| if service.Spec.Type == corev1.ServiceTypeNodePort { | ||
| return true | ||
| } | ||
| return service.Spec.Type == corev1.ServiceTypeLoadBalancer && | ||
| (service.Spec.AllocateLoadBalancerNodePorts == nil || *service.Spec.AllocateLoadBalancerNodePorts) | ||
| } | ||
|
|
||
| func findMatchingServicePort(updated corev1.ServicePort, current []corev1.ServicePort) *corev1.ServicePort { | ||
| for i := range current { | ||
| if updated.Name != "" && updated.Name == current[i].Name { | ||
| return ¤t[i] | ||
| } | ||
| if updated.Name == "" && current[i].Name == "" && servicePortProtocol(updated) == servicePortProtocol(current[i]) { | ||
| return ¤t[i] | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func servicePortProtocol(port corev1.ServicePort) corev1.Protocol { | ||
| if port.Protocol == "" { | ||
| return corev1.ProtocolTCP | ||
| } | ||
| return port.Protocol | ||
| } | ||
|
|
||
| // For Service Account, keep secrets if exists | ||
| func (s *stateSkel) mergeServiceAccount(updated, current *unstructured.Unstructured) error { | ||
| curSecrets, ok, err := unstructured.NestedSlice(current.Object, "secrets") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '430,540p' internal/state/state_skel.go
sed -n '1,100p' manifests/state-dcgm/0550_service.yaml
sed -n '1,120p' manifests/state-dcgm-exporter/0490_service.yaml
sed -n '4910,5000p' controllers/object_controls.go
rg -n 'nvidia-dcgm(-exporter)?|selector|mergeObjects|mergeService' internal/state controllers manifests assets | head -240Repository: NVIDIA/gpu-operator
Length of output: 31109
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- state skeleton reconciliation ---'
sed -n '280,390p' internal/state/state_skel.go
printf '%s\n' '--- state skeleton declarations and callers ---'
rg -n -C 3 'NewState|stateSkel|Reconcile|mergeObjects|GPUCluster|ClusterPolicy|State\(' internal/state controllers | head -320
printf '%s\n' '--- DCGM state definitions and registration ---'
rg -n -C 5 'func (.*DCGM|func DCGM|DCGMExporter|dcgm|state-dcgm|state-dcgm-exporter' internal/state controllers | head -360
printf '%s\n' '--- Service update paths and selector preservation ---'
rg -n -C 5 'Spec\.Selector|Selector.*current|current.*Selector|preserve.*selector|selector.*preserve|Service\(' internal/state controllers | head -360
printf '%s\n' '--- rollout/upgrade requirement evidence ---'
rg -n -i -C 4 'selector.*(preserv|rollout|upgrade)|preserv.*selector|endpoint|downtime|availability|DCGM' . --glob '!internal/state/testdata/golden/**' --glob '!vendor/**' | head -360Repository: NVIDIA/gpu-operator
Length of output: 50375
🤖 get_repo_knowledge executed:
get_repo_knowledge NVIDIA/gpu-operator /tmp/coderabbit-repo-knowledge/nvidia-gpu-operator-eed98df4/conventions
Length of output: 4401
🏁 Script executed:
set -e
sed -n '280,390p' internal/state/state_skel.go
rg -n -C 4 'stateSkel|mergeObjects|GPUCluster|state-dcgm|state-dcgm-exporter|Spec\.Selector|preserve.*selector|selector.*preserv' internal/state controllersRepository: NVIDIA/gpu-operator
Length of output: 50376
🏁 Script executed:
set -e
printf '%s\n' '--- changed state skeleton diff ---'
git diff -- internal/state/state_skel.go
printf '%s\n' '--- state manager/controller GPUCluster entrypoints ---'
rg -n -C 8 'GPUCluster|SyncState|Sync\(|NewStateDCGM|NewStateDCGMExporter|newStates|stateManager|Reconcile' controllers/state_manager.go internal/state/manager.go internal/state/*.go | head -500
printf '%s\n' '--- focused selector tests and requirements ---'
rg -n -C 8 'selector|endpoint|rollout|preserv|DRA' internal/state/state_skel*_test.go internal/state/dcgm*_test.go internal/state/*test.go controllers/*test.go docs README.md CONTRIBUTING.md 2>/dev/null | head -500Repository: NVIDIA/gpu-operator
Length of output: 50375
🏁 Script executed:
set -e
printf '%s\n' '--- internal state manager ---'
cat -n internal/state/manager.go
printf '%s\n' '--- direct state manager invocations ---'
rg -n -C 10 'SyncState\(|\.Sync\(ctx|Sync\(.*GPUCluster|SyncState\(' controllers internal --glob '*.go' | head -320
printf '%s\n' '--- state factory registration ---'
rg -n -C 12 'NewStates|NewStateDCGM|NewStateDCGMExporter|GPUClusterCRDName|GPUCluster' internal/state/manager.go controllers | head -320Repository: NVIDIA/gpu-operator
Length of output: 50375
Preserve existing Service selectors during GPUCluster updates. GPUClusterReconciler reaches the DCGM and DCGM exporter states, and createOrUpdateObjs updates existing Services through mergeService. That helper preserves allocated fields but leaves the desired Spec.Selector unchanged. An upgrade can therefore replace a selector for serving legacy pods with app: nvidia-dcgm-dra or app: nvidia-dcgm-exporter-dra before those pods are ready. Kubernetes then removes the legacy pods from the Service endpoints, causing a temporary hostengine or metrics outage.
Copy the current selector in mergeService for existing Services. Keep the DRA selector for newly created Services.
| metadata: | ||
| labels: | ||
| app: nvidia-dcgm-exporter | ||
| nvidia.com/gpu-operator.dcgm-exporter: "true" |
There was a problem hiding this comment.
Can we look at alternate names for this label key?
The format of nvidia.com/gpu-operator.dcgm-exporter: "true" seems non-standard for labels.
There was a problem hiding this comment.
Maybe app.kubernetes.io/component: nvidia-dcgm-exporter?
There was a problem hiding this comment.
Yes, that could work. It is a standardised label
There was a problem hiding this comment.
Sure, we can. This label was selected so that we use the same scheme we used before like here:
gpu-operator/internal/consts/consts.go
Line 71 in 93c0ff5
There was a problem hiding this comment.
Looks like app.kubernetes.io/component has a specific meaning as per this K8s docs
How about app.kubernetes.io/name instead?
There was a problem hiding this comment.
@rahulait I missed to mention this earlier, using a boolean as the label value typed seemed a bit off to me.
3 participants
Description
Partially fixes #2917
Restore stable DCGM and DCGM Exporter identities for GPUCluster/DRA deployments while retaining the released DRA-specific Services.
Changes
nvidia-dcgmnvidia-dcgm-exporternvidia-dcgm-dranvidia-dcgm-exporter-dranvidia.com/gpu-operator.dcgm: "true"nvidia.com/gpu-operator.dcgm-exporter: "true"nvidia-dcgm-exporterfor Prometheus scraping to avoid duplicate targets.Checklist
make lint)make validate-generated-assets)make validate-modules)Testing