feat: add support for health-check flag #1271
Conversation
|
Reading some more about healthchecks here (and in the linked items), I'm a bit worried about including dial attempts in a readiness check, but given that was the initial implementation, I'm inclined to perserve it for now. The liveness probe, I think, should also return to the original implementation where it's mostly a process health check. |
|
@hessjcg also we have some example settings here: https://github.com/GoogleCloudPlatform/cloudsql-proxy/blob/main/examples/k8s-health-check/proxy_with_http_health_check.yaml |
cmd/root.go
Outdated
|
|
||
| // Start the HTTP server is either Prometheus or the health check was | ||
| // enabled. | ||
| if cmd.prometheusNamespace != "" || cmd.healthCheck { |
There was a problem hiding this comment.
maybe just add "useHttpServe" var somewhere? there maybe be more things in the future, it'll be easier to just set a variable to true when each flag is called rather than update the if statement here
There was a problem hiding this comment.
Since we have to take different actions for prometheus vs health check, we do need two separate values. Nonetheless, looking again at prometheus, I see the namespace is optional, so I'll update the flag to be a boolean (and add a namespace flag separately), which cleans this up slightly.
There was a problem hiding this comment.
On second thought, I'll make this change as a separate PR to keep things focused here.
There was a problem hiding this comment.
So what I was trying to suggest was that the need for an http server is probably orthogonal to which specific httpPorts have been specified.
So something like this:
if cmd.prometheusNamespace != "" {
cmd.needHttpServer = true
// other health check stuff
}
if cmd.healthCheck {
cmd.needHttpServer = true
// other health check stuff
}
if cmd.needHttpServer {
// start http server
}(Consider this a nit)
cmd/root.go
Outdated
| select { | ||
| case <-ctx.Done(): | ||
| // Give the HTTP server a second to shutdown cleanly. | ||
| ctx2, _ := context.WithTimeout(context.Background(), time.Second) |
There was a problem hiding this comment.
any reason not to call defer cancel here?
internal/healthcheck/healthcheck.go
Outdated
| } | ||
|
|
||
| if open, max := c.proxy.ConnCount(); max > 0 && open == max { | ||
| c.logger.Errorf("[Health Check] Readiness failed: %v", errMaxConnections) |
There was a problem hiding this comment.
nit: consider reporting the max number of connections already made
There was a problem hiding this comment.
I'll update the error to capture that information.
|
I had to rebase v2 onto main with this PR open. As a result, there are a lot of commits here that already exist on v2. But the diff is correct and will cleanly apply onto v2 now. Update: I rebased onto v2 and removed the merge commit to get a clean result. |
Co-authored-by: Kurtis Van Gent <[email protected]>
Co-authored-by: Kurtis Van Gent <[email protected]>
Co-authored-by: Kurtis Van Gent <[email protected]>
cmd/root.go
Outdated
|
|
||
| // Start the HTTP server is either Prometheus or the health check was | ||
| // enabled. | ||
| if cmd.prometheusNamespace != "" || cmd.healthCheck { |
There was a problem hiding this comment.
So what I was trying to suggest was that the need for an http server is probably orthogonal to which specific httpPorts have been specified.
So something like this:
if cmd.prometheusNamespace != "" {
cmd.needHttpServer = true
// other health check stuff
}
if cmd.healthCheck {
cmd.needHttpServer = true
// other health check stuff
}
if cmd.needHttpServer {
// start http server
}(Consider this a nit)
This is an adaptation of GoogleCloudPlatform/cloud-sql-proxy#1271.
This is an adaptation of GoogleCloudPlatform/cloud-sql-proxy#1271.
No description provided.