-
Notifications
You must be signed in to change notification settings - Fork 92
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
Feat: coordinator ha #842
base: main
Are you sure you want to change the base?
Feat: coordinator ha #842
Conversation
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.
I think you can keep using a normal service + deployment for this. The coordinator only cares that it can contact some running coordinator. If we change the seed node to the coordinator service instead of localhost and add some liveness/readiness/startup probes, it should be good enough.
The biggest open question for me is how bootstrap will work - what happens for the first seed node to start?
operations/k8s/helm/values.yaml
Outdated
@@ -30,9 +30,13 @@ api: | |||
|
|||
coordinator: | |||
enabled: true | |||
|
|||
name: coordinator |
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.
I would very strongly discourage this. It is a safe assumption that software like this is being added to its own namespace and threading the name through everything results in crazy amount of complexity. If you really want to, make sure to create a helper function to handle the name.
config.yaml: |- | ||
coordinator_addr: {{ $statefulsetName }}-0.{{ $statefulsetName }}.{{ $.Release.Namespace }}.svc.cluster.local:8950 | ||
raft_port: 8970 | ||
seed_node: {{ $statefulsetName }}-0.{{ $statefulsetName }}.{{ $.Release.Namespace }}.svc.cluster.local:8970 |
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.
We called this HA, but won't it fail in any scenario where 0
isn't running?
coordinator_addr: {{ $statefulsetName }}-0.{{ $statefulsetName }}.{{ $.Release.Namespace }}.svc.cluster.local:8950 | ||
raft_port: 8970 | ||
seed_node: {{ $statefulsetName }}-0.{{ $statefulsetName }}.{{ $.Release.Namespace }}.svc.cluster.local:8970 | ||
node_id: {{ $i }} |
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.
What do you think about adding --node-id
to the args for serve
? That way, we can have a single config map that gets updated/reloaded and doesn't require any generation like this.
Yes, I am planning to modify the chart to include readiness and liveness, among other several customizations that we are already using in our clusters. Readiness and Liveness are good for some sort of "HA" because it would replace coordinator if not healthy or present, but it still is not HA, that is why we thought of this alternative, although I agree probes should be fine for now. |
Enable High Availability (HA) in Kubernetes for Coordinator
This PR introduces changes to enable High Availability (HA) in Kubernetes by leveraging StatefulSets and headless services. The key goal is to allow identification of individual replicas and ensure each replica has its own service, which can be achieved by replacing the existing Deployment with a StatefulSet.
Key Changes:
node_id
s to each replica. The first replica (coordinator-0
) is designated as theseed_node
.clusterIP: None
), allowing direct communication with individual pods.app:
label, ensuring proper routing to the correct pods.Local Testing
The changes were tested on a local Kubernetes cluster with the following results:
coordinator
StatefulSet show that the coordinator service is running and the leader has been established.➜ TENSORLAKE_WORKSPACE kubectl logs statefulset/coordinator -n indexify Found 2 pods, using pod/coordinator-1 Running with tracing filter info 2024-08-18T22:08:45.246398Z INFO indexify::cmd::coordinator: starting indexify coordinator, version: git branch: main - sha:2608f783cc6f10e9d0e725bd4e0289781252b4f1 ... 2024-08-18T22:08:45.401516Z INFO indexify::coordinator_service: leader change detected: true
api
deployment indicate that the API server is running and successfully connecting to the coordinator service.NOTE: This was deployed with a new helm release, I removed the one that was using deployment and static configmpa.