xds/resolver: cleanup tests to use real xDS client 2/n #5952
Conversation
easwars
changed the title
easwars
force-pushed
the
xds_resolver_test_cleanup_2
branch
from
5e36aed
to
b319ee4
Compare
| // dummy management server. Also close a channel when the returned xDS | ||
| // client is closed. |
There was a problem hiding this comment.
Minor nit don't feel strongly. Can you move this comment about closing channel right before closeCh is declared?
| // client is closed. | ||
| bootstrapCfg := &bootstrap.Config{ | ||
| XDSServer: &bootstrap.ServerConfig{ | ||
| ServerURI: "dummy-management-server-address", |
There was a problem hiding this comment.
So xDS Client creation doesn't fail with the NewConfigForTesting even with a non-existent management server? When would that fail, first request sent out?
There was a problem hiding this comment.
The management server URI is used as the dial target when creating the ClientConn to the management server. And we don't perform a blocking dial there. So, if the address does not exist, we will get to know only at RPC time.
| // the test goroutine signals the management server when the resolver is | ||
| // closed. | ||
| waitForRouteConfigCh := make(chan struct{}) | ||
| waitForCloseCh := make(chan struct{}) |
There was a problem hiding this comment.
Nit: "close" has different semantical meaning here and the test below. This one waits for the resolver close, below waits for client close. Perhaps prefix with what component is closing?
| // it receives a discovery request for a route configuration resource. And | ||
| // the test goroutine signals the management server when the resolver is | ||
| // closed. | ||
| waitForRouteConfigCh := make(chan struct{}) |
There was a problem hiding this comment.
Nit: is this waiting for the route config, or a request FOR a route config. The route configuration is what is sent back right, not the discovery request.
There was a problem hiding this comment.
Yes, it is a discovery request for a route configuration resource. So, i renamed it as waitForRouteConfigDiscoveryReqCh. It is long, but that's fine I guess, since it is used some 60 lines after the definition.
| select { | ||
| case <-waitForRouteConfigCh: | ||
| case <-ctx.Done(): | ||
| t.Fatal("Timeout when waiting for a discovery request with a route configuration resource") |
There was a problem hiding this comment.
Same here. switch "With" to "For"?
|
|
||
| // Verify that the update from the management server is not propagated to | ||
| // the ClientConn. The xDS resolver, once closed, is expected to drop | ||
| // updates from the xDS client. |
There was a problem hiding this comment.
Question for myself: so is this what happens here? The management server is running one line at a time not concurrently. So when you unblock the management server above, it is free to process/send back the update on line 500? Which is why you block, to make sure update happens after the resolver closure?
There was a problem hiding this comment.
I don't quite understand what you mean by The management server is running one line at a time not concurrently?
Which is why you block, to make sure update happens after the resolver closure?
Yes. The point of this test is to ensure that any updates received from the management server after the resolver is closed, is not propagated up the channel.
There was a problem hiding this comment.
Meaning it is only running one function at a time and not splicing between them (i.e. the function with the receive that is blocking), and actually sending the update you specified.
There was a problem hiding this comment.
The management server can run more than one function at any point in time. Whenever it gets a discovery request, it will invoke this callback. But we know that our xDS client does not re-send discovery requests randomly (if that happens, more than one callback will get blocked on this channel, and a goroutine will leak and the test will fail), and therefore we can reliably block the callback until we close the resolver and unblock it afterwards.
#resource-agnostic-xdsclient-api
RELEASE NOTES: n/a