xds/resolver: Add support for cluster specifier plugins #4987

zasweq · 2021-11-15T22:35:47Z

This PR adds support for cluster specifier specifier plugins in the xds resolver/config selector, as per RLS in xDS design. The functionality still left to implement is the RLS Cluster Specifier Plugin.

RELEASE NOTES: None

dfawley · 2021-11-15T22:50:28Z

xds/internal/resolver/serviceconfig.go

@@ -121,7 +133,11 @@ type routeCluster struct {

 type route struct {
 	m                 *xdsresource.CompositeMatcher // converted from route matchers
+
+	// Exactly one of clusterSpecifierPlugin or clusters will be set.
+    clusterSpecifierPlugin string


Can we put the CSP name into clusters instead so we don't need another field?

I see what your saying (persist as little state as possible), but I much prefer keeping this as a separate field. Adding csp to clusters would conflate an individual csp name to a WRR type which can hold onto many cluster names and chooses one randomly. This keeps the branching logic (two fields in state, two separate ways of persistence/handling), which started from the xdsclient.

It's not about "persisting state", it's about reducing & simplifying code and data structures. We use the same code paths already for when WRR is used vs. when a single cluster is specified directly, so why not fold CSP into it as well? In gRPC, they are equivalent: CSPs are the same as ordinary clusters from the name resolver and cluster manager LB policy's perspectives.

Kept the branching in newConfigSelector (for prefix + hardcoded weight 1 logic, which was in unmarshal_rds for singular cluster)...but now it's one codepath in SelectConfig/one piece of state clusters.

xds/internal/resolver/watch_service.go

zasweq

Thanks for the comments :D!

zasweq · 2021-11-16T18:11:27Z

xds/internal/resolver/serviceconfig.go

@@ -121,7 +133,11 @@ type routeCluster struct {

 type route struct {
 	m                 *xdsresource.CompositeMatcher // converted from route matchers
+
+	// Exactly one of clusterSpecifierPlugin or clusters will be set.
+    clusterSpecifierPlugin string


I see what your saying (persist as little state as possible), but I much prefer keeping this as a separate field. Adding csp to clusters would conflate an individual csp name to a WRR type which can hold onto many cluster names and chooses one randomly. This keeps the branching logic (two fields in state, two separate ways of persistence/handling), which started from the xdsclient.

xds/internal/resolver/watch_service.go

zasweq

Thanks for the comments! I like the simplicity push. Easwar mentioned you are good at "finding simple solutions to problems".

zasweq · 2021-11-16T20:30:06Z

xds/internal/resolver/serviceconfig.go

@@ -121,7 +133,11 @@ type routeCluster struct {

 type route struct {
 	m                 *xdsresource.CompositeMatcher // converted from route matchers
+
+	// Exactly one of clusterSpecifierPlugin or clusters will be set.
+    clusterSpecifierPlugin string


Kept the branching in newConfigSelector (for prefix + hardcoded weight 1 logic, which was in unmarshal_rds for singular cluster)...but now it's one codepath in SelectConfig/one piece of state clusters.

xds/internal/resolver/watch_service.go

dfawley · 2021-11-16T21:20:17Z

xds/internal/resolver/serviceconfig.go

 		if rt.ClusterSpecifierPlugin != "" {
+			clusters.Add(&routeCluster{
+				name:                     "cluster:" + rt.ClusterSpecifierPlugin,


Shouldn't this be "cluster_specifier_plugin:" + rt.ClusterSpecifierPlugin?

Please use a local to hold that instead of repeating it 4 times. Also, global consts for "cluster:" and "cluster_specifier_plugin:" would be a good idea to avoid any chance of a typo in one usage.

Yeah, good catch. Added global consts and also a local var for both branches.

Also, the comment isn't showing up on this PR, but I refactored the cluster initialization into another function. It made it much cleaner.

dfawley · 2021-11-16T21:21:15Z

xds/internal/resolver/serviceconfig.go

 			ci := r.activeClusters["cluster_specifier_plugin:" + rt.ClusterSpecifierPlugin]
 			if ci == nil {
 				ci = &clusterInfo{refCount: 0}
 				r.activeClusters["cluster_specifier_plugin:" + rt.ClusterSpecifierPlugin] = ci
 			}
 			cs.clusters["cluster_specifier_plugin:" + rt.ClusterSpecifierPlugin] = ci


Can this bit be shared with the below section? Maybe factored into another function?

zasweq

Thanks for the comments :D!

zasweq · 2021-11-16T23:09:18Z

xds/internal/resolver/serviceconfig.go

 		if rt.ClusterSpecifierPlugin != "" {
+			clusters.Add(&routeCluster{
+				name:                     "cluster:" + rt.ClusterSpecifierPlugin,


Yeah, good catch. Added global consts and also a local var for both branches.

zasweq · 2021-11-16T23:11:01Z

xds/internal/resolver/serviceconfig.go

 		if rt.ClusterSpecifierPlugin != "" {
+			clusters.Add(&routeCluster{
+				name:                     "cluster:" + rt.ClusterSpecifierPlugin,


Also, the comment isn't showing up on this PR, but I refactored the cluster initialization into another function. It made it much cleaner.

dfawley · 2021-11-22T23:56:48Z

xds/internal/balancer/cdsbalancer/cdsbalancer.go

@@ -179,7 +181,7 @@ func (b *cdsBalancer) handleClientConnUpdate(update *ccUpdate) {
 		b.handleErrorFromUpdate(err, true)
 		return
 	}
-	b.clusterHandler.updateRootCluster(update.clusterName)
+	b.clusterHandler.updateRootCluster(strings.TrimPrefix(update.clusterName, clusterPrefix))


Should this be stripped by the xds cluster manager LB policy instead? That is what I was expecting we'd do... I'm not sure how other languages are implementing this, either, since it apparently wasn't specified in the design.

Discussed in person, decided it would be best it to strip this in the name resolver, while constructing the Cluster Manager LB Config itself. (kept the cluster: prefix for the name of the cluster, but stripped it for the underlying cds policy)

dfawley · 2021-11-23T00:02:14Z

xds/internal/resolver/serviceconfig.go

-			ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: cluster}),
+		// Look into cluster specifier plugins, which hasn't had any prefix attached to it's cluster specifier plugin names,
+		// to determine the LB Config if the cluster is a CSP.
+		cspCfg, ok := clusterSpecifierPlugins[strings.TrimPrefix(cluster, clusterSpecifierPluginPrefix)]


So... if a CSP is called "cluster:foo" and a cluster named "foo" exists, then "cluster:foo" will be found in the CSP map, even though that isn't what was intended.

Should we be looking at whether the cluster from activeClusters's key StartsWith clusterSpecifierPluginPrefix instead here?

Simply persisted the config in clusterInfo, which solves this correctness issue.

dfawley · 2021-11-23T00:02:22Z

xds/internal/resolver/serviceconfig.go

+		// Look into cluster specifier plugins, which hasn't had any prefix attached to it's cluster specifier plugin names,
+		// to determine the LB Config if the cluster is a CSP.
+		cspCfg, ok := clusterSpecifierPlugins[strings.TrimPrefix(cluster, clusterSpecifierPluginPrefix)]
+		if ok {


Are we handling this case correctly (I think we're not):

First xDS update contains a cluster specifier plugin for RLS

RPC starts that selects that CSP

New xDS update removes that CSP

When generating this JSON here, we need to maintain the CSP config data and keep the "cluster" it references around until (2) is done

Ah, I found a solution that solves this problem and the comment about name collisions on the branch in serviceConfigJSON. I simply added the csp balancer configuration (if the cluster is a csp) to the cluster info. This reuses all the plumbing around active clusters, and also solves the correctness issue on the branch of the cluster specifier, as you can branch on clusterInfo.cspCfg != nil.

zasweq

Thanks for the comments :D! Adding test case for the situation outlined by you.

zasweq · 2021-11-23T07:33:45Z

xds/internal/resolver/serviceconfig.go

+		// Look into cluster specifier plugins, which hasn't had any prefix attached to it's cluster specifier plugin names,
+		// to determine the LB Config if the cluster is a CSP.
+		cspCfg, ok := clusterSpecifierPlugins[strings.TrimPrefix(cluster, clusterSpecifierPluginPrefix)]
+		if ok {


Ah, I found a solution that solves this problem and the comment about name collisions on the branch in serviceConfigJSON. I simply added the csp balancer configuration (if the cluster is a csp) to the cluster info. This reuses all the plumbing around active clusters, and also solves the correctness issue on the branch of the cluster specifier, as you can branch on clusterInfo.cspCfg != nil.

zasweq · 2021-11-23T07:34:16Z

xds/internal/resolver/serviceconfig.go

-			ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: cluster}),
+		// Look into cluster specifier plugins, which hasn't had any prefix attached to it's cluster specifier plugin names,
+		// to determine the LB Config if the cluster is a CSP.
+		cspCfg, ok := clusterSpecifierPlugins[strings.TrimPrefix(cluster, clusterSpecifierPluginPrefix)]


Simply persisted the config in clusterInfo, which solves this correctness issue.

dfawley · 2021-11-24T20:45:49Z

As mentioned offline, this is blocked on a decision of how to handle CSP config changes.

dfawley · 2021-11-30T16:56:07Z

xds/internal/resolver/serviceconfig.go

+			}
+		} else {
+			children[cluster] = xdsChildConfig{
+				ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: strings.TrimPrefix(cluster, clusterPrefix)}),


Can we store this in the activeClusters map, too, instead of creating it here? That would give us a more unified flow through here. I.e.:

type clusterInfo struct { ... cfg balancerConfig // either the csp config or the cds cluster config }

And this loop becomes simply:

for cluster, ci := range activeClusters { children[cluster] = xdsChildConfig{ChildPolicy: ci.cfg} }

(Or store an xdsChildConfig in clusterInfo, even.)

Thought about this for a while, chose to persist the whole child config.

dfawley · 2021-11-30T17:05:32Z

xds/internal/resolver/serviceconfig.go

+// initializeCluster initializes entries in cs.clusters map, creating entries in
+// r.activeClusters as necessary.  Any created entries will be set to zero as
+// they will be incremented by incRefs.
+func (r *xdsResolver) initializeCluster(clusterName string, cs *configSelector) {


I think this wants to be a method on configSelector instead. It can access the xdsResolver that way, too for adding to activeClusters. You can also pass it a balancerConfig to store in the clusterInfo as mentioned above.

Ah, great idea. These two comments make it much cleaner. Switched.

dfawley · 2021-11-30T17:17:39Z

xds/internal/resolver/watch_service.go

@@ -35,6 +36,9 @@ import (
 type serviceUpdate struct {
 	// virtualHost contains routes and other configuration to route RPCs.
 	virtualHost *xdsresource.VirtualHost
+	// clusterSpecifierPlugins contain the configurations for any cluster


nit: containS (the ...Plugins map is a singular)

Ah, I see, switched.

dfawley · 2021-11-30T17:22:45Z

xds/internal/resolver/xds_resolver_test.go

 	if !reflect.DeepEqual(picks, want) {
 		t.Errorf("picked clusters = %v; want %v", picks, want)
 	}
 }

+func init() {


Can you move this & related tests to a new file? This one is already too big.

Maybe cluster_specifier_plugin_test.go?

dfawley · 2021-11-30T17:25:47Z

xds/internal/resolver/xds_resolver_test.go

+	ArbitraryField string `json:"arbitrary_field"`
+}
+
+func TestXDSResolverClusterSpecifierPlugin(t *testing.T) {


A quick 1-liner description of what this tests would be helpful. E.g. // Tests that cluster specifier plugins produce the correct service config. or something SG.

Added a comment with what you had and also with explaining picking a cluster as well.

dfawley · 2021-11-30T17:30:16Z

xds/internal/resolver/xds_resolver_test.go

+		t.Fatal("want: ", cmp.Diff(nil, wantSCParsed3.Config))
+	}
+}
+


Please add another test that covers updates to a CSP configuration - it should change in the output service config.

Added. I'm assuming this is for the the missing functionality for the graceful switch with the same LB policy.

dfawley · 2021-11-30T17:31:55Z

xds/internal/resolver/xds_resolver_test.go

+        "cluster_specifier_plugin:cspA":{
+          "childPolicy":[{"csp_experimental":{"arbitrary_field":"anythingA"}}]
+        },
+		"cluster_specifier_plugin:cspB":{


The formatting here looks slightly off.

I just checked the other json with multiple children in xds_resolver_test.go, and it looks the same? In what way does this look off? Leaving as is for now.

dfawley · 2021-11-30T17:32:28Z

xds/internal/resolver/xds_resolver_test.go

+	wantJSON3 := `{"loadBalancingConfig":[{
+    "xds_cluster_manager_experimental":{
+      "children":{
+		"cluster_specifier_plugin:cspB":{


Formatting again

See comment above.

zasweq

Thanks for the comments :D!

zasweq · 2021-11-30T17:57:44Z

xds/internal/resolver/watch_service.go

@@ -35,6 +36,9 @@ import (
 type serviceUpdate struct {
 	// virtualHost contains routes and other configuration to route RPCs.
 	virtualHost *xdsresource.VirtualHost
+	// clusterSpecifierPlugins contain the configurations for any cluster


Ah, I see, switched.

zasweq · 2021-11-30T18:03:03Z

xds/internal/resolver/xds_resolver_test.go

+	ArbitraryField string `json:"arbitrary_field"`
+}
+
+func TestXDSResolverClusterSpecifierPlugin(t *testing.T) {


Added a comment with what you had and also with explaining picking a cluster as well.

zasweq · 2021-11-30T18:25:48Z

xds/internal/resolver/xds_resolver_test.go

 	if !reflect.DeepEqual(picks, want) {
 		t.Errorf("picked clusters = %v; want %v", picks, want)
 	}
 }

+func init() {


zasweq · 2021-11-30T18:38:29Z

xds/internal/resolver/xds_resolver_test.go

+        "cluster_specifier_plugin:cspA":{
+          "childPolicy":[{"csp_experimental":{"arbitrary_field":"anythingA"}}]
+        },
+		"cluster_specifier_plugin:cspB":{


I just checked the other json with multiple children in xds_resolver_test.go, and it looks the same? In what way does this look off? Leaving as is for now.

zasweq · 2021-11-30T18:38:58Z

xds/internal/resolver/xds_resolver_test.go

+	wantJSON3 := `{"loadBalancingConfig":[{
+    "xds_cluster_manager_experimental":{
+      "children":{
+		"cluster_specifier_plugin:cspB":{


See comment above.

zasweq · 2021-11-30T18:48:33Z

xds/internal/resolver/xds_resolver_test.go

+		t.Fatal("want: ", cmp.Diff(nil, wantSCParsed3.Config))
+	}
+}
+


Added. I'm assuming this is for the the missing functionality for the graceful switch with the same LB policy.

zasweq · 2021-11-30T18:57:26Z

xds/internal/resolver/serviceconfig.go

+			}
+		} else {
+			children[cluster] = xdsChildConfig{
+				ChildPolicy: newBalancerConfig(cdsName, cdsBalancerConfig{Cluster: strings.TrimPrefix(cluster, clusterPrefix)}),


Thought about this for a while, chose to persist the whole child config.

zasweq · 2021-11-30T19:14:41Z

xds/internal/resolver/serviceconfig.go

+// initializeCluster initializes entries in cs.clusters map, creating entries in
+// r.activeClusters as necessary.  Any created entries will be set to zero as
+// they will be incremented by incRefs.
+func (r *xdsResolver) initializeCluster(clusterName string, cs *configSelector) {


Ah, great idea. These two comments make it much cleaner. Switched.

dfawley

LGTM modulo the formatting nit.

dfawley · 2021-12-03T21:14:31Z

xds/internal/resolver/cluster_specifier_plugin_test.go

+        "cluster_specifier_plugin:cspA":{
+          "childPolicy":[{"csp_experimental":{"arbitrary_field":"anythingA"}}]
+        },
+		"cluster_specifier_plugin:cspB":{


New comment from old comment thread, since the code was moved:

This should line up with "cluster_specifier_plugin:cspA" (outdent 1 tab). And the same situation below @ line 356.

It's not a correctness thing, it's just formatting.

Added support for cluster specifier plugins in xds resolver

44160e9

zasweq requested review from menghanl, dfawley and easwars November 15, 2021 22:35

dfawley reviewed Nov 15, 2021

View reviewed changes

zasweq commented Nov 16, 2021

View reviewed changes

Responded to Doug's comments

8a6bf55

dfawley reviewed Nov 16, 2021

View reviewed changes

zasweq commented Nov 16, 2021

View reviewed changes

Responded to Doug's comments

a616529

zasweq added the Type: Feature New features or improvements in behavior label Nov 17, 2021

zasweq added this to the 1.43 release milestone Nov 17, 2021

zasweq changed the title ~~Draft: Added support for cluster specifier plugins in xds resolver~~ xds: Added support for cluster specifier plugins in xds resolver Nov 17, 2021

Responded to Doug's comments and added tests

6393ab4

zasweq force-pushed the add-csp branch from 179acf6 to 6393ab4 Compare November 18, 2021 02:11

zasweq requested review from dfawley and removed request for menghanl and easwars November 18, 2021 02:22

zasweq assigned dfawley Nov 18, 2021

zasweq mentioned this pull request Nov 19, 2021

draft: Added RLS Cluster Specifier Plugin #4996

Closed

dfawley reviewed Nov 23, 2021

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Nov 23, 2021

zasweq commented Nov 23, 2021

View reviewed changes

Responded to Doug's comments

af713af

zasweq assigned dfawley and unassigned zasweq Nov 23, 2021

zasweq mentioned this pull request Nov 24, 2021

xds: Add RLS Cluster Specifier Plugin #5004

Merged

dfawley changed the title ~~xds: Added support for cluster specifier plugins in xds resolver~~ xds/resolver: Add support for cluster specifier plugins Nov 24, 2021

dfawley added the Status: Blocked label Nov 24, 2021

dfawley reviewed Nov 30, 2021

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Nov 30, 2021

zasweq commented Nov 30, 2021

View reviewed changes

Responded to Doug's comments

d219ac6

zasweq assigned dfawley and unassigned zasweq Nov 30, 2021

dfawley removed the Status: Blocked label Dec 3, 2021

dfawley approved these changes Dec 3, 2021

View reviewed changes

dfawley assigned zasweq and unassigned dfawley Dec 3, 2021

Formatting

5b4f327

zasweq merged commit 3786ae1 into grpc:master Dec 6, 2021

github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2022

xds/resolver: Add support for cluster specifier plugins #4987

xds/resolver: Add support for cluster specifier plugins #4987

Conversation

zasweq commented Nov 15, 2021 • edited Loading

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

zasweq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq left a comment • edited Loading

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

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

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

dfawley commented Nov 24, 2021

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

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

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zasweq commented Nov 15, 2021 •

edited

Loading

zasweq left a comment •

edited

Loading