Repository navigation
fix: keep xDS watches whose response has to be withheld - #592
Merged
Merged
Conversation
In ADS mode the snapshot cache deliberately does not answer a request whose resource names do not cover the snapshot's resources of that type: sending it would hand the client resources it has not subscribed to. The withheld response was then treated as a completed request by both call sites, and the watch was lost: - SetSnapshot called respond() and deleted the watch unconditionally, so a parked watch was discarded even though nothing had been sent to it. - CreateWatch called respond() for a request whose version was stale and returned a cancel function that only closes the channel, registering no watch at all. The comment above respond() explains the intent as "it is expected that envoy makes another request", but a client only issues a new request after it receives a response: Envoy re-subscribes EDS once it has applied a CDS update, and otherwise waits. So when the mismatch is caused by a resource the client will not subscribe to soon -- a cluster it has just rejected, or one it has not applied yet -- no new request arrives, the watch is gone, and every later snapshot has nothing to deliver to. The client keeps serving its last configuration until it reconnects, with no error surfaced anywhere: gateway proxies observed holding removed endpoints indefinitely after control-plane churn while readiness and endpoint health stayed green. This makes respond() report whether it sent a response, and fixes both paths: - SetSnapshot discards the watch only when a response was sent, and keeps it otherwise, so the next snapshot the request can accept is delivered to it. - CreateWatch falls through to opening a watch when the response is withheld, instead of returning a request that nothing will ever answer. Nothing changes when a response is sent, when the requested version is already up to date, or for wildcard requests, which are never withheld. PR #586 fixed the SetSnapshot path only. The request path is reachable in the same window -- a proxy that (re)subscribes while the snapshot is misaligned, which is exactly what happens during a rejected cluster update or a reconnect mid-churn -- so both are closed here. For reference, go-control-plane's equivalent rework (bf9b60b56, "[Snapshot Cache] Simplify Snapshot cache responses") also closed only the publish path and left the request path behind a TODO. Supersedes #586. Tests: five specs in pkg/api/v1/control-plane/cache covering retention on both paths, delivery once the snapshot realigns, immediate answers, an up-to-date request still parking, and wildcard behaviour unchanged. Signed-off-by: David L. Chandler <david.chandler@solo.io>
|
Issues linked to changelog: |
Signed-off-by: David L. Chandler <david.chandler@solo.io>
…86-but-better' into chandler/solo-kit-586-but-better
Signed-off-by: David L. Chandler <david.chandler@solo.io>
andy-fong
reviewed
Sep 17, 2026
| // RetainUnansweredXDSWatchesEnv controls retention of withheld ADS watches. | ||
| // "on" and "off" pin the behavior; "default", unset, and empty follow the | ||
| // release default. Invalid values warn and also follow the release default. | ||
| const RetainUnansweredXDSWatchesEnv = "SOLOKIT_RETAIN_UNANSWERED_XDS_WATCHES" |
There was a problem hiding this comment.
maybe just call this ENV "EXP_FIX", I know it's very generic but if we are doing similar in other area to gate risky fix, we can re-use this. The cons is of course all or nothing. I don't think we need to keep it for long. The fix itself looks good to me.
andy-fong
approved these changes
Sep 17, 2026
andy-fong
approved these changes
Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix both paths that lose an xDS watch when ADS withholds a response because the client has not subscribed to every resource in the snapshot:
SetSnapshotretains the unanswered watch.CreateWatchregisters a watch when its immediate response is withheld.Once a compatible snapshot arrives, the retained watch receives it.
The fixes ship disabled by default, controlled by
SOLOKIT_RETAIN_UNANSWERED_XDS_WATCHES:onoffdefault, empty, or unsetThe setting is read when each snapshot cache is created. The release default can change later; explicit
onandoffremain pinned.Supersedes #586 by covering both the snapshot-update and request paths.