Differentiate liveness and readiness probes for router pods #19009

JacobTanenbaum · 2018-03-16T19:37:50Z

Create upstream commit that allows for multiple groups of checks to be associated with health checking. Using the multiple groupings differentiate the liveness and readiness probes for the Haproxy router

Bug 1550007

knobunc · 2018-03-16T19:48:20Z

@smarterclayton is this a reasonable way to approach the problem?

knobunc · 2018-03-16T19:48:37Z

@openshift/networking PTAL

rajatchopra

Looks good to me.
But this is just separating the urls. The logic served on either needs to be differentiated too. So there is going to be a part 2? Or am I missing something?

rajatchopra · 2018-03-16T20:29:55Z

pkg/oc/admin/router/router.go

@@ -436,7 +436,7 @@ func generateSecretsConfig(cfg *RouterConfig, namespace string, defaultCert []by
 	return secrets, volumes, mounts, nil
 }

-func generateProbeConfigForRouter(cfg *RouterConfig, ports []kapi.ContainerPort) *kapi.Probe {
+func generateProbeConfigForRouter(Path string, cfg *RouterConfig, ports []kapi.ContainerPort) *kapi.Probe {


camelCase for 'Path'

pravisankar

As Rajat pointed out, need to camelCase the variable. Otherwise, LGTM

pravisankar · 2018-03-16T23:58:29Z

@rajatchopra I don't think there will be part 2 here.
https://github.com/openshift/origin/pull/19009/files#diff-43ed12581474d0d4e9bd5b12bdcf6417R40
creates additional '/livez' endpoint (eg: http://localhost:1936/livez) which always returns success and this is used for liveness probe. Success from this endpoint means router is up but not ready until /healthz returns success.

rajatchopra · 2018-03-19T17:40:51Z

pkg/oc/admin/router/router.go

@@ -447,7 +447,7 @@ func generateProbeConfigForRouter(cfg *RouterConfig, ports []kapi.ContainerPort)
 		}

 		probe.Handler.HTTPGet = &kapi.HTTPGetAction{
-			Path: "/healthz",
+			Path: path,
 			Port: intstr.IntOrString{
 				Type:   intstr.Int,
 				IntVal: int32(healthzPort),


Even though a path is being passed (which is either livez or healthz), the port is always healthz?

Yes, both /livez and /healthz paths are using StatsPort
https://github.com/JacobTanenbaum/origin/blob/1f93335b3f25944c202f7f05e3e52201d07ff972/pkg/cmd/infra/router/template.go#L206 and
https://github.com/JacobTanenbaum/origin/blob/1f93335b3f25944c202f7f05e3e52201d07ff972/pkg/oc/admin/router/router.go#L446

JacobTanenbaum · 2018-03-19T18:38:35Z

pkg/oc/admin/router/router.go

 		if cfg.StatsPort > 0 {
-			healthzPort = cfg.StatsPort
+			probePort = cfg.StatsPort


@rajatchopra @pravisankar does this make it clearer? removing the reference to the healthzPort in favour of a more generic term

pravisankar

/lgtm

pravisankar · 2018-03-19T20:14:50Z

pkg/oc/admin/router/router.go

 		if cfg.StatsPort > 0 {
-			healthzPort = cfg.StatsPort
+			probePort = cfg.StatsPort


smarterclayton · 2018-03-19T22:17:23Z

pkg/oc/admin/router/router.go

@@ -466,15 +465,15 @@ func generateProbeConfigForRouter(cfg *RouterConfig, ports []kapi.ContainerPort)
 }

 func generateLivenessProbeConfig(cfg *RouterConfig, ports []kapi.ContainerPort) *kapi.Probe {
-	probe := generateProbeConfigForRouter(cfg, ports)
+	probe := generateProbeConfigForRouter("/livez", cfg, ports)


This should be /healthz/ready which is our standard readiness check path

/healthz is liveness, /healthz/ready is readiness.

smarterclayton · 2018-03-19T22:18:22Z

pkg/router/metrics/metrics.go

@@ -37,6 +37,10 @@ type Listener struct {
 func (l Listener) handler() http.Handler {
 	mux := http.NewServeMux()
 	healthz.InstallHandler(mux, l.Checks...)
+	mux.Handle("/livez", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


What are you actually trying to check for?

smarterclayton · 2018-03-19T22:20:38Z

This should be following our standard conventions, but you cannot change the meaning of the existing health endpoint without breaking backwards compatibility.

Can you describe what the goal is here - have the endpoint available for load balancers prior to the router actually being loaded? Generally if you want that you just use the service field spec.publishNotReadyAddresses, which doesn't need any of these changes.

knobunc · 2018-03-20T14:21:05Z

@smarterclayton the router added ROUTER_BIND_PORTS_AFTER_SYNC so that external load balancers could have useful health checks on port 80 or 443 for the router to know when it was online. Before we added ROUTER_BIND_PORTS_AFTER_SYNC the router would bind those ports immediately and potentially serve HTTP 503 statuses for valid routes because the route had not been loaded yet. So, we added ROUTER_BIND_PORTS_AFTER_SYNC so that it would not bind 80/443 until a full sync had happened, BUT would bind 1936 so that the liveness probes worked (otherwise the router gets killed).

After the refactor to make the router controller return status directly, if you set ROUTER_BIND_PORTS_AFTER_SYNC then you start failing liveness probes. @JacobTanenbaum found out that this was due to the liveness check done by the router controller contacting the haproxies it controlled to see if they were up, and if not, returning false. So, when ROUTER_BIND_PORTS_AFTER_SYNC is set, the haproxy doesn't bind 80 and 443 so the delegated liveness check fails. And the pod gets terminated.

So, the goal here is to have an endpoint indicating that the router controller is live, but that it is not ready yet... but we have to work with what we have at the moment:

/healthz -- Returns only when the backends are ready (changed in 3.7)
/healthz/backend-http -- Returns only when haproxy is up (added in 3.7)

Before 3.7 /healthz was a liveness check

Can we call this a bugfix and add /healthz/ready and move the multiplexer that is on /healthz there? (Should we also support /healthz/ready/backend-http since that's what the multiplexer will set up)?

Then /healtz can go back to returning true as soon as the router controller becomes active.

smarterclayton · 2018-03-20T14:37:19Z

The liveness check is to prevent a crashed haproxy router from staying dead due to a route controller bug. The primary purpose is to keep the pod running. The fundamental behavior of that check didn't change (pre 3.7, if that failed haproxy was dead, and post 3.7, if that failed haproxy is dead), but the secondary effect you're referencing did. No external load balancer should be using /healthz to determine whether to put the router in rotation - it should be using /healthz/ready or something equivalent. If we want to health check the router controller we should add `/healthz/controller` or similar. When we move to service load balancer for router on cloud providers, the readiness check for the router service should be /healthz/backend-http (if you only want to be in rotation if haproxy is listening), and set the service to preserveUnreadyEndpoints (if you want to be in rotation regardless of readiness)

…

On Tue, Mar 20, 2018 at 10:21 AM, Ben Bennett ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> the router added ROUTER_BIND_PORTS_AFTER_SYNC so that *external* load balancers could have useful health checks on port 80 or 443 for the router to know when it was online. Before we added ROUTER_BIND_PORTS_AFTER_SYNC the router would bind those ports immediately and potentially serve HTTP 503 statuses for valid routes because the route had not been loaded yet. So, we added ROUTER_BIND_PORTS_AFTER_SYNC so that it would not bind 80/443 until a full sync had happened, BUT would bind 1936 so that the liveness probes worked (otherwise the router gets killed). After the refactor to make the router controller return status directly, if you set ROUTER_BIND_PORTS_AFTER_SYNC then you start failing liveness probes. @JacobTanenbaum <https://github.com/jacobtanenbaum> found out that this was due to the liveness check done by the router controller contacting the haproxies it controlled to see if they were up, and if not, returning false. So, when ROUTER_BIND_PORTS_AFTER_SYNC is set, the haproxy doesn't bind 80 and 443 so the delegated liveness check fails. And the pod gets terminated. So, the goal here is to have an endpoint indicating that the router controller is live, but that it is not ready yet... but we have to work with what we have at the moment: - /healthz -- Returns only when the backends are ready (changed in 3.7) - /healthz/backend-http -- Returns only when haproxy is up (added in 3.7) Before 3.7 /healthz was a liveness check Can we call this a bugfix and add /healthz/ready and move the multiplexer that is on /healthz there? (Should we also support /healthz/ready/backend-http since that's what the multiplexer will set up)? Then /healtz can go back to returning true as soon as the router controller becomes active. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19009 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p0bjOks6ht4R6hRcQ67mxP1aMOzDks5tgRBWgaJpZM4SuVWN> .

knobunc · 2018-03-21T15:06:52Z

@smarterclayton The problem is that the external load balancers are looking at 80 and 443 directly to decide whether to put it in rotation. BUT the liveness check defined for the pod is on /healthz and that is checking that the haproxy is live too before returning 200. So when we don't bind to 80 the liveness probes fail. What would you suggest we do to make the liveness check work?

I see two options for this

Document that this option is deprecated and only works when the old stats are in use and then create and document proper readiness and liveness checks
Work out some way to make the option work with the current router so we can provide a liveness check

smarterclayton · 2018-03-22T22:45:24Z

Sorry for not responding faster, this is on my list to respond to.

…

On Thu, Mar 22, 2018 at 1:54 PM, OpenShift CI Robot < ***@***.***> wrote: New changes are detected. LGTM label has been removed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19009 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p76NHHtju7Gs2H_Y5ch7qTV9DFheks5tg-VZgaJpZM4SuVWN> .

pravisankar · 2018-05-11T17:36:22Z

vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz.go

@@ -66,16 +66,25 @@ func NamedCheck(name string, check func(r *http.Request) error) HealthzChecker {
 // exactly one call to InstallHandler. Calling InstallHandler more
 // than once for the same mux will result in a panic.
 func InstallHandler(mux mux, checks ...HealthzChecker) {
+	InstallPathHandler(mux, "/healthz", checks...)


Do we need to change vendor code, can't we do this stuff in openshift code?

AFAIK changing vendor code needs 'UPSTREAM: ' commit in this case.

@pravisankar I did post an upstream PR to accompany this kubernetes/PR63716. The commit that includes the kubernetes code is tagged UPSTREAM:63716, should I add this tag to the PR title?

No as far was we can tell there is no way that we can do this stuff in only openshift. We use InstallHandler for the checks and currently you can only have one set of checks that all have to pass for both liveness and readiness, The changes in vendor allows us to create two sets of checks.

Separate UPSTREAM:63716 commit is good, no need to add upstream tag to the PR title.

knobunc · 2018-05-14T15:50:21Z

@JacobTanenbaum Can you fix your description to capture the current behavior please.

knobunc

/lgtm

knobunc · 2018-05-14T15:52:39Z

/approve

knobunc · 2018-05-15T14:22:18Z

/assign @deads2k

knobunc · 2018-05-15T14:22:48Z

/retest

pravisankar

LGTM

pravisankar · 2018-05-15T21:02:59Z

vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/pkg/server/healthz/healthz.go

@@ -66,16 +66,25 @@ func NamedCheck(name string, check func(r *http.Request) error) HealthzChecker {
 // exactly one call to InstallHandler. Calling InstallHandler more
 // than once for the same mux will result in a panic.
 func InstallHandler(mux mux, checks ...HealthzChecker) {
+	InstallPathHandler(mux, "/healthz", checks...)


Separate UPSTREAM:63716 commit is good, no need to add upstream tag to the PR title.

pravisankar · 2018-05-15T21:12:30Z

/retest

knobunc · 2018-05-18T15:16:59Z

@deads2k can you approve the upstream commit to vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver please. (Since kubernetes/kubernetes#63716 landed upstream)

Thanks

JacobTanenbaum · 2018-05-21T10:12:16Z

/retest

knobunc · 2018-05-21T17:10:27Z

@deads2k -- can you approve this since the upstream PR has merged.

…e path to be associated with health checking. Currently it is only possible to have one group of checks which must all pass for the handler to report success. Allowing multiple paths for these checks allows use of the same machinery for other kinds of checks, i.e. readiness. This upstream change allows for the differentiation of health and readiness checks

Add a backend to the router controller "/livez" that always returns true. This differentiates the liveness and readiness probes so that a router can be alive and not ready. Bug 1550007

deads2k · 2018-05-21T18:14:15Z

/approve

knobunc · 2018-05-22T17:58:39Z

/lgtm

openshift-ci-robot · 2018-05-22T17:59:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JacobTanenbaum, knobunc, pravisankar, ramr

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/cmd/infra/router/OWNERS~~ [deads2k,knobunc]
~~pkg/oc/admin/router/OWNERS~~ [deads2k,knobunc]
~~pkg/router/OWNERS~~ [deads2k,knobunc]
~~vendor/k8s.io/kubernetes/staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]

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

smarterclayton · 2018-05-27T17:37:38Z

pkg/router/metrics/health.go

@@ -35,6 +36,28 @@ func HTTPBackendAvailable(u *url.URL) healthz.HealthzChecker {
 	})
 }

+// HasSynced returns a healthz check that verifies the router has been synced at least
+// once.
+func HasSynced(router **templateplugin.TemplatePlugin) healthz.HealthzChecker {


This sort of construct is a violation of our style guides. You should be passing an interface here that exposes the correct check method. Double pointers should never be used. Nothing in metrics should be aware of template plugin at all.

smarterclayton · 2018-05-27T17:38:10Z

pkg/router/metrics/health.go

+func HasSynced(router **templateplugin.TemplatePlugin) healthz.HealthzChecker {
+	return healthz.NamedCheck("has-synced", func(r *http.Request) error {
+		if router != nil {
+			if (*router).Router.SyncedAtLeastOnce() == true {


This construct is not appropriate. It should always be if booleancondition {

smarterclayton · 2018-05-27T17:38:25Z

pkg/router/metrics/health.go

+		if router != nil {
+			if (*router).Router.SyncedAtLeastOnce() == true {
+				return nil
+			} else {


When you return early, elide the else, as per our style guide.

smarterclayton · 2018-05-27T17:38:42Z

pkg/router/metrics/health.go

+			if (*router).Router.SyncedAtLeastOnce() == true {
+				return nil
+			} else {
+				return fmt.Errorf("Router not synced")


Errors should always be lower case, as per the style guide.

openshift-ci-robot requested review from juanvallejo and knobunc March 16, 2018 19:37

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 16, 2018

knobunc requested review from ramr and rajatchopra March 16, 2018 19:47

knobunc self-assigned this Mar 16, 2018

knobunc added kind/bug Categorizes issue or PR as related to a bug. component/routing labels Mar 16, 2018

rajatchopra suggested changes Mar 16, 2018

View reviewed changes

pravisankar reviewed Mar 16, 2018

View reviewed changes

JacobTanenbaum force-pushed the BZ1550007 branch 2 times, most recently from fefb00b to 1f93335 Compare March 19, 2018 14:22

rajatchopra reviewed Mar 19, 2018

View reviewed changes

JacobTanenbaum commented Mar 19, 2018

View reviewed changes

JacobTanenbaum force-pushed the BZ1550007 branch from 34283c4 to 3b65bf9 Compare March 19, 2018 19:13

pravisankar approved these changes Mar 19, 2018

View reviewed changes

pkg/oc/admin/router/router.go

if cfg.StatsPort > 0 {

healthzPort = cfg.StatsPort

probePort = cfg.StatsPort

Copy link

pravisankar Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

openshift-ci-robot assigned pravisankar Mar 19, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2018

smarterclayton reviewed Mar 19, 2018

View reviewed changes

JacobTanenbaum force-pushed the BZ1550007 branch from 3b65bf9 to 2f18e8c Compare March 22, 2018 17:54

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2018

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 11, 2018

pravisankar reviewed May 11, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 14, 2018

knobunc approved these changes May 14, 2018

View reviewed changes

openshift-ci-robot assigned deads2k May 15, 2018

pravisankar approved these changes May 15, 2018

View reviewed changes

JacobTanenbaum added 2 commits May 21, 2018 13:24

Differentiate liveness and readiness probes for router

978d2bc

Add a backend to the router controller "/livez" that always returns true. This differentiates the liveness and readiness probes so that a router can be alive and not ready. Bug 1550007

JacobTanenbaum force-pushed the BZ1550007 branch from 5c12069 to 978d2bc Compare May 21, 2018 17:25

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 21, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 22, 2018

openshift-merge-robot merged commit 13d42ec into openshift:master May 22, 2018

smarterclayton reviewed May 27, 2018

View reviewed changes

Miciah mentioned this pull request Jul 24, 2018

Remove ROUTER_BIND_PORTS_BEFORE_SYNC configuration #20410

Closed

Differentiate liveness and readiness probes for router pods #19009

Differentiate liveness and readiness probes for router pods #19009

Conversation

JacobTanenbaum commented Mar 16, 2018 • edited Loading

knobunc commented Mar 16, 2018

knobunc commented Mar 16, 2018

rajatchopra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar left a comment

Choose a reason for hiding this comment

pravisankar commented Mar 16, 2018

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

pravisankar 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

smarterclayton commented Mar 19, 2018

knobunc commented Mar 20, 2018

smarterclayton commented Mar 20, 2018 via email

knobunc commented Mar 21, 2018

smarterclayton commented Mar 22, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobTanenbaum May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented May 14, 2018

knobunc left a comment

Choose a reason for hiding this comment

knobunc commented May 14, 2018

knobunc commented May 15, 2018

knobunc commented May 15, 2018

pravisankar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented May 15, 2018

knobunc commented May 18, 2018

JacobTanenbaum commented May 21, 2018

knobunc commented May 21, 2018

deads2k commented May 21, 2018

knobunc commented May 22, 2018

openshift-ci-robot commented May 22, 2018

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

JacobTanenbaum commented Mar 16, 2018 •

edited

Loading

JacobTanenbaum May 11, 2018 •

edited

Loading