fix: add `readyz` endpoint #2442

rexagod · 2024-07-03T19:43:00Z

Discourage the usage of /metrics for any probe, and use /readyz in place of the earlier telemetry metrics endpoint to secure the exposition data.

/cc @mrueg

Discourage the usage of `/metrics` for any probe, and use `/readyz` in place of the earlier telemetry metrics endpoint to secure the exposition data. Signed-off-by: Pranshu Srivastava <[email protected]>

k8s-ci-robot · 2024-07-03T19:43:08Z

This issue is currently awaiting triage.

If kube-state-metrics contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-sigs/prow repository.

mrueg · 2024-07-04T18:48:30Z

Is this passing through /readyz from the apiserver to ksm? I think that's not an ideal approach. It should only be ready if metrics are available.

rexagod · 2024-07-05T13:00:07Z

Right, an oversight on my part. Patched.

mrueg · 2024-07-05T14:21:29Z

pkg/app/server.go

@@ -376,6 +378,18 @@ func buildTelemetryServer(registry prometheus.Gatherer) *http.ServeMux {
 	// Add metricsPath
 	mux.Handle(metricsPath, promhttp.HandlerFor(registry, promhttp.HandlerOpts{ErrorLog: promLogger{}}))

+	// Add readyzPath
+	mux.Handle(readyzPath, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		count, err := testutil.GatherAndCount(registry)


I like the approach, I am a bit concerned that we're getting a testutil into a production path.
Is this considered stable?

"Package testutil provides helpers to test code using the prometheus package of client_golang."

I've extracted the function in a manner that makes it only dependent on the official library, PTAL.

mrueg · 2024-07-07T19:13:29Z

README.md

-* `/healthz`: Returns a 200 status code if the application is running. We recommend to use this as a startup probe.
+* `/healthz` (exposed on `main`): Returns a 200 status code if the application is running. We recommend to use this for the startup probe.
+* `/livez` (exposed on `main`): Returns a 200 status code if the application is not affected by an outage of the Kubernetes API Server. We recommend to using this for the liveness probe.
+* `/readyz` (exposed on `self`): Returns a 200 status code if the application is ready to accept traffic. We recommend using this for the readiness probe.


Suggested change

* `/readyz` (exposed on `self`): Returns a 200 status code if the application is ready to accept traffic. We recommend using this for the readiness probe.

* `/readyz` (exposed on `self`): Returns a 200 status code if the application is ready to accept requests and expose metrics. We recommend using this for the readiness probe.

mrueg · 2024-07-07T19:14:47Z

pkg/app/server.go

-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte(http.StatusText(http.StatusOK)))
-	})
+	mux.Handle(healthzPath, handleClusterDelegationForProber(client, healthzPath))


Why is healthz depending on the health of the apiserver?

We're making /healthz depend on the cluster's health in addition to the binary's. As is visible in the list below, there are multiple components in the cluster that are critical to KSM's health (for eg., crd-informer-synced and start-apiextensions-informers).

┌[rexagod@nebuchadnezzar] [/dev/ttys002] [readyz] └[~/repositories/oss/kube-state-metrics]> curl "127.0.0.1:8001/healthz?verbose" [+]ping ok [+]log ok [+]etcd ok [+]poststarthook/start-kube-apiserver-admission-initializer ok [+]poststarthook/generic-apiserver-start-informers ok [+]poststarthook/priority-and-fairness-config-consumer ok [+]poststarthook/priority-and-fairness-filter ok [+]poststarthook/storage-object-count-tracker-hook ok [+]poststarthook/start-apiextensions-informers ok [+]poststarthook/start-apiextensions-controllers ok [+]poststarthook/crd-informer-synced ok [+]poststarthook/start-system-namespaces-controller ok [+]poststarthook/bootstrap-controller ok [+]poststarthook/rbac/bootstrap-roles ok [+]poststarthook/scheduling/bootstrap-system-priority-classes ok [+]poststarthook/priority-and-fairness-config-producer ok [+]poststarthook/start-cluster-authentication-info-controller ok [+]poststarthook/start-kube-apiserver-identity-lease-controller ok [+]poststarthook/start-deprecated-kube-apiserver-identity-lease-garbage-collector ok [+]poststarthook/start-kube-apiserver-identity-lease-garbage-collector ok [+]poststarthook/start-legacy-token-tracking-controller ok [+]poststarthook/aggregator-reload-proxy-client-cert ok [+]poststarthook/start-kube-aggregator-informers ok [+]poststarthook/apiservice-registration-controller ok [+]poststarthook/apiservice-status-available-controller ok [+]poststarthook/kube-apiserver-autoregistration ok [+]autoregister-completion ok [+]poststarthook/apiservice-openapi-controller ok [+]poststarthook/apiservice-openapiv3-controller ok [+]poststarthook/apiservice-discovery-controller ok healthz check passed

