Refactor usageNanoCores be to used for all OSes #7186
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @jsturtevant. 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 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/test-infra repository. |
jsturtevant
force-pushed
the
windows-usage-nano-cores
branch
from
28cba6a to
648af75
Compare
Signed-off-by: James Sturtevant <[email protected]>
jsturtevant
force-pushed
the
windows-usage-nano-cores
branch
from
648af75 to
0d68818
Compare
Contributor
Author
|
Looks like the windows jobs failed when pulling the images. Linux job looks like a flake Is there a way to retrigger those jobs? |
Contributor
Author
|
/assign @dcantah @bobbypage |
|
@jsturtevant: GitHub didn't allow me to assign the following users: bobbypage. Note that only containerd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn response to this:
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. |
dmcgowan
approved these changes
Jul 20, 2022
dcantah
approved these changes
Jul 20, 2022
kzys
approved these changes
Jul 20, 2022
Contributor
|
This PR has caused a regression - we now panic when entering line 69 in kubernetes CI that targets containerd main.
|
Member
|
thanks! I think there is some created containers which doesn't start. The metrics from shim doesn't have stat for the container. But the line69 just reads the cpu from nil instance. |
Contributor
|
yeah - just testing a patch that handles that now |
mikebrow
reviewed
Jul 22, 2022
| } | ||
|
|
||
| // this is a calculated value and should be computed for all OSes | ||
| nanoUsage, err := c.getUsageNanoCores(cntr.Metadata.ID, false, cs.Cpu.UsageCoreNanoSeconds.Value, time.Unix(0, cs.Cpu.Timestamp)) |
Member
There was a problem hiding this comment.
not sure why the code was refactored, always passing false even for the sandbox "pause" container?
Member
There was a problem hiding this comment.
kk code works, fix is fine.. later we can refactor this so windows/ other impls of containerMetrics() have usage nano as well.. and the other stats necessary
Contributor
Author
There was a problem hiding this comment.
I guess could have cleaned that up a bit more. merged much faster than I expected :-).
I can revisit this in #7099 once we have some agreement on WindowsPodsandbox stats for kubernetes/enhancements#3439
mxpv
added
cherry-picked/sbserver
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: James Sturtevant [email protected]
fixes: #7184
using same steps as in #7184: