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

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

Merged
merged 2 commits into from
Mar 6, 2023
Merged

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

merged 2 commits into from
Mar 6, 2023

Conversation

jsturtevant
Copy link
Contributor

Implements ListPodSandboxStats and PodSandboxStats for Windows.

Part of kubernetes/enhancements#2371

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
Copy link

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.

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
Copy link
Contributor Author

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

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

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

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 jsturtevant force-pushed the cri-only-stats-windows branch 5 times, most recently from 6b33d83 to 77b7464 Compare July 7, 2022 21:07
@jsturtevant
Copy link
Contributor Author

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.

This was referenced Jul 18, 2022
Merged
Closed
@jsturtevant
Copy link
Contributor Author

jsturtevant commented Jul 19, 2022 •
edited
Loading

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 jsturtevant mentioned this pull request Jul 22, 2022
Merged
@jsturtevant
Copy link
Contributor Author

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

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

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.

@jsturtevant jsturtevant force-pushed the cri-only-stats-windows branch 7 times, most recently from a682290 to 618ebd7 Compare December 2, 2022 18:43
@jsturtevant jsturtevant force-pushed the cri-only-stats-windows branch 3 times, most recently from 806ae42 to 6b45e96 Compare December 7, 2022 23:53
@marosset
Copy link
Contributor

Should this issue get added to the 1.7 milestone?

@MikeZappa87
Copy link
Contributor

Should this issue get added to the 1.7 milestone?

@dmcgowan thoughts?

@jsturtevant
Copy link
Contributor Author

@containerd/maintainers this is ready for final review

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

/assign @dmcgowan @kevpar @kzys

@jsturtevant jsturtevant force-pushed the cri-only-stats-windows branch from 830a9c5 to 39b6a5d Compare March 2, 2023 19:12
kevpar
kevpar approved these changes Mar 2, 2023
View reviewed changes
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@MikeZappa87
Copy link
Contributor

Lgtm

marosset
marosset approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@marosset marosset left a comment

Choose a reason for hiding this comment

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

LGTM

@dcantah dcantah added this to the 1.7 milestone Mar 3, 2023
@dcantah
Copy link
Member

dcantah commented Mar 3, 2023

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

@jsturtevant
Add Windows Sandbox Stats
08aa576 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant
Add Windows Sandbox Stats (sbserver)
32ed559 
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@jsturtevant jsturtevant force-pushed the cri-only-stats-windows branch from 39b6a5d to 32ed559 Compare March 3, 2023 22:37
@jsturtevant
Copy link
Contributor Author

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

@dcantah
Copy link
Member

dcantah commented Mar 3, 2023

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

@kevpar kevpar merged commit 31c9a66 into containerd:main Mar 6, 2023
@jsturtevant jsturtevant mentioned this pull request Jun 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschott daschott daschott left review comments

@MikeZappa87 MikeZappa87 MikeZappa87 approved these changes

@dcantah dcantah dcantah approved these changes

@marosset marosset marosset approved these changes

@kevpar kevpar kevpar approved these changes

@kzys kzys kzys approved these changes

Assignees

@kzys kzys

@dmcgowan dmcgowan

@kevpar kevpar

Labels
ok-to-test
Projects
Pull Request Review
Status: Done
Milestone
1.7
Development

Successfully merging this pull request may close these issues.
9 participants
@jsturtevant @k8s-ci-robot @marosset @dcantah @kevpar @kzys @MikeZappa87 @daschott @dmcgowan