xdsclient: add an e2e style test for fallback involving more than 2 servers #7817 #8427

vinothkumarr227 · 2025-07-03T05:15:41Z

RELEASE NOTES: N/A

codecov · 2025-07-03T05:20:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.35%. Comparing base (8adcc94) to head (07ec5df).
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8427      +/-   ##
==========================================
+ Coverage   82.31%   82.35%   +0.03%     
==========================================
  Files         414      414              
  Lines       40464    40531      +67     
==========================================
+ Hits        33308    33378      +70     
+ Misses       5791     5785       -6     
- Partials     1365     1368       +3

see 26 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

eshitachandwani · 2025-07-10T06:11:06Z

xds/internal/xdsclient/tests/fallback_test.go

+
+	// Secondary2 fallback, RPCs reach backend3.
+	secondary2Lis.Restart()
+	if err := waitForRPCsToReachBackend(ctx, client, backend3.Address); err != nil {


Test to verify if connections are attempted to primary and secondary1 backends.

eshitachandwani · 2025-07-10T06:12:36Z

xds/internal/xdsclient/tests/fallback_test.go

+	if conn, err := secondary2WrappedLis.NewConnCh.Receive(ctx); err == nil {
+		cw := conn.(*testutils.ConnWrapper)
+		if _, err := cw.CloseCh.Receive(ctx); err != nil {
+			t.Fatalf("Failed to get connection to secondary2: %v", err)


The error message is wrong. Here we are checking if connection to secondary2 is closed or not because we not have secondary1 running. Change the error message.

eshitachandwani · 2025-07-10T06:13:24Z

xds/internal/xdsclient/tests/fallback_test.go

+	}))
+	if conn, err := secondary1WrappedLis.NewConnCh.Receive(ctx); err == nil {
+		cw := conn.(*testutils.ConnWrapper)
+		cw.CloseCh.Receive(ctx)


Is there a reason we do not have a similar check here?

Thanks for pointing that out! I’ve updated the code.

…back-e2e-test

eshitachandwani · 2025-07-23T21:50:53Z

xds/internal/xdsclient/tests/fallback_test.go

@@ -728,3 +728,169 @@ func (s) TestFallback_OnStartup_RPCSuccess(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+// TestXDSFallback_ThreeServerPromotion verifies fallback and promotion


This could be changed to something like :
The test verifies that when the primary management server is unavailable, the system attempts to connect to the first fallback server, and if that is also down, to the second fallback server. It also ensures that the system switches back to the first fallback server once it becomes available again, and eventually returns to the primary server when it comes back online, closing connections to the fallback servers accordingly.

eshitachandwani · 2025-07-23T21:53:38Z

xds/internal/xdsclient/tests/fallback_test.go

+		t.Fatalf("Failed to create gRPC client: %v", err)
+	}
+	defer cc.Close()
+	cc.Connect()


I dont think this is needed since we will automatically connect when trying to make the first RPC.

eshitachandwani · 2025-07-23T21:55:01Z

xds/internal/xdsclient/tests/fallback_test.go

+	// Verify that connection attempts were made to primaryWrappedLis and
+	// secondary1WrappedLis, even though the fallback succeeded using
+	// secondary2WrappedLis and RPCs reached backend3.


Suggested change

// Verify that connection attempts were made to primaryWrappedLis and

// secondary1WrappedLis, even though the fallback succeeded using

// secondary2WrappedLis and RPCs reached backend3.

// Verify that connection attempts were made to primaryWrappedLis and

// secondary1WrappedLis, before using secondary2WrappedLis to

// make succesful RPCs to backend3.

eshitachandwani

LGTM modulo one comment. Adding @easwars as a second reviewer.

eshitachandwani · 2025-07-29T14:02:53Z

xds/internal/xdsclient/tests/fallback_test.go

+	}
+	if _, err := secondary1WrappedLis.NewConnCh.Receive(ctx); err != nil {
+		t.Fatalf("Expected connection attempt to secondary1 but got error: %v", err)
+	}


Move this block to before waitForRPCsToReachBackend(ctx, client, backend3.Address) so that it is consistent with the comment and the order of events.

Yes, we should verify that RPCs are successful to backend3 only after we verify that connections to the primary and secondary were attempted.

easwars · 2025-07-29T22:51:11Z

xds/internal/xdsclient/tests/fallback_test.go

+	secondary1WrappedLis := testutils.NewListenerWrapper(t, nil)
+	secondary1Lis := testutils.NewRestartableListener(secondary1WrappedLis)
+
+	secondary2WrappedLis := testutils.NewListenerWrapper(t, nil)
+	secondary2Lis := testutils.NewRestartableListener(secondary2WrappedLis)


Nit: instead of secondary1Xxx and secondary2Xxx, you could name them as secondaryXxx and tertiaryXxx.

easwars · 2025-07-29T22:53:53Z

xds/internal/xdsclient/tests/fallback_test.go

+	secondary1ManagementServer.Update(ctx, e2e.UpdateOptions{
+		NodeID:    nodeID,
+		Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(serviceName, "route-s1")},
+		Routes:    []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig("route-s1", serviceName, "cluster-s1")},
+		Clusters:  []*v3clusterpb.Cluster{e2e.DefaultCluster("cluster-s1", "endpoints-s1", e2e.SecurityLevelNone)},
+	})


Can we only have the listener resource here as well (just like in the primary server)?

easwars · 2025-07-29T22:54:21Z

xds/internal/xdsclient/tests/fallback_test.go

