✨ added health probes #419

GrigoriyMikhalkin · 2019-05-09T12:53:59Z

Attempt to solve #297. Adds server, that can serve readiness and liveness probes. Probes return response based on manager's fields isReady and isAlive, that can be changed via methods SetReady/SetAlive

GrigoriyMikhalkin · 2019-05-09T13:10:17Z

/assign @pwittrock

DirectXMan12 · 2019-06-04T00:37:34Z

/kind feature

DirectXMan12 · 2019-06-07T22:52:47Z

pkg/manager/internal.go

+	readinessEndpointName string
+
+	// Liveness probe endpoint name
+	livenessEndpointName string


these are pretty standard in kubernetes (e.g. /healthz and /readyz), so we should default them at the very least

DirectXMan12 · 2019-06-07T22:55:08Z

pkg/manager/internal.go

+		mux.Handle(cm.readinessEndpointName, readinessHandler)
+	}
+	if cm.livenessEndpointName != "" {
+		var livenessHandler http.HandlerFunc = func(w http.ResponseWriter, r *http.Request) {


we should copy the k8s.io/apiserver healthz code, and then switch to that whenever it moves to component-base (@luxas ?). It provides support for pluggable health checks, so we could have multiple in a single manager (e.g. health check per controller).

Why can't we directly use k8s.io/apiserver? Its description is Library for writing a Kubernetes-style API server. If it's a lib, why can't we just use it instead of copying it?
Did I miss anything?

GrigoriyMikhalkin · 2019-06-09T22:17:58Z

@DirectXMan12 updated code. Now it's using healthz package from k8s.io/apiserver.

DirectXMan12

a few last comments inline, mainly around cleaning up the imported code.

DirectXMan12 · 2019-06-11T20:00:19Z

pkg/healthz/healthz.go

+		checks = []Checker{PingHealthz}
+	}
+
+	log.V(5).Info("Installing healthz checkers:", formatQuoted(checkerNames(checks...)...))


err... this log line isn't quite right, since it's structured logging. At an rate, I'd recommend logging once per checker anyway, like:

log.V(1).Info("installing healthz checker", "checker", check.Name

DirectXMan12 · 2019-06-11T20:02:45Z

pkg/healthz/healthz.go

+		}
+		// always be verbose on failure
+		if failed {
+			log.V(2).Info(fmt.Sprintf("%vhealthz check failed", verboseOut.String()))


no fmt in our logging. Log messages should be constant. Use key-value pairs to attach variable information.

DirectXMan12 · 2019-06-11T20:04:16Z

pkg/healthz/healthz_test.go

+	contentType = "text/plain; charset=utf-8"
+)
+
+func TestInstallPathHandler(t *testing.T) {


please adapt these to ginkgo, like below

in fact, you can probably prune a few of these, if they overlap heavily with the ginkgo tests below.

DirectXMan12

few more comments inline. sorry for the hug delay here

DirectXMan12 · 2019-08-06T20:58:25Z

pkg/healthz/healthz.go

+// specified in exactly one call to InstallPathHandler. Calling
+// InstallPathHandler more than once for the same path and mux will
+// result in a panic.
+func InstallPathHandler(mux mux, path string, checks ...Checker) {


nit: we should have a better name for this. We can also just take an http.Handler here, probably.

IIUC, healthz code will be moved to component-base, and we planning to switch to it, right? In that case, isn't it better to leave this function as it is?

if we're going to carry an interface in controller-runtime, I'd rather have it up to controller-runtime standards.

DirectXMan12 · 2019-08-06T21:01:29Z

pkg/healthz/healthz.go

+// InstallPathHandler more than once for the same path and mux will
+// result in a panic.
+func InstallPathHandler(mux mux, path string, checks ...Checker) {
+	if len(checks) == 0 {


I'm not a huge fan of this logic here -- I'd rather just have something where you call a single call per health check, like Register with the webhook server

DirectXMan12 · 2019-08-06T21:02:44Z

pkg/healthz/healthz.go

+			if err := check.Check(r); err != nil {
+				// don't include the error since this endpoint is public.  If someone wants more detail
+				// they should have explicit permission to the detailed checks.
+				log.V(4).Info("healthz check failed", "checker", check.Name(), "error", err)


We generally don't log at anything above 1 by default in CR.

DirectXMan12 · 2019-08-06T21:03:05Z

pkg/healthz/healthz.go

+				// don't include the error since this endpoint is public.  If someone wants more detail
+				// they should have explicit permission to the detailed checks.
+				log.V(4).Info("healthz check failed", "checker", check.Name(), "error", err)
+				fmt.Fprintf(&verboseOut, "[-]%v failed: reason withheld\n", check.Name())


shouldn't this be %q or %s, since name is a string?

DirectXMan12 · 2019-08-06T21:04:23Z

pkg/healthz/healthz.go

+			return
+		}
+		fmt.Fprint(w, "healthz check passed\n")
+	})


do we not need a WriteHeader here?

It's already set to http.StatusOK by default

DirectXMan12 · 2019-08-06T21:05:18Z

pkg/manager/internal.go

@@ -180,6 +201,32 @@ func (cm *controllerManager) SetFields(i interface{}) error {
 	return nil
 }

+// AddHealthzChecks allows you to add Healthz checkers
+func (cm *controllerManager) AddHealthzChecks(checks ...healthz.Checker) error {


I'd probably just make this AddHealthzCheck and have it built up the list

DirectXMan12 · 2019-08-13T23:58:37Z

pkg/healthz/healthz.go

+// specified in exactly one call to InstallPathHandler. Calling
+// InstallPathHandler more than once for the same path and mux will
+// result in a panic.
+func InstallPathHandler(mux mux, path string, checks ...Checker) {


if we're going to carry an interface in controller-runtime, I'd rather have it up to controller-runtime standards.

DirectXMan12 · 2019-09-19T01:22:39Z

Refactored to look closer to what I'd want this to look like. Thanks for the initial work here!

@mengqiy or @droot can one of you take a look for sanity, please?

DirectXMan12 · 2019-09-19T01:43:28Z

/ok-to-test

droot · 2019-09-19T17:52:44Z

@DirectXMan12 Looks solid to me.

Do we intend to plug in in scaffolded manifests in KB ? (opt-in may be through a kustomize patch ?)

mengqiy

LGTM but with a few nits

pkg/healthz/healthz.go

pkg/manager/internal.go

mengqiy · 2019-09-19T17:56:29Z

pkg/manager/internal.go

+		cm.healthzHandler = &healthz.Handler{Checks: map[string]healthz.Checker{}}
+	}
+
+	cm.healthzHandler.Checks[name] = check


Should we add a check to see if the name already presents?

🤷‍♀️ we don't have a way to replace otherwise, so I'm not sure. Let's leave it like this for the moment, and see what people do with it.

mengqiy · 2019-09-19T17:56:37Z

pkg/manager/internal.go

+		cm.readyzHandler = &healthz.Handler{Checks: map[string]healthz.Checker{}}
+	}
+
+	cm.readyzHandler.Checks[name] = check


Same comment here.

ditto above

Changes: - Add Helm Chart testing scripts - Fix LABEL entry on our Dockerfiles - Add license boilerplate for our clients pkg - Fix helm chart installation (hardcoded URL for etcd cluster) - Add missing entries in the chart to be compliant with official chart-testing validators - Remove duplicated `platform` directory (content moved under /internal/platform pkg) - Remove `contrib` dir, and leave only `hack` folder, reasons: Contrib meaning: > It is for software that has been contributed to the project, but which might not actually be maintained by the core developers. Naming it "contrib" or "Contrib" is a long-established convention - remove the `heath` package from `pkg` directory, why: - In `pkg` we should put library code that's ok to use by external applications (e.g., /pkg/mypubliclib). So it's good to have there our `apis` and `clients` because it's our contract and we are supporting those libs and it's normal that someone will vendor that in own project. In case of `health` pkg we shouldn't expose such a contract because is not helm-broker domain and w do not want to support that. What's more, the `health` pkg will be removed when this will be merge: kubernetes-sigs/controller-runtime#419. **We should think twice before putting something here :-)**

This refactors the health probes, bringing the style a bit more in line with CR. The health probes themselves are now their own handler, and you add checks simply by using a map from check name to checker (which is just a function). The internal structure should now be more condusive to JSON output as well.

DirectXMan12 · 2019-10-10T23:47:28Z

@mengqiy ready for another round

mengqiy

/lgtm

k8s-ci-robot · 2019-10-11T01:15:05Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: GrigoriyMikhalkin

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

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

seh · 2019-10-22T20:12:19Z

pkg/manager/manager.go

@@ -304,6 +332,19 @@ func defaultNewClient(cache cache.Cache, config *rest.Config, options client.Opt
 	}, nil
 }

+// defaultHealthProbeListener creates the default health probes listener bound to the given address
+func defaultHealthProbeListener(addr string) (net.Listener, error) {
+	if addr == "" || addr == "0" {


Did we intend for health probes to not be served by default? By my reading, since Options.HealthProbeBindAddress has no default value set apart from the empty string, we get no probe server unless we set it explicitly.

omerlh · 2020-03-12T11:15:41Z

Is this documented somewhere? It took me a while to figure how to enable it, and I had to read the code...

GrigoriyMikhalkin · 2020-03-12T19:31:49Z

@omerlh Does controller-runtime has documentation at all)? I mean, besides mentions in https://book.kubebuilder.io/

alexeldeib · 2020-03-12T19:45:00Z

Generally, the godoc: https://godoc.org/sigs.k8s.io/controller-runtime/pkg/healthz

https://godoc.org/sigs.k8s.io/controller-runtime is really useful IMO

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2019

k8s-ci-robot requested review from DirectXMan12 and droot May 9, 2019 12:54

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 9, 2019

k8s-ci-robot assigned pwittrock May 9, 2019

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

GrigoriyMikhalkin force-pushed the CR-297 branch from 0597abd to 474cdca Compare May 17, 2019 22:52

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 17, 2019

GrigoriyMikhalkin force-pushed the CR-297 branch from 474cdca to 97b5a8a Compare May 18, 2019 07:47

DirectXMan12 changed the title ~~✨added health probes~~ ✨ added health probes Jun 4, 2019

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 4, 2019

DirectXMan12 suggested changes Jun 7, 2019

View reviewed changes

GrigoriyMikhalkin force-pushed the CR-297 branch from 97b5a8a to c01da8c Compare June 9, 2019 22:06

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 9, 2019

DirectXMan12 reviewed Jun 11, 2019

View reviewed changes

GrigoriyMikhalkin force-pushed the CR-297 branch from c01da8c to ea95ee0 Compare June 14, 2019 20:30

DirectXMan12 suggested changes Aug 6, 2019

View reviewed changes

GrigoriyMikhalkin force-pushed the CR-297 branch from ea95ee0 to 1692d51 Compare August 7, 2019 20:18

DirectXMan12 suggested changes Aug 13, 2019

View reviewed changes

GrigoriyMikhalkin force-pushed the CR-297 branch from 1692d51 to b28d04b Compare August 16, 2019 22:47

polskikiel mentioned this pull request Sep 10, 2019

Improve controller readiness probe kyma-project/helm-broker#29

Closed

added health probes

9bc08d8

DirectXMan12 force-pushed the CR-297 branch 2 times, most recently from 5d06f04 to 864a936 Compare September 19, 2019 01:21

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 19, 2019

DirectXMan12 force-pushed the CR-297 branch from 864a936 to 0085b01 Compare September 19, 2019 01:57

mengqiy reviewed Sep 19, 2019

View reviewed changes

mszostok mentioned this pull request Sep 20, 2019

Add Helm Chart testing scripts kyma-project/helm-broker#32

Merged

DirectXMan12 force-pushed the CR-297 branch from 0085b01 to 52d6dbf Compare October 10, 2019 23:47

mengqiy approved these changes Oct 11, 2019

View reviewed changes

k8s-ci-robot assigned mengqiy Oct 11, 2019

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

mengqiy added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2019

k8s-ci-robot merged commit 16c93b0 into kubernetes-sigs:master Oct 11, 2019

seh reviewed Oct 22, 2019

View reviewed changes

estroz mentioned this pull request Nov 11, 2019

Support health and liveness probes #297

Closed

johnugeorge mentioned this pull request Jan 21, 2020

Notebook controller should expose liveness/readiness signal kubeflow/kubeflow#4060

Closed

phoracek mentioned this pull request Feb 21, 2020

Add default readiness check for webhook server? #723

Closed

GrigoriyMikhalkin mentioned this pull request Mar 12, 2020

Document controller health probes #855

Closed

captainroy-hy mentioned this pull request Oct 12, 2020

add liveness & readiness probes kubevela/kubevela#379

Merged

skonto mentioned this pull request Jan 26, 2023

Add health checks knative/pkg#2671

Merged

ramperher mentioned this pull request Jan 9, 2024

Support startup probes #2644

Closed

✨ added health probes #419

✨ added health probes #419

Conversation

GrigoriyMikhalkin commented May 9, 2019

GrigoriyMikhalkin commented May 9, 2019

DirectXMan12 commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy Jun 19, 2019 • edited

Choose a reason for hiding this comment

GrigoriyMikhalkin commented Jun 9, 2019

DirectXMan12 left a 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

DirectXMan12 left a 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

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

DirectXMan12 commented Sep 19, 2019

DirectXMan12 commented Sep 19, 2019

droot commented Sep 19, 2019

mengqiy left a 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

DirectXMan12 commented Oct 10, 2019

mengqiy left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 11, 2019

Choose a reason for hiding this comment

omerlh commented Mar 12, 2020

GrigoriyMikhalkin commented Mar 12, 2020

alexeldeib commented Mar 12, 2020 • edited

mengqiy Jun 19, 2019 •

edited

alexeldeib commented Mar 12, 2020 •

edited