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

Return 200 on / when ready #527

Closed
wants to merge 1 commit into from
Closed

Conversation

Legogris
Copy link
Contributor

This is for playing nicely with Google's Load balancer, which requires that pods respond with 200 on /. Reuses the /ready response with 200 instead of 204 on ready.

@CLAassistant
Copy link

CLAassistant commented Apr 29, 2019

CLA assistant check
All committers have signed the CLA.

@steven-sheehy
Copy link
Contributor

You need to sign the CLA.

Also, I don't use GCP but just glancing at the docs it seems like you can specify a --request-path for the health check?

@Legogris
Copy link
Contributor Author

Legogris commented Apr 30, 2019

Also, I don't use GCP but just glancing at the docs it seems like you can specify a --request-path for the health check?

Indeed. However, when using GKE Ingresses with Google's Loadbalancer, health checks will be automatically created (see https://cloud.google.com/kubernetes-engine/docs/concepts/ingress#health_checks and https://cloud.google.com/kubernetes-engine/docs/tutorials/http-balancer#remarks).
If user overrides this (by editing the backend/health check), it will work but be overriden by GCP automatically (for example I had this happen when a pod was replaced due to OoM).

Actually overriding the default / health check is pretty finicky and failprone (though possible) and expects a 200 (just verified it failing on 204). This is different from the k8s readiness/liveness checks that seem to be happy with a 204.

I see two alternatives to this:

  1. Make /ready return 200 instead of 204
  2. Change the probes in the provided helm charts to use /metrics as url instead of /ready

In general it will make things a lot less more straightforward for GKE users if / returns 200, or at least if the default readiness and liveness probes do.

@steven-sheehy
Copy link
Contributor

If you're using the helm chart, you can already override the health check to use /metrics in your custom values.yaml. I think making /ready return 200 sounds reasonable, just return some success message in the content as well.

@Legogris
Copy link
Contributor Author

Legogris commented Apr 30, 2019

If you're using the helm chart, you can already override the health check to use /metrics in your custom values.yaml. I think making /ready return 200 sounds reasonable, just return some success message in the content as well.

Yeah, that's what we ended up doing to make it work. How about /ready returning 200 with the server timestamp as content perhaps?

Is there any particular reason against deciding against 200 for /? I am certain that there are other users who would appreciate that.

@Legogris Legogris closed this Apr 30, 2019
@steven-sheehy
Copy link
Contributor

I think a string constant "Health check successful" or similar would be fine.

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

Successfully merging this pull request may close these issues.

3 participants