cdsbalancer: switch cluster watch to generic xDS client API #6600

easwars · 2023-08-30T03:20:07Z

This is a significant simplication of the existing implementation of the CDS LB policy:

Replaces the existing clusterHandler, clusterNode types with an extremely simple, dumbed down type called clusterWatcher that simply forwards updates from the xDS client to the parent LB policy
The logic for building the discovery mechanisms is now solely under the responsibility of the type implementing the CDS LB policy, and it does so by maintaining a set of clusterWatchers, and tries to recursively traverse the cluster graph every time one of the underlying clusters receives an update
Also, added a couple of new tests for scenarios relating to cycles in the aggregate graph

And as part of all this, replaced the call to xdsclient.WatchCluster() with the helper function for the generic xDS API, xdsresource.WatchCluster().

In a follow-up PR, I will delete the WatchCluster method from the xdsclient.XDSClient interface. I wanted to keep that separate to keep the focus of this PR on the changes to the CDS LB policy.

#resource-agnostic-xdsclient-api

RELEASE NOTES: none

zasweq

I think it's a good direction. Mark mentioned that since it's not reused it makes sense for this logic to be part of the cds balancer, especially since the graph like structure can be handled from a traversal owned by cds balancer.

xds/internal/balancer/cdsbalancer/cdsbalancer.go

xds/internal/balancer/cdsbalancer/cluster_handler_test.go

zasweq · 2023-09-08T01:03:38Z

xds/internal/balancer/cdsbalancer/cluster_handler_test.go

@@ -657,8 +657,10 @@ func (s) TestAggregatedClusterSuccess_IgnoreDups(t *testing.T) {
 // Tests the scenario where the aggregate cluster graph has a node that has


this file can be renamed now :)

Thought about it, but resisted the urge. Will probably do it after I this PR is approved, as the last step before merging.

Alright sg.

xds/internal/balancer/cdsbalancer/cluster_watcher.go

easwars · 2023-09-08T18:29:21Z

Thanks for the review @zasweq

xds/internal/balancer/cdsbalancer/cdsbalancer.go

zasweq · 2023-09-11T20:10:36Z

xds/internal/balancer/cdsbalancer/cluster_handler_test.go

@@ -657,8 +657,10 @@ func (s) TestAggregatedClusterSuccess_IgnoreDups(t *testing.T) {
 // Tests the scenario where the aggregate cluster graph has a node that has


Alright sg.

xds/internal/balancer/cdsbalancer/cluster_handler_test.go

xds/internal/balancer/cdsbalancer/cdsbalancer.go

zasweq

LGTM. Some minor nits. Great change overall, just scales up the generation of DMs to recurse through list. Fun one to review :).

zasweq · 2023-09-12T02:12:23Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+
+// Handles a good Cluster update from the xDS client. Kicks off the discovery
+// mechanism generation process from the top-level cluster and if the cluster
+// graph is resolved, generated child policy config and pushes it down.


Nit: generates*

zasweq · 2023-09-12T02:15:51Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+		b.logger.Warningf("Failed to generate discovery mechanisms: %v", err)
+		b.onClusterError(b.lbCfg.ClusterName, err)


Line 450 and 451 send the same error string down as the logging string. I think the prefix "Failed to generate discovery mechanisms" adds valuable information here.

Realized that onClusterError also logs the error. So, got rid of the log statement here, and in other places where I was logging and passing the error to onClusterError.

zasweq · 2023-09-12T02:17:42Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+		if ok {
+			continue
+		}
+


Nit: nix newline.

zasweq · 2023-09-12T02:20:12Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+// Generates discovery mechanisms corresponding to the clusters in the aggregate
+// cluster graph. If a new cluster is encountered when traversing the aggregate
+// cluster graph, a watcher is created for it.


Optional: this doesn't necessarily apply only to aggregate clusters (root cluster can be a leaf, or it can recurse and hit a leaf). I'm fine as is though.

