csds: unskip e2e test #7502
csds: unskip e2e test #7502
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7502 +/- ##
==========================================
+ Coverage 81.66% 81.93% +0.27%
==========================================
Files 359 360 +1
Lines 27531 27533 +2
==========================================
+ Hits 22484 22560 +76
+ Misses 3824 3782 -42
+ Partials 1223 1191 -32 |
xds/csds/csds_e2e_test.go
Outdated
| // useful when a resource is NACKed, because the go-control-plane management | ||
| // server continuously resends the same resource in this case, and applying flow | ||
| // control from these watchers ensures that xDS client does not spend all of its | ||
| // time receiving and NACKing updates from the maangement server. This was |
xds/csds/csds_e2e_test.go
Outdated
| // indeed the case on arm64 (before we had support for ADS stream level flow | ||
| // control), and was causing CSDS to not receive any updates in this case. | ||
|
|
||
| func blockAndDone(testDoneCh <-chan struct{}, waitCh chan struct{}, onDone xdsresource.DoneNotifier) { |
There was a problem hiding this comment.
Can you write a comment explaining what this function does? Or encode it in the function name? I don't think blockAndDone describes what is going on here. Something along the lines of calls onDone if test finishes or signaled through waitCh.
There was a problem hiding this comment.
Oops. I was supposed to write a comment for this. Totally forgot. Thanks.
xds/csds/csds_e2e_test.go
Outdated
| @@ -332,6 +406,9 @@ func (s) TestCSDS(t *testing.T) { | |||
| t.Fatal(err) | |||
| } | |||
|
|
|||
| // Unblock the resource watchers. | |||
There was a problem hiding this comment.
Optional here and below: delete this since just repeating the function name?
xds/csds/csds_e2e_test.go
Outdated
|
|
||
| type blockingListenerWatcher struct { | ||
| testDoneCh <-chan struct{} // Closed when the test is done. | ||
| waitCh chan struct{} // Written to by the test to unblock the watch callback. |
There was a problem hiding this comment.
Nit: here, other watchers, and the helper unblockResourceWatchers. "wait" to me is encoded implicitly in the semantics of a go channel. Perhaps call this onDoneCh? triggerOnDoneCh? watchCallbackCh?
02a05c2
to
9530e69
Compare
|
@zasweq : Do you mind taking another look. Based on our offline discussion, I split the tests into two. The second one now deals with NACKs, but is simpler because it only deals with one resource type. |
| // Tests CSDS functionality. The test performs the following: | ||
| // - Spins up a management server and creates two xDS clients talking to it. | ||
| // - Registers one watch on each xDS client, and verifies that the CSDS | ||
| // response reports resources in REQUESTED state. |
There was a problem hiding this comment.
In between REQUESTED and NACK you also test if both of them are in ACKED state.
xds/csds/csds_e2e_test.go
Outdated
| // writeOnDone attempts to writes the onDone callback on the onDone | ||
| // channel. It returns when it can successfully write to the channel or when the | ||
| // test is done, which is signalled by testCtxDone being closed. |
This PR unskips a test which was previously failing on
arm64because the xDS client was being overwhelmed by the go-control-plane management server, because the latter was continuously resending NACKed resources. This PR uses the ADS stream level flow control mechanism from the resource watchers used in the test, and thereby will be able to throttle receipt of messages from the management server.Fixes #7383
RELEASE NOTES: none