xds/internal/server: stop using a fake xDS client in rds handler tests #6689

easwars · 2023-10-04T23:01:58Z

This is in prep for xDS generic API changes for the server side.

#resource-agnostic-xdsclient-api

RELEASE NOTES: none

codecov · 2023-10-04T23:04:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@93dbc05). Click here to learn what that means.
The diff coverage is n/a.

Additional details and impacted files

zasweq

LGTM.

zasweq · 2023-10-06T03:51:37Z

xds/internal/server/rds_handler_test.go

+				return fmt.Errorf("unexpected resources %v of type %q requested", req.GetResourceNames(), req.GetTypeUrl())
+			}
+			select {
+			case <-namesCh:


We ok with losing the old update?

We do create the channel with a buffer of 1. So, we are not concerned about losing an update completely. And when the tests do read the update when they expect one, we are good. Most of our tests which verify request names sent to the management server do this and run without flakes.

The issue with blocking here (instead of losing the update) is that unpredictability around go-control-plane server's behavior. For example, if you have resources of two types and you update one of them, the server will send all resources back to the client because we use a snapshot cache, and once the snapshot changes, the server will send the whole thing to the client.

zasweq · 2023-10-06T03:52:40Z

xds/internal/server/rds_handler_test.go

+func verifyUpdateFromChannel(ctx context.Context, t *testing.T, updateCh chan rdsHandlerUpdate, wantUpdate rdsHandlerUpdate) {
+	t.Helper()
+
+	opts := []cmp.Option{


Hmmmm nice I always do this inline. This is nice though with this many options.

Same. I use this approach only when I have a lot of options. And here it makes total sense since some of those options are taking a function.

zasweq · 2023-10-06T03:53:40Z

xds/internal/server/rds_handler_test.go

+		cmpopts.SortSlices(func(s1, s2 string) bool { return s1 < s2 }),
+		cmpopts.SortMaps(func(s1, s2 string) bool { return s1 < s2 }),


Can you briefly comment what non determinism this takes away and perhaps why the non determinism arises? I had a lot of non determinism in my OpenCensus tests since rows were emitted non deterministically.

Interesting. I remember there was some non-determinism which is why I added these options, but now I don't see them anymore. I ran the tests a thousand times locally and couldn't repro any flakes. So, I'm getting rid of these now.

zasweq · 2023-10-06T03:56:06Z

xds/internal/server/rds_handler_test.go

+			t.Fatalf("Got unexpected route update, diff (-got, +want): %v", diff)
 		}
 	case <-ctx.Done():
-		t.Fatal("Timed out waiting for update from update channel.")
+		t.Fatal("Timed out waiting for a route config update")


Nit route update or route config update? Feels like everywhere it's referred to as route config update, I think I'd prefer that.

Switched the one above this to route config update

zasweq · 2023-10-06T04:02:08Z

xds/internal/server/rds_handler_test.go

-	rh, fakeClient, ch := setupTests()
+// Tests the case where the rds handler receives an update with two routes, then
+// receives an update with only one route. The rds handler is expected to cancel
+// the watch for the route no longer present, and push a corresponding update.


a corresponding update with only one route.

zasweq · 2023-10-06T04:02:47Z

xds/internal/server/rds_handler_test.go

+// Tests the case where the rds
 // handler receives an update with two routes, and then receives an update with
 // two routes, one previously there and one added (i.e. 12 -> 23). This should
 // cause the route that is no longer there to be deleted and cancelled, and the
 // route that was added should have a watch started for it.


Wrap this comment to 80 characters please. Also optionally can you talk about the verifications after 23 is updated such as it doesn't send an update until 3 is received on management server etc.

zasweq · 2023-10-06T04:05:25Z

xds/internal/server/rds_handler_test.go

+	// Create an rds handler and give it two routes to watch.
+	updateCh := make(chan rdsHandlerUpdate, 1)
+	rh := newRDSHandler(xdsC, updateCh)
 	rh.updateRouteNamesToWatch(map[string]bool{route1: true, route2: true})

+	// Verify that the given routes are requested.
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer cancel()
-	if err := waitForFuncWithNames(ctx, fakeClient.WaitForWatchRouteConfig, route1, route2); err != nil {
-		t.Fatalf("Error while waiting for names: %v", err)
-	}
+	waitForResourceNames(ctx, t, resourceNamesCh, []string{route1, route2})


Optional: this seems pretty common to many tests, can maybe pull this out to helper.

I don't see what is common that can be pulled into a helper. Not all tests start with two routes. Some start with one. And I think this is a core portion of the test: creating the handler, giving it one or more names to watch and verifying that it sends RDS requests for the same. Pulling that out into a helper isn't very helpful in my opinion since the reader of the test will not be able to glance over that detail.

zasweq · 2023-10-06T04:05:43Z

xds/internal/server/rds_handler_test.go

-	}
-	if routeNameDeleted != route1 {
-		t.Fatalf("xdsClient.CancelRDS called for route %v, want %v", routeNameDeleted, route1)
+	// Verify that the update is pushed to the handler's update channel.


rds handler's (for consistency)

Same as mentioned in the other comment.

zasweq · 2023-10-06T04:08:10Z

xds/internal/server/rds_handler_test.go

+	// Close the rds handler and verify that the watch is canceled.
+	rh.close()
+	waitForResourceNames(ctx, t, resourceNamesCh, []string{})


This is done at the end of every test. I guess I'm fine with it because it closes from different state spaces. Also isn't it more than one watch that gets cancelled haha?

Also isn't it more than one watch that gets cancelled haha?

In some tests, it is more than one watch and in some it it one. I think the comment conveys the message enough.

zasweq · 2023-10-06T04:09:20Z

xds/internal/server/rds_handler_test.go

-			t.Fatalf("Did not receive the expected error, instead received: %v", rhu.err.Error())
+	case update := <-updateCh:
+		if !strings.Contains(update.err.Error(), wantErr) {
+			t.Fatalf("Update received with error %v, want error containing %q", update.err, wantErr)


Why is the second formatting directive %q vs %v?

easwars

Thanks for the review.

easwars · 2023-10-06T17:40:57Z

xds/internal/server/rds_handler_test.go

+				return fmt.Errorf("unexpected resources %v of type %q requested", req.GetResourceNames(), req.GetTypeUrl())
+			}
+			select {
+			case <-namesCh:


We do create the channel with a buffer of 1. So, we are not concerned about losing an update completely. And when the tests do read the update when they expect one, we are good. Most of our tests which verify request names sent to the management server do this and run without flakes.

The issue with blocking here (instead of losing the update) is that unpredictability around go-control-plane server's behavior. For example, if you have resources of two types and you update one of them, the server will send all resources back to the client because we use a snapshot cache, and once the snapshot changes, the server will send the whole thing to the client.

easwars · 2023-10-06T17:42:16Z

xds/internal/server/rds_handler_test.go

+func verifyUpdateFromChannel(ctx context.Context, t *testing.T, updateCh chan rdsHandlerUpdate, wantUpdate rdsHandlerUpdate) {
+	t.Helper()
+
+	opts := []cmp.Option{


Same. I use this approach only when I have a lot of options. And here it makes total sense since some of those options are taking a function.

easwars · 2023-10-06T18:08:07Z

xds/internal/server/rds_handler_test.go

+		cmpopts.SortSlices(func(s1, s2 string) bool { return s1 < s2 }),
+		cmpopts.SortMaps(func(s1, s2 string) bool { return s1 < s2 }),


Interesting. I remember there was some non-determinism which is why I added these options, but now I don't see them anymore. I ran the tests a thousand times locally and couldn't repro any flakes. So, I'm getting rid of these now.

easwars · 2023-10-06T18:10:22Z

xds/internal/server/rds_handler_test.go

+			t.Fatalf("Got unexpected route update, diff (-got, +want): %v", diff)
 		}
 	case <-ctx.Done():
-		t.Fatal("Timed out waiting for update from update channel.")
+		t.Fatal("Timed out waiting for a route config update")


Switched the one above this to route config update

easwars · 2023-10-06T18:11:11Z

xds/internal/server/rds_handler_test.go

-	rh, fakeClient, ch := setupTests()
+// Tests the case where the rds handler receives an update with two routes, then
+// receives an update with only one route. The rds handler is expected to cancel
+// the watch for the route no longer present, and push a corresponding update.


easwars · 2023-10-06T18:12:52Z

xds/internal/server/rds_handler_test.go

+// Tests the case where the rds
 // handler receives an update with two routes, and then receives an update with
 // two routes, one previously there and one added (i.e. 12 -> 23). This should
 // cause the route that is no longer there to be deleted and cancelled, and the
 // route that was added should have a watch started for it.


easwars · 2023-10-06T18:15:35Z

xds/internal/server/rds_handler_test.go

-			t.Fatalf("Did not receive the expected error, instead received: %v", rhu.err.Error())
+	case update := <-updateCh:
+		if !strings.Contains(update.err.Error(), wantErr) {
+			t.Fatalf("Update received with error %v, want error containing %q", update.err, wantErr)


easwars · 2023-10-06T18:17:00Z

xds/internal/server/rds_handler_test.go

+	// Close the rds handler and verify that the watch is canceled.
+	rh.close()
+	waitForResourceNames(ctx, t, resourceNamesCh, []string{})


Also isn't it more than one watch that gets cancelled haha?

In some tests, it is more than one watch and in some it it one. I think the comment conveys the message enough.

easwars · 2023-10-06T18:17:11Z

xds/internal/server/rds_handler_test.go

-	}
-	if routeNameDeleted != route1 {
-		t.Fatalf("xdsClient.CancelRDS called for route %v, want %v", routeNameDeleted, route1)
+	// Verify that the update is pushed to the handler's update channel.


Same as mentioned in the other comment.

easwars · 2023-10-06T18:19:24Z

xds/internal/server/rds_handler_test.go

+	// Create an rds handler and give it two routes to watch.
+	updateCh := make(chan rdsHandlerUpdate, 1)
+	rh := newRDSHandler(xdsC, updateCh)
 	rh.updateRouteNamesToWatch(map[string]bool{route1: true, route2: true})

+	// Verify that the given routes are requested.
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
 	defer cancel()
-	if err := waitForFuncWithNames(ctx, fakeClient.WaitForWatchRouteConfig, route1, route2); err != nil {
-		t.Fatalf("Error while waiting for names: %v", err)
-	}
+	waitForResourceNames(ctx, t, resourceNamesCh, []string{route1, route2})


I don't see what is common that can be pulled into a helper. Not all tests start with two routes. Some start with one. And I think this is a core portion of the test: creating the handler, giving it one or more names to watch and verifying that it sends RDS requests for the same. Pulling that out into a helper isn't very helpful in my opinion since the reader of the test will not be able to glance over that detail.

xds/internal/server: stop using a fake client in rds handler tests

b83ebb4

easwars changed the title ~~xds/internal/server: stop using a fake client in rds handler tests~~ xds/internal/server: stop using a fake xDS client in rds handler tests Oct 4, 2023

easwars requested a review from zasweq October 4, 2023 23:03

easwars assigned zasweq Oct 4, 2023

easwars added the Type: Testing label Oct 4, 2023

easwars added this to the 1.59 Release milestone Oct 4, 2023

ginayeh modified the milestones: 1.59 Release, 1.60 Release Oct 5, 2023

zasweq approved these changes Oct 6, 2023

View reviewed changes

zasweq assigned easwars and unassigned zasweq Oct 6, 2023

easwars commented Oct 6, 2023

View reviewed changes

minor review comments

50ae574

easwars merged commit eb33677 into grpc:master Oct 6, 2023
13 checks passed

easwars deleted the xds_server_cleanup_1 branch October 6, 2023 18:38

github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xds/internal/server: stop using a fake xDS client in rds handler tests #6689

xds/internal/server: stop using a fake xDS client in rds handler tests #6689

easwars commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

zasweq left a comment

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

zasweq Oct 6, 2023

easwars Oct 6, 2023

easwars left a comment

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

easwars Oct 6, 2023

		cmpopts.SortSlices(func(s1, s2 string) bool { return s1 < s2 }),
		cmpopts.SortMaps(func(s1, s2 string) bool { return s1 < s2 }),

xds/internal/server: stop using a fake xDS client in rds handler tests #6689

xds/internal/server: stop using a fake xDS client in rds handler tests #6689

Conversation

easwars commented Oct 4, 2023 • edited Loading

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

zasweq left a 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

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 left a 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 Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading