updater: fetch pods matching VPA selectors

[omerap12]

What type of PR is this?

/kind bug

What this PR does / why we need it:

As discussed in #8773, The updater currently will list all pods when it attempts to find pods that are controlled by VPAs. This approach will only the pods based on the targetRef selector of the VPA object. Hence improve the performance

Which issue(s) this PR fixes:

Fixes #8773

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: omerap12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• vertical-pod-autoscaler/OWNERS [omerap12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[adrianmoisey]

Is this filtering needed?

Later on GetControllingVPAForPod is called, which finds the Pod's upper most controller, and compares that controller to the one defined in the VPA spec, see

autoscaler/vertical-pod-autoscaler/pkg/utils/vpa/api.go

Lines 171 to 201 in 722902d

	func GetControllingVPAForPod(ctx context.Context, pod *core.Pod, vpas []*VpaWithSelector, ctrlFetcher controllerfetcher.ControllerFetcher) *VpaWithSelector {
	parentController, err := FindParentControllerForPod(ctx, pod, ctrlFetcher)
	if err != nil {
	klog.ErrorS(err, "Failed to get parent controller for pod", "pod", klog.KObj(pod))
	return nil
	}
	if parentController == nil {
	return nil
	}
	var controlling *VpaWithSelector
	var controllingVpa *vpa_types.VerticalPodAutoscaler
	// Choose the strongest VPA from the ones that match this Pod.
	for _, vpaWithSelector := range vpas {
	if vpaWithSelector.Vpa.Spec.TargetRef == nil {
	klog.V(5).InfoS("Skipping VPA object because targetRef is not defined. If this is a v1beta1 object, switch to v1", "vpa", klog.KObj(vpaWithSelector.Vpa))
	continue
	}
	if vpaWithSelector.Vpa.Spec.TargetRef.Kind != parentController.Kind ||
	vpaWithSelector.Vpa.Namespace != parentController.Namespace ||
	vpaWithSelector.Vpa.Spec.TargetRef.Name != parentController.Name {
	continue // This pod is not associated to the right controller
	}
	if PodMatchesVPA(pod, vpaWithSelector) && Stronger(vpaWithSelector.Vpa, controllingVpa) {
	controlling = vpaWithSelector
	controllingVpa = controlling.Vpa
	}
	}
	return controlling
	}

My assumption is that this filtering isn't needed here, as it will happen later.

[adrianmoisey]

Oh, I see. We want to ensure that the podsList doesn't contain duplicate pods.

[adrianmoisey]

I don't know if this is a good idea, but Kubernetes does provide a Set.
I wonder if that would be more memory efficient than maintaining a list and a map.

It is used in various other controllers (example), so I assume it's performant.

[omerap12]

Yeah totally a better idea.
Addressed in: ac977ed

[adrianmoisey]

Oh, I meant use the Set for both the deduplication, and the final list to iterate over

[omerap12]

Why does this matter? We already have:

allLivePods := filterDeletedPods(podsList)

This function receives a list and returns a list, and then we iterate over that result here:

for _, pod := range allLivePods {
    controllingVPA := vpa_api_util.GetControllingVPAForPod(ctx, pod, vpas, u.controllerFetcher)
    if controllingVPA != nil {
        controlledPods[controllingVPA.Vpa] = append(controlledPods[controllingVPA.Vpa], pod)
    }
}

I can update the filterDeletedPods function if needed, but I’m not seeing the benefit here.

[adrianmoisey]

I assume it's more memory efficient.
Currently both seenPods and seenPods contain the same list of pods.

Why define it twice? It also simplifies the code slightly, making it easier to follow

[omerap12]

Fair enough. how about now?

[omerap12]

/label tide/merge-method-squash

[adrianmoisey]

Generally speaking, I think this change makes sense.
However, I think we need some concrete data that this is actually improves the updater. I think it's worth figuring out a way to test it (I have some ideas I'll try explore)

What would be amazing, is some sort of test/benchmark that we can run in prow that will fail if the situation gets worse. But that may be complicated to setup.

[omerap12]

Generally speaking, I think this change makes sense. However, I think we need some concrete data that this is actually improves the updater. I think it's worth figuring out a way to test it (I have some ideas I'll try explore)

What would be amazing, is some sort of test/benchmark that we can run in prow that will fail if the situation gets worse. But that may be complicated to setup.

That would be great, but I think a way to test it (even manually) is a good option at the moment