[cri] Implement CRI Pod and Container stats for Windows #7099

jsturtevant · 2022-06-23T23:55:33Z

Implements ListPodSandboxStats and PodSandboxStats for Windows.

Currently is using the branch from kubernetes/kubernetes#110754 for the CRI API updates that were needed for Windows

Special notes for your reviewer:

Still need to add some tests and refactor some of the linux functionality to be able to implement UsageNanoCores.

I made a custom build of crictl that can handle the windows stats but can use any version that has the statsp command.

k8s-ci-robot · 2022-06-23T23:55:42Z

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 /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.

jsturtevant · 2022-06-23T23:57:02Z

/cc @dcantah @kevpar @bobbypage @haircommander @marosset

k8s-ci-robot · 2022-06-23T23:57:06Z

@jsturtevant: GitHub didn't allow me to request PR reviews from the following users: haircommander, marosset, bobbypage.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @dcantah @kevpar @bobbypage @haircommander @marosset

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.

jsturtevant · 2022-07-07T21:30:12Z

The linter is failing on mac because I pulled two functions up that would allow me to re-use code across the windows and linux implementations.

  Error: func `(*criService).getSandboxPidCount` is unused (unused)
  Error: func `(*criService).getUsageNanoCores` is unused (unused)

They are not used in the _other.go so need to re-think how that is done. the commit 77b7464 is fixing a missing UsageNanoCores in the containers stats so I think it makes sense to split that out as a separate PR and can fix one of the linting issues. The other I think I can refactor the stats some even further for re-use which would take care of the second.

jsturtevant · 2022-07-19T23:54:53Z

Moved getUsageNanoCores to seperate PR: #7186

We discussed CRI changed required at sig-node this morning and we will be do a bit more design via the KEP. Once those are merged, I can refresh this PR but wouldn't anticipate drastic changes to the design except may what values we return.

jsturtevant · 2022-10-11T22:06:19Z

We've reach consensus on the changes to CRI: kubernetes/enhancements#3439 and the cri changes have been updated in kubernetes/kubernetes#110754

jsturtevant · 2022-11-22T00:34:16Z

with the cri api update in #7656 this work can continue 🚀. I've rebased and fixed it up to use the new fields.

Next step is to clean it up and get it ready for review.

marosset · 2023-02-15T17:56:12Z

Should this issue get added to the 1.7 milestone?

MikeZappa87 · 2023-02-16T01:43:40Z

Should this issue get added to the 1.7 milestone?

@dmcgowan thoughts?

jsturtevant · 2023-02-21T18:57:22Z

@containerd/maintainers this is ready for final review

pkg/cri/sbserver/sandbox_stats_windows.go

jsturtevant · 2023-03-01T22:04:48Z

/assign @dmcgowan @kevpar @kzys

kevpar

LGTM. Thanks!

MikeZappa87 · 2023-03-02T19:28:27Z

Lgtm

marosset

LGTM

dcantah · 2023-03-03T22:25:40Z

@jsturtevant Can you squash this? I think we're good for check-in after.

Signed-off-by: James Sturtevant <[email protected]>

jsturtevant · 2023-03-03T22:39:14Z

I left the changes separate from Cri Sandbox code and sandbox. Let me know if I should squash to a single commit

dcantah · 2023-03-03T22:54:03Z

@jsturtevant That's fine, makes it easier to revert if anything was wrong in one and not the other

rzlink · 2025-05-31T01:29:56Z

pkg/cri/server/sandbox_stats_windows.go

+	if containerRunTimeStats.Memory.AvailableBytes != nil {
+		if podRuntimeStats.Memory.AvailableBytes == nil {
+			podRuntimeStats.Memory.AvailableBytes = &runtime.UInt64Value{Value: 0}
+		}
+		podRuntimeStats.Memory.AvailableBytes.Value += containerRunTimeStats.Memory.AvailableBytes.Value
+	}


It looks like these lines may never be executed, since the AvailableBytes fields in the podRuntimeStats and containerRuntimeStats input parameters appear to always be nil.

k8s-ci-robot added the needs-ok-to-test label Jun 23, 2022

k8s-ci-robot requested review from dcantah and kevpar June 23, 2022 23:57

