feat: Add GRPC health check in suggestions #779
Conversation
Signed-off-by: Ce Gao <gaoce@caicloud.io>
gaocegege
changed the title
Signed-off-by: Ce Gao <gaoce@caicloud.io>
|
/assign @johnugeorge |
| }, | ||
| InitialDelaySeconds: defaultInitialDelaySeconds, | ||
| } | ||
| c.LivenessProbe = &corev1.Probe{ |
There was a problem hiding this comment.
Will liveness probe has any hidden consequences? Say if service is not accessible for some time
There was a problem hiding this comment.
Yeah, it will. But I think we should have it. Since we can mark the deployment unavailable then avoid timeout requests
|
|
||
| logger = logging.getLogger("HyperoptRandomService") | ||
|
|
||
|
|
||
| class HyperoptService( | ||
| api_pb2_grpc.SuggestionServicer): | ||
| api_pb2_grpc.SuggestionServicer, HealthServicer): |
There was a problem hiding this comment.
With this change, it is mandatory for all suggestion service to use HealthServices. Right?
There was a problem hiding this comment.
Yes, we need to add health servicer to all algorithms
There was a problem hiding this comment.
Sounds good. We should add them in the suggestion doc separately. Also, Dockerfile instructions to have graph-health-check binary also has to be documented.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gaocegege The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| RUN if [ "$(uname -m)" = "ppc64le" ]; then \ | ||
| apt-get -y update && \ | ||
| apt-get -y install gfortran libopenblas-dev liblapack-dev && \ | ||
| pip install cython; \ | ||
| fi | ||
| RUN GRPC_HEALTH_PROBE_VERSION=v0.3.0 && \ | ||
| wget -qO/bin/grpc_health_probe https://github.com/grpc-ecosystem/grpc-health-probe/releases/download/${GRPC_HEALTH_PROBE_VERSION}/grpc_health_probe-linux-amd64 && \ |
There was a problem hiding this comment.
Hi @gaocegege , I am wondering if you have tested the multi-arch binary file of the GRPC health probe, as your patch only wget the amd64 architecture.
There was a problem hiding this comment.
No, I have not tried to run on other architecture.
But we could support it since the probe can be built other platforms.
https://github.com/grpc-ecosystem/grpc-health-probe/releases
There was a problem hiding this comment.
Do you have some problem when you use it?
There was a problem hiding this comment.
Since I am working on the arm64 support of images, therefore I will propose a patch to fix this, thanks for noticing.
There was a problem hiding this comment.
well I didn't test but I am working on the arm64 image support, by going through the Dockerfile I noticed this. I will then try to take care of it :)
There was a problem hiding this comment.
Sure, thanks. It seems that we can support it with an arch check.
Signed-off-by: Ce Gao gaoce@caicloud.io
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Release note:
This change is