feat: using indexes to optimize Pod-VariantAutoscaling mapping#603
feat: using indexes to optimize Pod-VariantAutoscaling mapping#603WheelyMcBones merged 17 commits intollm-d:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a module that registers custom field indexes to enable faster lookups for VariantAutoscalings and Pods, addressing issue #546. The optimization moves away from label-based list operations to owner reference traversal combined with indexed lookups.
Changes:
- Added new indexers.go module with three custom field indexes (Pod-to-owner, ReplicaSet-to-Deployment, and VA-to-Deployment)
- Updated handleDeploymentEvent in variantautoscaling_controller.go to use indexed VA lookup
- Refactored pod_va_mapper.go to use owner reference traversal instead of label selector matching
- Enhanced test coverage in pod_va_mapper_test.go with proper ReplicaSet owner references
- Integrated index setup in main.go initialization
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/controller/indexers.go | New module defining three custom field indexes and helper functions for efficient lookup of Pods, ReplicaSets, VAs, and their relationships |
| internal/controller/variantautoscaling_controller.go | Updated handleDeploymentEvent to use indexed FindVAsForDeployment lookup instead of listing all VAs |
| internal/collector/source/pod_va_mapper.go | Refactored findDeploymentForPod to traverse owner references (Pod→ReplicaSet→Deployment) instead of using label selectors |
| internal/collector/source/pod_va_mapper_test.go | Enhanced tests with helper functions for creating Pods and ReplicaSets with proper owner references; added tests for caching behavior and edge cases |
| cmd/main.go | Added SetupIndexes call during initialization to register custom field indexes with the manager |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
internal/collector/source/pod_va_mapper.go:99
- The findVAForDeployment function matches VAs based only on ScaleTargetRef.Name and Namespace, without checking ScaleTargetRef.Kind. This means if a VariantAutoscaling targets a non-Deployment resource (e.g., StatefulSet) with the same name as a Deployment in the same namespace, it would be incorrectly matched. Consider adding a check for ScaleTargetRef.Kind == "Deployment" to ensure only VAs targeting Deployments are matched.
// findVAForDeployment finds the VariantAutoscaling object that targets a Deployment.
func (m *PodVAMapper) findVAForDeployment(
deploymentName string,
namespace string,
variantAutoscalings map[string]*llmdv1alpha1.VariantAutoscaling,
) string {
for vaName, va := range variantAutoscalings {
if va == nil {
continue
}
if va.Spec.ScaleTargetRef.Name == deploymentName &&
va.Namespace == namespace {
return vaName
}
}
return ""
|
/ok-to-test |
|
🚀 E2E tests triggered by /ok-to-test |
WheelyMcBones
added
ready-for-review
|
🚀 E2E tests triggered by /ok-to-test |
|
@WheelyMcBones OpenShift E2E's fail ? |
WheelyMcBones
force-pushed
the
pod-va-indexer
branch
from
f4ab3e2 to
506c50d
Compare
|
/ok-to-test |
|
🚀 E2E tests triggered by /ok-to-test |
| // Note: add other Kinds when support to other scaleTargetRefs is added | ||
| // By default, assume 'apps/v1' for unsupported Kinds | ||
| default: | ||
| logger := ctrl.LoggerFrom(context.TODO()) |
There was a problem hiding this comment.
Using context.TODO() here to get a logger is problematic because this indexer function is called by the controller-runtime framework without a proper context attached. The logger retrieved from context.TODO() won't have request-scoped fields and may not be properly initialized. Consider using the global logger directly or removing the logging entirely since this is a fast-path index function that should avoid side effects.
| logger := ctrl.LoggerFrom(context.TODO()) | |
| logger := ctrl.Log |
| } | ||
|
|
||
| // Verify the Deployment is in our map of tracked Deployments | ||
| deploymentKey := namespace + "/" + rsOwner.Name |
There was a problem hiding this comment.
The key format namespace + "/" + rsOwner.Name is hardcoded here, but there's a utility function utils.GetDeploymentKey(namespace, name) that should be used for consistency across the codebase. This ensures all deployment keys follow the same format and makes future changes easier to maintain.
|
/ok-to-test |
|
🚀 E2E tests triggered by /ok-to-test |
This PR introduces a module that registers a custom field index to enable for faster look-ups for
VariantAutoscalings.Note: to avoid breaking changes, in case of missing
spec.scaleTargetRef.apiVersion, a default toapps/v1is added for Deployments or unsupported Kinds.Closes #546.