jsturtevant force-pushed the cri-only-stats-windows branch 5 times, most recently from 6b33d83 to 77b7464 Compare July 7, 2022 21:07

This was referenced Jul 18, 2022

CRI: Add Windows Podsandbox Stats kubernetes/kubernetes#110754

Merged

Windows cri container stats call doesn't return usageNanoCores #7184

Closed

jsturtevant mentioned this pull request Jul 22, 2022

Refactor usageNanoCores be to used for all OSes #7186

Merged

jsturtevant force-pushed the cri-only-stats-windows branch from 77b7464 to 8527033 Compare November 22, 2022 00:30

jsturtevant force-pushed the cri-only-stats-windows branch 7 times, most recently from a682290 to 618ebd7 Compare December 2, 2022 18:43

jsturtevant force-pushed the cri-only-stats-windows branch 4 times, most recently from 6b45e96 to c9c59d7 Compare December 8, 2022 00:06

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 14, 2023

kevpar reviewed Feb 21, 2023

View reviewed changes

pkg/cri/sbserver/sandbox_stats_windows.go Outdated Show resolved Hide resolved

kevpar reviewed Feb 21, 2023

View reviewed changes

pkg/cri/sbserver/sandbox_stats_windows.go Outdated Show resolved Hide resolved

jsturtevant force-pushed the cri-only-stats-windows branch 3 times, most recently from ed90205 to 830a9c5 Compare February 22, 2023 19:19

k8s-ci-robot assigned dmcgowan, kevpar and kzys Mar 1, 2023

jsturtevant force-pushed the cri-only-stats-windows branch from 830a9c5 to 39b6a5d Compare March 2, 2023 19:12

kevpar approved these changes Mar 2, 2023

View reviewed changes

MikeZappa87 approved these changes Mar 2, 2023

View reviewed changes

marosset approved these changes Mar 3, 2023

View reviewed changes

dcantah added this to the 1.7 milestone Mar 3, 2023

dcantah approved these changes Mar 3, 2023

View reviewed changes

jsturtevant added 2 commits March 3, 2023 14:37

Add Windows Sandbox Stats

08aa576

Signed-off-by: James Sturtevant <[email protected]>

Add Windows Sandbox Stats (sbserver)

32ed559

Signed-off-by: James Sturtevant <[email protected]>

jsturtevant force-pushed the cri-only-stats-windows branch from 39b6a5d to 32ed559 Compare March 3, 2023 22:37

kevpar merged commit 31c9a66 into containerd:main Mar 6, 2023

jsturtevant mentioned this pull request Jun 1, 2023

Fix issue for HPC pod metrics #8626

Merged

rzlink reviewed May 31, 2025

View reviewed changes

[cri] Implement CRI Pod and Container stats for Windows #7099

[cri] Implement CRI Pod and Container stats for Windows #7099

Uh oh!

Conversation

jsturtevant commented Jun 23, 2022

Special notes for your reviewer:

Uh oh!

k8s-ci-robot commented Jun 23, 2022

Uh oh!

jsturtevant commented Jun 23, 2022

Uh oh!

k8s-ci-robot commented Jun 23, 2022

Uh oh!

jsturtevant commented Jul 7, 2022

Uh oh!

jsturtevant commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsturtevant commented Oct 11, 2022

Uh oh!

jsturtevant commented Nov 22, 2022

Uh oh!

marosset commented Feb 15, 2023

Uh oh!

MikeZappa87 commented Feb 16, 2023

Uh oh!

jsturtevant commented Feb 21, 2023

Uh oh!

Uh oh!

Uh oh!

jsturtevant commented Mar 1, 2023

Uh oh!

kevpar left a comment

Choose a reason for hiding this comment

Uh oh!

MikeZappa87 commented Mar 2, 2023

Uh oh!

marosset left a comment

Choose a reason for hiding this comment

Uh oh!

dcantah commented Mar 3, 2023

Uh oh!

jsturtevant commented Mar 3, 2023

Uh oh!

dcantah commented Mar 3, 2023

Uh oh!

rzlink May 31, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jsturtevant commented Jul 19, 2022 •

edited

Loading