xdsclient: don't reset version info after stream restart #5422

easwars · 2022-06-14T21:17:12Z

When resending resource names after an ADS stream failure, we were resetting the resource version information received on the previous stream. This was our behavior from day 1, but this was in conflict with that is specified in the spec. This change brings the Go implementation to be in accordance with the spec.

Summary of changes:

Fix the code in xdsClient to not reset version information about ADS stream restart
- Also, fix the code to not send empty version strings upon ADS stream restart. Instead, use the previously received version strings.
  - The actual code changes are limited to xdsclient/controller/controller.go and xdsclient/controller/transport/go. The remaining changes are test related.
Add a new e2e test to verify that version information is preserved across stream restarts.
Add a type to specify options to the ManagementServer type defined in xds/e2e package.
- The options accept a set of callbacks to be invoked for stream level events.
- It also accepts a net.Listener for tests which need to customize the listener.
Modify the utility functions StartManagementServer and SetupManagementServer to accept these options.
Move management server logging functions to a separate file.

Fixes #5351

RELEASE NOTES:

xdsClient will not reset resource version information after ADS stream restart

dfawley · 2022-06-17T22:57:41Z

test/xds/xds_client_ack_nack_test.go

+				// corresponds to ACKs, and this is what we want to capture.
+				if len(request.GetResourceNames()) != 0 && request.GetVersionInfo() != "" {
+					ackVersionsBeforeRestart[request.GetTypeUrl()] = request.GetVersionInfo()
+					if len(ackVersionsBeforeRestart) == wantResources {


Should there be an else here so it fails if we go over the expectation?

Another option perhaps: maybe push the resources to the channel instead and validate them back in the main test code?

Pushed resources into a channel and changed to validate them from the main test goroutine.

Discussed further test simplification ideas offline.

Done. Thanks.

dfawley · 2022-06-17T22:58:06Z

test/xds/xds_client_ack_nack_test.go

+				if ackVersionsAfterRestart[request.GetTypeUrl()] == "" {
+					ackVersionsAfterRestart[request.GetTypeUrl()] = request.GetVersionInfo()
+				}
+				if len(ackVersionsAfterRestart) == wantResources {


(Same here)

dfawley · 2022-06-17T22:58:57Z

test/xds/xds_client_ack_nack_test.go

+						select {
+						case resourcesRequestedBeforeStreamClose <- struct{}{}:
+						default:
+						}


Why not just push to the channel normally?

Not required anymore.

dfawley · 2022-06-17T23:00:26Z

test/xds/xds_client_ack_nack_test.go

+	// Channels to notify that all expected resources have been requested by the
+	// xdsClient before and after stream restart.
+	resourcesRequestedBeforeStreamClose := make(chan struct{}, 1)
+	resourcesRequestedAfterStreamClose := make(chan struct{}, 1)


Should these just be normal, unbuffered channels, that are closed when the condition happens? Or use a grpcsync.Event?

Now, these channels are buffered to have enough space for 4 resources.

…t goroutine

dfawley · 2022-06-29T23:48:50Z

test/xds/xds_client_ack_nack_test.go

+	ackVersionsAfterRestart := make(map[string]string)
+AcksAfterRestart:
+	for {
+		select {
+		case tuple := <-requestsCh:
+			if tuple.id == idBeforeRestart {
+				// Ignore any stray requests from the old stream.
+				continue
+			}
+			if tuple.id != idAfterRestart {
+				t.Fatalf("Received request with stream ID %d, expecting %d", tuple.id, idAfterRestart)
+			}
+			// After stream closure, capture the first request for every resource.
+			// This should not be set to an empty version string, but instead should
+			// be set to the version last ACKed before stream closure.
+			req := tuple.req
+			if len(req.GetResourceNames()) != 0 {
+				ackVersionsAfterRestart[req.GetTypeUrl()] = req.GetVersionInfo()
+			}
+			if len(ackVersionsAfterRestart) == wantResources {
+				break AcksAfterRestart
+			}
+		case <-ctx.Done():
+			t.Fatal("timeout when waiting for resources to be re-requested after stream restart")
+		}
 	}


Can we do this in the loop above? I.e. store the first wantResources requests for each stream ID. Could either be [<id int>]map[string]string or map[<id int>]map[string]string, or keep it as two different maps.

You could potentially go even further: get rid of the channel entirely, accumulate the maps directly in the callback instead, and close a channel / fire an event / cancel a context (can have the timeout on it) when the maps have been populated.

Done. Moved the logic to populate the map into the callback. The test goroutine now waits for a signal from the callback to go ahead and verify the acks before and after stream restart.

The logic to extract and compare the acks from the populated map seems a little verbose. Not sure if I can do any better though. Thanks.

dfawley · 2022-06-30T22:27:31Z

test/xds/xds_client_ack_nack_test.go

+	acksBeforeRestart := ackVersionsMap[idBeforeRestart]
+	acksAfterRestart := ackVersionsMap[idAfterRestart]
+	if !cmp.Equal(acksBeforeRestart, acksAfterRestart) {
+		t.Fatalf("ACKs before restart: %v and ACKs after restart: %v don't match", acksBeforeRestart, acksAfterRestart)


Use cmp.Diff instead of cmp.Equal and display that output instead?

dfawley · 2022-06-30T22:29:41Z

test/xds/xds_client_ack_nack_test.go

+	var idBeforeRestart, idAfterRestart int64
+	for id := range ackVersionsMap {
+		if id > idAfterRestart {
+			idBeforeRestart = idAfterRestart
+			idAfterRestart = id
+		} else {
+			idBeforeRestart = id


I don't understand why this needs to be so complicated now. Why can't we use 1 and 2 directly anymore as before?

Since we moved to having a map indexed by the stream id, I thought this should be the way to go. Over engineering .. sigh.

Done now. Thanks.

If Go had better generics support already, I wouldn't mind it so much as:

idBeforeRestart := min(keys(ackVersionsMap)) idAfterRestart := max(keys(ackVersionsMap))

easwars requested a review from dfawley June 14, 2022 21:17

easwars assigned dfawley Jun 14, 2022

easwars added the Type: Bug label Jun 14, 2022

easwars added this to the 1.48 Release milestone Jun 14, 2022

easwars force-pushed the management_server_callbacks branch from 7ebf9e4 to 65a605f Compare June 14, 2022 22:36

dfawley reviewed Jun 17, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley Jun 17, 2022

easwars assigned dfawley and unassigned easwars Jun 22, 2022

dfawley assigned easwars and unassigned dfawley Jun 28, 2022

easwars assigned dfawley and unassigned easwars Jun 29, 2022

easwars added 5 commits June 29, 2022 16:09

xdsclient: don't reset version info after stream restart

f6b7322

some test improvements

0207493

push onto channel from callback, and read from the channel in the tes…

c212657

…t goroutine

make vet happy

3bc796a

simplify management server callback functionality in test

e6adc7b

easwars force-pushed the management_server_callbacks branch from d593272 to e6adc7b Compare June 29, 2022 23:10

dfawley reviewed Jun 29, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley Jun 29, 2022

more test simplification

58fe71c

easwars assigned dfawley and unassigned easwars Jun 30, 2022

dfawley reviewed Jun 30, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley Jun 30, 2022

use hardcoded stream ids

be30e1f

easwars assigned dfawley and unassigned easwars Jun 30, 2022

dfawley approved these changes Jul 1, 2022

View reviewed changes

dfawley assigned easwars and unassigned dfawley Jul 1, 2022

dfawley modified the milestones: 1.48 Release, 1.49 Release Jul 1, 2022

easwars merged commit a6dcb71 into grpc:master Jul 6, 2022

easwars deleted the management_server_callbacks branch July 15, 2022 17:24

moul-bot mentioned this pull request Oct 7, 2022

chore(deps): bump google.golang.org/grpc from 1.38.0 to 1.50.0 moul/depviz#581

Closed

github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xdsclient: don't reset version info after stream restart #5422

xdsclient: don't reset version info after stream restart #5422

easwars commented Jun 14, 2022 •

edited

dfawley Jun 17, 2022

easwars Jun 22, 2022

dfawley Jun 28, 2022

easwars Jun 29, 2022

dfawley Jun 17, 2022

easwars Jun 22, 2022

dfawley Jun 17, 2022

easwars Jun 22, 2022

dfawley Jun 17, 2022

easwars Jun 22, 2022

dfawley Jun 29, 2022

easwars Jun 30, 2022

dfawley Jun 30, 2022

easwars Jul 6, 2022

dfawley Jun 30, 2022

easwars Jun 30, 2022

dfawley Jul 1, 2022

xdsclient: don't reset version info after stream restart #5422

xdsclient: don't reset version info after stream restart #5422

Conversation

easwars commented Jun 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars commented Jun 14, 2022 •

edited