-
Notifications
You must be signed in to change notification settings - Fork 513
[Bugfix][Router] Reconcile the engine list when the K8s watch reconnects #1092
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -668,9 +668,35 @@ def _get_model_label(self, pod) -> Optional[str]: | |
| return None | ||
| return pod.metadata.labels.get("model") | ||
|
|
||
| def _reconcile_engines(self): | ||
| """ | ||
| Drop engines whose pod is gone from the cluster. | ||
|
|
||
| The watch stream is the only source of removals, so a DELETED event | ||
| that lands while the connection is down is lost for good and the | ||
| engine keeps receiving traffic. Listing on every (re)connection | ||
| closes that window: the watch keeps handling the steady state, the | ||
| list repairs whatever it missed. | ||
| """ | ||
| pods = self.k8s_api.list_namespaced_pod( | ||
| namespace=self.namespace, | ||
| label_selector=self.label_selector, | ||
| ) | ||
| live_pods = {pod.metadata.name for pod in pods.items} | ||
|
|
||
| with self.available_engines_lock: | ||
| stale = set(self.available_engines) - live_pods | ||
| for engine_name in stale: | ||
| logger.warning( | ||
| f"Serving engine {engine_name} no longer exists but was " | ||
| f"still registered: dropping it" | ||
| ) | ||
| del self.available_engines[engine_name] | ||
|
|
||
| def _watch_engines(self): | ||
| while self.running: | ||
| try: | ||
| self._reconcile_engines() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| for event in self.k8s_watcher.stream( | ||
| self.k8s_api.list_namespaced_pod, | ||
| namespace=self.namespace, | ||
|
|
||
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.
There is a subtle race condition between
_reconcile_engines()andself.k8s_watcher.stream(). Because they are two separate API calls (list_namespaced_podin_reconcile_enginesand anotherlist_namespaced_podinternally insideself.k8s_watcher.stream), if a pod is deleted in the short window between these two calls:\n\n1._reconcile_engineswill see the pod as alive (since it was alive during the first list call) and won't remove it.\n2.self.k8s_watcher.streamwill start its watch after the pod is already deleted, so it won't see the pod in its initial list and won't receive aDELETEDevent for it.\n\nAs a result, the deleted pod will remain inself.available_enginesas a ghost pod indefinitely until the next reconnect.\n\nTo completely eliminate this race condition and also avoid redundant HTTP requests to all pods on every reconnect (since the watch stream withoutresource_versionlists all pods and triggersADDEDevents for all of them), you can:\n1. Perform the list call once in_reconcile_enginesand return both the live pods and theresource_version(pods.metadata.resource_version).\n2. Populate/reconcileself.available_enginesusing that list.\n3. Start the watch stream using thatresource_versionso it only streams subsequent events.