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

Inconsistent behaviour within VPA components regarding targetRef #6924

Open
adrianmoisey opened this issue Jun 13, 2024 · 4 comments
Open

Inconsistent behaviour within VPA components regarding targetRef #6924

adrianmoisey opened this issue Jun 13, 2024 · 4 comments
Assignees
Labels
area/vertical-pod-autoscaler kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@adrianmoisey
Copy link
Member

adrianmoisey commented Jun 13, 2024

Which component are you using?:

vertical-pod-autoscaler

What version of the component are you using?:

Component version: 1.1.2

What k8s version are you using (kubectl version)?:

Various: 1.28 and 1.30

What environment is this in?:

Any. I've reproduced in GKE, AWS (kOps) and Kind.

What did you expect to happen?:

When setting a targetRef with a lowecase kind, the VPA admission-controller doesn't match the new Pods.
However, the updater component does match the Pod.

What happened instead?:

Pod wasn't matched in the admission-controller

How to reproduce it (as minimally and precisely as possible):

Use the hamster example in this repo, and replace "Deployment" with "deployment"

Anything else we need to know?:

  1. This might be related to Vertical Pod Autoscaler is not recreating pods at runtime #6915
  2. The HPA accepts kind with a lowercase kind ie: deployment

/area vertical-pod-autoscaler
/assign

@adrianmoisey adrianmoisey added the kind/bug Categorizes issue or PR as related to a bug. label Jun 13, 2024
adrianmoisey added a commit to adrianmoisey/autoscaler that referenced this issue Jun 16, 2024
Relates to kubernetes#6924

Previously the case of the `Kind` in the `targetRef` didn't matter. Not
it seems to matter for the admission-controller.

This change attempts to copy what the HPA does. https://github.com/kubernetes/kubernetes/blob/v1.30.0/pkg/controller/podautoscaler/horizontal.go#L1321-L1325
@voelzmo
Copy link
Contributor

voelzmo commented Jul 15, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 15, 2024
@raywainman
Copy link
Contributor

Some notes:

  • In the admission controller, we assume "Deployment" here when fetching the label selector, however I think if you use deployment here, it will fail this check but still ultimately work because it implements the /scale subresource. So here you would be doing additional lookups against API server.
  • I wonder if we ultimately fail the matching here when we compare the pod's owner (which could be Deployment) with the VPA which is deployment.

@raywainman
Copy link
Contributor

I don't fully understand though why there is a difference between admission-controller and updater. They both use the same logic.

Here is the logic for the Updater, it is using the same mechanics as the admission-controller.

@adrianmoisey
Copy link
Member Author

  • I wonder if we ultimately fail the matching here when we compare the pod's owner (which could be Deployment) with the VPA which is deployment.

Yup, this is where I think the issue lies.
I made a PR to use the RESTMapper (similar to the HPA) here: #6929
I'm unsure if it's the right approach though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/vertical-pod-autoscaler kind/bug Categorizes issue or PR as related to a bug. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants