[Router][Bugfix] reset K8s watcher on stale resource version errors - #999
Conversation
5f3ae72 to
ec1210c
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces recovery logic for Kubernetes service discovery watchers when they encounter stale resource version errors. It adds helper functions to detect stale resource versions via HTTP status codes (410, 504) or specific error message phrases, and resets the watcher accordingly. Unit tests are also added to verify this recovery behavior. The review feedback points out that unconditionally treating all 504 Gateway Timeout errors as stale is risky and could overload the API server; it suggests removing 504 from the unconditional status check and relying on phrase matching instead, along with a corresponding update to the test suite.
| # HTTP status codes that indicate the watch bookmark is no longer in the API | ||
| # server's watch cache and the client must list the resource again. | ||
| _STALE_RESOURCE_VERSION_STATUSES: Tuple[int, ...] = (410, 504) | ||
|
|
||
|
|
||
| def _is_stale_resource_version_error(exc: Exception) -> bool: | ||
| """Return True if exc indicates the K8s watch bookmark is stale.""" | ||
| status = getattr(exc, "status", None) | ||
| if isinstance(status, int) and status in _STALE_RESOURCE_VERSION_STATUSES: | ||
| return True | ||
| message = str(exc).lower() | ||
| return any(phrase in message for phrase in _STALE_RESOURCE_VERSION_PHRASES) |
There was a problem hiding this comment.
Unconditionally treating all 504 Gateway Timeout errors as stale resource version errors is risky. A 504 is a generic HTTP status code that can be returned by any intermediate proxy, load balancer, or the Kubernetes API server itself when a request times out (e.g., due to high load or network issues).
If the API server is already struggling and timing out, resetting the watcher and performing a full LIST request on every generic 504 timeout can cause a 'thundering herd' effect, severely overloading the API server further.
Instead, we should only treat 410 Gone as unconditionally stale (since it always indicates a stale resource version), and rely on the phrase-matching logic for 504 errors to ensure we only reset when the error message explicitly mentions a stale or too large resource version.
| # HTTP status codes that indicate the watch bookmark is no longer in the API | |
| # server's watch cache and the client must list the resource again. | |
| _STALE_RESOURCE_VERSION_STATUSES: Tuple[int, ...] = (410, 504) | |
| def _is_stale_resource_version_error(exc: Exception) -> bool: | |
| """Return True if exc indicates the K8s watch bookmark is stale.""" | |
| status = getattr(exc, "status", None) | |
| if isinstance(status, int) and status in _STALE_RESOURCE_VERSION_STATUSES: | |
| return True | |
| message = str(exc).lower() | |
| return any(phrase in message for phrase in _STALE_RESOURCE_VERSION_PHRASES) | |
| # HTTP status codes that indicate the watch bookmark is no longer in the API | |
| # server's watch cache and the client must list the resource again. | |
| _STALE_RESOURCE_VERSION_STATUSES: Tuple[int, ...] = (410,) | |
| def _is_stale_resource_version_error(exc: Exception) -> bool: | |
| """Return True if exc indicates the K8s watch bookmark is stale.""" | |
| status = getattr(exc, "status", None) | |
| if isinstance(status, int) and status in _STALE_RESOURCE_VERSION_STATUSES: | |
| return True | |
| message = str(exc).lower() | |
| return any(phrase in message for phrase in _STALE_RESOURCE_VERSION_PHRASES) |
| assert _is_stale_resource_version_error(Exception(message)) is expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("status", [410, 504]) |
There was a problem hiding this comment.
Update the test to only parameterize over 410 for unconditional status-based stale detection, since 504 is no longer unconditionally treated as stale without matching the error message phrases.
| @pytest.mark.parametrize("status", [410, 504]) | |
| @pytest.mark.parametrize("status", [410]) |
loicrouillermonay
changed the title
ec1210c to
273e803
Compare
273e803 to
c19e4a7
Compare
|
I rebased my branch with master, now PR workflows requires approval from a maintainer 😉 |
|
This PR is important enough since it might affect absolutely everyone using the vLLM Router when the API servers are not in sync with the latest version of ETCD. I cannot speak to the quality of the implementation, but it will definitely improve the router's stability in production workloads where state drift occurs regularly. |
|
Head branch was pushed to by a user without write access
632604f to
25d1efd
Compare
|
Hi Could you try the PR #1013 and see if this fixed the problem? |
|
Will do |
Add detection for 410/504 ApiException status codes and stale-resource-version phrases. When detected, replace the Watch() instance so the next stream() performs a fresh LIST and resumes from the current resourceVersion. Signed-off-by: Loïc Rouiller-Monay <loic.rouiller-monay@exoscale.ch>
Use ApiException.status and _STALE_RESOURCE_VERSION_PHRASES from k8s apiserver Go sources
25d1efd to
d02c8c0
Compare
3 participants
Getting
504 Timeout: Too large resource versionlooping in the router logs. vLLM pods were healthy, but the K8s watcher was stuck.The bug: both
K8sPodIPServiceDiscoveryandK8sServiceNameServiceDiscoverycreate onekubernetes.watch.Watchinstance and never replace it. The Python client stores the lastresourceVersionit saw and feeds it back into every reconnect. Once the API server's watch cache moves past that version, the server throws the504(or410 Gone) and the router catches it, sleeps 0.5s then callsstream()again with the same stale bookmark forever.The Kubernetes Python client docs call this out explicitly in
Watch.stream()here:So the fix is to reset the watcher when we hit a stale resource version. A new
Watch()starts withresource_version=None, which forces a freshLISTon the next attempt. Then we grab the current bookmark and resume. I only do this for stale-version errors.Here is below an anonymized snippet so you can see the watcher logs
504with a stale resource version over and over while the router reports 0 serving engine(s).