Skip to content

VPA: Fix nil ptr when loading metrics and memory-saver=true #8052

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

VPA: Fix nil ptr when loading metrics and memory-saver=true #8052

wants to merge 1 commit into from

Conversation

plkokanov
Copy link
Contributor

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR fixes a nil pointer exception that was introduced with #7891 that can happen if memory-saver=true.

The reason this happens is that feeder.metricsClient.GetContainersMetrics loads metrics for all pods in the cluster whereas when memory-saver=true only pods for which there are VPAs are added to the feeder.clusterState.Pods() map

I also added a test that breaks without this change.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

I have left NONE in the release not as I think there isn't a release yet that contains the nil pointer exception.

Does this PR introduce a user-facing change?

NONE

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

@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. area/vertical-pod-autoscaler needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 23, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @plkokanov. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: plkokanov
Once this PR has been reviewed and has the lgtm label, please assign adrianmoisey for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 23, 2025
@plkokanov plkokanov changed the title Fix nil ptr when loading metrics and memory-saver=true VPA: Fix nil ptr when loading metrics and memory-saver=true Apr 23, 2025
@raywainman
Copy link
Contributor

/assign @voelzmo

raywainman
raywainman reviewed Apr 24, 2025
View reviewed changes
Copy link
Contributor

@raywainman raywainman left a comment

Choose a reason for hiding this comment

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

Thanks a lot for fixing this!

vertical-pod-autoscaler/pkg/recommender/input/cluster_feeder.go
klog.V(3).InfoS("Skipping metric samples for init container", "pod", klog.KRef(containerMetrics.ID.PodID.Namespace, containerMetrics.ID.PodID.PodName), "container", containerMetrics.ID.ContainerName)
droppedSampleCount += len(containerMetrics.Usage)
continue
if pod, exists := feeder.clusterState.Pods()[containerMetrics.ID.PodID]; exists && pod != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a similar comment as below? Something like:

// Not all pod states are tracked in memory saver mode

Just helps give context behind the defensive check here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raywainman raywainman raywainman left review comments

@adrianmoisey adrianmoisey Awaiting requested review from adrianmoisey

Assignees

@voelzmo voelzmo

Labels
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. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
vertical-pod-autoscaler-1.4.0
Development

Successfully merging this pull request may close these issues.
4 participants
@plkokanov @k8s-ci-robot @raywainman @voelzmo