Skip to content

cri_stats: handle missing cpu stats#7198

Merged
fuweid merged 1 commit intocontainerd:mainfrom
endocrimes:dani/fix-panic
Jul 22, 2022
Merged

cri_stats: handle missing cpu stats#7198
fuweid merged 1 commit intocontainerd:mainfrom
endocrimes:dani/fix-panic

Conversation

@endocrimes
Copy link
Copy Markdown
Contributor

@endocrimes endocrimes commented Jul 22, 2022

#7186 changed how container cpu stats are populated. Previously, UsageNanoCores were computed at the same time as populating UsageCoreNanoSeconds, and only when UsageCoreNanoSeconds could be populated.

After the change however, we attempt to compute UsageNanoCore's regardless of the state of the provided metrics. This can panic in cases where we do not yet have CPU stats (e.g if the container did not start).

This change restores the previous behavior, by skipping the computation if cpu metrics are unavailable.

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Jul 22, 2022

Hi @endocrimes. Thanks for your PR.

I'm waiting for a containerd 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.

Details

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/test-infra repository.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jul 22, 2022

thanks! Please use git commit -s to sign off~

@endocrimes
cri_stats: handle missing cpu stats
3125f7e 
Signed-off-by: Danielle Lancashire <[email protected]>
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Jul 22, 2022

/ok-to-test

@endocrimes
Copy link
Copy Markdown
Contributor Author

endocrimes commented Jul 22, 2022

@fuweid Done - lack of habit, sorry!

fuweid
fuweid approved these changes Jul 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM on green

dcantah
dcantah reviewed Jul 22, 2022
View reviewed changes
Comment thread pkg/cri/server/container_stats_list.go
dcantah
dcantah approved these changes Jul 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@dcantah dcantah left a comment

Choose a reason for hiding this comment

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

One small suggestion. LGTM on green also, thanks! Windows failure looks unrelated if anyone's able to reschedule the run.

@dims
Copy link
Copy Markdown
Member

dims commented Jul 22, 2022

restarted the failed jobs

estesp
estesp approved these changes Jul 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 05a71fd into containerd:main Jul 22, 2022
@fuweid fuweid mentioned this pull request Jul 22, 2022
Merged
@fuweid fuweid added the area/cri Container Runtime Interface (CRI) label Jul 22, 2022
@jsturtevant
Copy link
Copy Markdown
Contributor

jsturtevant commented Jul 22, 2022

Thanks @endocrimes! Sorry I missed this!

@endocrimes endocrimes deleted the dani/fix-panic branch July 22, 2022 19:08
@endocrimes
Copy link
Copy Markdown
Contributor Author

endocrimes commented Jul 22, 2022

@jsturtevant no worries - it's an easy mistake to make, the "it doesn't have stats" case was not super explicit before 😅

@jsturtevant
Copy link
Copy Markdown
Contributor

jsturtevant commented Jul 22, 2022

just for history, our nightly windows CI hit this as well: https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-kubernetes-e2e-capz-master-containerd-nightly-windows/1550549379679522816

This was referenced Jul 22, 2022
Closed
Merged
@mxpv mxpv mentioned this pull request Apr 7, 2023
Merged
@mxpv mxpv added cherry-picked/sbserver Changes are backported to sbserver and removed cherry-pick/sbserver labels Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dcantah dcantah dcantah approved these changes

@estesp estesp estesp approved these changes

@fuweid fuweid fuweid approved these changes

Assignees

No one assigned

Labels

area/cri Container Runtime Interface (CRI) cherry-picked/sbserver Changes are backported to sbserver ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@endocrimes @k8s-ci-robot @fuweid @dims @jsturtevant @estesp @dcantah @mxpv