kubelet: add support for broadcasting metrics from CRI #113609

haircommander · 2022-11-03T21:02:13Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR includes #113025 as well as support in the kubelet for pulling the metrics from CRI and broadcasting them to prometheus

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

There are two open questions that I need someone who knows more about the way these endpoints work:

should the collecting be async for each metric
is it idiomatic to return the key and value of labels as a map pair/struct. I see most examples as having constant labels, and others as building each slice each time, but it seems inefficient to me

Does this PR introduce a user-facing change?

Add alpha support for returning container and pod metrics from CRI, instead of cAdvsior

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/2371

haircommander · 2022-11-03T21:03:09Z

/sig node
/priority important-soon

haircommander · 2022-11-03T21:09:54Z

I was able to get a successful e2e by using https://github.com/haircommander/cri-o/tree/metrics-wip and this. Running with a local cluster, I could curl the Kubelet's HTTP endpoint sudo curl -kv http://10.0.2.15:10255/metrics/cadvisor and it got me the metrics cri-o was spoofing 😄

bobbypage · 2022-11-03T21:10:11Z

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

+
+message Metric {
+    string name = 1;
+    string help = 2;


are we sure we want help here? This is a bit unfortunate because it means we need to send help text for every single metric when help is the same for the same metric "name". It seems like it will increase the rpc payload size alot due to the duplication (since the metric is per container).

Maybe we can omit this for now and figure out best way to include this later?

pkg/kubelet/metrics/collectors/cri_metrics.go

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

pkg/kubelet/metrics/collectors/cri_metrics.go

LuBingtan

nice work :)

haircommander · 2022-11-04T19:06:17Z

a couple of highlights to the updates:

Separated CRI calls for ListMetricDescriptors and ListPodSandboxMetrics. That allows for there to be less overhead, as more information can be called once and then cached
Added NewConstMetric call and moved to comp-base metrics instead of prometheus
- I went for INTERNAL metric type to start (WDYT @bobbypage )
Did a bit of cleanup here and there

PTAL @dashpole @bobbypage

pkg/kubelet/cri/remote/fake/fake_runtime.go

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto

haircommander · 2022-11-08T15:50:12Z

thanks for the review @bobbypage, comments addressed

derekwaynecarr

/approve
/lgtm

we need to measure performance before enabling beta.

derekwaynecarr · 2022-11-08T16:45:50Z

pkg/kubelet/server/server.go

 	r.RawMustRegister(metrics.NewPrometheusMachineCollector(prometheusHostAdapter{s.host}, includedMetrics))
