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

Fix NPE in vpa-updater when Pod owner isn't scalable #6712

Merged

Conversation

voelzmo
Copy link
Contributor

@voelzmo voelzmo commented Apr 16, 2024

What type of PR is this?

/kind bug

What this PR does / why we need it:

The method ctrlFetcher.FindTopMostWellKnownOrScalable returns nil without an additional error, when there is no owner that is either scalable (implementing the /scale subresource) or is well-known. In this case, just skip the current Pod, as it cannot be controlled by a VPA.

Which issue(s) this PR fixes:

Fixes #6709

Special notes for your reviewer:

This PR adds two tests to make sure this cannot regress:

  • vertical-pod-autoscaler/pkg/target/controller_fetcher/controller_fetcher_test.go gets the new testcase custom resource with no scale subresource which ensures that for a Pod, which is not under VPA control and has an ownerRef to a custom resource that doesn't implement /scale, the method FindTopMostWellKnownOrScalable returns nil, nil
  • vertical-pod-autoscaler/pkg/utils/vpa/api_test.go gets an additional test assertion which uses a fake controllerFetcher, which always returns nil to make sure that GetControllingVPAForPod can deal with a nil, nil response from ctrlFetcher.FindTopMostWellKnownOrScalable

Does this PR introduce a user-facing change?

Fix: NPE in vpa-updater when Pods have an `ownerReference` to a Custom Resource which doesn't implement the `/scale` subresource

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/vertical-pod-autoscaler labels Apr 16, 2024
@voelzmo voelzmo changed the title parentController may be nil when owner isn't scalable Fix NPE in vpa-updater when Pod owner isn't scalable Apr 16, 2024
@kwiesmueller
Copy link
Member

Does it replicate if we just make the owner be a ConfigMap or any other resource?

@kwiesmueller
Copy link
Member

I would prefer a test, otherwise lgtm.

@voelzmo
Copy link
Contributor Author

voelzmo commented Apr 17, 2024

Does it replicate if we just make the owner be a ConfigMap or any other resource?

Great Idea! I've replicated it by setting the ownerReference to kind: ConfigMap. I'll add a test case and comment in there, why we would be doing a seemingly stupid thing :)

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 17, 2024
@voelzmo
Copy link
Contributor Author

voelzmo commented Apr 17, 2024

@kwiesmueller I ended up taking a simpler approach, outlined in the PR original comment above. WDYT?

Copy link
Member

@kwiesmueller kwiesmueller left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 17, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kwiesmueller, voelzmo

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 Apr 17, 2024
@k8s-ci-robot k8s-ci-robot merged commit af1e610 into kubernetes:master Apr 17, 2024
6 checks passed
@voelzmo
Copy link
Contributor Author

voelzmo commented Apr 17, 2024

/cherry-pick vpa-release-1.1

Edit: seems like the bot doesn't pick this up, I ended up cherry-picking it manually to vpa-release-1.1

k8s-ci-robot added a commit that referenced this pull request Apr 18, 2024
…-npe-to-release-1.1

[vpa-release-1.1] Cherry-pick of #6712: Fix NPE in vpa-updater when Pod owner isn't scalable
@akloss-cibo
Copy link

I hoped to update to 1.1.0 to see if #6705 is addressed there, but I'm stuck behind this. Is there a new release planned soon?

@kwiesmueller
Copy link
Member

We'll work on a 1.1.1 release this week.

@kwiesmueller kwiesmueller mentioned this pull request Apr 24, 2024
@kwiesmueller
Copy link
Member

Released as https://github.com/kubernetes/autoscaler/releases/tag/vertical-pod-autoscaler-1.1.1

@nikimanoledaki
Copy link
Contributor

I hoped to update to 1.1.0 to see if #6705 is addressed there, but I'm stuck behind this. Is there a new release planned soon?

Hi! Just saw that the other issue was referenced here. @akloss-cibo do you have an update on whether #6705 was addressed for you? More info on your theory around this would be appreciated too. Perhaps best to speak in the open issue rather than here :) Thank you!

@raywainman raywainman mentioned this pull request Aug 6, 2024
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
5 participants