feat: Add DRA-aware GPU accounting#1378
Conversation
ev-shindin
left a comment
There was a problem hiding this comment.
Thanks @MohanKumar21! Nice feature please rebase PR and address review comments.
Signed-off-by: MohanKumar21! <mohanmrm20@gmail.com>
Signed-off-by: MohanKumar21! <mohanmrm20@gmail.com>
0740c2e to
f887463
Compare
ev-shindin
left a comment
There was a problem hiding this comment.
Thanks @MohanKumar21 ! Please address two correctness gaps ans several provide missing test-cases.
| // GetLeaderPodTemplateSpec falls back to the worker template. Size counts the | ||
| // leader, so total = leaderGPUs + (Size-1)*workerGPUs is correct in both the | ||
| // leaderless (N*workerGPUs) and distinct-leader cases. | ||
| total := leaderGPUs + (int(groupSize)-1)*workerGPUs |
There was a problem hiding this comment.
The shared-claim dedup added to DiscoverUsage (the seenClaims map) is not applied on this capacity path. GetPodTemplateGPUs (called at L383 and L401) takes no seen map, so if a scale-target pod template references a literal ResourceClaimName (a single shared claim) rather than a ResourceClaimTemplateName, that one physical claim is counted in the leader term and again in (groupSize-1)*workerGPUs. Thread a shared seen map through the two GetPodTemplateGPUs calls, or explicitly treat ResourceClaimName in a scale-target template as unsupported/ignored and document why.
|
|
||
| // CountDRADeviceRequests returns the count for DRA requests that declare an | ||
| // exact count. The Kubernetes default for ExactCount with omitted count is one. | ||
| func CountDRADeviceRequests(requests []resourcev1.DeviceRequest) int { |
There was a problem hiding this comment.
CountDRADeviceRequests (and the allocation-results path in getResourceClaimDeviceCount L169 / getResourceClaimTemplateDeviceCount L189) sum every device regardless of DeviceClassName. The standard path (GetContainersGPUs) filters against constants.VendorResources; here a claim mixing a GPU with a NIC/FPGA — or a non-GPU claim entirely — inflates the GPU total that drives scaling. Filter by a known GPU device-class allow-list, or document the "every DRA claim on a WVA-managed pod is GPU-only" assumption and why it holds.
| containerGPUs += int(qty.Value()) | ||
| } | ||
| } | ||
| containerGPUs += resources.GetContainersDRAExtendedResources([]corev1.Container{container}) |
There was a problem hiding this comment.
The regular-container equivalent (L279) is covered by TestGetPodGPURequests_DRAExtendedResource, but no test exercises an init container requesting a deviceclass.resource.kubernetes.io/* resource, leaving this line and its max(initMax, regularTotal) interaction uncovered.
| } | ||
|
|
||
| total := 0 | ||
| for _, podClaim := range spec.ResourceClaims { |
There was a problem hiding this comment.
The loop accumulates total and returns the partial value with the error (L143/L153/L161); DiscoverUsage adds draCount even when err != nil. No test has a pod with >1 spec.resourceClaims where the first resolves and a later one fails. Add a two-claim pod (first → 2, second → Get fails) asserting 2, err != nil.
| assert.Equal(t, 2, result["NVIDIA-H100-SXM5-80GB"]) | ||
| } | ||
|
|
||
| func TestDiscoverUsage_DRAErrorDegradesPerPod(t *testing.T) { |
There was a problem hiding this comment.
The pod has no non-DRA GPU request, so the result is 0 regardless of whether the DRA-error-degrades path runs. Give the pod a real nvidia.com/gpu request so the expected result is non-zero - the test then fails if a DRA error aborts the whole pod's accounting instead of just the DRA piece.
| } | ||
| } | ||
|
|
||
| func TestGetDRAAwareGPUsPerReplica(t *testing.T) { |
There was a problem hiding this comment.
All table cases use plain nvidia.com/gpu templates; only the fallback branch (...FallsBackOnError, L111) touches real DRA objects. Add a case where leader/worker templates reference a ResourceClaimTemplateName resolving in the fake client, e.g. leader 1 GPU + worker template 2 devices + size 3 -> expect 1 + 2*2 = 5.
| // GetPodTemplateGPUs returns the accelerator count declared by a pod template, | ||
| // including standard extended GPU resources and exact-count DRA claims. | ||
| func GetPodTemplateGPUs(ctx context.Context, c client.Reader, namespace string, template *corev1.PodTemplateSpec) (int, error) { | ||
| if template == nil { |
There was a problem hiding this comment.
The nil guards in GetPodTemplateGPUs (L68), GetPodDRADeviceCount (L93), and GetPodSpecDRADeviceCount (L106) sit on a per-reconcile / per-pod hot path; a refactor that dereferences before the check would panic uncaught. Add three one-line tests asserting each returns 0, nil for nil input.
| require.True(t, apierrors.IsForbidden(err)) | ||
| } | ||
|
|
||
| func TestCountDRADeviceRequests(t *testing.T) { |
There was a problem hiding this comment.
Every table case passes a single-element requests slice, so the total += accumulation across multiple DeviceRequests in one claim is unverified. Add a case with two requests (Exactly count=2 + Exactly count=1) asserting the sum (3).
Fixes #
Proposed Changes
nvidia.com/gpu).internal/resourceshelpers resolve device counts frompod.spec.resourceClaims— directResourceClaimreferences,ResourceClaimTemplatereferences (including generated claims frompod.status.resourceClaimStatuses),exactCountallocation mode (with the Kubernetes default of 1 when count is omitted), andfirstAvailablesubrequests (max across alternatives). Allocated claims usestatus.allocation.devices.results, which also coversallocationMode: All.deviceclass.resource.kubernetes.io/<device-class>) are summed alongside vendor GPU resources.K8sWithGpuOperator.DiscoverUsage) now includes DRA device requests from running pods when computing per-accelerator usage.BuildVariantStatesand the v2 analysis path) resolvesgpusPerReplicavia a new DRA-aware helper (utils.GetDRAAwareGPUsPerReplica) that handles leader/worker (LWS) pod templates and falls back to the legacy pod-template count if DRA objects cannot be resolved.resource.k8s.io/v1types in the scheme and extend the manager ClusterRole with read access (get/list/watch) todeviceclasses,resourceclaims,resourceclaimtemplates, andresourceslices. Lookups degrade gracefully (count 0) on clusters without the DRA API (NoMatchError) or when claims are not found.Pre-review Checklist
internal/resources/resource_test.go,internal/discovery/k8s_with_gpu_operator_test.go); no e2e added since the kind-emulator environment does not provision a DRA driver.Release Note
Docs