That doesn't make so much sense to me that /livez is on /livez and /healthz is on /healthz of the API Server.

In particular since /healthz is a deprecated endpoint.

The healthz endpoint is deprecated (since Kubernetes v1.16), and you should use the more specific livez and readyz endpoints instead.

https://kubernetes.io/docs/reference/using-api/health-checks/#api-endpoints-for-health

As we're suggesting healthz to be for the startup probe, I think it's fine to simply return 200 on /healthz once the application is running and not change the semantics there.

I didn't realize this was deprecated. Fair enough. 👍🏼

mrueg · 2024-07-15T12:13:54Z

/lgtm

k8s-ci-robot · 2024-07-15T12:14:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrueg, rexagod

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:

~~OWNERS~~ [mrueg,rexagod]

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

fix: add readyz endpoint

dae1f06

Discourage the usage of `/metrics` for any probe, and use `/readyz` in place of the earlier telemetry metrics endpoint to secure the exposition data. Signed-off-by: Pranshu Srivastava <[email protected]>

k8s-ci-robot requested a review from mrueg July 3, 2024 19:43

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 3, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 3, 2024

rexagod mentioned this pull request Jul 3, 2024

OCPBUGS-33620: define *probes for KSM openshift/cluster-monitoring-operator#2352

Open

2 tasks

mrueg added this to the v2.13.0 milestone Jul 4, 2024

fixup! fix: add readyz endpoint

b980579

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 5, 2024

mrueg reviewed Jul 5, 2024

View reviewed changes

mrueg reviewed Jul 7, 2024

View reviewed changes

rexagod force-pushed the readyz branch from b440bc2 to eba7384 Compare July 8, 2024 08:30

rexagod requested a review from mrueg July 9, 2024 09:03

fixup! fixup! fix: add readyz endpoint

dbb0276

rexagod force-pushed the readyz branch from eba7384 to dbb0276 Compare July 15, 2024 12:02

k8s-ci-robot assigned mrueg Jul 15, 2024

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

k8s-ci-robot merged commit f7618df into kubernetes:main Jul 15, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add `readyz` endpoint #2442

fix: add `readyz` endpoint #2442

rexagod commented Jul 3, 2024

k8s-ci-robot commented Jul 3, 2024

mrueg commented Jul 4, 2024

rexagod commented Jul 5, 2024

mrueg Jul 5, 2024

mrueg Jul 5, 2024

rexagod Jul 9, 2024

mrueg Jul 7, 2024

mrueg Jul 7, 2024

rexagod Jul 8, 2024

mrueg Jul 12, 2024

rexagod Jul 15, 2024

mrueg commented Jul 15, 2024

k8s-ci-robot commented Jul 15, 2024

	* `/readyz` (exposed on `self`): Returns a 200 status code if the application is ready to accept traffic. We recommend using this for the readiness probe.
	* `/readyz` (exposed on `self`): Returns a 200 status code if the application is ready to accept requests and expose metrics. We recommend using this for the readiness probe.

fix: add readyz endpoint #2442

fix: add readyz endpoint #2442

Conversation

rexagod commented Jul 3, 2024

k8s-ci-robot commented Jul 3, 2024

mrueg commented Jul 4, 2024

rexagod commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrueg commented Jul 15, 2024

k8s-ci-robot commented Jul 15, 2024

fix: add `readyz` endpoint #2442

fix: add `readyz` endpoint #2442