clusterimpl: use gsb.UpdateClientConnState instead of switchTo, on receipt of config update (#7567)

Author: aranjans

xds/internal/balancer/clusterimpl/balancer_test.go

@@ -20,6 +20,7 @@ package clusterimpl
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"strings"
@@ -40,6 +41,7 @@ import (
 	"google.golang.org/grpc/internal/xds"
 	"google.golang.org/grpc/internal/xds/bootstrap"
 	"google.golang.org/grpc/resolver"
+	"google.golang.org/grpc/serviceconfig"
 	xdsinternal "google.golang.org/grpc/xds/internal"
 	"google.golang.org/grpc/xds/internal/testutils/fakeclient"
 	"google.golang.org/grpc/xds/internal/xdsclient"
@@ -839,6 +841,108 @@ func (s) TestUpdateLRSServer(t *testing.T) {
 	}
 }
 
+// Test verifies that child policies was updated on receipt of
+// configuration update.
+func (s) TestChildPolicyUpdatedOnConfigUpdate(t *testing.T) {
+	xdsC := fakeclient.NewClient()
+
+	builder := balancer.Get(Name)
+	cc := testutils.NewBalancerClientConn(t)
+	b := builder.Build(cc, balancer.BuildOptions{})
+	defer b.Close()
+
+	// Keep track of which child policy was updated
+	updatedChildPolicy := ""
+
+	// Create stub balancers to track config updates
+	const (
+		childPolicyName1 = "stubBalancer1"
+		childPolicyName2 = "stubBalancer2"
+	)
+
+	stub.Register(childPolicyName1, stub.BalancerFuncs{
+		UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error {
+			updatedChildPolicy = childPolicyName1
+			return nil
+		},
+	})
+
+	stub.Register(childPolicyName2, stub.BalancerFuncs{
+		UpdateClientConnState: func(_ *stub.BalancerData, _ balancer.ClientConnState) error {
+			updatedChildPolicy = childPolicyName2
+			return nil
+		},
+	})
+
+	// Initial config update with childPolicyName1
+	if err := b.UpdateClientConnState(balancer.ClientConnState{
+		ResolverState: xdsclient.SetClient(resolver.State{Addresses: testBackendAddrs}, xdsC),
+		BalancerConfig: &LBConfig{
+			Cluster: testClusterName,
+			ChildPolicy: &internalserviceconfig.BalancerConfig{
+				Name: childPolicyName1,
+			},
+		},
+	}); err != nil {
+		t.Fatalf("Error updating the config: %v", err)
+	}
+
+	if updatedChildPolicy != childPolicyName1 {
+		t.Fatal("Child policy 1 was not updated on initial configuration update.")
+	}
+
+	// Second config update with childPolicyName2
+	if err := b.UpdateClientConnState(balancer.ClientConnState{
+		ResolverState: xdsclient.SetClient(resolver.State{Addresses: testBackendAddrs}, xdsC),
+		BalancerConfig: &LBConfig{
+			Cluster: testClusterName,
+			ChildPolicy: &internalserviceconfig.BalancerConfig{
+				Name: childPolicyName2,
+			},
+		},
+	}); err != nil {
+		t.Fatalf("Error updating the config: %v", err)
+	}
+
+	if updatedChildPolicy != childPolicyName2 {
+		t.Fatal("Child policy 2 was not updated after child policy name change.")
+	}
+}
+
+// Test verifies that config update fails if child policy config
+// failed to parse.
+func (s) TestFailedToParseChildPolicyConfig(t *testing.T) {
+	xdsC := fakeclient.NewClient()
+
+	builder := balancer.Get(Name)
+	cc := testutils.NewBalancerClientConn(t)
+	b := builder.Build(cc, balancer.BuildOptions{})
+	defer b.Close()
+
+	// Create a stub balancer which fails to ParseConfig.
+	const parseConfigError = "failed to parse config"
+	const childPolicyName = "stubBalancer-FailedToParseChildPolicyConfig"
+	stub.Register(childPolicyName, stub.BalancerFuncs{
+		ParseConfig: func(_ json.RawMessage) (serviceconfig.LoadBalancingConfig, error) {
+			return nil, errors.New(parseConfigError)
+		},
+	})
+
+	err := b.UpdateClientConnState(balancer.ClientConnState{
+		ResolverState: xdsclient.SetClient(resolver.State{Addresses: testBackendAddrs}, xdsC),
+		BalancerConfig: &LBConfig{
+			Cluster: testClusterName,
+			ChildPolicy: &internalserviceconfig.BalancerConfig{
+				Name: childPolicyName,
+			},
+		},
+	})
+
+	if err == nil || !strings.Contains(err.Error(), parseConfigError) {
+		t.Fatalf("Got error: %v, want error: %s", err, parseConfigError)
+	}
+}
+
 func assertString(f func() (string, error)) string {
 	s, err := f()
 	if err != nil {

xds/internal/balancer/clusterimpl/clusterimpl.go

@@ -231,11 +231,16 @@ func (b *clusterImplBalancer) updateClientConnState(s balancer.ClientConnState)
 		return err
 	}
 
-	if b.config == nil || b.config.ChildPolicy.Name != newConfig.ChildPolicy.Name {
-		if err := b.child.SwitchTo(bb); err != nil {
-			return fmt.Errorf("error switching to child of type %q: %v", newConfig.ChildPolicy.Name, err)
-		}
+	// Build config for the gracefulswitch balancer. It is safe to ignore JSON
+	// marshaling errors here, since the config was already validated as part of
+	// ParseConfig().
+	cfg := []map[string]any{{newConfig.ChildPolicy.Name: newConfig.ChildPolicy.Config}}
+	cfgJSON, _ := json.Marshal(cfg)
+	parsedCfg, err := gracefulswitch.ParseConfig(cfgJSON)
+	if err != nil {
+		return err
 	}
+
 	b.config = newConfig
 
 	b.telemetryLabels = newConfig.TelemetryLabels
@@ -250,7 +255,7 @@ func (b *clusterImplBalancer) updateClientConnState(s balancer.ClientConnState)
 	// Addresses and sub-balancer config are sent to sub-balancer.
 	return b.child.UpdateClientConnState(balancer.ClientConnState{
 		ResolverState:  s.ResolverState,
-		BalancerConfig: b.config.ChildPolicy.Config,
+		BalancerConfig: parsedCfg,
 	})
 }