Add health checks #2671

skonto · 2023-01-26T13:25:02Z

Changes

Sets up a handler for readiness/liveness probes to be enabled on demand in sharedMain. Moves work here from:
Add missing deployment probes serving#13563
Checks if start up work has been done.

codecov · 2023-01-26T13:31:54Z

Codecov Report

Base: 81.42% // Head: 81.30% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (d296136) compared to base (408ad07).
Patch coverage: 62.74% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2671      +/-   ##
==========================================
- Coverage   81.42%   81.30%   -0.13%     
==========================================
  Files         162      163       +1     
  Lines        9849     9900      +51     
==========================================
+ Hits         8020     8049      +29     
- Misses       1590     1609      +19     
- Partials      239      242       +3

Impacted Files	Coverage Δ
injection/health_check.go	`62.74% <62.74%> (ø)`
apis/duck/cached.go	`91.89% <0.00%> (-8.11%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

skonto · 2023-01-27T12:29:20Z

@dprotaso @evankanderson gentle ping for review.

evankanderson · 2023-01-27T17:30:52Z

We should not have the same problem as the controller-runtime issue, because we always start controllers, even if they don't win the leader election.

I agree that the check should start up and run independently of the controller startup, but it seems like we shouldn't return healthy until informer caches have been filled (which, for example, can be blocked by incorrect RBAC permissions).

evankanderson · 2023-01-27T17:36:32Z

I'd also advocate for having health endpoints always enabled. If you don't want to use them, they should be minimal overhead.

Ideally, we could provide this same server to the Prometheus metrics implementation and share a port, but I don't want to force this behind refactoring that big ball of mud.

skonto · 2023-01-30T14:42:28Z

@evankanderson hi, thanks for reviewing. Some thoughts:

We should not have the same problem as the controller-runtime issue, because we always start controllers, even if they don't win the leader election. I agree that the check should start up and run independently of the controller startup, but it seems like we shouldn't return healthy until informer caches have been filled (which, for example, can be blocked by incorrect RBAC permissions).

Initially I had it like that and spend some time thinking about options. So not elected instances dont own any bucket right? That technically means they are not ready no? Even with multiple buckets there is bucket migration. That is a less of a problem though it depends what is defined as ready here.

Now, if health probes (liveness) fail here it does not help much if you have to re-sync the cache when you restart. Also, if we have an error at this stage I think it is unlikely that a restart could help. Even with the old problem we had wrt OOM and informers, restarting didnt help or if RBAC permissions are wrong there is nothing that is improved with restarts. Health checks (liveness probes) and restarts are usually for deadlocks, fixable ooms etc. I think having the check after cache sync it opens the door to a configuration headache depending on the cluster api server communication and cache size, no? Then the user will have to set initialDelaySeconds accordingly or other settings.
The good thing is that it would clearly signal that the container is failing if it does not finish the controller/informers startup thing and user would know, instead of looking at the logs in those scenarios. Also it enforces a more complete notion of healthiness, instead of just checking if main is running etc.
Controller runtime btw even for the leader pod does not check that part so I am kind of puzzled as to what level of health check we should offer and if we re doing too much to cover all cases. I can try move it there and see how it goes for sure. @dprotaso @psschwei any thoughts?

I'd also advocate for having health endpoints always enabled. If you don't want to use them, they should be minimal overhead.

Then it could be the opposite the flag to be a disable flag in case someone wants to disable it and override it with his own. I can easily change it. Not sure though if any project eg. sandbox projects, will be affected by this. A scenario could be that a project uses sharedmain and has a custom probe.

Ideally, we could provide this same server to the Prometheus metrics implementation and share a port, but I don't want to force this behind refactoring that big ball of mud.

Yes indeed I think there was such a reference in some controller runtime issue but we can keep it simple here for now.
Do you mean use the Prometheus server for the check endpoint? If so metrics could be off or user may not use Prometheus no?

skonto · 2023-02-03T13:21:44Z

/assign @evankanderson

skonto · 2023-02-03T13:21:59Z

Will update the PR.

evankanderson

Thanks for doing this! I'm happy to build this up incrementally.

It feels to me like it might make sense to have a registration mechanism; something like AddReadinessCheck() and AddLivenessCheck().

I'm also wondering if these interfaces should live in sharedmain instead of network, but I think we could work that out now or later.

I am going to insist on switching the usage of log for getting the logger from the Context, though.

evankanderson · 2023-02-03T16:31:50Z

network/health_check.go

+	}()
+}
+
+func newHealthCheck(sigCtx context.Context) func() error {


Any thoughts about how we might let users customize this later?

We could refactor this to allow pass a func and propagate it from ServeHealthProbes(ctx context.Context) or use context to register another callback there and get it with some func such as healthCheckFromContext. WDYTH?

evankanderson · 2023-02-03T16:35:30Z

network/health_check.go

+}
+
+func (h *healthHandler) handle(w http.ResponseWriter, r *http.Request) {
+	if IsKubeletProbe(r) {


Checking IsKubeletProbe here keeps this health checker from being used for e.g. proxy or apiserver health checks.

I know we may not need them today, but I'm wondering if we should be passing the request to the health checking function rather than doing this check here. i.e. func(http.Request) error. This might be a half-baked idea, though.

evankanderson · 2023-02-03T16:36:08Z

network/health_check.go

+func (h *healthHandler) handle(w http.ResponseWriter, r *http.Request) {
+	if IsKubeletProbe(r) {
+		if err := h.HealthCheck(); err != nil {
+			log.Println("Healthcheck failed: ", err.Error())


Should this extract the logger from the context and use that?

skonto · 2023-02-07T12:10:09Z

network/health_check.go

+}
+
+// UserReadinessProbe checks if user has explicitly requested to set a readiness probe in the related context.
+func UserReadinessProbe(ctx context.Context) bool {


We can extend with a custom url in the future to change the default path too.

I'm struggling to figure out why you'd want to disable these on the server. I understand why you might want to remove them from the manifest, but what's the rationale for not registering them with the HTTPServer?

skonto · 2023-02-07T12:14:23Z

@evankanderson pls review. Updates:
a) Added a mechanism to register only specific probe types.
b) Made the health handler easy to override
c) Added unit tests

Not sure how queue proxy could utilize this because drainer is used as part of a handler chain. Also I think there are more requirements there like TLS which we dont address at the moment (some user services require this).
Thus the scope of this for now is for regular deployments of Knative control plane but probably we can refactor further.

evankanderson · 2023-02-09T00:06:35Z

Yeah, I'm not deeply concerned about using this for queue-proxy or activator, which will both be a little funny, but I just didn't want to over-specialize and then have to walk it back.

At the same time, it would be nice if we could use this for e.g. eventing brokers, and have them fail readiness or liveness if they can't reach their storage backends. That probably needs that we need something like an ability to register health checks in the injection.ControllerConstructor, which probably means adding the healthcheck registration(s) to the context before calling the constructors.

evankanderson · 2023-02-09T00:06:44Z

(Reviewing now)

evankanderson

I like the idea of enabling health probes -- is there any reason not to have them on all the time (it might simplify the code)?

I'd also sort of like to be able to say:

func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
  ...
  checker := func (r http.Request) error {
    // pretend we have appropriate locking
    return impl.connectionError
  }

  // internally, calls getReadinessRegisterFunc(ctx).Register(checker)
  injection.AddReadiness(ctx, checker)

  return impl
}

Of course, checker could also be impl.IsHealthy or the like.

I'm okay with either a func (http.Request) error or a func () error signature -- whichever seems less of a pain in the common case.

/hold
/lgtm

If you like the above interface, I don't know if we want to introduce this one and then remove it.

If you want to just enable health checks, I'd probably remove most of the public interfaces in network for now except the one needed from sharedmain.go.

evankanderson · 2023-02-09T00:14:51Z

network/health_check.go

+}
+
+// UserReadinessProbe checks if user has explicitly requested to set a readiness probe in the related context.
+func UserReadinessProbe(ctx context.Context) bool {


I'm struggling to figure out why you'd want to disable these on the server. I understand why you might want to remove them from the manifest, but what's the rationale for not registering them with the HTTPServer?

evankanderson · 2023-02-09T00:17:27Z

network/health_check.go

+	if h := getOverrideHealthCheckHandler(ctx); h != nil {
+		handler = *h
+	} else {
+		defaultHandle := newDefaultProbesHandle(ctx)
+		handler = HealthCheckHandler{
+			ReadinessProbeRequestHandle: defaultHandle,
+			LivenessProbeRequestHandle:  defaultHandle,
+		}
+	}
+	mux := http.NewServeMux()
+	if UserReadinessProbe(ctx) {
+		mux.HandleFunc("/readiness", handler.ReadinessProbeRequestHandle)
+	}
+	if UserLivenessProbe(ctx) {
+		mux.HandleFunc("/health", handler.LivenessProbeRequestHandle)
+	}
+	// Set both probes if user does not want to explicitly set only one.
+	if !UserReadinessProbe(ctx) && !UserLivenessProbe(ctx) {
+		mux.HandleFunc("/readiness", handler.ReadinessProbeRequestHandle)
+		mux.HandleFunc("/health", handler.LivenessProbeRequestHandle)
+	}


This feels complicated -- could it simplify to something like:

readiness := getReadinessHandlerOrDefault(ctx) liveness := getLivenessHandlerOrDefault(ctx) mux := http.NewServeMux() mux.HandleFunc("/readiness", readiness) mux.HandleFunc("/liveness", liveness) return mux

And then the ...OrDefault(ctx) calls would get the correct handler function.

Ok let me work on the interface and see what I can come up with.

evankanderson · 2023-02-09T00:35:24Z

network/health_check.go

+
+func newDefaultProbesHandle(sigCtx context.Context) http.HandlerFunc {
+	logger := logging.FromContext(sigCtx)
+	once := sync.Once{}


Why do we need this once?

Activator has the same approach too for its healthcheck. So since the probe handler could be part of a handler chain (we might want to make it generic) we want the msg to be printed once. Of course I can simplify it for now if we expect SIGTERM to be managed once.

evankanderson · 2023-02-09T00:37:01Z

network/health_check.go

+			}
+		}
+
+		if IsKubeletProbe(r) {


I'm still wondering if we need this IsKubeletProbe check.

We want to reject a request that is not coming from kubelet. The validation is weak but it seemed to me it is better than nothing. Also it could used by queue proxy at some point. Anyway I can remove it for now.

evankanderson · 2023-02-09T00:45:19Z

injection/sharedmain/main.go

+}
+
+// HealthProbesDisabled checks if default health checks are disabled in the related context.
+func HealthProbesDisabled(ctx context.Context) bool {


Does this need to be an exported method, or can it be internal-only?

Should be private.

skonto · 2023-02-09T09:58:50Z

I like the idea of enabling health probes -- is there any reason not to have them on all the time (it might simplify the code)?

The probes are on by default, the default ones are used. HealthProbesDisabled checks if user disabled them via context. However, user could choose to provide only one probe not both by default (as K8s allows at the spec side).
Otherwise, if we have both handles in any case, we have extra probe handles available/enabled by default. It does not add much but thought it is redundant.

skonto · 2023-02-09T10:25:22Z

I'd also sort of like to be able to say:
func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
  ...
  checker := func (r http.Request) error {
    // pretend we have appropriate locking
    return impl.connectionError
  }

  // internally, calls getReadinessRegisterFunc(ctx).Register(checker)
  injection.AddReadiness(ctx, checker)

  return impl
}

Now you can say:

ctx = WithOverrideHealthCheckHandler(ctx, &HealthCheckHandler{
			ReadinessProbeRequestHandle: checker
		})
networkpkg.ServeHealthProbes(ctx)

Anyway I will take a look and see if it can be simpler or more explicit.

skonto · 2023-02-09T12:11:25Z

@evankanderson I simplified the impl. Pls review.

skonto · 2023-02-09T12:55:12Z

{"level":"error","ts":1675945698.819293,"logger":"fallback","caller":"consumergroup/reconciler.go:302","msg":"Returned an error","targetMethod":"FinalizeKind","error":"unable to delete the consumer group my.group.id: kafka server: The client is not authorized to send this request type","stacktrace":"knative.dev/eventing-kafka-broker/control-plane/pkg/client/internals/kafka/injection/reconciler/eventing/v1alpha1/consumergroup.(*reconcilerImpl).Reconcile\n\t/home/runner/work/pkg/pkg/downstream/control-plane/pkg/client/internals/kafka/injection/reconciler/eventing/v1alpha1/consumergroup/reconciler.go:302\nknative.dev/pkg/reconciler/testing.(*TableRow).Test\n\t/home/runner/work/pkg/pkg/downstream/vendor/knative.dev/pkg/reconciler/testing/table.go:160\nknative.dev/pkg/reconciler/testing.TableTest.Test.func1\n\t/home/runner/work/pkg/pkg/downstream/vendor/knative.dev/pkg/reconciler/testing/table.go:389\ntesting.tRunner\n\t/opt/hostedtoolcache/go/1.18.10/x64/src/testing/testing.go:1439"}

skonto · 2023-02-10T20:21:13Z

@evankanderson gentle ping.

skonto · 2023-02-13T18:45:47Z

@evankanderson pls review

evankanderson

/lgtm
/approve

Looks good for now! Sorry about the delays in reviewing; I had Friday as a day off.

knative-prow · 2023-02-13T18:58:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, skonto

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~~ [evankanderson]

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

skonto · 2023-02-14T16:14:50Z

/unhold

skonto · 2023-02-14T16:15:31Z

/hold @evankanderson is it ok to unhold?

dprotaso · 2023-02-14T16:52:44Z

injection/health_check.go

+	port := os.Getenv("KNATIVE_HEALTH_PROBES_PORT")
+	if port == "" {
+		port = "8080"
+	}


Why not make port an arg of the function?

We could just used what Eventing had for alignment. I will update this.

dprotaso · 2023-02-14T16:53:30Z

injection/health_check.go

+		logger.Infof("Probes server listening on port %s", port)
+
+		if err := server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			logger.Fatal(err)


I don't think we should fatal - we should return an error

Ok we can just emit the error but eventually we will fail since probes are failing.

dprotaso · 2023-02-14T16:55:03Z

injection/sharedmain/main.go

@@ -313,6 +313,11 @@ func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, cto
 		return controller.StartAll(ctx, controllers...)
 	})

+	// Setup default health checks to catch issues with cache sync etc.
+	if !healthProbesDisabled(ctx) {
+		injection.ServeHealthProbes(ctx)


I think we should make this part of the errgroup so that if it fails to start it should shut everything down

Ok makes sense we can also avoid some nesting of the go routines.

evankanderson · 2023-02-14T17:33:36Z

Yes, I hadn't even realized there was still a hold.

I'll typically use a hold if I want to give the author a choice between merge now and change based on optional feedback.

skonto · 2023-02-15T14:12:30Z

@evankanderson @dprotaso pls take another look. I addressed the latest comments.
For the use of the PR check here: knative/serving#13563.

One scenario not addressed here is what default-domain job uses:

	// Start an HTTP Server
	h := netprobe.NewHandler(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
		w.WriteHeader(http.StatusOK)
	}))
	//nolinlt:gosec https://github.com/knative/serving/issues/13439
	server := http.Server{Addr: ":8080", Handler: h, ReadHeaderTimeout: time.Minute}
	go server.ListenAndServe()

This is a special case as we need to modify the path, have only a common handler and use only "/" as the path. Net-kourier probes that path only, any other path will not work. This is left for a separate PR. Btw we could have "/" as the default path for all probes but I like the cleaner approach of having explicit paths specified for both probes.

evankanderson · 2023-02-15T15:00:43Z

I think it's fine to add the option to change the path (register additional paths?) as a follow-up.

/lgtm

skonto · 2023-02-15T20:32:39Z

@dprotaso is it ok to unhold?

dprotaso · 2023-02-15T23:27:53Z

/unhold

add health checks

7f452a1

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 26, 2023

knative-prow bot requested review from tcnghia and vagababov January 26, 2023 13:25

skonto changed the title ~~Add health checks~~ [wip] Add health checks Jan 26, 2023

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2023

skonto changed the title ~~[wip] Add health checks~~ Add health checks Jan 26, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2023

knative-prow bot assigned evankanderson Feb 3, 2023

updates

0da1d16

evankanderson reviewed Feb 3, 2023

View reviewed changes

make probes more flexible

af5d2b9

skonto commented Feb 7, 2023

View reviewed changes

evankanderson reviewed Feb 9, 2023

View reviewed changes

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 9, 2023

simplify

8ad9c50

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2023

minor

3d90f13

internal only

a145416

evankanderson reviewed Feb 13, 2023

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 13, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2023

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2023

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2023

dprotaso reviewed Feb 14, 2023

View reviewed changes

fixes

d296136

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2023

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2023

knative-prow bot merged commit 4a80605 into knative:main Feb 15, 2023

skonto mentioned this pull request Feb 21, 2023

[release-v1.7][SRVKS-999] Add probes to all deployments openshift-knative/serving#176

Merged

skonto mentioned this pull request Apr 14, 2023

Deployment probe fixes knative/serving#13885

Merged

skonto mentioned this pull request Apr 28, 2023

[RELEASE-1.8] Add default health checks for sharedmain openshift-knative/net-istio#38

Merged

Add health checks #2671

Add health checks #2671

Conversation

skonto commented Jan 26, 2023 • edited Loading

Changes

codecov bot commented Jan 26, 2023 • edited Loading

Codecov Report

skonto commented Jan 27, 2023

evankanderson commented Jan 27, 2023

evankanderson commented Jan 27, 2023

skonto commented Jan 30, 2023 • edited Loading

skonto commented Feb 3, 2023

skonto commented Feb 3, 2023

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Feb 6, 2023 • edited Loading

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

skonto commented Feb 7, 2023 • edited Loading

evankanderson commented Feb 9, 2023

evankanderson commented Feb 9, 2023

evankanderson left a comment • edited Loading

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

skonto Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Feb 9, 2023 • edited Loading

skonto commented Feb 9, 2023 • edited Loading

skonto commented Feb 9, 2023

skonto commented Feb 9, 2023

skonto commented Feb 10, 2023

skonto commented Feb 13, 2023

evankanderson left a comment

Choose a reason for hiding this comment

knative-prow bot commented Feb 13, 2023

skonto commented Feb 14, 2023

skonto commented Feb 14, 2023 • edited Loading

dprotaso Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

skonto Feb 14, 2023 • edited Loading

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

skonto Feb 14, 2023 • edited Loading

Choose a reason for hiding this comment

evankanderson commented Feb 14, 2023

skonto commented Feb 15, 2023 • edited Loading

evankanderson commented Feb 15, 2023

skonto commented Feb 15, 2023

dprotaso commented Feb 15, 2023

skonto commented Jan 26, 2023 •

edited

Loading

codecov bot commented Jan 26, 2023 •

edited

Loading

skonto commented Jan 30, 2023 •

edited

Loading

skonto Feb 6, 2023 •

edited

Loading

skonto commented Feb 7, 2023 •

edited

Loading

evankanderson left a comment •

edited

Loading

skonto Feb 9, 2023 •

edited

Loading

skonto Feb 9, 2023 •

edited

Loading

skonto commented Feb 9, 2023 •

edited

Loading

skonto commented Feb 9, 2023 •

edited

Loading

skonto commented Feb 14, 2023 •

edited

Loading

dprotaso Feb 14, 2023 •

edited

Loading

skonto Feb 14, 2023 •

edited

Loading

skonto Feb 14, 2023 •

edited

Loading

skonto commented Feb 15, 2023 •

edited

Loading