KEP-2371: update kep to reflect current state of enhancement #2812

haircommander · 2021-07-07T21:18:15Z

One-line PR description:
This fixes some nits found in the cri stats kep. They're mostly cosmetic:
drops some TODOs (as we've figured out what to do)
Updates some spacing
Fixes some names to be more clear/consistent

more detail in each commit

Issue link:
Add KEP for cAdvisor-less, CRI-full Container and Pod Stats #2364

Other comments:

Signed-off-by: Peter Hunt <pehunt@redhat.com>

mrunalp

/lgtm

haircommander · 2021-07-07T21:34:15Z

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

-+    // TODO: Unclear how the remaining fields relate to container stats. Is it filled in cAdvisor?
-+    // AvailableBytes represents the storage space available (bytes) for the filesystem.
-+    UInt64Value available_bytes = 5;
-+    // CapacityBytes represents the total capacity (bytes) of the filesystems underlying storage.
-+    UInt64Value capacity_bytes = 6;
-+    // InodesFree represents the free inodes in the filesystem.
-+    UInt64Value inodes_free = 7;
-+    // Inodes represents the total inodes in the filesystem.
-+    UInt64Value inodes = 8;


these are whole disk metrics, not specific to the writable layer or image fs size. they can be populated by cadvsior who is responsible for monitoring node-level information (whole disk)

haircommander · 2021-07-07T21:34:45Z

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

-=// ContainerStats provides the resource usage statistics for a container.
-=message ContainerStats {
-=    // Information of the container.
-+    // Corresponds to the Stats Summary API ContainerStats Name field
-=    ContainerAttributes attributes = 1;
-=    // CPU usage gathered from the container.
-+    // Corresponds to Stats Summary API ContainerStats CPUStats field
-=    CpuUsage cpu = 2;
-=    // Memory usage gathered from the container.
-+    // Corresponds to Stats Summary API ContainerStats MemoryStats field
-=    MemoryUsage memory = 3;
-=    // Usage of the writable layer.
-+    // Corresponds to Stats Summary API ContainerStats Rootfs field
-=    FilesystemUsage writable_layer = 4;
-+    // Stats pertaining to container logs usage of filesystem resources
-+    // Logs.UsedBytes is the number of bytes used for the container logs.
-+    FilesystemUsage logs = 5;
-=}


no changes needed here, just the extension of MemoryUsage and CpuUsage (covered above)

haircommander · 2021-07-07T21:35:03Z

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

-=    FilesystemUsage writable_layer = 4;
-+    // Stats pertaining to container logs usage of filesystem resources
-+    // Logs.UsedBytes is the number of bytes used for the container logs.
-+    FilesystemUsage logs = 5;


logs are the responsibility of the kubelet to watch and collect information for.

haircommander · 2021-07-07T21:35:36Z

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

@@ -304,7 +304,7 @@ These correspond to some fields of the [ContainerStats](#summary-container-stats
 +    // Corresponds to Stats Summary API CPUStats UsageCoreNanoSeconds
 =    UInt64Value usage_core_nano_seconds = 2;
 +    // Total CPU usage (sum of all cores) averaged over the sample window.
-+    UInt64Value usage_nano_seconds = 3;
+    UInt64Value usage_nano_cores = 3;


this is the actual variable name

and matches summary API naming:

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubelet/pkg/apis/stats/v1alpha1/types.go#L205-L208

The "core" unit can be interpreted as CPU core-nanoseconds per second
maybe add ^ that sentence...

haircommander · 2021-07-07T21:35:56Z

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

+    // Stats from linux.
+    LinuxPodSandboxStats linux = 2;
+    // Stats from windows.
+    WindowsPodSandboxStats windows = 3;


added two sections for windows and linux as discussed but not added

haircommander · 2021-07-07T21:36:19Z

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

-    // Each of these fields will be calculated Kubelet-level
-    // ProcessStats pertaining to processes.
-    ProcessStats process = 5;
+    NetworkUsage network = 3;


memory and cpu have the suffix Usage, make this more consistent

haircommander · 2021-07-07T21:36:33Z

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

-    InterfaceStats default_interface = 2;
-
-    repeated InterfaceStats interfaces = 3;
+    NetworkInterfaceUsage default_interface = 2;


based on kubernetes/kubernetes#102789 (comment)

bobbypage · 2021-07-07T21:37:29Z

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

 }

-// InterfaceStats contains resource value data about interface.
-type InterfaceStats struct {
+// NetworkInterfaceUsage contains resource value data about interface.


nit: s/interface/network interface

addressed :)

derekwaynecarr · 2021-07-07T21:42:41Z

after addressing the nits raised earlier, this looks fine.

bobbypage · 2021-07-07T21:43:28Z

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

+    // Stats from linux.
+    LinuxPodSandboxStats linux = 2;
+    // Stats from windows.
+    WindowsPodSandboxStats windows = 3;


just leaving note: This message design differs a bit from existing message ContainerStats in that we have sub messages for stats for windows/linux. existing message ContainerStats does have separate submessages for windows/linux though.

I prefer to have it this way though since it has more flexibility of allowing to extend stats for windows/linux separately.

bobbypage · 2021-07-07T21:44:22Z

Thanks for updating KEP, changes here make sense modulo above small nits.

/lgtm

bobbypage · 2021-07-07T21:51:46Z

/lgtm

mikebrow

LGTM
just a nit to add core description..

mikebrow · 2021-07-07T23:02:03Z

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

@@ -304,7 +304,7 @@ These correspond to some fields of the [ContainerStats](#summary-container-stats
 +    // Corresponds to Stats Summary API CPUStats UsageCoreNanoSeconds
 =    UInt64Value usage_core_nano_seconds = 2;
 +    // Total CPU usage (sum of all cores) averaged over the sample window.
-+    UInt64Value usage_nano_seconds = 3;
+    UInt64Value usage_nano_cores = 3;


The "core" unit can be interpreted as CPU core-nanoseconds per second
maybe add ^ that sentence...

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander · 2021-07-08T16:48:06Z

updated with the final nits from kubernetes/kubernetes#102789

mikebrow

LGTM

mrunalp · 2021-07-08T17:08:02Z

/lgtm

haircommander · 2021-08-27T19:19:18Z

I've updated this PR to also bump the alpha version to 1.23

bobbypage · 2021-08-27T22:23:37Z

/lgtm
/assign @mrunalp

bobbypage · 2021-08-27T22:24:04Z

/cc @qiutongs

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander · 2021-09-08T14:55:46Z

updated the kep.yaml file a bit

haircommander · 2021-09-08T15:17:30Z

btw @mikebrow I have added you as a reviewer, which I figured was alright because you've already reviewed the CRI changes :)

mrunalp · 2021-09-08T16:35:09Z

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


-    repeated InterfaceStats interfaces = 3;
+// NetworkUsage contains data about network resources.
+message NetworkUsage {


Should this be named NetworkStats?

it is mirroring the Cpu and Memory structs: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto#L611-L621

mrunalp · 2021-09-08T16:35:22Z

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

-type InterfaceStats struct {
-    // The name of the interface
+// NetworkInterfaceUsage contains resource value data about a network interface.
+message NetworkInterfaceUsage {


How about NetworkInterfaceStats?

same as above :)

mrunalp · 2021-09-08T16:52:29Z

/lgtm

derekwaynecarr · 2021-09-09T13:58:53Z

thanks @haircommander @bobbypage and others for the collaboration.

the updates look good to me too.

/approve
/lgtm

k8s-ci-robot · 2021-09-09T13:59:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, haircommander, mikebrow, mrunalp

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:

~~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

haircommander added 2 commits July 7, 2021 17:07

cri stats: drop unused changes

105b187

Signed-off-by: Peter Hunt <pehunt@redhat.com>

cri stats: use spaces, not tabs

5d0400f

Signed-off-by: Peter Hunt <pehunt@redhat.com>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 7, 2021

k8s-ci-robot requested review from dchen1107 and derekwaynecarr July 7, 2021 21:18

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2021

haircommander mentioned this pull request Jul 7, 2021

CRI: add fields for pod level stats to satisfy the /stats/summary API kubernetes/kubernetes#102789

Merged

mrunalp approved these changes Jul 7, 2021

View reviewed changes

k8s-ci-robot assigned mrunalp Jul 7, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2021

haircommander commented Jul 7, 2021

View reviewed changes

bobbypage reviewed Jul 7, 2021

View reviewed changes

k8s-ci-robot assigned bobbypage Jul 7, 2021

haircommander force-pushed the stats-2 branch from a861c12 to bc98678 Compare July 7, 2021 21:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2021

mikebrow approved these changes Jul 7, 2021

View reviewed changes

stats kep: more updating for how the CRI changes are acutally used

9f29229

Signed-off-by: Peter Hunt <pehunt@redhat.com>

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021

haircommander force-pushed the stats-2 branch from 0349958 to 2c9d1e8 Compare July 8, 2021 16:47

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021

mikebrow approved these changes Jul 8, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2021

haircommander force-pushed the stats-2 branch from 2c9d1e8 to dcf007b Compare August 27, 2021 19:19

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2021

k8s-ci-robot requested a review from qiutongs August 27, 2021 22:24

cri stats: update for review comments

024717e

Signed-off-by: Peter Hunt <pehunt@redhat.com>

haircommander force-pushed the stats-2 branch from dcf007b to 024717e Compare September 8, 2021 14:54

haircommander mentioned this pull request Sep 8, 2021

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

Open

17 tasks

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

mrunalp reviewed Sep 8, 2021

View reviewed changes

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

k8s-ci-robot assigned derekwaynecarr Sep 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021

k8s-ci-robot merged commit f4de431 into kubernetes:master Sep 9, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 9, 2021

KEP-2371: update kep to reflect current state of enhancement #2812

KEP-2371: update kep to reflect current state of enhancement #2812

Uh oh!

Conversation

haircommander commented Jul 7, 2021

Uh oh!

mrunalp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbypage Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbypage commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bobbypage commented Jul 7, 2021

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haircommander commented Jul 8, 2021

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Jul 8, 2021

Uh oh!

haircommander commented Aug 27, 2021

Uh oh!

bobbypage commented Aug 27, 2021

Uh oh!

bobbypage commented Aug 27, 2021

Uh oh!

haircommander commented Sep 8, 2021

Uh oh!

haircommander commented Sep 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Sep 8, 2021

Uh oh!

derekwaynecarr commented Sep 9, 2021

Uh oh!

k8s-ci-robot commented Sep 9, 2021

Uh oh!

Uh oh!

bobbypage Jul 7, 2021 •

edited

Loading

bobbypage commented Jul 7, 2021 •

edited

Loading