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

Multiple services check support #92

Open
jvmlet opened this issue Nov 4, 2021 · 9 comments
Open

Multiple services check support #92

jvmlet opened this issue Nov 4, 2021 · 9 comments

Comments

@jvmlet
Copy link

jvmlet commented Nov 4, 2021

like -service=service1,service2
Another option is to support -discoverServices flag that will use reflection API to discover services and then runs check for each discovered service.

@ahmetb
Copy link
Collaborator

ahmetb commented Nov 4, 2021

The Check(serviceName) is a standard RPC defined by gRPC. We just mirror that in this tool and so far this never came up, that's why I am inclined to wait for more priority this.

In the meantime, you can use GNU Parallel or a similar bash script equivalent to achieve the same task?

@jvmlet
Copy link
Author

jvmlet commented Nov 5, 2021

I understand that grpc check supports single service check. My proposition is to loop over passed/discovered services names and call the check for each service.

@ahmetb
Copy link
Collaborator

ahmetb commented Nov 9, 2021

I am inclined to say that you can probably do that loop yourself in a little wrapper script.

It's mostly a philosophical question as to whether CLI tools should support multiple arguments instead of making the caller write that loop. Some tools like curl, wc, ... accept multiple arguments and do the same task on them.

One can also make an argument that since you control the implementation of the HealthService.Check rpc, you know which services exist in that server, so you can also loop over them. Furthermore, you can perhaps even control the concurrency around how multiple services are checked (perhaps in parallel). It is really up to you how you want to interpret the HealthCheckRequest.service field.

I'll keep this issue open, but I highly doubt we'd implement something that discovers services and loop over them.

@jvmlet
Copy link
Author

jvmlet commented Nov 9, 2021

I'll keep this issue open, but I highly doubt we'd implement something that discovers services and loop over them.

No need to implement something that discovers services. Its already implemented, you just need to make standard grpc reflection call, same way you call health check.
https://github.com/grpc/grpc/blob/master/src/proto/grpc/reflection/v1alpha/reflection.proto

@ahmetb
Copy link
Collaborator

ahmetb commented Nov 9, 2021

I think adding something like this requires more thought. Not all gRPC servers offer reflection (it's a minority that I'd say allows reflection API in practice), furthermore even the HealthService itself is included in the reflection. Furthermore, I am not sure why this can't be implemented in the server by leaving the service="" empty (indicating all services registered), as the server code has access to the service list (without needing reflection). That's why I am not fully convinced that this is a common scenario that people need and cannot achieve without this tool supporting it.

@jvmlet
Copy link
Author

jvmlet commented Nov 9, 2021

If user passes -discoverServices then he probably understands what he is doing and he has reflection enabled.
It's not hard to exclude reflection service by its name from the list of discovered services, as well as health check service.
Implementing service check one by one is easier, if you have a look at default implementation of grpc health manager service in grpc-java, for example.

@abscondment
Copy link

abscondment commented Sep 7, 2022

@ahmetb wrote:

I am inclined to say that you can probably do that loop yourself in a little wrapper script.

If you're shipping your application in a shell-free image like gcr/distroless, the typical for/in/do/done shell scripts won't work -- there is literally no sh to interpret them. It'd be much, much easier if the probe binary accepted multiple service names.

So, my two cents: -service=service1,service2 would be very valuable. -discoverServices would not.

@ahmetb
Copy link
Collaborator

ahmetb commented Sep 7, 2022

If you're using Kubernetes, I recommend giving this feedback to the builtin grpc health probe in Kubernetes. This project is largely in maintenance mode at this point.

@abscondment
Copy link

We're not on 1.23 yet, so we're stuck with maintenance mode :)

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

3 participants