xds/federation: update xdsclient to support multi authority #5042
Conversation
menghanl
force-pushed
the
xds_client_federation_multi_controller
branch
from
9ab1734 to
fd4a99f
Compare
- added an authority struct - consists of controller (the xds stream) and an pubsub (cache for resources received from this authority) - an authority is created when the first watch is called on this authority, and deleted when the last watch is canceled - authorities are ref-counted - authorities are kept in a `map[server-config]authority`, note that key is the config, not authority name, so that two authoritie with different names but same config will share the xds stream and pubsub - authority is kept in a cache after deletion, and only actually deleted after a timeout, so it can be revived if a new watch is started - watch functions find (or build) the authority first, then call watch on this authority - they also handle ref/unref of the authority - dump (for CSDS) is updated to dump resources from all the authorities - resource handling (when unmarshalling receive xds resources) parses the resource name first, and then turns it back to a string, to canonicalize (especially the context parameters) - tests - update handler is only available after the watch is started, so many tests need updating - make many LDS/RDS/CDS/EDS tests share code, and also added a federation version
menghanl
force-pushed
the
xds_client_federation_multi_controller
branch
from
fd4a99f to
026827e
Compare
| // Clear the item in the watch channel, otherwise the next watch will block. | ||
| authority := xdsresource.ParseName(resourceName).Authority | ||
| var config *bootstrap.ServerConfig | ||
| if authority == "" { | ||
| config = client.config.XDSServer | ||
| } else { | ||
| authConfig, ok := client.config.Authorities[authority] | ||
| if !ok { | ||
| t.Fatalf("failed to find authority %q", authority) | ||
| } | ||
| config = authConfig.XDSServer | ||
| } | ||
| a := client.authorities[config.String()] | ||
| if a == nil { | ||
| t.Fatalf("authority for %q is not created", authority) | ||
| } | ||
| ctrlTemp := a.controller.(*testController) | ||
| ctrlTemp.addWatches[xdsresource.ClusterResource].ReceiveOrFail() |
There was a problem hiding this comment.
I don't quite follow why we need to do this in the test? When/why will there be an item in the watch channel already?
There was a problem hiding this comment.
This is to clear the channel so the next watch on this controller doesn't block.
| } | ||
| t.Cleanup(client.Close) | ||
|
|
||
| ctrl, ok, _ := watchAndFetchNewController(t, client, testCDSName, ctrlCh) |
There was a problem hiding this comment.
Why are we not calling the returned cancel func here and in many other places?
There was a problem hiding this comment.
Because that cancel just sends an item to a cancel channel. So I skipped that if there's no check for that item.
|
|
||
| // TestAuthorityRevive covers that the authorities in the idle cache is revived | ||
| // when a new watch is started on this authority. | ||
| func (s) TestAuthorityRevive(t *testing.T) { |
There was a problem hiding this comment.
Can this test be combined with the idleness test, because they are mostly doing the same thing.
There was a problem hiding this comment.
But they are actually testing quite different things.
idleness is testing ref counting
This is testing getting items back from the timeout cache.
| // | ||
| // Note that this function checks for the controller after watching. This is the | ||
| // new behavior after federation. | ||
| func testWatchSetup(ctx context.Context, t *testing.T, typ xdsresource.ResourceType, resourceName string, overrideWatchExpiryTimeout bool) (_ *clientImpl, _ *testController, _ pubsub.UpdateHandler, updateCh *testutils.Channel, cancelWatch func()) { |
There was a problem hiding this comment.
I feel like we are sacrificing test code readability for test code brevity. This function does a lot of things, and not all callers use all the return values. Do you think we can simplify things at the code of writing repetitive test code?
See: https://engdoc.corp.google.com/eng/doc/tott/episodes/598.md?cl=head
There was a problem hiding this comment.
Done.
Federation made this util function more complex, because now controller is only available after a watch.
But the code doesn't look that cleaner even after splitting... There are just too many steps.
map[server-config]authority, note that key is the config, not authority name, so that two authoritie with different names but same config will share the xds stream and pubsubRELEASE NOTES: N/A