Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing health check endpoint #124

Closed
tothandras opened this issue Sep 7, 2017 · 12 comments
Closed

Missing health check endpoint #124

tothandras opened this issue Sep 7, 2017 · 12 comments

Comments

@tothandras
Copy link

tothandras commented Sep 7, 2017

The proxy doesn't have a health check endpoint, which is very handy in a Kubernetes environment for readiness and liveness probes.

(I'm happy to contribute with a PR, when the details are discussed.)

@minrk
Copy link
Member

minrk commented Sep 8, 2017

PR sounds great! What should a health check look like?

I would put it at /api/health/

@tothandras
Copy link
Author

tothandras commented Sep 8, 2017

@minrk It has to be exposed on the proxy itself (proxy-public port), because some limitations of GCE Ingress. I would go with /healthz. The health-check handler could ping the db for example. It is also a problem that load balancing multiple proxy instances (to have HA) with memory store is not possible.

@minrk
Copy link
Member

minrk commented Sep 8, 2017

If it has to be on the public port, I think it has to be something highly unlikely for any proxied application to serve, such as /_chp_health.

@tothandras
Copy link
Author

tothandras commented Sep 9, 2017

I think /healthz is quite unlikely and common for health checks at the same time.

@rgbkrk
Copy link
Member

rgbkrk commented Sep 9, 2017

I'm a fan of the leading _ style.

@tothandras
Copy link
Author

I think we won't be able to agree on this detail, so let's make it configurable. 🙂

@willingc
Copy link
Contributor

Naming is hard 🤷‍♀️

Why don't we start with _healthz as the name? The rationale: healthz is used in K8 for probes and prefixing with _ satisfies IMHO @minrk's and @rgbkrk's preference for a something unlikely.

@rgbkrk
Copy link
Member

rgbkrk commented Sep 11, 2017

The rationale: healthz is used in K8 for probes

That does make it tempting to align with if it makes deployment easier. It does look like the value used for probes is configurable.

@minrk
Copy link
Member

minrk commented Sep 12, 2017

Why don't we start with _healthz as the name?

That sounds great! I still would prefer a CHP prefix, though, so maybe /_chp_healthz?

I think /healthz is quite unlikely and common for health checks at the same time.

If it's common, then it's likely to also be exposed by something behind CHP. For instance, if JupyterHub implemented this standard endpoint and deployed with CHP in a single container (the default behavior), this should be proxied to the Hub and not served by CHP itself.

Since CHP is a proxy, we have a special case because all URLs are in a shared namespace with proxied applications, so anything CHP-specific should be on a CHP-specific URL.

Let's go with non-configurable /_chp_healthz.

@yuvipanda
Copy link
Contributor

Can't we have the health check only on the api port and not the proxy port? I'd much prefer that than overloading the proxying port :)

This is what traefik does for example.

@minrk
Copy link
Member

minrk commented Sep 12, 2017

@yuvipanda if that's possible, let's definitely do that. I thought @tothandras said this wasn't possible, though.

@yuvipanda
Copy link
Contributor

Ah I see. I don't think kubernetes has that limitation - it's possible that GCE has it for their ingress, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants