Add readiness endpoint #43
Conversation
| readinessProbe: | ||
| httpGet: | ||
| path: /readyz | ||
| port: 8081 |
There was a problem hiding this comment.
The healthz endpoint was setup already in main. This adds port/path to make use of it. Not sure about the time values here
|
Much needed! Will give this a thorough review soon. |
There was a problem hiding this comment.
This is a very important feature to have!
I have a question on whether the client returned from mgr is ready to use before the call to mgr.Start()... It is a cache-backed client and I am not sure the cache will be synced when the readiness check calls it (perhaps leading to a .List() that returns fewer than actual items - resulting in a false-positive readiness check).
By default, when the controller starts up, it will automatically reconcile all Deployments... as far as I could tell there is no way to check when that initial run has completed, so I think it makes sense to do what you are doing here. However, I wonder if a cleaner way of accomplishing this would be to run a one-time List() (after cache is synced - or using a non-cache client) and iterate through and call Reconcile() (i.e. a forced full-reconciliation), then register an always-200 readiness handler.
PS: I don't know if there is any value in using this controller-runtime pkg to do the health checks or not, but might be worth a look: https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/healthz#Handler
|
Good questions! I had looked at https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/healthz#Handler before but this is just a generic framework to customize the healthz server handlers. Most examples point to the Ping handler as generic |
* main: fix GHA to work for external contributors (substratusai#45) Add Prometheus metrics (substratusai#39)
cmd/lingo/main.go
Outdated
| @@ -103,10 +105,15 @@ func run() error { | |||
| return fmt.Errorf("starting manager: %w", err) | |||
| } | |||
|
|
|||
| if err := mgr.AddReadyzCheck("readyz", healthz.Ping); err != nil { | |||
There was a problem hiding this comment.
This is the always 200 handler
There was a problem hiding this comment.
Does this start serving after mgr.Start()? If so, I think we need to move .Bootstrap() above mgr.Start().
There was a problem hiding this comment.
This is a bit of a chicken/ egg problem. The mgr.Start() call also starts the cache syncs that we need for the bootstrap call. So with this implementation, the container may be "ready" but not alive, yet.
This can be solved with a custom handler that checks bootstrap was called once.
Good 👁️
cmd/lingo/main.go
Outdated
| @@ -103,10 +105,15 @@ func run() error { | |||
| return fmt.Errorf("starting manager: %w", err) | |||
| } | |||
|
|
|||
| if err := mgr.AddReadyzCheck("readyz", healthz.Ping); err != nil { | |||
There was a problem hiding this comment.
Does this start serving after mgr.Start()? If so, I think we need to move .Bootstrap() above mgr.Start().
* main: Handle model undeployment (substratusai#44)
Resolves #5