xdsdepmgr: add functionality for cluster subscription #8792
Conversation
eshitachandwani
added
Type: Internal Cleanup
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8792 +/- ##
==========================================
- Coverage 83.31% 82.07% -1.24%
==========================================
Files 414 414
Lines 32753 32838 +85
==========================================
- Hits 27288 26952 -336
- Misses 4064 4120 +56
- Partials 1401 1766 +365
🚀 New features to boost your workflow:
|
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
Can you please make the PR description a bit more detailed to clearly indicate what is included in this PR and what will follow in subsequent ones.
| clusterResourcesSeen := make(map[string]bool) | ||
| haveAllResources := true | ||
|
|
||
| // Get all clusters to be watched: from route config and from cluster |
There was a problem hiding this comment.
Shouldn't we be treating the dynamic clusters differently here? i.e., if we haven't received an update for a dynamic cluster subscription, that should not cause an update to existing cluster from the route config to be blocked.
See https://github.com/grpc/proposal/blob/master/A74-xds-config-tears.md#changes-in-cds-lb-policy. Specifically, the following paragraph:
Note that when a cds LB policy obtains a dynamic subscription to a cluster, there may be subsequent updates from the xds resolver that do not yet include the newly subscribed-to cluster. Therefore, the cds policy will need to ignore updates in which the XdsConfig does not contain the cluster. However, this case should not occur with non-dynamic clusters, because the xds resolver ensures that those clusters are being subscribed to until they are removed from the LB policy config.
So, I think we should run the loop (currently starting in line 252) two times, one for clusters from route config and one for clusters from dynamic subscriptions. And in the second loop, we should not set haveAllResources to false if a dynamic cluster hasn't been resolved yet (and instead keep going).
And we need a test for this scenario as well.
There was a problem hiding this comment.
I dont' think it is necessary as only one out of WeightedCluster or ClusterSpecifierPlugin will be set as mentioned in our comments. The only way that we have clusters in both the arrays is in the case where we have cluster A referenced by RPCs and not in route(it will be in clusterSubscriptions map) and cluster B currently in routeconfig (it will be in clustersFromRouteConfig )
There was a problem hiding this comment.
Based on our offline discussion yesterday, I thought we agreed that dynamic clusters have to be handled differently compared to route clusters. The only thing that we did not completely have a handle on was the why and I thought we decided to ask other grpc maintainers about that.
|
@eshitachandwani |
| clusterResourcesSeen := make(map[string]bool) | ||
| haveAllResources := true | ||
|
|
||
| // Get all clusters to be watched: from route config and from cluster |
There was a problem hiding this comment.
Based on our offline discussion yesterday, I thought we agreed that dynamic clusters have to be handled differently compared to route clusters. The only thing that we did not completely have a handle on was the why and I thought we decided to ask other grpc maintainers about that.
Done. |
4 participants
Part of A74 changes.
This PR adds the functions to subscribe to clusters which increases the refcount and to unsubscribe to the cluster which decreases the refcount. These functions will be used in a later PR to start cluster watches for dynamic clusters. Will also be used by xds_resolver to make sure PRCs subscribe to clusters they are routed to and that the watches for those clusters remain active.
RELEASE NOTES: None