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.

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

Cumulative stats can't decrease #10652

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Cumulative stats can't decrease #10652

merged 1 commit into from
Aug 29, 2024

Conversation

jsturtevant
Copy link
Contributor

@jsturtevant jsturtevant commented Aug 28, 2024 •
edited
Loading

During removal of the container a stat value might be reported as zero; in this case the calculation could end up with an extremely large number. If the cumulative stat decreases report zero.

fixes kubernetes/kubernetes#114057

@dosubot dosubot bot added area/cri Container Runtime Interface (CRI) kind/bug labels Aug 28, 2024
@jsturtevant
Copy link
Contributor Author

follows https://github.com/google/cadvisor/blob/c15f44e578c77800b1b82a7bbb67614364f4aedc/info/v2/conversion.go#L247-L249

@jsturtevant jsturtevant mentioned this pull request Aug 28, 2024
Closed
@jsturtevant
Cumulative stats can't decrease
f6677a4 
During removal of the container a stat value might be reported as zero; in this case the caluclation could end up with an extremely large number.  If the cumulative stat decreases report zero.

Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant jsturtevant force-pushed the fix-nano-calculation branch from b5cc531 to f6677a4 Compare August 28, 2024 23:23
cpuguy83
cpuguy83 approved these changes Aug 28, 2024
View reviewed changes
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@jsturtevant
Copy link
Contributor Author

/test pull-containerd-node-e2e
looks like unrelated flake

@dmcgowan
Copy link
Member

Backport 1.6 and 1.7?

@cpuguy83 cpuguy83 added cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Aug 29, 2024
@cpuguy83
Copy link
Member

Yep, I think so. I added those labels.

@jsturtevant
Copy link
Contributor Author

yes, we are seeing it in 1.6

@dmcgowan dmcgowan added this pull request to the merge queue Aug 29, 2024
Merged via the queue into containerd:main with commit dacde84 Aug 29, 2024
53 checks passed
@jsturtevant jsturtevant mentioned this pull request Sep 3, 2024
Merged
@samuelkarp
Copy link
Member

/cherrypick release/1.6

@k8s-infra-cherrypick-robot
Copy link

@samuelkarp: #10652 failed to apply on top of branch "release/1.6":

Applying: Cumulative stats can't decrease
Using index info to reconstruct a base tree...
A	internal/cri/server/container_stats_list.go
A	internal/cri/server/container_stats_list_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cri/server/container_stats_list_linux_test.go
CONFLICT (content): Merge conflict in pkg/cri/server/container_stats_list_linux_test.go
CONFLICT (modify/delete): internal/cri/server/container_stats_list.go deleted in HEAD and modified in Cumulative stats can't decrease. Version Cumulative stats can't decrease of internal/cri/server/container_stats_list.go left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Cumulative stats can't decrease
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release/1.6

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.

@jsturtevant
Copy link
Contributor Author

jsturtevant commented Sep 3, 2024 •
edited
Loading

didn't know there was a bot, since this is in cri server it required some manual changes for SBServer changes in 1.6/1.7: #10671

This was referenced Sep 3, 2024
Merged
Open
@austinvazquez austinvazquez added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpuguy83 cpuguy83 cpuguy83 approved these changes

@dmcgowan dmcgowan dmcgowan approved these changes

Assignees
No one assigned
Labels
area/cri Container Runtime Interface (CRI) cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch kind/bug size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Impossibly high cpu usage value from kubelet embedded cadvisor (k8s 1.23.12)
7 participants
@jsturtevant @dmcgowan @cpuguy83 @samuelkarp @k8s-infra-cherrypick-robot @k8s-ci-robot @austinvazquez