Add Windows pod sandbox stats information to KEP 2371 - cri pod container stats #3439

jsturtevant · 2022-07-22T03:57:10Z

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

One-line PR description:

Adding Windows stats details to the cri pod container stats KEP

Issue link: cAdvisor-less, CRI-full Container and Pod Stats #2371

Other comments:

After opening kubernetes/kubernetes#110754 we discovered there was more design work need for Windows and was suggested at sig-node to do the work in KEP.

/sig windows
/sig node

jsturtevant · 2022-07-22T03:58:15Z

/cc @haircommander @bobbypage @marosset @dcantah

keps/sig-node/2371-cri-pod-container-stats/README.md

jsturtevant · 2022-07-29T15:40:22Z

/assign @haircommander @bobbypage @dchen1107

keps/sig-node/2371-cri-pod-container-stats/README.md

haircommander · 2022-07-30T15:18:37Z

keps/sig-node/2371-cri-pod-container-stats/README.md

+
+    The field `LinuxPodSandboxStats` would stay the same (with duplicated fields) for backwards compatibility.  We will also leave `WindowsPodSandboxStats` with a comment that it is not currently used.   This option allows us to add specific fields in the future, Linux and Windows sub fields could be added to `LinuxPodSandboxStats` and `WindowsPodSandboxStats` if there are use cases for specific fields.  
+
+    The downside to this approach is that we have two fields (`LinuxPodSandboxStats` and `PodSandboxStats`) which are initially identical. We could mark the individual fields in the `LinuxPodSandboxStats` as depreciated and allow only new linux only fields be added to `LinuxPodSandboxStats`.


I think this approach makes sense. I think we should mention that eventually we will drop LinuxPodSandboxStats and WindowsPodSandboxStats in favor of the single structure

sounds good, I was thinking that we could possibly use the fields in the future for OS specific stats but probably better to handle at a later point if it ever comes up in the future.

haircommander · 2022-07-30T15:19:46Z

one more set of notes, but the approach LGTM

marosset · 2022-09-06T17:00:01Z

The original review comments in the first KEP around linux/windows fields asked for linux/windows specific structs to allow for differences, and that was agreed. Certain duplicate fields across all known platforms (more platforms coming) could be allowed in the base struct and we can comment fields with platform exceptions if needed, but that doesn't mean we have to make the pod stats response generic across platforms at the CRI api and deprecate linux differences that, I believe, will become more evident if and when pod resources currently being managed at the kubelet start to be managed at the lower levels.
My gut says keep with the original plan/path rather than start deprecating right away. If the windows team really wants to use the exact same fields, I don't see how that would be more complex for kubelet.

@haircommander @marosset what are your thoughts?

IIRC when this was discussed on the July 19th meeting we decided there wasn't a known use-case for having separate structs. Someone (I think @dchen1107 and/or @bobbypage ) stated that these metrics were primary used by the kubelet for eviction and metrics-server for scaling purposed.
If other non-core components needed more detailed metrics they could get them by other means.

jsturtevant · 2022-09-06T19:17:22Z

IIRC when this was discussed on the July 19th meeting we decided there wasn't a known use-case for having separate structs. Someone (I think @dchen1107 and/or @bobbypage ) stated that these metrics were primary used by the kubelet for eviction and metrics-server for scaling purposed.
If other non-core components needed more detailed metrics they could get them by other means.

That was my understanding as well.

At sig-node meeting today, there was an update on the performance for moving the additional cadvisor metrics into the ListStats API: https://docs.google.com/document/d/1Ne57gvidMEWXR70OxxnRkYquAoMpt56o75oZtg-OeBg/edit#heading=h.i2n5jj2i4vso

In the proposal Linux specific fields are added to the stats call. I wasn't aware of this use case, but if this is the direction, then I think we would want to have these fields as separate as was initially proposed.

jsturtevant · 2022-09-12T22:27:10Z

@bobbypage @haircommander @mikebrow I have this on the agenda to discuss tomorrow. I won't be able to make it but @marosset is going to attend. Hopefully we can get some consensus on the direction forward. Thanks!

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

jsturtevant · 2022-09-19T16:52:33Z

After further discussion at sig-node on Sep 13th, it was decided that we will keep Windows and Linux fields separate.

I've updated the KEP @haircommander @marosset @bobbypage @dcantah

keps/sig-node/2371-cri-pod-container-stats/README.md

marosset · 2022-09-22T16:57:43Z

/milestone v1.26

haircommander · 2022-09-22T17:05:17Z

keps/sig-node/2371-cri-pod-container-stats/README.md

+- balancing_enabled
+- dm_operation_in_progress 
+
+These are not used currently but are be very specific to each implementation and could be collected by specialized tools on a as needed basis.


or maybe reported as an unstructured Metric through the endpoint formally known as /metrics/cadvisor?

had the same thoughts, exposing them through that would be a good way to provide them to end users. If kubelet ends up needing them for decisions, we could introduce them to the formal Pod Stats call but can be done at a later time if need arises

haircommander · 2022-09-22T17:06:10Z

/lgtm

there's one leading question I added, but I don't think we need to hammer out the details of it yet.

edit: oops, missed #3439 (comment), I'm on standby to re-lgtm :)

Co-authored-by: Danny Canter <[email protected]>

haircommander · 2022-09-22T18:00:12Z

/lgtm

jsturtevant · 2022-09-26T18:51:47Z

