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.
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
Prevent returning invalid usageNanoCores value when cpuacct is reset in a live container #83507
Conversation
Hi @ashleykasim. 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. 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. |
/cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
1f9ddff
to
b9b957f
Compare
/retest |
/assign @tallclair |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/assign @dashpole @derekwaynecarr
b9b957f
to
7d4f555
Compare
@@ -649,7 +649,7 @@ func (p *criStatsProvider) getAndUpdateContainerUsageNanoCores(stats *runtimeapi | |||
defer p.mutex.Unlock() | |||
|
|||
cached, ok := p.cpuUsageCache[id] | |||
if !ok || cached.stats.UsageCoreNanoSeconds == nil { | |||
if !ok || cached.stats.UsageCoreNanoSeconds == nil || stats.Cpu.UsageCoreNanoSeconds.Value < cached.stats.UsageCoreNanoSeconds.Value { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we return an error to match what we do for negative time values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My one concern with making this an error is currently this condition will also get triggered by the bug addressed in #83504. Though I suppose an error message for hitting this case would have aided me in debugging what was going on when I was hitting that bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I am wrong, but it is a bug if cpu usage reported is reset for a running container. We should fail loudly if at all possible, rather than fail silently. If there are cases where it is expected to be reset, we should log a warning.
/retest |
/priority important-soon |
Hi @ashleykasim Bug triage team here. |
@dashpole @derekwaynecarr @yujuhong @tallclair Bug triage team here. |
Yeah, this would be good to have in the release. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashleykasim, dashpole 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 |
Looks like a test flake (storage tests). /retest |
/retest Review the full test history for this PR. Silence the bot with an |
8 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 |
/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 |
/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 |
/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 |
…3507-upstream-release-1.16 Automated cherry pick of #83507: correctly handle resetting cpuacct in a live container
…3507-upstream-release-1.17 Automated cherry pick of #83507: correctly handle resetting cpuacct in a live container
What type of PR is this?
/kind bug
What this PR does / why we need it:
This fixes an issue with that causes similar incorrect reporting of cpu as in #83504. For debugging purposes, it is possible to reset cpuacct to 0 for a live container, which would create the same issue with the calculation for cpu usage, where it is subtracting a nonzero number from zero. We should skip trying to calculate usage if cpuacct has been reset instead in order to prevent returning an invalid value for usageNanoCores.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: