Skip to content
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

vpa-admission-controller: Improve finding the matching VPA for Pod #7010

Merged

Conversation

ialidzhikov
Copy link
Contributor

@ialidzhikov ialidzhikov commented Jul 8, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

As outlined in #6925, right now the logic in the vpa-admission-controller to find the matching VPA for a Pod is expensive/ineffective. In a summary, no matter the given Pod is scaled by VPA or not, the vpa-admission-controller gets all VPAs in the namespace and fetches the selector for every VPA. Fetching the selector for a VPA is an expensive operation when the VPA's targetRef is a custom resource. For such cases, the /scale subresource is being fetched. Note that the used selectorFetcher does not have a cache client-side and it always does a GET request for the /scale subresource:

This PR improves the effectiveness of the operations by using the following steps.

  1. Find to the "top most well-known scalable controller" (aka parent contrller) for the Pod
  2. Find all VPAs that match the parent controller
  3. Fetch the selector (fetch /scale subresource when the targetRef is a CR) only for VPAs which targetRef is matching the parent controller
  4. Find the strongest VPA that matches the Pod

In this way the selector would be fetched only for VPAs that match the Pod's parent controller. Previously it was fetched always for all VPAs in the namespace.

In GetControllingVPAForPod func it was always fetching the parent controller for a Pod, hence we are not introducing a change by fetching always the Pod's parent controller at the beginning of the GetMatchingVPA func.

Which issue(s) this PR fixes:

N/A
but a result of the following issues #6925, #6884

Special notes for your reviewer:

Does this PR introduce a user-facing change?

vpa-admission-controller: The logic for finding the matching VPA for Pod is now more effective.

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


@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. labels Jul 8, 2024
@k8s-ci-robot k8s-ci-robot added area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 8, 2024
@ialidzhikov ialidzhikov force-pushed the enh/improve-get-matching-vpa branch 2 times, most recently from 9fe3a2b to 4bd7ae0 Compare July 8, 2024 10:49
@ialidzhikov ialidzhikov marked this pull request as ready for review July 8, 2024 11:10
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2024
Copy link

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening the PR! Just one small comment on my side.

Copy link

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the logs, just some very minor nits.

vertical-pod-autoscaler/pkg/target/fetcher.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/vpa/api.go Outdated Show resolved Hide resolved
controlling = vpaWithSelector
controllingVpa = controlling.Vpa
}
}
return controlling
}

// FindParentControllerForPod returns the parent controller (topmost well-known or scalable controller) for the given Pod.
func FindParentControllerForPod(pod *core.Pod, ctrlFetcher controllerfetcher.ControllerFetcher) (*controllerfetcher.ControllerKeyWithAPIVersion, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a test be written for this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added unit test in dcbd716.

@ialidzhikov
Copy link
Contributor Author

I am sorry for the delay. I got stuck with other high priority tasks. I will get back to the PR review and address it next week.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2024
@ialidzhikov
Copy link
Contributor Author

@adrianmoisey @plkokanov @voelzmo I rebased the PR and address review comments! PTAL.

Copy link

@plkokanov plkokanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm on my side

@adrianmoisey
Copy link
Member

I've had a look and I'm also generally happy with it
I'd like to still give it a test run locally

@adrianmoisey
Copy link
Member

I gave this a test, seems to work.
I'm unsure how to compare the performance of this change to what it was before hand though.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 31, 2024
@kwiesmueller
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ialidzhikov, kwiesmueller, plkokanov

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2024
@k8s-ci-robot k8s-ci-robot merged commit d06fe04 into kubernetes:master Nov 1, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/vertical-pod-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants