Repository navigation
Conversation
Merged
1 of 4 tasks
jenshu
approved these changes
Feb 10, 2026
Author
|
Talked to Yuval and since this is not critical for the fix, prefer not to touch solo-kit. So, closing this PR. |
chandler-solo
added a commit
that referenced
this pull request
Sep 3, 2026
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>
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.
This is a PR to illustrate the problem. Once we agree on the proper fix, I will clean this up with proper changelog.