-
Notifications
You must be signed in to change notification settings - Fork 2
FCP-1434: Add resource-level wildcard support for snapshot cache #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
zhiyanfoo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM mostly some nits
| // cacheVersion is the version of the cache at the time of last update, used in sotw. | ||
| cacheVersion string | ||
|
|
||
| // onDemandOnly indicates if this resource is only sent when explicitly requested (ODCDS support). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove reference to the ODCDS part. As Valerian mentioned this could apply to Listeners as well.
| // onDemandOnly indicates if this resource is only sent when explicitly requested (ODCDS support). | |
| // onDemandOnly indicates if this resource is only sent when explicitly requested: for example when performing ODCDS (on-demand cluster discovery). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair - Making it more use case agnostic is probably better for when we upstream this.
| return func(r *CachedResource) { r.ttl = ttl } | ||
| } | ||
|
|
||
| // OnDemandOnly marks the resource as on-demand only (for ODCDS support). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // OnDemandOnly marks the resource as on-demand only (for ODCDS support). | |
| // OnDemandOnly marks the resource as on-demand only |
pkg/cache/v3/simple.go
Outdated
| resourcesToReturn = make(map[string]*internal.CachedResource, len(resources)) | ||
| for _, res := range resources { | ||
| addResource(res) | ||
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) | |
| // Include wildcard-eligible resources for wildcard subscriptions |
pkg/cache/v3/simple.go
Outdated
| _, known := knownResources[name] | ||
| if known { | ||
| for name, res := range resources { | ||
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) | |
| // Include wildcard-eligible resources for wildcard subscriptions |
| } | ||
|
|
||
| if watch.subscription.IsWildcard() { | ||
| if watch.subscription.IsWildcard() && !res.OnDemandOnly() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if watch.subscription.IsWildcard() && !res.OnDemandOnly() { | |
| // the only case where we can keep a resource if it's a not explicitly subscribed is when we are a wildcard subscription and the resource is also a wildcard resource. | |
| if watch.subscription.IsWildcard() && !res.OnDemandOnly() { |
pkg/cache/v3/simple.go
Outdated
| for _, res := range resources { | ||
| if err := addIfChanged(res); err != nil { | ||
| return nil, fmt.Errorf("failed to compute resource version for %s: %w", res.Name, err) | ||
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) | |
| // Include wildcard-eligible resources for wildcard subscriptions |
pkg/cache/v3/simple.go
Outdated
| for name, resource := range resources { | ||
| resourcesToReturn = append(resourcesToReturn, resource) | ||
| returnedResources[name] = version | ||
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Include wildcard-eligible resources for wildcard subscriptions (ODCDS support) | |
| // Include wildcard-eligible resources for wildcard subscriptions |
pkg/cache/v3/simple.go
Outdated
| continue | ||
| } | ||
|
|
||
| if watch.subscription.IsWildcard() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do the same pattern here as for the delta wildcard requests
| if watch.subscription.IsWildcard() { | |
| // the only case where we can keep a resource if it's a not explicitly subscribed is when we are a wildcard subscription and the resource is also a wildcard resource. | |
| if watch.subscription.IsWildcard() && !res.OnDemandOnly() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, updating
801d766 to
988f432
Compare
|
@mohit-pal-kohli can you rebase the PR on the upstream branches and confirm when tested? |
Part 1 of implementing explicit wildcard support for xds-control-plane.
Context
We leverage the Snapshot Cache in go-control-plane, where delta for a node ID snapshot is proliferated to Envoys with that node ID when there is a change. This is acceptable in the current state where Fabric Proxy declares the destinations (clusters) it needs at start time in YAML.
The introduction of ODCDS makes the current state unpalatable, even though it is functional. The issue is that we do not want a mass proliferation of new clusters to all Envoys with node ID X when one pod with that node ID X requests a resource. Not solving this could create significant load on our control plane, especially for Fabric Proxy users with hundreds or thousands of destinations.
To solve this, we should add support for resource-level wildcard support for control plane. By default, a snapshot resource will be considered wildcard (OnDemandOnly = false) and proliferated to all Envoys with the relevant node ID. By contrast, we will allow certain resources to not be wildcard, allowing ODCDS clusters to only be proliferated on demand.
Implementation details
This PR aims to make changes in a way that are fully backward compatible for Delta and SoTW. Users using the deprecated cache will have all resources default to wildcard-eligible. Users using the new cache will be able to specify
OnDemandOnly. If not specified, thenOnDemandOnly=false, meaning that the resource is wildcard-eligible.