[sotw][linear] Fix missing watch cleanup in linear cache for sotw watches subscribing to multiple resources #854
Conversation
…ches subscribing to multiple resources Properly return the request in sotw responses to allow proper handling in callbacks Signed-off-by: Valerian Roche <valerian.roche@datadoghq.com>
| @@ -140,18 +140,18 @@ func (cache *LinearCache) respond(value chan Response, staleResources []string) | |||
|
|
|||
| func (cache *LinearCache) notifyAll(modified map[string]struct{}) { | |||
| // de-duplicate watches that need to be responded | |||
There was a problem hiding this comment.
This comment threw me off a bit when reading the code. It's not de-duplicating iiuc (there are no duplicates), it is constructing a mapping of watches -> list of resources names, no?
For linear cache/SOTW, here the list of strings in the mapping should simply be request.ResourceNames, no? Maybe it'd be clearer to just use this, since you have the request at heand in the ResponseWatch?
There was a problem hiding this comment.
It is deduplicating through the map to send a simple reply with n resources rather than n replies with one
On whether to use request.ResourceNames, this would yield a different behavior as:
- this is not normalized for wildcard parts for instance
- can target resources which don't exist in the cache, and the respond implementation might not expect that
- for eds/rds, we don't want to return all resources but only the ones matching modified
There was a problem hiding this comment.
I don't think it makes a difference in theory, the request contains all the resources you may need to respond to, so
- for CDS/LDS and wildcard you need to send all resources for this type
- for RDS/EDS/* you need the intersection of resource names in
modifiedand what is inrequest.ResourceNames
| } | ||
| for value := range cache.watchAll { | ||
| cache.respond(value, nil) | ||
| for watch := range cache.watchAll { |
There was a problem hiding this comment.
So watchAll are wildcards, right? I think it would be more clear to simply call that watchWildcards
There was a problem hiding this comment.
I can update that in a separate PR
It would be nice to have tracking issues for all those things that you know don't actually work. |
No issues for now. We need to review if we really want to support this as this would require a major rework of the mux cache
|
|
Thank you for creating the issues.
This should be documented alongside
|
Currently when using Linear users will set resources in the different caches at different times, so we can hardly take ordering decisions in watches as they'll trigger based on the user Update ordering. Using a Mux with multiple linear caches with envoy ADS will likely create issues if the user does not insert data in specific order, but if the order is respected it should work |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
When using the linear cache in sotw mode, there are multiple issues when using non-wildcard requests
This PR addresses two issues potentially impactful:
UpdateResourceinstead of usingUpdateResources). Any server implementation which would consider a watch properly cleaned if triggered (which is the initial design of the cache interface) would leak the channel and potentially trigger a deadlock later onOther issues are not yet tackled in this PR as they depend for some on changes in the cache interface proposed in other PRs
Other issues to be addressed in other PRs