Modified the comment a bit to indicate that this is a recursive function and that the base case for the recursion is a leaf cluster.

zasweq · 2023-09-12T02:21:17Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

+			dms, ok, err = b.generateDMsForCluster(child, depth+1, dms, clustersSeen)
+			if err != nil || !ok {
+				missingCluster = true
+


Nix newline please.

easwars · 2023-09-12T16:37:50Z

@bitcoinfinancier : Just get out of here.

easwars · 2023-09-12T16:38:32Z

LGTM. Some minor nits. Great change overall, just scales up the generation of DMs to recurse through list.

Thanks for the detailed review.

Fun one to review :).

I have a few more coming up to switch LDS and RDS :)

cdsbalancer: switch cluster watch to generic xDS client API

e8d43f5

easwars requested a review from zasweq August 30, 2023 03:20

easwars assigned zasweq Aug 30, 2023

easwars added the Type: Internal Cleanup Refactors, etc label Aug 30, 2023

easwars added this to the 1.58 Release milestone Aug 30, 2023

arvindbr8 modified the milestones: 1.58 Release, 1.59 Release Sep 6, 2023

zasweq requested changes Sep 8, 2023

View reviewed changes

zasweq assigned easwars and unassigned zasweq Sep 8, 2023

easwars added 3 commits September 8, 2023 17:23

add the actual max depth value to the error string

d64a13b

fix a comment for correctness

d6c60b2

other review comments

e338278

easwars assigned zasweq and unassigned easwars Sep 8, 2023

zasweq requested changes Sep 11, 2023

View reviewed changes

zasweq assigned easwars and unassigned zasweq Sep 11, 2023

ensure maximal graph coverage even during errors

822bc0b

easwars assigned zasweq and unassigned easwars Sep 12, 2023

zasweq approved these changes Sep 12, 2023

View reviewed changes

zasweq assigned easwars and unassigned zasweq Sep 12, 2023

last round of review comments

d16a312

easwars added 2 commits September 12, 2023 16:40

rename one of the tests

c5e5b53

rename the test file

cd8e820

easwars merged commit 82a568d into grpc:master Sep 12, 2023
1 check passed

easwars deleted the cdsbalancer_cleanup branch September 12, 2023 17:02

easwars mentioned this pull request Sep 12, 2023

xdsclient: completely remove the old WatchCluster API #6621

Merged

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

cdsbalancer: switch cluster watch to generic xDS client API #6600

cdsbalancer: switch cluster watch to generic xDS client API #6600

easwars commented Aug 30, 2023 •

edited

Loading

zasweq left a comment

zasweq Sep 8, 2023

easwars Sep 8, 2023

zasweq Sep 11, 2023

easwars commented Sep 8, 2023

zasweq Sep 11, 2023

zasweq left a comment

zasweq Sep 12, 2023

easwars Sep 12, 2023

zasweq Sep 12, 2023

easwars Sep 12, 2023

zasweq Sep 12, 2023

easwars Sep 12, 2023

zasweq Sep 12, 2023

easwars Sep 12, 2023

zasweq Sep 12, 2023

easwars Sep 12, 2023

easwars commented Sep 12, 2023

easwars commented Sep 12, 2023

		@@ -657,8 +657,10 @@ func (s) TestAggregatedClusterSuccess_IgnoreDups(t *testing.T) {
		// Tests the scenario where the aggregate cluster graph has a node that has

		b.logger.Warningf("Failed to generate discovery mechanisms: %v", err)
		b.onClusterError(b.lbCfg.ClusterName, err)

cdsbalancer: switch cluster watch to generic xDS client API #6600

cdsbalancer: switch cluster watch to generic xDS client API #6600

Conversation

easwars commented Aug 30, 2023 • edited Loading

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

easwars commented Sep 8, 2023

Choose a reason for hiding this comment

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

easwars commented Sep 12, 2023

easwars commented Sep 12, 2023

easwars commented Aug 30, 2023 •

edited

Loading