diff --git a/test/pickfirst_test.go b/test/pickfirst_test.go index 7f36ed400c8..55659b928a5 100644 --- a/test/pickfirst_test.go +++ b/test/pickfirst_test.go @@ -382,6 +382,7 @@ func (s) TestPickFirst_StickyTransientFailure(t *testing.T) { wg.Wait() } +// Tests the PF LB policy with shuffling enabled. func (s) TestPickFirst_ShuffleAddressList(t *testing.T) { defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig) envconfig.PickFirstLBConfig = true @@ -393,6 +394,7 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) { grpcrand.Shuffle = func(n int, f func(int, int)) { if n != 2 { t.Errorf("Shuffle called with n=%v; want 2", n) + return } f(0, 1) // reverse the two addresses } @@ -435,6 +437,8 @@ func (s) TestPickFirst_ShuffleAddressList(t *testing.T) { } } +// Tests the PF LB policy with the environment variable support of address list +// shuffling disabled. func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) { defer func(old bool) { envconfig.PickFirstLBConfig = old }(envconfig.PickFirstLBConfig) envconfig.PickFirstLBConfig = false @@ -446,6 +450,7 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) { grpcrand.Shuffle = func(n int, f func(int, int)) { if n != 2 { t.Errorf("Shuffle called with n=%v; want 2", n) + return } f(0, 1) // reverse the two addresses } @@ -457,15 +462,10 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer cancel() - // Push an update with both addresses and shuffling disabled. We should - // connect to backend 0. - r.UpdateState(resolver.State{Addresses: []resolver.Address{addrs[0], addrs[1]}}) - if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil { - t.Fatal(err) - } - // Send a config with shuffling enabled. This will reverse the addresses, - // but the channel should still be connected to backend 0. + // so we should connect to backend 1 if shuffling is supported. However + // with it disabled at the start of the test, we will connect to backend 0 + // instead. shufState := resolver.State{ ServiceConfig: parseServiceConfig(t, r, serviceConfig), Addresses: []resolver.Address{addrs[0], addrs[1]}, @@ -474,18 +474,4 @@ func (s) TestPickFirst_ShuffleAddressListDisabled(t *testing.T) { if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil { t.Fatal(err) } - - // Send a resolver update with no addresses. This should push the channel - // into TransientFailure. - r.UpdateState(resolver.State{}) - awaitState(ctx, t, cc, connectivity.TransientFailure) - - // Send the same config as last time with shuffling enabled. Since we are - // not connected to backend 0, we should connect to backend 1 if shuffling - // is supported. However with it disabled at the start of the test, we - // will connect to backend 0 instead. - r.UpdateState(shufState) - if err := pickfirst.CheckRPCsToBackend(ctx, cc, addrs[0]); err != nil { - t.Fatal(err) - } } diff --git a/xds/internal/xdsclient/xdslbregistry/converter/converter.go b/xds/internal/xdsclient/xdslbregistry/converter/converter.go index 6bafa647794..c5d5afe4ebd 100644 --- a/xds/internal/xdsclient/xdslbregistry/converter/converter.go +++ b/xds/internal/xdsclient/xdslbregistry/converter/converter.go @@ -28,7 +28,9 @@ import ( "strings" "github.com/golang/protobuf/proto" + "google.golang.org/grpc" "google.golang.org/grpc/balancer" + "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/balancer/weightedroundrobin" "google.golang.org/grpc/internal/envconfig" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" @@ -110,11 +112,11 @@ func convertPickFirstProtoToServiceConfig(rawProto []byte, _ int) (json.RawMessa if err != nil { return nil, fmt.Errorf("error marshaling JSON for type %T: %v", pfCfg, err) } - return makeBalancerConfigJSON("pick_first", js), nil + return makeBalancerConfigJSON(grpc.PickFirstBalancerName, js), nil } func convertRoundRobinProtoToServiceConfig([]byte, int) (json.RawMessage, error) { - return makeBalancerConfigJSON("round_robin", json.RawMessage("{}")), nil + return makeBalancerConfigJSON(roundrobin.Name, json.RawMessage("{}")), nil } type wrrLocalityLBConfig struct { diff --git a/xds/internal/xdsclient/xdslbregistry/xdslbregistry_test.go b/xds/internal/xdsclient/xdslbregistry/xdslbregistry_test.go index 9a7f5be5310..8ffb6411424 100644 --- a/xds/internal/xdsclient/xdslbregistry/xdslbregistry_test.go +++ b/xds/internal/xdsclient/xdslbregistry/xdslbregistry_test.go @@ -106,7 +106,7 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) { wantConfig: `[{"ring_hash_experimental": { "minRingSize": 10, "maxRingSize": 100 }}]`, }, { - name: "pick_first", + name: "pick_first_shuffle", policy: &v3clusterpb.LoadBalancingPolicy{ Policies: []*v3clusterpb.LoadBalancingPolicy_Policy{ { @@ -120,6 +120,19 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) { }, wantConfig: `[{"pick_first": { "shuffleAddressList": true }}]`, }, + { + name: "pick_first", + policy: &v3clusterpb.LoadBalancingPolicy{ + Policies: []*v3clusterpb.LoadBalancingPolicy_Policy{ + { + TypedExtensionConfig: &v3corepb.TypedExtensionConfig{ + TypedConfig: testutils.MarshalAny(&v3pickfirstpb.PickFirst{}), + }, + }, + }, + }, + wantConfig: `[{"pick_first": { "shuffleAddressList": false }}]`, + }, { name: "round_robin", policy: &v3clusterpb.LoadBalancingPolicy{ @@ -301,6 +314,8 @@ func (s) TestConvertToServiceConfigSuccess(t *testing.T) { if err != nil { t.Fatalf("ConvertToServiceConfig(%s) failed: %v", pretty.ToJSON(test.policy), err) } + // got and want must be unmarshalled since JSON strings shouldn't + // generally be directly compared. var got []map[string]interface{} if err := json.Unmarshal(rawJSON, &got); err != nil { t.Fatalf("Error unmarshalling rawJSON (%q): %v", rawJSON, err)