[Bugfix][Router] service_discovery: add has_ever_seen_model to K8sServiceNameServiceDiscovery

[saikpr]

Summary

• Adds has_ever_seen_model hook to K8sServiceNameServiceDiscovery, tracked from each Service's spec.selector["model"] label at watch time — independent of pod readiness.
• Lets request.py distinguish a configured-but-scaled-to-zero model (503, retriable) from a bogus model name (404, terminal).
• Mirrors the hook already implemented on StaticServiceDiscovery (fix(service_discovery): correctly return 503 on missing endpoints #889) and K8sPodIPServiceDiscovery.

Motivation

When KEDA scales a Deployment to zero on idle, the Service remains but no pod ever answers a /v1/models probe, so the router never registers the model. Any client request for that model hits request.py's 404 branch, and the OpenAI SDK treats 404 as terminal (no retry) — masking KEDA's scale-from-zero. The has_ever_seen_model hook (already consumed by request.py) converts the response to 503, and clients retry normally.

Test plan

• Unit test: service ADDED with selector.model=X → has_ever_seen_model("X") == True even with no Ready pod.
• Unit test: service DELETED → has_ever_seen_model returns False only after the last Service referencing that label is removed.
• E2E: scale backing Deployment to zero with KEDA; request to /v1/chat/completions for that model returns 503 (not 404).

Related: #889

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to track models even when their backing deployments are scaled to zero by monitoring Kubernetes Service selectors. It adds state management for known models and a helper method to check if a model has been previously observed. Feedback focuses on performance and correctness: a synchronous Kubernetes API call in the watcher path should be removed to avoid blocking the thread, and the tracking logic needs to be updated to correctly handle the removal of stale model entries when a service is updated to point to a different model.

[gemini-code-assist]

This synchronous K8s API call is performed on every MODIFIED event when the service is not ready (since model_label is passed as None from _watch_engines). This can block the watcher thread and significantly degrade performance. It is recommended to extract the model label from the service object directly in _watch_engines and pass it to _on_engine_update regardless of readiness, which would eliminate the need for this fallback API call.

[gemini-code-assist]

The current implementation of _track_known_model does not clean up old model labels when a service is updated to point to a new model. This leads to stale entries remaining in known_models even if no other service references them. The suggested change handles the update case by checking if the old label is still in use before discarding it.

    def _track_known_model(
        self,
        engine_name: str,
        event: str,
        model_label: Optional[str],
    ) -> None:
        """Record every model observed via Service spec.selector["model"].

        Ensures has_ever_seen_model() returns True even when the backing
        Deployment is scaled to zero (no Ready pod -> no /v1/models probe
        -> never registered by _on_engine_update without this hook).
        """
        if event == "DELETED":
            with self.known_models_lock:
                label = self._service_to_model.pop(engine_name, None)
                if label is not None and not any(
                    v == label for v in self._service_to_model.values()
                ):
                    self.known_models.discard(label)
            return

        label = model_label
        if label is None:
            try:
                svc = self.k8s_api.read_namespaced_service(engine_name, self.namespace)
                label = svc.spec.selector.get("model") if svc.spec.selector else None
            except Exception:
                label = None

        if not label:
            return

        with self.known_models_lock:
            old_label = self._service_to_model.get(engine_name)
            if old_label == label:
                return

            self._service_to_model[engine_name] = label
            self.known_models.add(label)

            # Clean up old label if it's no longer used by any service
            if old_label is not None and not any(
                v == old_label for v in self._service_to_model.values()
            ):
                self.known_models.discard(old_label)

[saikpr]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to track models discovered via Kubernetes Service labels, ensuring they remain 'known' even when deployments are scaled to zero. It adds state management and helper methods to the K8sServiceNameServiceDiscovery class, along with corresponding unit tests. Feedback identifies a logic error in the _track_known_model method where removing a model label during a MODIFIED event fails to clean up the old label, potentially leading to stale data. A refactoring is suggested to handle event types more robustly and improve efficiency.

[gemini-code-assist]

The current implementation of _track_known_model contains a logic error when a service's model label is removed (i.e., the label is deleted from the Service spec) during a MODIFIED event. The early return at line 1304 prevents the cleanup of the stale old_label from self._service_to_model and self.known_models, leading to incorrect tracking and a potential memory leak of stale mappings.

Additionally, the logic can be simplified and unified for both DELETED and MODIFIED events. Note that the check old_label not in self._service_to_model.values() is an $O(N)$ operation performed while holding a lock. While $N$ is typically small for Kubernetes services in a single namespace, using a reference counting dictionary would be more efficient for larger deployments.

            old_label = self._service_to_model.get(engine_name)
            new_label = model_label if event != "DELETED" else None

            if old_label == new_label:
                return

            if new_label:
                self._service_to_model[engine_name] = new_label
                self.known_models.add(new_label)
            else:
                self._service_to_model.pop(engine_name, None)

            if old_label is not None and old_label not in self._service_to_model.values():
                self.known_models.discard(old_label)

[saikpr]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements model tracking in K8sServiceNameServiceDiscovery to support scale-from-zero by monitoring Kubernetes service labels. It includes new unit tests for event handling and reference counting. Feedback highlights the need to track actual model names from engines for consistency and suggests optimizing the reference counting logic to O(1) to avoid performance issues during service updates.

[gemini-code-assist]

This implementation tracks the model_label in known_models to support scale-from-zero, but it misses the actual model_names returned by the engine (such as adapters or models with IDs different from the label). To maintain consistency with K8sPodIPServiceDiscovery and ensure that all discovered models are recognized by has_ever_seen_model, you should also update known_models with the actual model_names when the service is ready.

Suggested change

	self._track_known_model(engine_name, event, model_label)
	self._track_known_model(engine_name, event, model_label)
	if event in ("ADDED", "MODIFIED") and is_service_ready and model_names:
	# Track all models we've ever seen
	with self.known_models_lock:
	self.known_models.update(model_names)

[gemini-code-assist]

The check old_label not in self._service_to_model.values() has $O(N)$ time complexity, where $N$ is the number of services. While $N$ is typically small for Kubernetes services in this context, this check is performed inside a lock on every service update or deletion event. Consider maintaining a reference count for model labels (e.g., using a dictionary to track how many services reference each label) to make this check $O(1)$.