xds/resolver: cleanup tests to use real xDS client 3/n #5953
Conversation
easwars
force-pushed
the
xds_resolver_test_cleanup_3
branch
from
ecc0c41
to
896f753
Compare
| @@ -104,12 +107,12 @@ type testClientConn struct { | |||
| } | |||
|
|
|||
| func (t *testClientConn) UpdateState(s resolver.State) error { | |||
| t.stateCh.Send(s) | |||
| t.stateCh.Replace(s) | |||
There was a problem hiding this comment.
FYI: this seems okay as long as everywhere we read from this channel is resilient - as in, if the expected value isn't read, it reads again. Otherwise, you can run into races if you read from the channel before it's written.
And even then, we can have false positives. E.g.:
- Random setup thing happens that pushes the expected value.
- Test condition occurs, which will result in pushing something bad here.
- Read expected value from channel and pass test.
- Bad value we were hoping to test is actually pushed.
Forcing pulling every update off the channel effectively ensures the above won't happen.
There was a problem hiding this comment.
It seems both options here have problems, so I'm fine with this change.
| return nil | ||
| } | ||
|
|
||
| func (t *testClientConn) ReportError(err error) { | ||
| t.errorCh.Send(err) | ||
| t.errorCh.Replace(err) |
There was a problem hiding this comment.
Should this use the same channel as above? The signals are supposed to be mutually exclusive, and at some point we may want to refactor things to make the two conditions happen via the same call.
There was a problem hiding this comment.
I switched to Replace() mainly to workaround go-control-plane's behavior when the xDS client NACKs an update. The go-control-plane keeps resending the same update until it is ACKed. In one of our test scenarios where we test that a NACKed update results in the error being propagated to the channel, the test logic only reads the error out of the testClientConn once. But since the go-control-plane keeps sending the same update again and again, the resolver receives the same error from the xDS client and has to push it up to the ClientConn. Without a Replace() here, the test will fail with a leaked goroutine.
Changing this to use the same channel will make things more complicated, because I cannot reliably call Replace() because I won't know whether the item that I'm pulling from the channel is an update or an error.
There was a problem hiding this comment.
So what will we do if we refactor the resolver API to merge ReportError and Update? We can still use a different channel if the Update reflects an error, I suppose?
There was a problem hiding this comment.
Yes, that seems doable to me. I think we have this testClientConn implementation in a few other places as well. So, we would have to get to them as well.
easwars
force-pushed
the
xds_resolver_test_cleanup_3
branch
from
57ac48a
to
c9e452a
Compare
easwars
force-pushed
the
xds_resolver_test_cleanup_3
branch
from
d591fa5
to
79007e1
Compare
#resource-agnostic-xdsclient-api
RELEASE NOTES: n/a