xds/cdsbalancer: changed the setupManagementServer helper to take listener and OnStreamReq as parameter#8467
Conversation
eshitachandwani
added
Type: Testing
Area: xDS
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8467 +/- ##
==========================================
- Coverage 82.36% 81.97% -0.39%
==========================================
Files 413 413
Lines 40532 40518 -14
==========================================
- Hits 33383 33216 -167
- Misses 5781 5941 +160
+ Partials 1368 1361 -7 🚀 New features to boost your workflow:
|
|
After an offline discussion we have decided to change it to be a blocking send , and have new helper functions which will not write to the channel on resource request to be used by tests that do not want to verify resource requests. |
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Dismissing my previous review.
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Dismissing my previous review.
arjan-bal
left a comment
arjan-bal
left a comment
There was a problem hiding this comment.
Dismissing my previous review.
arjan-bal
dismissed
their stale review
Changes requested by main reviewer. Will need a re-review
eshitachandwani
assigned
easwars and
eshitachandwani and unassigned
eshitachandwani and
easwars
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
Also, can you please verify that with your changes, all tests in the cds balancer package runs without flakes on forge. There is a way to directly import changes from a PR. Let me know if you cannot figure it out. Thanks.
| // - Creates a manual resolver that configures the cds LB policy as the | ||
| // top-level policy, and pushes an initial configuration to it | ||
| // - Creates a gRPC channel with the above manual resolver | ||
| // - Executes OnStreamRequest callback provided by the caller |
There was a problem hiding this comment.
This doesn't make sense. This should be part of the first bullet point. Something like
- Spins up an xDS management server and passes it the onStreamRequest callback that will get invoked every time a request is received on the ADS stream.
There was a problem hiding this comment.
We don't want the bullet point "// - Executes OnStreamRequest callback provided by the caller", because we don't directly call it. We only pass it to function that creates the management server.
| case 0: | ||
| select { | ||
| case cdsResourceCanceledCh <- struct{}{}: | ||
| default: |
There was a problem hiding this comment.
Do we still need this default case here?
| if len(req.GetResourceNames()) == 0 { | ||
| select { | ||
| case cdsResourceCanceledCh <- struct{}{}: | ||
| default: |
easwars
left a comment
easwars
left a comment
There was a problem hiding this comment.
LGTM, modulo one minor comment.
| // - Creates a manual resolver that configures the cds LB policy as the | ||
| // top-level policy, and pushes an initial configuration to it | ||
| // - Creates a gRPC channel with the above manual resolver | ||
| // - Executes OnStreamRequest callback provided by the caller |
There was a problem hiding this comment.
We don't want the bullet point "// - Executes OnStreamRequest callback provided by the caller", because we don't directly call it. We only pass it to function that creates the management server.
eshitachandwani
changed the title
…n test (grpc#8467) RELEASE NOTES: N/A Fixes: grpc#8462 The main issue was that the requests were getting dropped since we use a [non-blocking send](https://github.com/grpc/grpc-go/blob/a5e7cd6d4c2c31b1e6649789c2ddc9a82ad6b5fa/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go#L222C5-L227C6) for resources in test along with buffer size of just [one](https://github.com/grpc/grpc-go/blob/a5e7cd6d4c2c31b1e6649789c2ddc9a82ad6b5fa/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go#L210) which was resulting in resource request updates being dropped if the receiver is not executing at the exact moment. Fix: Changed the `setupManagementServer` to take `listener` and `OnStreamReq` function as a parameter and in the `TestWatcher` added a blocking send whenever a cluster resource is requested.
…n test (grpc#8467) RELEASE NOTES: N/A Fixes: grpc#8462 The main issue was that the requests were getting dropped since we use a [non-blocking send](https://github.com/grpc/grpc-go/blob/a5e7cd6d4c2c31b1e6649789c2ddc9a82ad6b5fa/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go#L222C5-L227C6) for resources in test along with buffer size of just [one](https://github.com/grpc/grpc-go/blob/a5e7cd6d4c2c31b1e6649789c2ddc9a82ad6b5fa/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go#L210) which was resulting in resource request updates being dropped if the receiver is not executing at the exact moment. Fix: Changed the `setupManagementServer` to take `listener` and `OnStreamReq` function as a parameter and in the `TestWatcher` added a blocking send whenever a cluster resource is requested.
4 participants
RELEASE NOTES: N/A
Fixes: #8462
The main issue was that the requests were getting dropped since we use a non-blocking send for resources in test along with buffer size of just one which was resulting in resource request updates being dropped if the receiver is not executing at the exact moment.
Fix:
Changed the
setupManagementServerto takelistenerandOnStreamReqfunction as a parameter and in theTestWatcheradded a blocking send whenever a cluster resource is requested.