-
Notifications
You must be signed in to change notification settings - Fork 566
fix: preserve DCGM service compatibility #2918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ import ( | |||||||||||
|
|
||||||||||||
| "github.com/go-logr/logr" | ||||||||||||
| appsv1 "k8s.io/api/apps/v1" | ||||||||||||
| corev1 "k8s.io/api/core/v1" | ||||||||||||
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||||||||||||
| "k8s.io/apimachinery/pkg/api/meta" | ||||||||||||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||||||||||||
|
|
@@ -442,9 +443,86 @@ func (s *stateSkel) mergeObjects(updated, current *unstructured.Unstructured) er | |||||||||||
| 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 { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question -- why is this needed now when it wasn't needed before? Is it because this commit is introducing a diff in the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||
| 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") | ||||||||||||
|
Comment on lines
443
to
528
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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:
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. Copy the current selector in |
||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
app.kubernetes.io/component: nvidia-dcgm-exporter?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that could work. It is a standardised label
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like
app.kubernetes.io/componenthas a specific meaning as per this K8s docsHow about
app.kubernetes.io/nameinstead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rahulait I missed to mention this earlier, using a boolean as the label value typed seemed a bit off to me.