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

Feature/SK-658 | Add gRPC healthchecking #515

Merged
merged 10 commits into from
Feb 5, 2024
Merged

Feature/SK-658 | Add gRPC healthchecking #515

merged 10 commits into from
Feb 5, 2024

Conversation

ahellander
Copy link
Member

@ahellander ahellander commented Feb 3, 2024

Uploading Screenshot 2024-02-03 at 14.54.39.png…

Description

This PR introduces the standardized gRPC HealthChecking protocol for the combiner server.

  • Add HealthCheck servicer
  • Add grpc_health_probe to Docker container image
  • Healthcheck on combiner service in docker-compose
  • Ensures that combiner is ready before clients start

This fixes the issue that intermittently when spinning up the pseudo-distributed sandbox with docker-compose ,the clients fail with UnMatchedConfig error due to the combiner not being ready yet.

We can also now use this for Kubernetes: https://kubernetes.io/blog/2018/10/01/health-checking-grpc-servers-on-kubernetes/

@Wrede Wrede changed the title Feature/sk 658 Feature/SK-658 | Add gRPC healthchecking Feb 5, 2024
Copy link
Collaborator

@Wrede Wrede left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! but there are something that need to be considered...

@@ -31,7 +31,8 @@
"plotly",
"pandas",
"bokeh<3.0.0",
"networkx"
"networkx",
"grpcio-health-checking"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this must be pinned, since grpcio is pinned at v1.57

Dockerfile Outdated
@@ -13,7 +13,13 @@ COPY config/settings-reducer.yaml.template /app/config/settings-reducer.yaml
COPY $REQUIREMENTS /app/config/requirements.txt

# Install developer tools (needed for psutil)
RUN apt-get update && apt-get install -y python3-dev gcc
RUN apt-get update && apt-get install -y python3-dev gcc wget
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

installing binary wget not good, remove executable after it has been used. (we should have a build layer in the image really)

Dockerfile Outdated
RUN apt-get update && apt-get install -y python3-dev gcc wget

# Install grpc health probe checker
RUN GRPC_HEALTH_PROBE_VERSION=v0.4.24 && \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but we won't need this binary for k8s. It's good for compose but other than that it just add more maintenance. For example, this version maybe need to be in sync with grpcio version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This image is for docker-compose not k8s?

@Wrede
Copy link
Collaborator

Wrede commented Feb 5, 2024

one possible solution is to introduce an ARG health_checking=0 in the dockerfile. Then download probe only if health_checking=1. Then we set this arg in compose file to be 1. I.e by default it will not be in the image, but the healthcheck endpoint will still be there for clients to use in what ever way they desire.

@ahellander ahellander requested a review from Wrede February 5, 2024 11:37
@Wrede Wrede merged commit e3c880c into master Feb 5, 2024
15 checks passed
@Wrede Wrede deleted the feature/SK-658 branch February 5, 2024 13:34
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.

None yet

2 participants