+	if utilfeature.DefaultFeatureGate.Enabled(features.PodAndContainerStatsFromCRI) {


this looks sufficiently isolated to make it safe for alpha.

all the code falls back to existing behavior when not enabled.

k8s-ci-robot · 2022-11-08T16:46:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/kubelet/OWNERS~~ [derekwaynecarr]
~~staging/src/k8s.io/component-base/metrics/OWNERS~~ [dashpole,derekwaynecarr]
~~staging/src/k8s.io/cri-api/pkg/OWNERS~~ [derekwaynecarr]

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

dims · 2022-11-08T17:17:16Z

/triage accepted

bobbypage · 2022-11-08T19:16:54Z

Thank you for the updates!

/lgtm

so that a caller can use the metrics.Metric structure but still handle errors Signed-off-by: Peter Hunt <pehunt@redhat.com>

Added new gRPC call 'ListPodSanboxMetrics' which would return additional container stats currently supported by cAdvisor, but outside the scope of /stats/summary api. Added new types to support metric exporting of prometheus, including Metric and other subfields. Added fake runtime changes associated with the CRI changes.

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

that pulls metrics from the CRI Signed-off-by: Peter Hunt <pehunt@redhat.com>

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

dims · 2022-11-08T19:51:32Z

re-apply LGTM

/lgtm

bobbypage · 2022-11-08T19:53:28Z

/lgtm

haircommander · 2022-11-08T20:20:07Z

/retest

k8s-ci-robot requested review from andrewsykim and feiskyer November 3, 2022 21:02

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 3, 2022

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 3, 2022

haircommander mentioned this pull request Nov 3, 2022

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

Open

8 tasks

haircommander force-pushed the sandbox-metrics branch from 4497b49 to 0ae21d7 Compare November 3, 2022 21:23

bobbypage reviewed Nov 3, 2022

View reviewed changes

LuBingtan reviewed Nov 4, 2022

View reviewed changes

haircommander force-pushed the sandbox-metrics branch from 0ae21d7 to ef4e749 Compare November 4, 2022 18:27

k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Nov 4, 2022

haircommander force-pushed the sandbox-metrics branch from ef4e749 to 34fa716 Compare November 4, 2022 18:58

haircommander force-pushed the sandbox-metrics branch 2 times, most recently from 5f6bd86 to 7a71cbf Compare November 4, 2022 20:02

mrunalp reviewed Nov 4, 2022

View reviewed changes

pkg/kubelet/cri/remote/fake/fake_runtime.go Outdated Show resolved Hide resolved

mrunalp reviewed Nov 4, 2022

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto Outdated Show resolved Hide resolved

mrunalp reviewed Nov 4, 2022

View reviewed changes

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto Outdated Show resolved Hide resolved

haircommander force-pushed the sandbox-metrics branch from cc338fd to 95df62f Compare November 8, 2022 15:07

derekwaynecarr approved these changes Nov 8, 2022

View reviewed changes

k8s-ci-robot assigned derekwaynecarr Nov 8, 2022

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

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

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 8, 2022

k8s-ci-robot assigned bobbypage Nov 8, 2022

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2022

haircommander force-pushed the sandbox-metrics branch from 95df62f to c20b002 Compare November 8, 2022 19:46

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 8, 2022

haircommander and others added 5 commits November 8, 2022 14:47

component-base/metrics: Add NewConstMetric function

435606b

so that a caller can use the metrics.Metric structure but still handle errors Signed-off-by: Peter Hunt <pehunt@redhat.com>

kubelet: wire ListPodSandboxMetrics

6298ce6

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

kubelet/metrics: add cri_metrics

1a7388c

that pulls metrics from the CRI Signed-off-by: Peter Hunt <pehunt@redhat.com>

kubelet: add cri metrics to server

95489a2

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

haircommander force-pushed the sandbox-metrics branch from c20b002 to 95489a2 Compare November 8, 2022 19:47

k8s-ci-robot assigned dims Nov 8, 2022

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

k8s-ci-robot merged commit b4040b3 into kubernetes:master Nov 8, 2022

k8s-ci-robot added this to the v1.26 milestone Nov 8, 2022

bobbypage mentioned this pull request Nov 9, 2022

Add fake runtimes and CRI changes for KEP-2371 #113025

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: add support for broadcasting metrics from CRI #113609

kubelet: add support for broadcasting metrics from CRI #113609

haircommander commented Nov 3, 2022 •

edited

haircommander commented Nov 3, 2022

haircommander commented Nov 3, 2022

bobbypage Nov 3, 2022

LuBingtan left a comment

haircommander commented Nov 4, 2022

haircommander commented Nov 8, 2022

derekwaynecarr left a comment

derekwaynecarr Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

dims commented Nov 8, 2022

bobbypage commented Nov 8, 2022

dims commented Nov 8, 2022

bobbypage commented Nov 8, 2022

haircommander commented Nov 8, 2022

		r.RawMustRegister(metrics.NewPrometheusMachineCollector(prometheusHostAdapter{s.host}, includedMetrics))
		if utilfeature.DefaultFeatureGate.Enabled(features.PodAndContainerStatsFromCRI) {

kubelet: add support for broadcasting metrics from CRI #113609

kubelet: add support for broadcasting metrics from CRI #113609

Conversation

haircommander commented Nov 3, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

haircommander commented Nov 3, 2022

haircommander commented Nov 3, 2022

bobbypage Nov 3, 2022

Choose a reason for hiding this comment

LuBingtan left a comment

Choose a reason for hiding this comment

haircommander commented Nov 4, 2022

haircommander commented Nov 8, 2022

derekwaynecarr left a comment

Choose a reason for hiding this comment

derekwaynecarr Nov 8, 2022

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 8, 2022

dims commented Nov 8, 2022

bobbypage commented Nov 8, 2022

dims commented Nov 8, 2022

bobbypage commented Nov 8, 2022

haircommander commented Nov 8, 2022

haircommander commented Nov 3, 2022 •

edited