kubelet_stats: fix potential e2e crash dereferencing ContainerStats.CPU #79057
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
lgtm
|
/test pull-kubernetes-bazel-build |
There was a problem hiding this comment.
/lgtm cancel
whoops, jumped the gun here. I'm not sure this is actually going to work the way we are hoping.
I think this change will just push the nil failure back to the computeContainerResourceUsage method.
Maybe we should also continue if either cStats or oldStats is nil?
k8s-ci-robot
removed
the
lgtm
rphillips
force-pushed
the
fixes/e2e_kubelet_stats_fix
branch
from
8bdc819
to
9553434
Compare
|
@mattjmcnaughton thanks for the review... I've tweaked the PR with your suggestion. |
|
/retest |
rphillips
force-pushed
the
fixes/e2e_kubelet_stats_fix
branch
from
9553434
to
b9feff9
Compare
|
/retest |
There was a problem hiding this comment.
Cool, I'm convinced that this won't crash :)
My only request is for someone else with more context on this code than me to confirm that we do not want to treat either oldStats or cStats being new as a test failure. I'll let whoever does that confirmation approve.
|
/assign @timothysc |
test/e2e/framework/kubelet_stats.go
Outdated
| @@ -598,6 +598,9 @@ func (r *resourceCollector) collectStats(oldStatsMap map[string]*stats.Container | |||
| } | |||
|
|
|||
| if oldStats, ok := oldStatsMap[name]; ok { | |||
| if oldStats.CPU == nil || cStats.CPU == nil { | |||
There was a problem hiding this comment.
cStats.Memory looks like it might be subject to the same problem if that comes back nil.
There was a problem hiding this comment.
Thanks for the review... Updated the PR :)
rphillips
force-pushed
the
fixes/e2e_kubelet_stats_fix
branch
from
b9feff9
to
997e1f6
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/retest |
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
There was a problem hiding this comment.
/approve
/hold
this seems reasonable, holding in case we still want input from @timothysc, feel free to hold cancel to unflake and follow up later :-)
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, rphillips 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 |
k8s-ci-robot
added
the
approved
|
I don't see an issue, but I'd defer to @kubernetes/sig-node-pr-reviews for final lgtm. |
|
/lgtm |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/retest Review the full test history for this PR. Silence the bot with an |
2 similar comments
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
What type of PR is this?
/kind flake
What this PR does / why we need it:
The kubelet's stats API can return nil within the CPU attribute of cadvisorapiv2.ContainerStats. This patch fixes the e2e test to check for nil CPU attributes before trying to dereference
oldStats.CPUandcStats.CPU. We are seeing this flake currently within CI.ref: https://bugzilla.redhat.com/show_bug.cgi?id=1679612
ref: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/stats/helper.go#L36-L40
ref: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/stats/helper.go#L77-L84
/cc @kubernetes/sig-node-pr-reviews @sjenning
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: