-
Notifications
You must be signed in to change notification settings - Fork 27
feat: Implement optional health server #208
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
Conversation
|
Hi, we would really like this feature. Is there anything we can do to contribute more to this PR? Thanks in advance and kind regards, |
|
The CI failures due to an outdated actions/cache version may have been fixed by #199. Try to rebase your fork/branch on the latest in main and force push, then see if that helps the CI checks run again :) |
jbw976
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you also update the How has this code been tested with some more details? e.g. show some command outputs to show it working, how you checked, etc. 🙇♂️
Signed-off-by: Hannes Leutloff <[email protected]>
|
Thanks for taking a look! I rebased my branch and cleaned up the history. |
|
hi @jbw976, I edited the pull request description to include a working example of the health server in production, including a kubernetes manifest with a successful liveness probe. |
|
I just saw this PR. It seems useful. Do users ever need to configure the health server? I see for example https://pkg.go.dev/google.golang.org/grpc/health has Shutdown and Resume methods that I imagine the server might call to indicate health. I ask because if users would never have to configure the health server, perhaps it should just be silently on by default. Would that work? |
|
I get the thought, but unfortunately it would not work that easily. Users usually want to set the health information that the health server exposes. For example if I have to run some setup code before my server is ready, I first want the health server to report "unready" and after the setup I want it to report "serving". My example doesn't include this, since the HealthServer by default reports the status "serving". This could also be done by silently activating the health server and somehow exposing it, so that access to the HealthServer's methods is possible. But I found this solution to be unintrusive with only two lines of setup and also useful for customization. |
Description of your changes
This MR adds a ServeOption that lets library users provide a HealthServer which will then be served with the go composition function via a common gRPC server. This allows setting up a kubernetes readiness probe to check whether a go composition function is ready to accept gRPC connections.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
I have used this fork in our go function composition and confirmed that the passed in health server is correctly exposed via the gRPC server. I configured a kubernetes grpc liveness-probe, which was successfully called.
Since there are currently no integration or e2e tests in this project, I wasn't sure how/where to add a new test for this behavior.
I've used this minimal example for testing:
And deployed it to kubernetes with this manifest:
This results in a successful liveness probe.