add metrics/slis to kubelet health checks #113030

richabanker · 2022-10-13T02:02:51Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #112962

Special notes for your reviewer:

Adds health check SLI metrics to kubelet. This is behind a feature gate.

Does this PR introduce a user-facing change?

If `ComponentSLIs` feature gate is enabled, then `/metrics/slis` becomes available on kubelet, allowing you to scrape health check metrics.

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

- [KEP]: https://kep.k8s.io/3466

# HELP kubernetes_healthcheck [ALPHA] This metric records the result of a single healthcheck.
# TYPE kubernetes_healthcheck gauge
kubernetes_healthcheck{name="log",type="healthz"} 1
kubernetes_healthcheck{name="ping",type="healthz"} 1
kubernetes_healthcheck{name="syncloop",type="healthz"} 1
# HELP kubernetes_healthchecks_total [ALPHA] This metric records the results of all healthcheck.
# TYPE kubernetes_healthchecks_total counter
kubernetes_healthchecks_total{name="log",status="success",type="healthz"} 1
kubernetes_healthchecks_total{name="ping",status="success",type="healthz"} 1
kubernetes_healthchecks_total{name="syncloop",status="success",type="healthz"} 1

k8s-ci-robot · 2022-10-13T02:02:59Z

Hi @richabanker. Thanks for your PR.

I'm waiting for a kubernetes 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.

pacoxu · 2022-10-13T02:10:44Z

/priority important-soon
/triage accepted
/ok-to-test

logicalhan · 2022-10-13T16:28:22Z

There is another healthz.InstallHandler in cmd/kubelet/app/server.go. I'm not sure that this change is needed there as well.

Why would we need to register the path twice? This change has been verified locally, IIUC, so it's probably(?) a moot point.

logicalhan

/lgtm

(from sig instrumentation)

logicalhan · 2022-10-14T17:22:36Z

/assign @dashpole

dashpole

/lgtm

pacoxu · 2022-10-17T04:23:36Z

There is another healthz.InstallHandler in cmd/kubelet/app/server.go. I'm not sure that this change is needed there as well.

Why would we need to register the path twice? This change has been verified locally, IIUC, so it's probably(?) a moot point.

I misunderstand the health handler of kubelet. The health handler is for 10248/healthz and we need to add the slis to :10250/metrics/.

/lgtm

pacoxu · 2022-10-17T04:24:07Z

/assign @tallclair

tallclair · 2022-10-18T18:59:42Z

pkg/kubelet/server/server.go

+func init() {
+	utilruntime.Must(metricsfeatures.AddFeatureGates(utilfeature.DefaultMutableFeatureGate))
+}
+


Should this be moved to here instead?

kubernetes/cmd/kubelet/app/server.go

Lines 113 to 115 in 1ab1b0a

func init() {

utilruntime.Must(logsapi.AddFeatureGates(utilfeature.DefaultMutableFeatureGate))

}

That seems to be the pattern used elsewhere, e.g.

kubernetes/cmd/kube-controller-manager/app/controllermanager.go

Line 82 in 1ab1b0a

utilruntime.Must(features.AddFeatureGates(utilfeature.DefaultMutableFeatureGate))

kubernetes/cmd/kube-proxy/app/server.go

Line 92 in 1ab1b0a

utilruntime.Must(metricsfeatures.AddFeatureGates(utilfeature.DefaultMutableFeatureGate))

It can be there or here, the init functions should evaluate at roughly the same time (though I think the cmd one is slightly sooner).

Added the feature gate initialization in kubernetes/cmd/kubelet/app/server.go

I actually think it needs to be in the original spot due to unit tests which do not import the cmd file.

Ah, gotcha. Reverted.

You could init in the unit test, but I think this is fine.

pkg/kubelet/server/server.go

logicalhan

/lgtm

tallclair

/lgtm
/approve

k8s-ci-robot · 2022-10-21T16:28:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, Richabanker, tallclair

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/server/OWNERS~~ [tallclair]

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

logicalhan · 2022-10-21T17:03:42Z

/retest

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 13, 2022