After further discussion at sig-node on Sep 13th, it was decided that we will keep Windows and Linux fields separate. The KEP has been updated to reflect this.

/assign @dchen1107 @mrunalp @mikebrow

marosset · 2022-10-04T20:14:40Z

/lgtm

haircommander · 2022-10-04T20:15:54Z

/assign @derekwaynecarr

bobbypage · 2022-10-05T07:21:22Z

/lgtm

jsturtevant · 2022-10-05T21:35:44Z

/cc @mikebrow @derekwaynecarr for 1.26 enhancement freeze

derekwaynecarr · 2022-10-06T22:40:19Z

/approve
/lgtm

k8s-ci-robot · 2022-10-06T22:40:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, jsturtevant

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~keps/sig-node/OWNERS~~ [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested a review from dchen1107 July 22, 2022 03:57

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Jul 22, 2022

k8s-ci-robot requested a review from derekwaynecarr July 22, 2022 03:57

k8s-ci-robot requested review from bobbypage, dcantah, haircommander and marosset July 22, 2022 03:58

jsturtevant commented Jul 22, 2022

View reviewed changes

keps/sig-node/2371-cri-pod-container-stats/README.md Outdated Show resolved Hide resolved

This was referenced Jul 22, 2022

Refactor usageNanoCores be to used for all OSes containerd/containerd#7186

Merged

CRI: Add Windows Podsandbox Stats kubernetes/kubernetes#110754

Merged

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 27, 2022

jsturtevant force-pushed the add-windows-podstats-details branch 3 times, most recently from acae793 to 1fa4600 Compare July 27, 2022 16:17

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 27, 2022

k8s-ci-robot assigned bobbypage, dchen1107 and haircommander Jul 29, 2022

haircommander reviewed Jul 30, 2022

View reviewed changes

keps/sig-node/2371-cri-pod-container-stats/README.md Outdated Show resolved Hide resolved

haircommander reviewed Jul 30, 2022

View reviewed changes

jsturtevant force-pushed the add-windows-podstats-details branch from 1fa4600 to c896c46 Compare August 1, 2022 15:58

Add Windows stats information

73a5c8d

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

jsturtevant force-pushed the add-windows-podstats-details branch from 2845607 to 73a5c8d Compare September 19, 2022 16:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2022

dcantah reviewed Sep 22, 2022

View reviewed changes

keps/sig-node/2371-cri-pod-container-stats/README.md Outdated Show resolved Hide resolved

k8s-ci-robot added this to the v1.26 milestone Sep 22, 2022

haircommander reviewed Sep 22, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2022

Update keps/sig-node/2371-cri-pod-container-stats/README.md

4dbc00a

Co-authored-by: Danny Canter <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2022

haircommander mentioned this pull request Oct 4, 2022

cAdvisor-less, CRI-full Container and Pod Stats #2371

Open

16 tasks

k8s-ci-robot assigned derekwaynecarr Oct 4, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2022

k8s-ci-robot merged commit beaebac into kubernetes:master Oct 6, 2022

jsturtevant mentioned this pull request Oct 11, 2022

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

Merged

claudiubelu mentioned this pull request Apr 6, 2023

SIG-windows-2022-annual-report kubernetes/community#7173

Merged


		The field `LinuxPodSandboxStats` would stay the same (with duplicated fields) for backwards compatibility. We will also leave `WindowsPodSandboxStats` with a comment that it is not currently used. This option allows us to add specific fields in the future, Linux and Windows sub fields could be added to `LinuxPodSandboxStats` and `WindowsPodSandboxStats` if there are use cases for specific fields.

		The downside to this approach is that we have two fields (`LinuxPodSandboxStats` and `PodSandboxStats`) which are initially identical. We could mark the individual fields in the `LinuxPodSandboxStats` as depreciated and allow only new linux only fields be added to `LinuxPodSandboxStats`.

Add Windows pod sandbox stats information to KEP 2371 - cri pod container stats #3439

Add Windows pod sandbox stats information to KEP 2371 - cri pod container stats #3439

Uh oh!

Conversation

jsturtevant commented Jul 22, 2022

Uh oh!

jsturtevant commented Jul 22, 2022

Uh oh!

Uh oh!

jsturtevant commented Jul 29, 2022

Uh oh!

Uh oh!

haircommander Jul 30, 2022

Choose a reason for hiding this comment

Uh oh!

jsturtevant Aug 1, 2022

Choose a reason for hiding this comment

Uh oh!

jsturtevant Aug 1, 2022

Choose a reason for hiding this comment

Uh oh!

haircommander commented Jul 30, 2022

Uh oh!

marosset commented Sep 6, 2022

Uh oh!

jsturtevant commented Sep 6, 2022

Uh oh!

jsturtevant commented Sep 12, 2022

Uh oh!

jsturtevant commented Sep 19, 2022

Uh oh!

Uh oh!

marosset commented Sep 22, 2022

Uh oh!

haircommander Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

jsturtevant Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

haircommander commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haircommander commented Sep 22, 2022

Uh oh!

jsturtevant commented Sep 26, 2022

Uh oh!

marosset commented Oct 4, 2022

Uh oh!

haircommander commented Oct 4, 2022

Uh oh!

bobbypage commented Oct 5, 2022

Uh oh!

jsturtevant commented Oct 5, 2022

Uh oh!

derekwaynecarr commented Oct 6, 2022

Uh oh!

k8s-ci-robot commented Oct 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

haircommander commented Sep 22, 2022 •

edited

Loading