+	secondary2ManagementServer.Update(ctx, e2e.UpdateOptions{
+		NodeID:    nodeID,
+		Listeners: []*v3listenerpb.Listener{e2e.DefaultClientListener(serviceName, "route-s2")},
+		Routes:    []*v3routepb.RouteConfiguration{e2e.DefaultRouteConfig("route-s2", serviceName, "cluster-s2")},
+		Clusters:  []*v3clusterpb.Cluster{e2e.DefaultCluster("cluster-s2", "endpoints-s2", e2e.SecurityLevelNone)},
+		Endpoints: []*v3endpointpb.ClusterLoadAssignment{
+			e2e.DefaultEndpoint("endpoints-s2", "localhost", []uint32{testutils.ParsePort(t, backend3.Address)}),
+		},
+	})


Maybe we can use e2e.DefaultClientResources to simplify this code?

easwars · 2025-07-29T22:57:35Z

xds/internal/xdsclient/tests/fallback_test.go

+	bootstrapContents, err := bootstrap.NewContentsForTesting(bootstrap.ConfigOptionsForTesting{
+		Servers: []byte(fmt.Sprintf(`[
+            {"server_uri": %q, "channel_creds":[{"type":"insecure"}]},
+            {"server_uri": %q, "channel_creds":[{"type":"insecure"}]},
+            {"server_uri": %q, "channel_creds":[{"type":"insecure"}]}
+        ]`, primaryManagementServer.Address, secondary1ManagementServer.Address, secondary2ManagementServer.Address)),
+		Node: []byte(fmt.Sprintf(`{"id":%q}`, nodeID)),
+	})


Nit: Can we have this block indented nicely for improved readability. Either like the one in https://github.com/grpc/grpc-go/blob/master/test/xds/xds_client_federation_test.go#L163 or like the ones in the existing tests in this file.

easwars · 2025-07-29T23:03:59Z

xds/internal/xdsclient/tests/fallback_test.go

+		t.Fatal(err)
+	}
+
+	// Verify that connection attempts were made to primaryWrappedLis and


Instead of stopping all the three listeners up front, would it not be easy to leave them open and then verify that initially a connection is attempted to the primary server. And because the primary does not respond with all resources, once the test stops the primary listener (and thereby shutting down the existing connection), the xDS client should attempt a connection to the secondary server. That can then be verified by the test. Again, after verifying that, the test would stop the secondary listener, thereby terminating that connection. And again, since the secondary also did not respond with all requested resources, the xDS client should attempt a connection to the tertiary server. Now, that one will return all resources, and RPCs should succeed.

Then we configure the secondary server with all resources, and bring it back to life. Then, we expect a connection to the secondary and eventually for RPCs to succeed.

Then we configure the primary server with all resources, and bring it back to life. Then, we expect a connection to the primary and eventually for RPCs to succeed.

Maybe you tried that and there was a problem with that approach? If not, that seems like a cleaner way to go about things. Thoughts?

easwars · 2025-07-29T23:05:13Z

xds/internal/xdsclient/tests/fallback_test.go

+	}
+	if _, err := secondary1WrappedLis.NewConnCh.Receive(ctx); err != nil {
+		t.Fatalf("Expected connection attempt to secondary1 but got error: %v", err)
+	}


Yes, we should verify that RPCs are successful to backend3 only after we verify that connections to the primary and secondary were attempted.

xdsclient: add e2e test verifying fallback across three servers

4ac302e

vinothkumarr227 added 2 commits July 4, 2025 06:42

small tweaks

4cb0a3b

Fixed the doc issues

99c102c

arjan-bal requested a review from eshitachandwani July 7, 2025 05:30

arjan-bal assigned eshitachandwani Jul 7, 2025

eshitachandwani reviewed Jul 10, 2025

View reviewed changes

eshitachandwani assigned vinothkumarr227 and unassigned eshitachandwani Jul 10, 2025

Fixed the review changes

ba3330f

vinothkumarr227 removed their assignment Jul 15, 2025

vinothkumarr227 requested a review from eshitachandwani July 15, 2025 04:47

eshitachandwani added the Type: Testing label Jul 15, 2025

arjan-bal assigned eshitachandwani Jul 22, 2025

Merge remote-tracking branch 'origin/master' into 7817-xdsclient-fall…

cd68f1d

…back-e2e-test

eshitachandwani reviewed Jul 23, 2025

View reviewed changes

eshitachandwani assigned vinothkumarr227 and unassigned eshitachandwani Jul 23, 2025

vinothkumarr227 added 2 commits July 24, 2025 08:02

Fixed the review changes

f45b467

Small tweaks

07ec5df

vinothkumarr227 removed their assignment Jul 25, 2025

vinothkumarr227 requested a review from eshitachandwani July 25, 2025 06:02

eshitachandwani self-assigned this Jul 28, 2025

eshitachandwani added this to the 1.75 Release milestone Jul 29, 2025

eshitachandwani approved these changes Jul 29, 2025

View reviewed changes

eshitachandwani requested a review from easwars July 29, 2025 14:05

eshitachandwani assigned easwars and vinothkumarr227 and unassigned eshitachandwani Jul 29, 2025

easwars reviewed Jul 29, 2025

View reviewed changes

easwars removed their assignment Jul 29, 2025

xdsclient: add an e2e style test for fallback involving more than 2 servers #7817 #8427

Are you sure you want to change the base?

xdsclient: add an e2e style test for fallback involving more than 2 servers #7817 #8427

Uh oh!

Conversation

vinothkumarr227 commented Jul 3, 2025 • edited by eshitachandwani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eshitachandwani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vinothkumarr227 commented Jul 3, 2025 •

edited by eshitachandwani

Loading

codecov bot commented Jul 3, 2025 •

edited

Loading