k8s-ci-robot requested review from derekwaynecarr and pacoxu October 13, 2022 02:03

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 Oct 13, 2022

pacoxu added this to Needs Reviewer in SIG Node PR Triage Oct 13, 2022

This comment was marked as off-topic.

Sign in to view

logicalhan reviewed Oct 13, 2022

View reviewed changes

k8s-ci-robot assigned logicalhan Oct 13, 2022

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

logicalhan mentioned this pull request Oct 13, 2022

Kubernetes Component Health SLIs kubernetes/enhancements#3466

Closed

25 tasks

k8s-ci-robot assigned dashpole Oct 14, 2022

dashpole approved these changes Oct 14, 2022

View reviewed changes

k8s-ci-robot assigned pacoxu Oct 17, 2022

pacoxu moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Oct 17, 2022

k8s-ci-robot assigned tallclair Oct 17, 2022

tallclair reviewed Oct 18, 2022

View reviewed changes

richabanker force-pushed the kubelet-metrics-slis branch from 87a7266 to 4e8fb9a Compare October 18, 2022 20:27

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

richabanker force-pushed the kubelet-metrics-slis branch from 4e8fb9a to c8af823 Compare October 18, 2022 20:29

add metrics/slis to kubelet health checks

047f6a7

richabanker force-pushed the kubelet-metrics-slis branch from c8af823 to 047f6a7 Compare October 18, 2022 21:06

logicalhan reviewed Oct 18, 2022

View reviewed changes

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

tallclair reviewed Oct 21, 2022

View reviewed changes

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

k8s-ci-robot merged commit a497c56 into kubernetes:master Oct 21, 2022

SIG Node PR Triage automation moved this from Needs Approver to Done Oct 21, 2022

k8s-ci-robot added this to the v1.26 milestone Oct 21, 2022

richabanker mentioned this pull request Nov 1, 2022

REQUEST: New membership for @Richabanker kubernetes/org#3801

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add metrics/slis to kubelet health checks #113030

add metrics/slis to kubelet health checks #113030

richabanker commented Oct 13, 2022

k8s-ci-robot commented Oct 13, 2022

pacoxu commented Oct 13, 2022

This comment was marked as off-topic.

logicalhan commented Oct 13, 2022

logicalhan left a comment

logicalhan commented Oct 14, 2022

dashpole left a comment

pacoxu commented Oct 17, 2022

pacoxu commented Oct 17, 2022

tallclair Oct 18, 2022

logicalhan Oct 18, 2022

richabanker Oct 18, 2022

logicalhan Oct 18, 2022

richabanker Oct 18, 2022

tallclair Oct 21, 2022

logicalhan left a comment

tallclair left a comment

k8s-ci-robot commented Oct 21, 2022

logicalhan commented Oct 21, 2022

	func init() {
	utilruntime.Must(logsapi.AddFeatureGates(utilfeature.DefaultMutableFeatureGate))
	}

add metrics/slis to kubelet health checks #113030

add metrics/slis to kubelet health checks #113030

Conversation

richabanker commented Oct 13, 2022

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

k8s-ci-robot commented Oct 13, 2022

pacoxu commented Oct 13, 2022

This comment was marked as off-topic.

logicalhan commented Oct 13, 2022

logicalhan left a comment

Choose a reason for hiding this comment

logicalhan commented Oct 14, 2022

dashpole left a comment

Choose a reason for hiding this comment

pacoxu commented Oct 17, 2022

pacoxu commented Oct 17, 2022

tallclair Oct 18, 2022

Choose a reason for hiding this comment

logicalhan Oct 18, 2022

Choose a reason for hiding this comment

richabanker Oct 18, 2022

Choose a reason for hiding this comment

logicalhan Oct 18, 2022

Choose a reason for hiding this comment

richabanker Oct 18, 2022

Choose a reason for hiding this comment

tallclair Oct 21, 2022

Choose a reason for hiding this comment

logicalhan left a comment

Choose a reason for hiding this comment

tallclair left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 21, 2022

logicalhan commented Oct 21, 2022