-
Notifications
You must be signed in to change notification settings - Fork 4.3k
updater: fetch pods matching VPA selectors #8807
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: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Omer Aplatony <[email protected]>
|
[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:
Approvers can indicate their approval by writing |
Signed-off-by: Omer Aplatony <[email protected]>
Signed-off-by: Omer Aplatony <[email protected]>
| for _, pod := range podsWithSelector { | ||
| uid := string(pod.UID) | ||
| if _, seen := seenPods[uid]; !seen { | ||
| seenPods[uid] = struct{}{} | ||
| podsList = append(podsList, pod) | ||
| } | ||
| } |
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.
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.
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.
Oh, I see. We want to ensure that the podsList doesn't contain duplicate pods.
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.
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: