-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
xds/resolver: cleanup tests to use real xDS client 2/n #5952
Conversation
5e36aed
to
b319ee4
Compare
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.
LGTM outside some naming/semantical nits.
// dummy management server. Also close a channel when the returned xDS | ||
// client is closed. |
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.
Minor nit don't feel strongly. Can you move this comment about closing channel right before closeCh is declared?
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.
Done.
// client is closed. | ||
bootstrapCfg := &bootstrap.Config{ | ||
XDSServer: &bootstrap.ServerConfig{ | ||
ServerURI: "dummy-management-server-address", |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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.
Done.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. switch "With" to "For"?
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.
Done.
|
||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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