-
Notifications
You must be signed in to change notification settings - Fork 4.6k
xdsdepmgr: add functionality for cluster subscription #8792
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8792 +/- ##
==========================================
- Coverage 83.42% 83.32% -0.10%
==========================================
Files 418 417 -1
Lines 32897 32963 +66
==========================================
+ Hits 27443 27466 +23
- Misses 4069 4094 +25
- Partials 1385 1403 +18
🚀 New features to boost your workflow:
|
arjan-bal
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.
Some minor comments, otherwise LGTM!
| ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
| defer cancel() | ||
|
|
||
| configureAllResourcesOnManagementServer(ctx, t, mgmtServer, nodeID, listeners, route, clusters, endpoints) |
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.
nit: This function name is pretty long, can it be shortened to something like setupManagementServer? The part AllResourcesOn doesn't seem to add too much value.
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.
We have a separate setupManagementServerForTest function which starts the management server. And we already had a function called configureResourcesOnManagementServer which configures listener and route resources only.
We can make it configureAllResources if that sounds better.
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.
Having multiple helpers that do very similar things is a bit confusing. Could we modify an existing function to support the extra functionality needed for these tests? Or do you think that would make the function too complex?
Questions regarding the cluster subscription API.
| if _, ok := m.clustersFromRouteConfig[name]; !ok { | ||
| m.maybeSendUpdateLocked() | ||
| } | ||
| return m.clusterSubscriptions[name].unsubscribe |
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.
Can you wrap this in a sync.Once so that the it's safe for callers to call it multiple times? This would make the API safer. Also mention in the godoc that the returned function is idempotent.
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.
In the Close method, can you add an error log if the clusterSubscriptions map is non-empty? This would serve as a leak check and help tests catch subscribers that fail to release their references in time.
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.
In the case that there are ongoing RPCs when the Close is called, there will still be subscriptions to the cluster and that is an expected case, so I am not sure that an error log will be useful.
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.
The resolver should only be closed when the channel is closed (or when it enters IDLE, at which point there are no ongoing RPCs). When the channel is closed, any ongoing RPCs should fail. I suspect the issue is that the resolver is being closed before the RPCs fail; consequently, the Unsubscribe call might be arriving too late for the Close method to track it.
Lines 1213 to 1228 in b3603ab
| cc.resolverWrapper.close() | |
| // The order of closing matters here since the balancer wrapper assumes the | |
| // picker is closed before it is closed. | |
| cc.pickerWrapper.close() | |
| cc.balancerWrapper.close() | |
| <-cc.resolverWrapper.serializer.Done() | |
| <-cc.balancerWrapper.serializer.Done() | |
| var wg sync.WaitGroup | |
| for ac := range conns { | |
| wg.Add(1) | |
| go func(ac *addrConn) { | |
| defer wg.Done() | |
| ac.tearDown(ErrClientConnClosing) | |
| }(ac) | |
| } |
We likely need a leak check to track the execution of the callbacks returned by the ClusterSubscription method. @mbissa recently added something similar for async gauge metrics here, which should be easy to reuse. This can be done in a follow-up PR.
Part of A74 changes.
This PR add the functions and functionalities to be used for cluster subscription for cluster refcounts and also for dynamic cluster subscription. These functions will be used in subsequent PRs.
RELEASE NOTES: None