Reduce cache memory footprint by optimizing what we store - #2886
kvalliyurnatt wants to merge 1 commit into
Conversation
Signed-off-by: Karthikeyan Valliyurnatt <kvalliyurnat@nvidia.com>
8793ed4 to
6f1405d
Compare
kvalliyurnatt
changed the title
📝 WalkthroughWalkthroughThe change adds centralized GPU Operator cache construction and configuration. The cache handles optional ImageStream and ResourceClaim APIs, propagates other discovery errors, scopes selected resources to workload namespaces, strips managed fields, and filters cached objects. Manager setup now uses the shared options and custom cache constructor. Tests cover discovery behavior, API watches, synchronization, selectors, object transformations, and repeated cached reads. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to DRA workloads outside the operator namespaces can be left stuck terminating during a driver transition. Fix the cache scope and provide the required Go validation results before merge. 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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
cmd/gpu-operator/cache.go-38-38 (1)
38-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCan you provide the
make fmtandmake unit-testresults before merge?Repository guidance requires both checks for Go changes, and the current PR evidence does not include their results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 0124bc9d-82bc-471d-b46e-41b6d6682f6d
📒 Files selected for processing (3)
cmd/gpu-operator/cache.gocmd/gpu-operator/cache_test.gocmd/gpu-operator/main.go
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
| func operatorCacheOptions(operatorNamespace string) cache.Options { | ||
| stripManagedFields := cache.TransformStripManagedFields() | ||
| // DRA teardown checks inspect Pods and their ResourceClaims across the existing | ||
| // cache namespaces. Preserve that scope independently of operand-only resources. | ||
| workloadNamespaces := map[string]cache.Config{ | ||
| operatorNamespace: {}, | ||
| consts.OpenshiftNamespace: {}, | ||
| } | ||
| return cache.Options{ | ||
| DefaultNamespaces: map[string]cache.Config{operatorNamespace: {}}, | ||
| DefaultTransform: stripManagedFields, | ||
| ByObject: map[client.Object]cache.ByObject{ | ||
| &corev1.Node{}: { | ||
| // Keep all nodes for GPU discovery and label cleanup, but omit the | ||
| // image inventory, which no operator consumer uses. | ||
| Transform: func(in any) (any, error) { | ||
| if node, ok := in.(*corev1.Node); ok { | ||
| node.Status.Images = nil | ||
| } | ||
| return stripManagedFields(in) | ||
| }, | ||
| }, | ||
| &corev1.Pod{}: {Namespaces: workloadNamespaces}, | ||
| &resourcev1.ResourceClaim{}: {Namespaces: workloadNamespaces}, | ||
| &apiextensionsv1.CustomResourceDefinition{}: { | ||
| // crdExists only checks ServiceMonitor availability. Apply the | ||
| // selector to LIST/WATCH so other CRD schemas never enter the cache. | ||
| Field: fields.OneTermEqualSelector("metadata.name", controllers.ServiceMonitorCRDName), | ||
| }, | ||
| &apiimagev1.ImageStream{}: { | ||
| // Only the Driver Toolkit ImageStream is read from openshift. | ||
| Namespaces: map[string]cache.Config{consts.OpenshiftNamespace: {}}, | ||
| Field: fields.OneTermEqualSelector("metadata.name", "driver-toolkit"), | ||
| }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cache Pods and ResourceClaims cluster-wide for DRA teardown
operatorCacheOptions caches both types only in the operator and OpenShift namespaces. During the DRA-to-device-plugin transition, nodeHasDRAClaimPods therefore misses claim-bearing Pods in other namespaces, removes gpu.deploy.dra-driver, and can leave those Pods stuck in Terminating when their claims cannot be unprepared. Configure both cache entries for cluster-wide coverage.
There was a problem hiding this comment.
I noticed this as well and this seems to be a pre existing bug even before these changes, I am happy to fix it in this PR or carve out a separate PR for that. This will increase our memory footprint though, since we will end up caching all Pods and ResourceClaims in all namespaces. I can look to see there are ways to better than that
There was a problem hiding this comment.
@kvalliyurnatt, please include the cluster-wide Pod and ResourceClaim cache coverage in this PR.
A separate PR is appropriate only for unrelated cleanup or broader pre-existing DRA behavior. This change must prevent nodeHasDRAClaimPods from missing claim-bearing Pods outside the operator and OpenShift namespaces.
You are interacting with an AI system.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
Description
We can optimize what we store in cache today to optimize operator's memory usage.
metadata.managedFieldsfrom cached objects and the unusedstatus.imagesinventory from Nodes.openshift/driver-toolkit.Checklist
make lint)make validate-generated-assets)make validate-modules)Testing
I have added some unit tests, but I plan to test with and without the changes to get a baseline of how much memory we can save here.
Cache memory validation results
Compared candidate 6f1405d against its exact parent 9f85a2a, using the same Go 1.27.1 compiler, dependencies, runtime base, Helm chart, operand versions, and operator resource limits.
Test setup
measurement windows with 30-second sampling.
window was collected.
Steady-state results
Both windows completed with zero operator restarts or reconciliation errors.
ClusterPolicy remained ready, expected operands were healthy, all three GPUs
remained allocatable, and CUDA vector-add validation passed on both nodes.
pprof observations
Separate warmed-up profiling runs captured:
The parent profile includes retained CRD-schema and decoding allocations,
consistent with the broader cache. These are sampled allocation-site estimates,
not exact cache-ownership measurements. Short idle CPU/allocation profiles were
insufficient for reliable hotspot comparisons.
Scope
These results demonstrate lower idle memory without an observed API-load increase
in this two-node test. DRA, and OpenShift validation remain outstanding.