VPA: Fix nil ptr when loading metrics and memory-saver=true
#8052
Conversation
k8s-ci-robot
added
kind/bug
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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
added
the
size/M
plkokanov
changed the title
memory-saver=truememory-saver=true
|
/assign @voelzmo |
raywainman
left a comment
raywainman
left a comment
There was a problem hiding this comment.
Thanks a lot for fixing this!
| 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've applied the suggestion. PTAL when you can :)
plkokanov
force-pushed
the
fix/nil-pointer
branch
from
30762ce to
260c306
Compare
|
/approve |
k8s-ci-robot
added
the
approved
|
/remove-approve seems like tests didn't run yet for the new commit |
k8s-ci-robot
added
ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: plkokanov, voelzmo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
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.GetContainersMetricsloads metrics for all pods in the cluster whereas whenmemory-saver=trueonly pods for which there are VPAs are added to thefeeder.clusterState.Pods()mapI also added a test that breaks without this change.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I have left
NONEin 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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: