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

Support the GRPC Health protocol #1980

Closed
poolski opened this issue Jan 10, 2020 · 5 comments · Fixed by #2008
Closed

Support the GRPC Health protocol #1980

poolski opened this issue Jan 10, 2020 · 5 comments · Fixed by #2008

Comments

@poolski
Copy link

poolski commented Jan 10, 2020

While trying to implement service discovery for Thanos components using Consul, we've run into an issue where we're unable to healthcheck Thanos using gRPC because it doesn't support the standard gRPC health protocol. (grpc.health.v1.Health) which would allow load balancers and the like to be able to run gRPC healthchecks successfully.

Is this something which is on the radar for future releases?

@kakkoyun
Copy link
Member

Hey, I have worked on HTTP health checks, happy to work on this as well.
As far as I can tell from the documentation, it's not very hard to achieve.

https://github.com/grpc/grpc/blob/master/doc/health-checking.md
https://github.com/grpc/grpc/blob/master/src/proto/grpc/health/v1/health.proto
https://godoc.org/google.golang.org/grpc/health/grpc_health_v1

Maintainers can assign this one to me if they don't have any objection.

@bwplotka
Copy link
Member

It's the first time we hear such use case (: But we can definitely discuss how we can unblock you.

@kakkoyun well let's first decide if it's required (:

Some initial thoughts:

  • We hold our healthchecks via HTTP endpoints like /~/healthy as standard way Kubernetes expects
  • Querier checks health via Info gRPC method since this gives up-to-date metadata.

None of this is sufficient for your case @poolski ?

@poolski
Copy link
Author

poolski commented Jan 13, 2020

@bwplotka @kakkoyun, thank you for replying! 🙇
Sorry, I've been away from work over the weekend

We've found that tools like Envoy and Consul which support gRPC proxying and service discovery (respectively) rely on the grpc.health.v1.Health endpoint being available as most gRPC services can be reasonably expected to implement that endpoint.

I can see the benefit of using Info for more detailed service information, but this would require additional configuration on the downstream end (Envoy/Consul/), which is sometimes not supported or is tricky to implement.

Using a standard health gRPC healthcheck endpoint can mean that folks can test the availability of the Thanos service much easier, since the service health information resides at a predictable location.

I understand that you don't natively integrate with Consul as a service discovery mechanism per se at present, which makes sense, but given that Consul can treat Thanos as a generic gRPC service without any special requirements, having the Health endpoint available seems like an easy win.

@poolski
Copy link
Author

poolski commented Jan 16, 2020

Hey folks,

Any thoughts so far? :)

@kakkoyun
Copy link
Member

@poolski I went ahead and have created a PR #2008 for this.
If maintainers decide to go ahead, it could be reviewed and merged.

@bwplotka @metalmatze @squat

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

Successfully merging a pull request may close this issue.

4 participants