From 72138c79b7c764341baf26e4d513e9b8f529ab11 Mon Sep 17 00:00:00 2001 From: Zach Reyes <39203661+zasweq@users.noreply.github.com> Date: Mon, 23 May 2022 15:14:50 -0400 Subject: [PATCH 1/6] Added branching logic for Outlier Detection configuration in config builder --- .../clusterresolver/clusterresolver_test.go | 117 ++++++++++ .../balancer/clusterresolver/configbuilder.go | 49 +++- .../clusterresolver/configbuilder_test.go | 216 ++++++++++++++++++ .../balancer/outlierdetection/balancer.go | 121 ++++++++++ .../outlierdetection/balancer_test.go | 192 ++++++++++++++++ 5 files changed, 693 insertions(+), 2 deletions(-) create mode 100644 xds/internal/balancer/outlierdetection/balancer.go create mode 100644 xds/internal/balancer/outlierdetection/balancer_test.go diff --git a/xds/internal/balancer/clusterresolver/clusterresolver_test.go b/xds/internal/balancer/clusterresolver/clusterresolver_test.go index 3b0843f6807..0b1c4bf1f30 100644 --- a/xds/internal/balancer/clusterresolver/clusterresolver_test.go +++ b/xds/internal/balancer/clusterresolver/clusterresolver_test.go @@ -25,12 +25,20 @@ import ( "time" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "google.golang.org/grpc/balancer" + "google.golang.org/grpc/balancer/roundrobin" + "google.golang.org/grpc/balancer/weightedtarget" "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/grpctest" + internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/resolver" "google.golang.org/grpc/xds/internal" + "google.golang.org/grpc/xds/internal/balancer/clusterimpl" + "google.golang.org/grpc/xds/internal/balancer/outlierdetection" + "google.golang.org/grpc/xds/internal/balancer/priority" "google.golang.org/grpc/xds/internal/testutils/fakeclient" "google.golang.org/grpc/xds/internal/xdsclient" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" @@ -130,6 +138,18 @@ func (f *fakeChildBalancer) Close() {} func (f *fakeChildBalancer) ExitIdle() {} +func (f *fakeChildBalancer) waitForClientConnStateChangeVerifyBalancerConfig(ctx context.Context, wantCCS balancer.ClientConnState) error { + ccs, err := f.clientConnState.Receive(ctx) + if err != nil { + return err + } + gotCCS := ccs.(balancer.ClientConnState) + if diff := cmp.Diff(gotCCS, wantCCS, cmpopts.IgnoreFields(resolver.State{}, "Addresses", "ServiceConfig", "Attributes")); diff != "" { + return fmt.Errorf("received unexpected ClientConnState, diff (-got +want): %v", diff) + } + return nil +} + func (f *fakeChildBalancer) waitForClientConnStateChange(ctx context.Context) error { _, err := f.clientConnState.Receive(ctx) if err != nil { @@ -500,3 +520,100 @@ func newLBConfigWithOneEDS(edsServiceName string) *LBConfig { }}, } } + +func newLBConfigWithOneEDSAndOutlierDetection(edsServiceName string, odCfg *outlierdetection.LBConfig) *LBConfig { + lbCfg := newLBConfigWithOneEDS(edsServiceName) + lbCfg.DiscoveryMechanisms[0].OutlierDetection = odCfg + return lbCfg +} + +// TestOutlierDetection tests the Balancer Config sent down to the child +// priority balancer when Outlier Detection is turned on. The Priority +// Configuration sent downward should have a top level Outlier Detection Policy +// for each priority. +func (s) TestOutlierDetection(t *testing.T) { + oldOutlierDetection := envconfig.XDSOutlierDetection + envconfig.XDSOutlierDetection = true + defer func() { + envconfig.XDSOutlierDetection = oldOutlierDetection + }() + + edsLBCh := testutils.NewChannel() + xdsC, cleanup := setup(edsLBCh) + defer cleanup() + builder := balancer.Get(Name) + edsB := builder.Build(newNoopTestClientConn(), balancer.BuildOptions{}) + if edsB == nil { + t.Fatalf("builder.Build(%s) failed and returned nil", Name) + } + defer edsB.Close() + + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + + // Update Cluster Resolver with Client Conn State with Outlier Detection + // configuration present. This is what will be passed down to this balancer, + // as CDS Balancer gets the Cluster Update and converts the Outlier + // Detection data to an Outlier Detection configuration and sends it to this + // level. + if err := edsB.UpdateClientConnState(balancer.ClientConnState{ + ResolverState: xdsclient.SetClient(resolver.State{}, xdsC), + BalancerConfig: newLBConfigWithOneEDSAndOutlierDetection(testEDSServcie, noopODCfg), + }); err != nil { + t.Fatal(err) + } + if _, err := xdsC.WaitForWatchEDS(ctx); err != nil { + t.Fatalf("xdsClient.WatchEndpoints failed with error: %v", err) + } + + // Invoke EDS Callback - causes child balancer to be built and then + // UpdateClientConnState called on it with Outlier Detection as a direct + // child. + xdsC.InvokeWatchEDSCallback("", defaultEndpointsUpdate, nil) + edsLB, err := waitForNewChildLB(ctx, edsLBCh) + if err != nil { + t.Fatal(err) + } + + localityID := internal.LocalityID{Zone: "zone"} + // The priority configuration generated should have Outlier Detection as a + // direct child due to Outlier Detection being turned on. + pCfgWant := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: &internalserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: testClusterName, + EDSServiceName: "test-eds-service-name", + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: weightedtarget.Name, + Config: &weightedtarget.LBConfig{ + Targets: map[string]weightedtarget.Target{ + assertString(localityID.ToString): { + Weight: 100, + ChildPolicy: &internalserviceconfig.BalancerConfig{Name: roundrobin.Name}, + }, + }, + }, + }, + }, + }, + }, + }, + IgnoreReresolutionRequests: true, + }, + }, + Priorities: []string{"priority-0-0"}, + } + + if err := edsLB.waitForClientConnStateChangeVerifyBalancerConfig(ctx, balancer.ClientConnState{ + BalancerConfig: pCfgWant, + }); err != nil { + t.Fatalf("EDS impl got unexpected update: %v", err) + } +} diff --git a/xds/internal/balancer/clusterresolver/configbuilder.go b/xds/internal/balancer/clusterresolver/configbuilder.go index 7eb76dd51d7..5e153c5be51 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder.go +++ b/xds/internal/balancer/clusterresolver/configbuilder.go @@ -26,11 +26,13 @@ import ( "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/balancer/weightedroundrobin" "google.golang.org/grpc/balancer/weightedtarget" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/hierarchy" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/resolver" "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/balancer/clusterimpl" + "google.golang.org/grpc/xds/internal/balancer/outlierdetection" "google.golang.org/grpc/xds/internal/balancer/priority" "google.golang.org/grpc/xds/internal/balancer/ringhash" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" @@ -128,30 +130,73 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi if err != nil { return nil, nil, err } + var odCfgs map[string]*outlierdetection.LBConfig + if envconfig.XDSOutlierDetection { + odCfgs = convertClusterImplMapToOutlierDetection(configs, p.mechanism.OutlierDetection) + } retConfig.Priorities = append(retConfig.Priorities, names...) + retAddrs = append(retAddrs, addrs...) + + if envconfig.XDSOutlierDetection { + for n, c := range odCfgs { + retConfig.Children[n] = &priority.Child{ + Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: c}, + // Ignore all re-resolution from EDS children. + IgnoreReresolutionRequests: true, + } + } + continue + } for n, c := range configs { retConfig.Children[n] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: c}, // Ignore all re-resolution from EDS children. IgnoreReresolutionRequests: true, } + } - retAddrs = append(retAddrs, addrs...) case DiscoveryMechanismTypeLogicalDNS: name, config, addrs := buildClusterImplConfigForDNS(p.childNameGen, p.addresses, p.mechanism) + var odCfg *outlierdetection.LBConfig + if envconfig.XDSOutlierDetection { + odCfg = makeClusterImplOutlierDetectionChild(*config, *p.mechanism.OutlierDetection) + } retConfig.Priorities = append(retConfig.Priorities, name) + retAddrs = append(retAddrs, addrs...) + if envconfig.XDSOutlierDetection { + retConfig.Children[name] = &priority.Child{ + Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: odCfg}, + // Not ignore re-resolution from DNS children, they will trigger + // DNS to re-resolve. + IgnoreReresolutionRequests: false, + } + continue + } retConfig.Children[name] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: config}, // Not ignore re-resolution from DNS children, they will trigger // DNS to re-resolve. IgnoreReresolutionRequests: false, } - retAddrs = append(retAddrs, addrs...) } } return retConfig, retAddrs, nil } +func convertClusterImplMapToOutlierDetection(ciCfgs map[string]*clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) map[string]*outlierdetection.LBConfig { + odCfgs := make(map[string]*outlierdetection.LBConfig) + for n, c := range ciCfgs { + odCfgs[n] = makeClusterImplOutlierDetectionChild(*c, *odCfg) + } + return odCfgs +} + +func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) *outlierdetection.LBConfig { + odCfgRet := odCfg + odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want. + return &odCfgRet +} + func buildClusterImplConfigForDNS(g *nameGenerator, addrStrs []string, mechanism DiscoveryMechanism) (string, *clusterimpl.LBConfig, []resolver.Address) { // Endpoint picking policy for DNS is hardcoded to pick_first. const childPolicy = "pick_first" diff --git a/xds/internal/balancer/clusterresolver/configbuilder_test.go b/xds/internal/balancer/clusterresolver/configbuilder_test.go index 80d46fa0119..4bde8e28fcd 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder_test.go +++ b/xds/internal/balancer/clusterresolver/configbuilder_test.go @@ -31,11 +31,13 @@ import ( "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/balancer/weightedroundrobin" "google.golang.org/grpc/balancer/weightedtarget" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/hierarchy" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/resolver" "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/balancer/clusterimpl" + "google.golang.org/grpc/xds/internal/balancer/outlierdetection" "google.golang.org/grpc/xds/internal/balancer/priority" "google.golang.org/grpc/xds/internal/balancer/ringhash" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" @@ -68,6 +70,10 @@ var ( }) return out })} + + noopODCfg = &outlierdetection.LBConfig{ + Interval: 1<<63 - 1, + } ) func init() { @@ -312,6 +318,142 @@ func TestBuildPriorityConfig(t *testing.T) { } } +// TestBuildPriorityConfigWithOutlierDetection tests the priority config +// generation with Outlier Detection toggled on. Each top level balancer per +// priority should be an Outlier Detection balancer, with a Cluster Impl +// Balancer as a child. +func TestBuildPriorityConfigWithOutlierDetection(t *testing.T) { + oldOutlierDetection := envconfig.XDSOutlierDetection + envconfig.XDSOutlierDetection = true + defer func() { + envconfig.XDSOutlierDetection = oldOutlierDetection + }() + + gotConfig, _, _ := buildPriorityConfig([]priorityConfig{ + { + // EDS - OD config should be the top level for both of the EDS + // priorities balancer This EDS priority will have multiple sub + // priorities. The Outlier Detection configuration specified in the + // Discovery Mechanism should be the top level for each sub + // priorities balancer. + mechanism: DiscoveryMechanism{ + Cluster: testClusterName, + Type: DiscoveryMechanismTypeEDS, + EDSServiceName: testEDSServiceName, + OutlierDetection: noopODCfg, + }, + edsResp: xdsresource.EndpointsUpdate{ + Localities: []xdsresource.Locality{ + testLocalitiesP0[0], + testLocalitiesP0[1], + testLocalitiesP1[0], + testLocalitiesP1[1], + }, + }, + childNameGen: newNameGenerator(0), + }, + { + // This OD config should wrap the Logical DNS priorities balancer. + mechanism: DiscoveryMechanism{ + Cluster: testClusterName2, + Type: DiscoveryMechanismTypeLogicalDNS, + OutlierDetection: noopODCfg, + }, + addresses: testAddressStrs[4], + childNameGen: newNameGenerator(1), + }, + }, nil) + + wantConfig := &priority.LBConfig{ + Children: map[string]*priority.Child{ + "priority-0-0": { + Config: &internalserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: testClusterName, + EDSServiceName: testEDSServiceName, + DropCategories: []clusterimpl.DropConfig{}, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: weightedtarget.Name, + Config: &weightedtarget.LBConfig{ + Targets: map[string]weightedtarget.Target{ + assertString(testLocalityIDs[0].ToString): { + Weight: 20, + ChildPolicy: &internalserviceconfig.BalancerConfig{Name: roundrobin.Name}, + }, + assertString(testLocalityIDs[1].ToString): { + Weight: 80, + ChildPolicy: &internalserviceconfig.BalancerConfig{Name: roundrobin.Name}, + }, + }, + }, + }, + }, + }, + }, + }, + IgnoreReresolutionRequests: true, + }, + "priority-0-1": { + Config: &internalserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: testClusterName, + EDSServiceName: testEDSServiceName, + DropCategories: []clusterimpl.DropConfig{}, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: weightedtarget.Name, + Config: &weightedtarget.LBConfig{ + Targets: map[string]weightedtarget.Target{ + assertString(testLocalityIDs[2].ToString): { + Weight: 20, + ChildPolicy: &internalserviceconfig.BalancerConfig{Name: roundrobin.Name}, + }, + assertString(testLocalityIDs[3].ToString): { + Weight: 80, + ChildPolicy: &internalserviceconfig.BalancerConfig{Name: roundrobin.Name}, + }, + }, + }, + }, + }, + }, + }, + }, + IgnoreReresolutionRequests: true, + }, + "priority-1": { + Config: &internalserviceconfig.BalancerConfig{ + Name: outlierdetection.Name, + Config: &outlierdetection.LBConfig{ + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: testClusterName2, + ChildPolicy: &internalserviceconfig.BalancerConfig{Name: "pick_first"}, + }, + }, + }, + }, + IgnoreReresolutionRequests: false, + }, + }, + Priorities: []string{"priority-0-0", "priority-0-1", "priority-1"}, + } + if diff := cmp.Diff(gotConfig, wantConfig); diff != "" { + t.Errorf("buildPriorityConfig() diff (-got +want) %v", diff) + } +} + func TestBuildClusterImplConfigForDNS(t *testing.T) { gotName, gotConfig, gotAddrs := buildClusterImplConfigForDNS(newNameGenerator(3), testAddressStrs[0], DiscoveryMechanism{Cluster: testClusterName2, Type: DiscoveryMechanismTypeLogicalDNS}) wantName := "priority-3" @@ -975,3 +1117,77 @@ func testAddrWithAttrs(addrStr string, weight *uint32, priority string, lID *int addr = hierarchy.Set(addr, path) return addr } + +func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { + tests := []struct { + name string + ciCfgsMap map[string]*clusterimpl.LBConfig + odCfg *outlierdetection.LBConfig + odCfgsMapWant map[string]*outlierdetection.LBConfig + }{ + { + name: "single-entry-noop", + ciCfgsMap: map[string]*clusterimpl.LBConfig{ + "child1": { + Cluster: "cluster1", + }, + }, + odCfg: &outlierdetection.LBConfig{ + Interval: 1<<63 - 1, + }, + odCfgsMapWant: map[string]*outlierdetection.LBConfig{ + "child1": { + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: "cluster1", + }, + }, + }, + }, + }, + { + name: "multiple-entries-noop", + ciCfgsMap: map[string]*clusterimpl.LBConfig{ + "child1": { + Cluster: "cluster1", + }, + "child2": { + Cluster: "cluster2", + }, + }, + odCfg: &outlierdetection.LBConfig{ + Interval: 1<<63 - 1, + }, + odCfgsMapWant: map[string]*outlierdetection.LBConfig{ + "child1": { + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: "cluster1", + }, + }, + }, + "child2": { + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: clusterimpl.Name, + Config: &clusterimpl.LBConfig{ + Cluster: "cluster2", + }, + }, + }, + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got := convertClusterImplMapToOutlierDetection(test.ciCfgsMap, test.odCfg) + if diff := cmp.Diff(got, test.odCfgsMapWant); diff != "" { + t.Fatalf("convertClusterImplMapToOutlierDetection() diff(-got +want) %v", diff) + } + }) + } +} diff --git a/xds/internal/balancer/outlierdetection/balancer.go b/xds/internal/balancer/outlierdetection/balancer.go new file mode 100644 index 00000000000..4a50578bc92 --- /dev/null +++ b/xds/internal/balancer/outlierdetection/balancer.go @@ -0,0 +1,121 @@ +/* + * + * Copyright 2022 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +// Package outlierdetection implements a balancer that implements +// Outlier Detection. +package outlierdetection + +import ( + "encoding/json" + "fmt" + + "google.golang.org/grpc/balancer" + "google.golang.org/grpc/serviceconfig" +) + +// Name is the name of the outlier detection balancer. +const Name = "outlier_detection_experimental" + +func init() { + balancer.Register(bb{}) +} + +type bb struct{} + +func (bb) Build(cc balancer.ClientConn, bOpts balancer.BuildOptions) balancer.Balancer { + return &outlierDetectionBalancer{} +} + +func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { + var lbCfg *LBConfig + if err := json.Unmarshal(s, &lbCfg); err != nil { + return nil, fmt.Errorf("xds: unable to unmarshal LBconfig: %s, error: %v", string(s), err) + } + + // Note: in the xds flow, these validations will never fail. The xdsclient + // performs the same validations as here on the xds Outlier Detection + // resource before parsing into the internal struct which gets marshaled + // into JSON before calling this function. A50 defines two separate places + // for these validations to take place, the xdsclient and this ParseConfig + // method. "When parsing a config from JSON, if any of these requirements is + // violated, that should be treated as a parsing error." - A50 + + // "The google.protobuf.Duration fields interval, base_ejection_time, and + // max_ejection_time must obey the restrictions in the + // google.protobuf.Duration documentation and they must have non-negative + // values." - A50 + + // Approximately 290 years is the maximum time that time.Duration (int64) + // can represent. The restrictions on the protobuf.Duration field are to be + // within +-10000 years. Thus, just check for negative values. + if lbCfg.Interval < 0 { + return nil, fmt.Errorf("LBConfig.Interval = %v; must be >= 0", lbCfg.Interval) + } + if lbCfg.BaseEjectionTime < 0 { + return nil, fmt.Errorf("LBConfig.BaseEjectionTime = %v; must be >= 0", lbCfg.BaseEjectionTime) + } + if lbCfg.MaxEjectionTime < 0 { + return nil, fmt.Errorf("LBConfig.MaxEjectionTime = %v; must be >= 0", lbCfg.MaxEjectionTime) + } + + // "The fields max_ejection_percent, + // success_rate_ejection.enforcement_percentage, + // failure_percentage_ejection.threshold, and + // failure_percentage.enforcement_percentage must have values less than or + // equal to 100." - A50 + if lbCfg.MaxEjectionPercent > 100 { + return nil, fmt.Errorf("LBConfig.MaxEjectionPercent = %v; must be <= 100", lbCfg.MaxEjectionPercent) + } + if lbCfg.SuccessRateEjection != nil && lbCfg.SuccessRateEjection.EnforcementPercentage > 100 { + return nil, fmt.Errorf("LBConfig.SuccessRateEjection.EnforcementPercentage = %v; must be <= 100", lbCfg.SuccessRateEjection.EnforcementPercentage) + } + if lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.Threshold > 100 { + return nil, fmt.Errorf("LBConfig.FailurePercentageEjection.Threshold = %v; must be <= 100", lbCfg.FailurePercentageEjection.Threshold) + } + if lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.EnforcementPercentage > 100 { + return nil, fmt.Errorf("LBConfig.FailurePercentageEjection.EnforcementPercentage = %v; must be <= 100", lbCfg.FailurePercentageEjection.EnforcementPercentage) + } + return lbCfg, nil +} + +func (bb) Name() string { + return Name +} + +type outlierDetectionBalancer struct { +} + +func (b *outlierDetectionBalancer) UpdateClientConnState(s balancer.ClientConnState) error { + return nil +} + +func (b *outlierDetectionBalancer) ResolverError(err error) { + +} + +func (b *outlierDetectionBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { + +} + +func (b *outlierDetectionBalancer) Close() { + +} + +func (b *outlierDetectionBalancer) ExitIdle() { + +} diff --git a/xds/internal/balancer/outlierdetection/balancer_test.go b/xds/internal/balancer/outlierdetection/balancer_test.go new file mode 100644 index 00000000000..69472928a43 --- /dev/null +++ b/xds/internal/balancer/outlierdetection/balancer_test.go @@ -0,0 +1,192 @@ +/* + * + * Copyright 2022 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package outlierdetection + +import ( + "encoding/json" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/balancer" + "google.golang.org/grpc/internal/grpctest" + internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" + "google.golang.org/grpc/serviceconfig" + "google.golang.org/grpc/xds/internal/balancer/clusterimpl" +) + +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + +// TestParseConfig verifies the ParseConfig() method in the CDS balancer. +func (s) TestParseConfig(t *testing.T) { + bb := balancer.Get(Name) + if bb == nil { + t.Fatalf("balancer.Get(%q) returned nil", Name) + } + parser, ok := bb.(balancer.ConfigParser) + if !ok { + t.Fatalf("balancer %q does not implement the ConfigParser interface", Name) + } + + tests := []struct { + name string + input json.RawMessage + wantCfg serviceconfig.LoadBalancingConfig + wantErr bool + }{ + { + name: "noop-lb-config", + input: json.RawMessage(`{"interval": 9223372036854775807}`), + wantCfg: &LBConfig{Interval: 1<<63 - 1}, + }, + { + name: "good-lb-config", + input: json.RawMessage(`{ + "interval": 10000000000, + "baseEjectionTime": 30000000000, + "maxEjectionTime": 300000000000, + "maxEjectionPercent": 10, + "successRateEjection": { + "stdevFactor": 1900, + "enforcementPercentage": 100, + "minimumHosts": 5, + "requestVolume": 100 + }, + "failurePercentageEjection": { + "threshold": 85, + "enforcementPercentage": 5, + "minimumHosts": 5, + "requestVolume": 50 + } + }`), + wantCfg: &LBConfig{ + Interval: 10 * time.Second, + BaseEjectionTime: 30 * time.Second, + MaxEjectionTime: 300 * time.Second, + MaxEjectionPercent: 10, + SuccessRateEjection: &SuccessRateEjection{ + StdevFactor: 1900, + EnforcementPercentage: 100, + MinimumHosts: 5, + RequestVolume: 100, + }, + FailurePercentageEjection: &FailurePercentageEjection{ + Threshold: 85, + EnforcementPercentage: 5, + MinimumHosts: 5, + RequestVolume: 50, + }, + }, + }, + { + name: "interval-is-negative", + input: json.RawMessage(`{"interval": -10}`), + wantErr: true, + }, + { + name: "base-ejection-time-is-negative", + input: json.RawMessage(`{"baseEjectionTime": -10}`), + wantErr: true, + }, + { + name: "max-ejection-time-is-negative", + input: json.RawMessage(`{"maxEjectionTime": -10}`), + wantErr: true, + }, + { + name: "max-ejection-percent-is-greater-than-100", + input: json.RawMessage(`{"maxEjectionPercent": 150}`), + wantErr: true, + }, + { + name: "enforcing-success-rate-is-greater-than-100", + input: json.RawMessage(`{ + "successRateEjection": { + "enforcingSuccessRate": 100, + }, + }`), + wantErr: true, + }, + { + name: "failure-percentage-threshold-is-greater-than-100", + input: json.RawMessage(`{ + "failurePercentageEjection": { + "threshold": 150, + }, + }`), + wantErr: true, + }, + { + name: "enforcing-failure-percentage-is-greater-than-100", + input: json.RawMessage(`{ + "failurePercentageEjection": { + "enforcingFailurePercentage": 150, + }, + }`), + wantErr: true, + }, + { + name: "child-policy", + input: json.RawMessage(`{ + "childPolicy": [ + { + "xds_cluster_impl_experimental": { + "cluster": "test_cluster" + } + } + ] + }`), + wantCfg: &LBConfig{ + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: "xds_cluster_impl_experimental", + Config: &clusterimpl.LBConfig{ + Cluster: "test_cluster", + }, + }, + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + gotCfg, gotErr := parser.ParseConfig(test.input) + if (gotErr != nil) != test.wantErr { + t.Fatalf("ParseConfig(%v) = %v, wantErr %v", string(test.input), gotErr, test.wantErr) + } + if test.wantErr { + return + } + if diff := cmp.Diff(gotCfg, test.wantCfg); diff != "" { + t.Fatalf("parseConfig(%v) got unexpected output, diff (-got +want): %v", string(test.input), diff) + } + }) + } +} + +func (lbc *LBConfig) Equal(lbc2 *LBConfig) bool { + if !lbc.EqualIgnoringChildPolicy(lbc2) { + return false + } + return cmp.Equal(lbc.ChildPolicy, lbc2.ChildPolicy) +} From 0f844787cc4b5fb66a07b183f55e7ca41bb983c5 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 3 Jun 2022 23:56:36 -0400 Subject: [PATCH 2/6] Responded to Doug's comments --- .../balancer/clusterresolver/configbuilder.go | 15 ++-- .../balancer/outlierdetection/balancer.go | 25 +----- .../outlierdetection/balancer_test.go | 84 +++++++++---------- 3 files changed, 46 insertions(+), 78 deletions(-) diff --git a/xds/internal/balancer/clusterresolver/configbuilder.go b/xds/internal/balancer/clusterresolver/configbuilder.go index 5e153c5be51..6464e01fb0b 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder.go +++ b/xds/internal/balancer/clusterresolver/configbuilder.go @@ -130,14 +130,11 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi if err != nil { return nil, nil, err } - var odCfgs map[string]*outlierdetection.LBConfig - if envconfig.XDSOutlierDetection { - odCfgs = convertClusterImplMapToOutlierDetection(configs, p.mechanism.OutlierDetection) - } retConfig.Priorities = append(retConfig.Priorities, names...) retAddrs = append(retAddrs, addrs...) - + var odCfgs map[string]*outlierdetection.LBConfig if envconfig.XDSOutlierDetection { + odCfgs = convertClusterImplMapToOutlierDetection(configs, p.mechanism.OutlierDetection) for n, c := range odCfgs { retConfig.Children[n] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: c}, @@ -157,13 +154,11 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi } case DiscoveryMechanismTypeLogicalDNS: name, config, addrs := buildClusterImplConfigForDNS(p.childNameGen, p.addresses, p.mechanism) - var odCfg *outlierdetection.LBConfig - if envconfig.XDSOutlierDetection { - odCfg = makeClusterImplOutlierDetectionChild(*config, *p.mechanism.OutlierDetection) - } retConfig.Priorities = append(retConfig.Priorities, name) retAddrs = append(retAddrs, addrs...) + var odCfg *outlierdetection.LBConfig if envconfig.XDSOutlierDetection { + odCfg = makeClusterImplOutlierDetectionChild(*config, *p.mechanism.OutlierDetection) retConfig.Children[name] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: odCfg}, // Not ignore re-resolution from DNS children, they will trigger @@ -184,7 +179,7 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi } func convertClusterImplMapToOutlierDetection(ciCfgs map[string]*clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) map[string]*outlierdetection.LBConfig { - odCfgs := make(map[string]*outlierdetection.LBConfig) + odCfgs := make(map[string]*outlierdetection.LBConfig, len(ciCfgs)) for n, c := range ciCfgs { odCfgs[n] = makeClusterImplOutlierDetectionChild(*c, *odCfg) } diff --git a/xds/internal/balancer/outlierdetection/balancer.go b/xds/internal/balancer/outlierdetection/balancer.go index 4a50578bc92..27465d12e64 100644 --- a/xds/internal/balancer/outlierdetection/balancer.go +++ b/xds/internal/balancer/outlierdetection/balancer.go @@ -38,7 +38,7 @@ func init() { type bb struct{} func (bb) Build(cc balancer.ClientConn, bOpts balancer.BuildOptions) balancer.Balancer { - return &outlierDetectionBalancer{} + return nil } func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { @@ -96,26 +96,3 @@ func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, err func (bb) Name() string { return Name } - -type outlierDetectionBalancer struct { -} - -func (b *outlierDetectionBalancer) UpdateClientConnState(s balancer.ClientConnState) error { - return nil -} - -func (b *outlierDetectionBalancer) ResolverError(err error) { - -} - -func (b *outlierDetectionBalancer) UpdateSubConnState(sc balancer.SubConn, state balancer.SubConnState) { - -} - -func (b *outlierDetectionBalancer) Close() { - -} - -func (b *outlierDetectionBalancer) ExitIdle() { - -} diff --git a/xds/internal/balancer/outlierdetection/balancer_test.go b/xds/internal/balancer/outlierdetection/balancer_test.go index 69472928a43..556d92e0d92 100644 --- a/xds/internal/balancer/outlierdetection/balancer_test.go +++ b/xds/internal/balancer/outlierdetection/balancer_test.go @@ -20,11 +20,11 @@ package outlierdetection import ( "encoding/json" + "strings" "testing" "time" "github.com/google/go-cmp/cmp" - "google.golang.org/grpc/balancer" "google.golang.org/grpc/internal/grpctest" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/serviceconfig" @@ -41,29 +41,22 @@ func Test(t *testing.T) { // TestParseConfig verifies the ParseConfig() method in the CDS balancer. func (s) TestParseConfig(t *testing.T) { - bb := balancer.Get(Name) - if bb == nil { - t.Fatalf("balancer.Get(%q) returned nil", Name) - } - parser, ok := bb.(balancer.ConfigParser) - if !ok { - t.Fatalf("balancer %q does not implement the ConfigParser interface", Name) - } + parser := bb{} tests := []struct { name string - input json.RawMessage + input string wantCfg serviceconfig.LoadBalancingConfig - wantErr bool + wantErr string }{ { name: "noop-lb-config", - input: json.RawMessage(`{"interval": 9223372036854775807}`), + input: `{"interval": 9223372036854775807}`, wantCfg: &LBConfig{Interval: 1<<63 - 1}, }, { name: "good-lb-config", - input: json.RawMessage(`{ + input: `{ "interval": 10000000000, "baseEjectionTime": 30000000000, "maxEjectionTime": 300000000000, @@ -80,7 +73,7 @@ func (s) TestParseConfig(t *testing.T) { "minimumHosts": 5, "requestVolume": 50 } - }`), + }`, wantCfg: &LBConfig{ Interval: 10 * time.Second, BaseEjectionTime: 30 * time.Second, @@ -102,54 +95,54 @@ func (s) TestParseConfig(t *testing.T) { }, { name: "interval-is-negative", - input: json.RawMessage(`{"interval": -10}`), - wantErr: true, + input: `{"interval": -10}`, + wantErr: "LBConfig.Interval = -10ns; must be >= 0", }, { name: "base-ejection-time-is-negative", - input: json.RawMessage(`{"baseEjectionTime": -10}`), - wantErr: true, + input: `{"baseEjectionTime": -10}`, + wantErr: "LBConfig.BaseEjectionTime = -10ns; must be >= 0", }, { name: "max-ejection-time-is-negative", - input: json.RawMessage(`{"maxEjectionTime": -10}`), - wantErr: true, + input: `{"maxEjectionTime": -10}`, + wantErr: "LBConfig.MaxEjectionTime = -10ns; must be >= 0", }, { name: "max-ejection-percent-is-greater-than-100", - input: json.RawMessage(`{"maxEjectionPercent": 150}`), - wantErr: true, + input: `{"maxEjectionPercent": 150}`, + wantErr: "LBConfig.MaxEjectionPercent = 150; must be <= 100", }, { - name: "enforcing-success-rate-is-greater-than-100", - input: json.RawMessage(`{ + name: "enforcement-percentage-success-rate-is-greater-than-100", + input: `{ "successRateEjection": { - "enforcingSuccessRate": 100, - }, - }`), - wantErr: true, + "enforcementPercentage": 150 + } + }`, + wantErr: "LBConfig.SuccessRateEjection.EnforcementPercentage = 150; must be <= 100", }, { name: "failure-percentage-threshold-is-greater-than-100", - input: json.RawMessage(`{ + input: `{ "failurePercentageEjection": { - "threshold": 150, - }, - }`), - wantErr: true, + "threshold": 150 + } + }`, + wantErr: "LBConfig.FailurePercentageEjection.Threshold = 150; must be <= 100", }, { - name: "enforcing-failure-percentage-is-greater-than-100", - input: json.RawMessage(`{ + name: "enforcement-percentage-failure-percentage-ejection-is-greater-than-100", + input: `{ "failurePercentageEjection": { - "enforcingFailurePercentage": 150, - }, - }`), - wantErr: true, + "enforcementPercentage": 150 + } + }`, + wantErr: "LBConfig.FailurePercentageEjection.EnforcementPercentage = 150; must be <= 100", }, { name: "child-policy", - input: json.RawMessage(`{ + input: `{ "childPolicy": [ { "xds_cluster_impl_experimental": { @@ -157,7 +150,7 @@ func (s) TestParseConfig(t *testing.T) { } } ] - }`), + }`, wantCfg: &LBConfig{ ChildPolicy: &internalserviceconfig.BalancerConfig{ Name: "xds_cluster_impl_experimental", @@ -170,11 +163,14 @@ func (s) TestParseConfig(t *testing.T) { } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - gotCfg, gotErr := parser.ParseConfig(test.input) - if (gotErr != nil) != test.wantErr { + gotCfg, gotErr := parser.ParseConfig(json.RawMessage(test.input)) + if gotErr != nil && !strings.Contains(gotErr.Error(), test.wantErr) { t.Fatalf("ParseConfig(%v) = %v, wantErr %v", string(test.input), gotErr, test.wantErr) } - if test.wantErr { + if (gotErr != nil) != (test.wantErr != "") { + t.Fatalf("ParseConfig(%v) = %v, wantErr %v", test.input, gotErr, test.wantErr) + } + if test.wantErr != "" { return } if diff := cmp.Diff(gotCfg, test.wantCfg); diff != "" { From 770161c49b54e37c3b263056060464dc2c594826 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Tue, 7 Jun 2022 17:16:20 -0400 Subject: [PATCH 3/6] Responded to Doug's comments --- .../balancer/clusterresolver/configbuilder.go | 15 +-- .../balancer/outlierdetection/balancer.go | 8 +- .../outlierdetection/balancer_test.go | 92 ++++++++++++++++++- 3 files changed, 105 insertions(+), 10 deletions(-) diff --git a/xds/internal/balancer/clusterresolver/configbuilder.go b/xds/internal/balancer/clusterresolver/configbuilder.go index 6464e01fb0b..ddb3cbfdb3e 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder.go +++ b/xds/internal/balancer/clusterresolver/configbuilder.go @@ -158,7 +158,7 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi retAddrs = append(retAddrs, addrs...) var odCfg *outlierdetection.LBConfig if envconfig.XDSOutlierDetection { - odCfg = makeClusterImplOutlierDetectionChild(*config, *p.mechanism.OutlierDetection) + odCfg = makeClusterImplOutlierDetectionChild(*config, p.mechanism.OutlierDetection) retConfig.Children[name] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: odCfg}, // Not ignore re-resolution from DNS children, they will trigger @@ -181,15 +181,18 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi func convertClusterImplMapToOutlierDetection(ciCfgs map[string]*clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) map[string]*outlierdetection.LBConfig { odCfgs := make(map[string]*outlierdetection.LBConfig, len(ciCfgs)) for n, c := range ciCfgs { - odCfgs[n] = makeClusterImplOutlierDetectionChild(*c, *odCfg) + odCfgs[n] = makeClusterImplOutlierDetectionChild(*c, odCfg) } return odCfgs } -func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) *outlierdetection.LBConfig { - odCfgRet := odCfg - odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} // This can panic if odCfg is nil. This shouldn't be nil though, as per CDS balancer. I can add check if you want. - return &odCfgRet +func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig { + odCfgRet := &outlierdetection.LBConfig{} + if odCfg != nil { + *odCfgRet = *odCfg + } + odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} + return odCfgRet } func buildClusterImplConfigForDNS(g *nameGenerator, addrStrs []string, mechanism DiscoveryMechanism) (string, *clusterimpl.LBConfig, []resolver.Address) { diff --git a/xds/internal/balancer/outlierdetection/balancer.go b/xds/internal/balancer/outlierdetection/balancer.go index 27465d12e64..c1b89e4ec71 100644 --- a/xds/internal/balancer/outlierdetection/balancer.go +++ b/xds/internal/balancer/outlierdetection/balancer.go @@ -22,6 +22,7 @@ package outlierdetection import ( "encoding/json" + "errors" "fmt" "google.golang.org/grpc/balancer" @@ -43,7 +44,7 @@ func (bb) Build(cc balancer.ClientConn, bOpts balancer.BuildOptions) balancer.Ba func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { var lbCfg *LBConfig - if err := json.Unmarshal(s, &lbCfg); err != nil { + if err := json.Unmarshal(s, &lbCfg); err != nil { // Validates child config if present as well. return nil, fmt.Errorf("xds: unable to unmarshal LBconfig: %s, error: %v", string(s), err) } @@ -90,6 +91,11 @@ func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, err if lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.EnforcementPercentage > 100 { return nil, fmt.Errorf("LBConfig.FailurePercentageEjection.EnforcementPercentage = %v; must be <= 100", lbCfg.FailurePercentageEjection.EnforcementPercentage) } + + if lbCfg.ChildPolicy == nil { + return nil, errors.New("LBConfig.ChildPolicy needs to be present") + } + return lbCfg, nil } diff --git a/xds/internal/balancer/outlierdetection/balancer_test.go b/xds/internal/balancer/outlierdetection/balancer_test.go index 556d92e0d92..b6692ef3518 100644 --- a/xds/internal/balancer/outlierdetection/balancer_test.go +++ b/xds/internal/balancer/outlierdetection/balancer_test.go @@ -20,11 +20,13 @@ package outlierdetection import ( "encoding/json" + "errors" "strings" "testing" "time" "github.com/google/go-cmp/cmp" + "google.golang.org/grpc/balancer" "google.golang.org/grpc/internal/grpctest" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/serviceconfig" @@ -50,9 +52,26 @@ func (s) TestParseConfig(t *testing.T) { wantErr string }{ { - name: "noop-lb-config", - input: `{"interval": 9223372036854775807}`, - wantCfg: &LBConfig{Interval: 1<<63 - 1}, + name: "noop-lb-config", + input: `{ + "interval": 9223372036854775807, + "childPolicy": [ + { + "xds_cluster_impl_experimental": { + "cluster": "test_cluster" + } + } + ] + }`, + wantCfg: &LBConfig{ + Interval: 1<<63 - 1, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: "xds_cluster_impl_experimental", + Config: &clusterimpl.LBConfig{ + Cluster: "test_cluster", + }, + }, + }, }, { name: "good-lb-config", @@ -72,7 +91,14 @@ func (s) TestParseConfig(t *testing.T) { "enforcementPercentage": 5, "minimumHosts": 5, "requestVolume": 50 + }, + "childPolicy": [ + { + "xds_cluster_impl_experimental": { + "cluster": "test_cluster" + } } + ] }`, wantCfg: &LBConfig{ Interval: 10 * time.Second, @@ -91,6 +117,12 @@ func (s) TestParseConfig(t *testing.T) { MinimumHosts: 5, RequestVolume: 50, }, + ChildPolicy: &internalserviceconfig.BalancerConfig{ + Name: "xds_cluster_impl_experimental", + Config: &clusterimpl.LBConfig{ + Cluster: "test_cluster", + }, + }, }, }, { @@ -140,6 +172,42 @@ func (s) TestParseConfig(t *testing.T) { }`, wantErr: "LBConfig.FailurePercentageEjection.EnforcementPercentage = 150; must be <= 100", }, + { + name: "child-policy-not-present", + input: `{ + "interval": 10000000000, + "baseEjectionTime": 30000000000, + "maxEjectionTime": 300000000000, + "maxEjectionPercent": 10, + "successRateEjection": { + "stdevFactor": 1900, + "enforcementPercentage": 100, + "minimumHosts": 5, + "requestVolume": 100 + }, + "failurePercentageEjection": { + "threshold": 85, + "enforcementPercentage": 5, + "minimumHosts": 5, + "requestVolume": 50 + } + }`, + wantErr: "LBConfig.ChildPolicy needs to be present", + }, + { + name: "child-policy-present-but-parse-error", + input: `{ + "interval": 9223372036854775807, + "childPolicy": [ + { + "errParseConfigBalancer": { + "cluster": "test_cluster" + } + } + ] + }`, + wantErr: "error parsing loadBalancingConfig for policy \"errParseConfigBalancer\"", + }, { name: "child-policy", input: `{ @@ -186,3 +254,21 @@ func (lbc *LBConfig) Equal(lbc2 *LBConfig) bool { } return cmp.Equal(lbc.ChildPolicy, lbc2.ChildPolicy) } + +func init() { + balancer.Register(errParseConfigBuilder{}) +} + +type errParseConfigBuilder struct{} + +func (errParseConfigBuilder) Build(cc balancer.ClientConn, opts balancer.BuildOptions) balancer.Balancer { + return nil +} + +func (errParseConfigBuilder) Name() string { + return "errParseConfigBalancer" +} + +func (errParseConfigBuilder) ParseConfig(c json.RawMessage) (serviceconfig.LoadBalancingConfig, error) { + return nil, errors.New("some error") +} From 2e16f0a436b1bac098d5ce80e0154c8f5627cc81 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 8 Jun 2022 20:21:46 -0400 Subject: [PATCH 4/6] Addressed comments outside the ones in config builder --- internal/internal.go | 16 ++++++ .../clusterresolver/clusterresolver_test.go | 8 ++- .../clusterresolver/configbuilder_test.go | 14 ++--- .../balancer/outlierdetection/balancer.go | 56 ++++++++++--------- .../outlierdetection/balancer_test.go | 35 ++++++++---- 5 files changed, 82 insertions(+), 47 deletions(-) diff --git a/internal/internal.go b/internal/internal.go index 0f451224817..bbc6d8bb881 100644 --- a/internal/internal.go +++ b/internal/internal.go @@ -103,6 +103,22 @@ var ( // // TODO: Remove this function once the RBAC env var is removed. UnregisterRBACHTTPFilterForTesting func() + + // RegisterOutlierDetectionBalancerForTesting registers the Outlier + // Detection Balancer for testing purposes, regardless of the Outlier + // Detection environment variable. + // + // TODO: Remove this function once the Outlier Detection env var is removed. + RegisterOutlierDetectionBalancerForTesting func() + + // UnregisterOutlierDetectionBalancerForTesting unregisters the Outlier + // Detection Balancer for testing purposes. This is needed because there is + // no way to unregister the Outlier Detection Balancer after registering it + // solely for testing purposes using + // RegisterOutlierDetectionBalancerForTesting(). + // + // TODO: Remove this function once the Outlier Detection env var is removed. + UnregisterOutlierDetectionBalancerForTesting func() ) // HealthChecker defines the signature of the client-side LB channel health checking function. diff --git a/xds/internal/balancer/clusterresolver/clusterresolver_test.go b/xds/internal/balancer/clusterresolver/clusterresolver_test.go index 0b1c4bf1f30..f0ec72b6b63 100644 --- a/xds/internal/balancer/clusterresolver/clusterresolver_test.go +++ b/xds/internal/balancer/clusterresolver/clusterresolver_test.go @@ -30,12 +30,13 @@ import ( "google.golang.org/grpc/balancer/roundrobin" "google.golang.org/grpc/balancer/weightedtarget" "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/internal" "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/internal/grpctest" internalserviceconfig "google.golang.org/grpc/internal/serviceconfig" "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/resolver" - "google.golang.org/grpc/xds/internal" + xdsinternal "google.golang.org/grpc/xds/internal" "google.golang.org/grpc/xds/internal/balancer/clusterimpl" "google.golang.org/grpc/xds/internal/balancer/outlierdetection" "google.golang.org/grpc/xds/internal/balancer/priority" @@ -61,7 +62,7 @@ var ( Localities: []xdsresource.Locality{ { Endpoints: []xdsresource.Endpoint{{Address: "endpoint1"}}, - ID: internal.LocalityID{Zone: "zone"}, + ID: xdsinternal.LocalityID{Zone: "zone"}, Priority: 1, Weight: 100, }, @@ -534,6 +535,7 @@ func newLBConfigWithOneEDSAndOutlierDetection(edsServiceName string, odCfg *outl func (s) TestOutlierDetection(t *testing.T) { oldOutlierDetection := envconfig.XDSOutlierDetection envconfig.XDSOutlierDetection = true + internal.RegisterOutlierDetectionBalancerForTesting() defer func() { envconfig.XDSOutlierDetection = oldOutlierDetection }() @@ -575,7 +577,7 @@ func (s) TestOutlierDetection(t *testing.T) { t.Fatal(err) } - localityID := internal.LocalityID{Zone: "zone"} + localityID := xdsinternal.LocalityID{Zone: "zone"} // The priority configuration generated should have Outlier Detection as a // direct child due to Outlier Detection being turned on. pCfgWant := &priority.LBConfig{ diff --git a/xds/internal/balancer/clusterresolver/configbuilder_test.go b/xds/internal/balancer/clusterresolver/configbuilder_test.go index 4bde8e28fcd..95962ac0116 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder_test.go +++ b/xds/internal/balancer/clusterresolver/configbuilder_test.go @@ -1120,10 +1120,10 @@ func testAddrWithAttrs(addrStr string, weight *uint32, priority string, lID *int func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { tests := []struct { - name string - ciCfgsMap map[string]*clusterimpl.LBConfig - odCfg *outlierdetection.LBConfig - odCfgsMapWant map[string]*outlierdetection.LBConfig + name string + ciCfgsMap map[string]*clusterimpl.LBConfig + odCfg *outlierdetection.LBConfig + wantODCfgs map[string]*outlierdetection.LBConfig }{ { name: "single-entry-noop", @@ -1135,7 +1135,7 @@ func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { odCfg: &outlierdetection.LBConfig{ Interval: 1<<63 - 1, }, - odCfgsMapWant: map[string]*outlierdetection.LBConfig{ + wantODCfgs: map[string]*outlierdetection.LBConfig{ "child1": { Interval: 1<<63 - 1, ChildPolicy: &internalserviceconfig.BalancerConfig{ @@ -1160,7 +1160,7 @@ func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { odCfg: &outlierdetection.LBConfig{ Interval: 1<<63 - 1, }, - odCfgsMapWant: map[string]*outlierdetection.LBConfig{ + wantODCfgs: map[string]*outlierdetection.LBConfig{ "child1": { Interval: 1<<63 - 1, ChildPolicy: &internalserviceconfig.BalancerConfig{ @@ -1185,7 +1185,7 @@ func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { got := convertClusterImplMapToOutlierDetection(test.ciCfgsMap, test.odCfg) - if diff := cmp.Diff(got, test.odCfgsMapWant); diff != "" { + if diff := cmp.Diff(got, test.wantODCfgs); diff != "" { t.Fatalf("convertClusterImplMapToOutlierDetection() diff(-got +want) %v", diff) } }) diff --git a/xds/internal/balancer/outlierdetection/balancer.go b/xds/internal/balancer/outlierdetection/balancer.go index c1b89e4ec71..674075c4b3f 100644 --- a/xds/internal/balancer/outlierdetection/balancer.go +++ b/xds/internal/balancer/outlierdetection/balancer.go @@ -26,6 +26,8 @@ import ( "fmt" "google.golang.org/grpc/balancer" + "google.golang.org/grpc/internal" + "google.golang.org/grpc/internal/envconfig" "google.golang.org/grpc/serviceconfig" ) @@ -33,7 +35,16 @@ import ( const Name = "outlier_detection_experimental" func init() { - balancer.Register(bb{}) + if envconfig.XDSOutlierDetection { + balancer.Register(bb{}) + } + // TODO: Remove these once the Outlier Detection env var is removed. + internal.RegisterOutlierDetectionBalancerForTesting = func() { + balancer.Register(bb{}) + } + internal.UnregisterOutlierDetectionBalancerForTesting = func() { + internal.BalancerUnregister(Name) + } } type bb struct{} @@ -56,44 +67,35 @@ func (bb) ParseConfig(s json.RawMessage) (serviceconfig.LoadBalancingConfig, err // method. "When parsing a config from JSON, if any of these requirements is // violated, that should be treated as a parsing error." - A50 + switch { // "The google.protobuf.Duration fields interval, base_ejection_time, and // max_ejection_time must obey the restrictions in the // google.protobuf.Duration documentation and they must have non-negative // values." - A50 - // Approximately 290 years is the maximum time that time.Duration (int64) // can represent. The restrictions on the protobuf.Duration field are to be // within +-10000 years. Thus, just check for negative values. - if lbCfg.Interval < 0 { - return nil, fmt.Errorf("LBConfig.Interval = %v; must be >= 0", lbCfg.Interval) - } - if lbCfg.BaseEjectionTime < 0 { - return nil, fmt.Errorf("LBConfig.BaseEjectionTime = %v; must be >= 0", lbCfg.BaseEjectionTime) - } - if lbCfg.MaxEjectionTime < 0 { - return nil, fmt.Errorf("LBConfig.MaxEjectionTime = %v; must be >= 0", lbCfg.MaxEjectionTime) - } - + case lbCfg.Interval < 0: + return nil, fmt.Errorf("OutlierDetectionLoadBalancingConfig.interval = %s; must be >= 0", lbCfg.Interval) + case lbCfg.BaseEjectionTime < 0: + return nil, fmt.Errorf("OutlierDetectionLoadBalancingConfig.base_ejection_time = %s; must be >= 0", lbCfg.BaseEjectionTime) + case lbCfg.MaxEjectionTime < 0: + return nil, fmt.Errorf("OutlierDetectionLoadBalancingConfig.max_ejection_time = %s; must be >= 0", lbCfg.MaxEjectionTime) // "The fields max_ejection_percent, // success_rate_ejection.enforcement_percentage, // failure_percentage_ejection.threshold, and // failure_percentage.enforcement_percentage must have values less than or // equal to 100." - A50 - if lbCfg.MaxEjectionPercent > 100 { - return nil, fmt.Errorf("LBConfig.MaxEjectionPercent = %v; must be <= 100", lbCfg.MaxEjectionPercent) - } - if lbCfg.SuccessRateEjection != nil && lbCfg.SuccessRateEjection.EnforcementPercentage > 100 { - return nil, fmt.Errorf("LBConfig.SuccessRateEjection.EnforcementPercentage = %v; must be <= 100", lbCfg.SuccessRateEjection.EnforcementPercentage) - } - if lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.Threshold > 100 { - return nil, fmt.Errorf("LBConfig.FailurePercentageEjection.Threshold = %v; must be <= 100", lbCfg.FailurePercentageEjection.Threshold) - } - if lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.EnforcementPercentage > 100 { - return nil, fmt.Errorf("LBConfig.FailurePercentageEjection.EnforcementPercentage = %v; must be <= 100", lbCfg.FailurePercentageEjection.EnforcementPercentage) - } - - if lbCfg.ChildPolicy == nil { - return nil, errors.New("LBConfig.ChildPolicy needs to be present") + case lbCfg.MaxEjectionPercent > 100: + return nil, fmt.Errorf("OutlierDetectionLoadBalancingConfig.max_ejection_percent = %v; must be <= 100", lbCfg.MaxEjectionPercent) + case lbCfg.SuccessRateEjection != nil && lbCfg.SuccessRateEjection.EnforcementPercentage > 100: + return nil, fmt.Errorf("OutlierDetectionLoadBalancingConfig.SuccessRateEjection.enforcement_percentage = %v; must be <= 100", lbCfg.SuccessRateEjection.EnforcementPercentage) + case lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.Threshold > 100: + return nil, fmt.Errorf("OutlierDetectionLoadBalancingConfig.FailurePercentageEjection.threshold = %v; must be <= 100", lbCfg.FailurePercentageEjection.Threshold) + case lbCfg.FailurePercentageEjection != nil && lbCfg.FailurePercentageEjection.EnforcementPercentage > 100: + return nil, fmt.Errorf("OutlierDetectionLoadBalancingConfig.FailurePercentageEjection.enforcement_percentage = %v; must be <= 100", lbCfg.FailurePercentageEjection.EnforcementPercentage) + case lbCfg.ChildPolicy == nil: + return nil, errors.New("OutlierDetectionLoadBalancingConfig.child_policy must be present") } return lbCfg, nil diff --git a/xds/internal/balancer/outlierdetection/balancer_test.go b/xds/internal/balancer/outlierdetection/balancer_test.go index b6692ef3518..106e2b64dbc 100644 --- a/xds/internal/balancer/outlierdetection/balancer_test.go +++ b/xds/internal/balancer/outlierdetection/balancer_test.go @@ -41,7 +41,8 @@ func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } -// TestParseConfig verifies the ParseConfig() method in the CDS balancer. +// TestParseConfig verifies the ParseConfig() method in the Outlier Detection +// Balancer. func (s) TestParseConfig(t *testing.T) { parser := bb{} @@ -128,22 +129,22 @@ func (s) TestParseConfig(t *testing.T) { { name: "interval-is-negative", input: `{"interval": -10}`, - wantErr: "LBConfig.Interval = -10ns; must be >= 0", + wantErr: "OutlierDetectionLoadBalancingConfig.interval = -10ns; must be >= 0", }, { name: "base-ejection-time-is-negative", input: `{"baseEjectionTime": -10}`, - wantErr: "LBConfig.BaseEjectionTime = -10ns; must be >= 0", + wantErr: "OutlierDetectionLoadBalancingConfig.base_ejection_time = -10ns; must be >= 0", }, { name: "max-ejection-time-is-negative", input: `{"maxEjectionTime": -10}`, - wantErr: "LBConfig.MaxEjectionTime = -10ns; must be >= 0", + wantErr: "OutlierDetectionLoadBalancingConfig.max_ejection_time = -10ns; must be >= 0", }, { name: "max-ejection-percent-is-greater-than-100", input: `{"maxEjectionPercent": 150}`, - wantErr: "LBConfig.MaxEjectionPercent = 150; must be <= 100", + wantErr: "OutlierDetectionLoadBalancingConfig.max_ejection_percent = 150; must be <= 100", }, { name: "enforcement-percentage-success-rate-is-greater-than-100", @@ -152,7 +153,7 @@ func (s) TestParseConfig(t *testing.T) { "enforcementPercentage": 150 } }`, - wantErr: "LBConfig.SuccessRateEjection.EnforcementPercentage = 150; must be <= 100", + wantErr: "OutlierDetectionLoadBalancingConfig.SuccessRateEjection.enforcement_percentage = 150; must be <= 100", }, { name: "failure-percentage-threshold-is-greater-than-100", @@ -161,7 +162,7 @@ func (s) TestParseConfig(t *testing.T) { "threshold": 150 } }`, - wantErr: "LBConfig.FailurePercentageEjection.Threshold = 150; must be <= 100", + wantErr: "OutlierDetectionLoadBalancingConfig.FailurePercentageEjection.threshold = 150; must be <= 100", }, { name: "enforcement-percentage-failure-percentage-ejection-is-greater-than-100", @@ -170,7 +171,7 @@ func (s) TestParseConfig(t *testing.T) { "enforcementPercentage": 150 } }`, - wantErr: "LBConfig.FailurePercentageEjection.EnforcementPercentage = 150; must be <= 100", + wantErr: "OutlierDetectionLoadBalancingConfig.FailurePercentageEjection.enforcement_percentage = 150; must be <= 100", }, { name: "child-policy-not-present", @@ -192,7 +193,7 @@ func (s) TestParseConfig(t *testing.T) { "requestVolume": 50 } }`, - wantErr: "LBConfig.ChildPolicy needs to be present", + wantErr: "OutlierDetectionLoadBalancingConfig.child_policy must be present", }, { name: "child-policy-present-but-parse-error", @@ -208,6 +209,20 @@ func (s) TestParseConfig(t *testing.T) { }`, wantErr: "error parsing loadBalancingConfig for policy \"errParseConfigBalancer\"", }, + { + name: "no-supported-child-policy", + input: `{ + "interval": 9223372036854775807, + "childPolicy": [ + { + "doesNotExistBalancer": { + "cluster": "test_cluster" + } + } + ] + }`, + wantErr: "invalid loadBalancingConfig: no supported policies found", + }, { name: "child-policy", input: `{ @@ -233,7 +248,7 @@ func (s) TestParseConfig(t *testing.T) { t.Run(test.name, func(t *testing.T) { gotCfg, gotErr := parser.ParseConfig(json.RawMessage(test.input)) if gotErr != nil && !strings.Contains(gotErr.Error(), test.wantErr) { - t.Fatalf("ParseConfig(%v) = %v, wantErr %v", string(test.input), gotErr, test.wantErr) + t.Fatalf("ParseConfig(%v) = %v, wantErr %v", test.input, gotErr, test.wantErr) } if (gotErr != nil) != (test.wantErr != "") { t.Fatalf("ParseConfig(%v) = %v, wantErr %v", test.input, gotErr, test.wantErr) From 6e8454ee1059b4cbca0762a97639aa9d833c56e7 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Wed, 8 Jun 2022 21:06:39 -0400 Subject: [PATCH 5/6] Responded to comments in config builder --- .../balancer/cdsbalancer/cdsbalancer.go | 6 ++--- .../cdsbalancer/cdsbalancer_security_test.go | 15 ++++++----- .../balancer/cdsbalancer/cdsbalancer_test.go | 26 +++++++++---------- .../clusterresolver/clusterresolver_test.go | 2 +- .../balancer/clusterresolver/config.go | 4 +-- .../balancer/clusterresolver/configbuilder.go | 17 +++++------- .../clusterresolver/configbuilder_test.go | 8 +++--- 7 files changed, 38 insertions(+), 40 deletions(-) diff --git a/xds/internal/balancer/cdsbalancer/cdsbalancer.go b/xds/internal/balancer/cdsbalancer/cdsbalancer.go index d057ed66a53..14c1c2e769a 100644 --- a/xds/internal/balancer/cdsbalancer/cdsbalancer.go +++ b/xds/internal/balancer/cdsbalancer/cdsbalancer.go @@ -272,12 +272,12 @@ func buildProviderFunc(configs map[string]*certprovider.BuildableConfig, instanc return provider, nil } -func outlierDetectionToConfig(od *xdsresource.OutlierDetection) *outlierdetection.LBConfig { // Already validated - no need to return error +func outlierDetectionToConfig(od *xdsresource.OutlierDetection) outlierdetection.LBConfig { // Already validated - no need to return error if od == nil { // "If the outlier_detection field is not set in the Cluster message, a // "no-op" outlier_detection config will be generated, with interval set // to the maximum possible value and all other fields unset." - A50 - return &outlierdetection.LBConfig{ + return outlierdetection.LBConfig{ Interval: 1<<63 - 1, } } @@ -308,7 +308,7 @@ func outlierDetectionToConfig(od *xdsresource.OutlierDetection) *outlierdetectio } } - return &outlierdetection.LBConfig{ + return outlierdetection.LBConfig{ Interval: od.Interval, BaseEjectionTime: od.BaseEjectionTime, MaxEjectionTime: od.MaxEjectionTime, diff --git a/xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go b/xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go index c58990ab34d..685e77adf46 100644 --- a/xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go +++ b/xds/internal/balancer/cdsbalancer/cdsbalancer_security_test.go @@ -34,6 +34,7 @@ import ( "google.golang.org/grpc/internal/testutils" "google.golang.org/grpc/internal/xds/matcher" "google.golang.org/grpc/resolver" + "google.golang.org/grpc/xds/internal/balancer/outlierdetection" "google.golang.org/grpc/xds/internal/testutils/fakeclient" "google.golang.org/grpc/xds/internal/xdsclient/bootstrap" "google.golang.org/grpc/xds/internal/xdsclient/xdsresource" @@ -250,7 +251,7 @@ func (s) TestSecurityConfigWithoutXDSCreds(t *testing.T) { // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName} - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { @@ -306,7 +307,7 @@ func (s) TestNoSecurityConfigWithXDSCreds(t *testing.T) { // newChildBalancer function as part of test setup. No security config is // passed to the CDS balancer as part of this update. cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName} - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { @@ -462,7 +463,7 @@ func (s) TestSecurityConfigUpdate_BadToGood(t *testing.T) { // create a new EDS balancer. The fake EDS balancer created above will be // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil { t.Fatal(err) } @@ -496,7 +497,7 @@ func (s) TestGoodSecurityConfig(t *testing.T) { // create a new EDS balancer. The fake EDS balancer created above will be // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil { @@ -549,7 +550,7 @@ func (s) TestSecurityConfigUpdate_GoodToFallback(t *testing.T) { // create a new EDS balancer. The fake EDS balancer created above will be // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil { @@ -599,7 +600,7 @@ func (s) TestSecurityConfigUpdate_GoodToBad(t *testing.T) { // create a new EDS balancer. The fake EDS balancer created above will be // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdateWithGoodSecurityCfg, nil}, wantCCS, edsB); err != nil { @@ -677,7 +678,7 @@ func (s) TestSecurityConfigUpdate_GoodToGood(t *testing.T) { SubjectAltNameMatchers: testSANMatchers, }, } - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { diff --git a/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go b/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go index b15481f318b..a88081e1d0e 100644 --- a/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go +++ b/xds/internal/balancer/cdsbalancer/cdsbalancer_test.go @@ -56,7 +56,7 @@ var ( ServerURI: "self_server", CredsType: "self_creds", } - noopODLBCfg = &outlierdetection.LBConfig{ + noopODLBCfg = outlierdetection.LBConfig{ Interval: 1<<63 - 1, } ) @@ -215,7 +215,7 @@ func cdsCCS(cluster string, xdsC xdsclient.XDSClient) balancer.ClientConnState { // edsCCS is a helper function to construct a good update passed from the // cdsBalancer to the edsBalancer. -func edsCCS(service string, countMax *uint32, enableLRS bool, xdslbpolicy *internalserviceconfig.BalancerConfig, odConfig *outlierdetection.LBConfig) balancer.ClientConnState { +func edsCCS(service string, countMax *uint32, enableLRS bool, xdslbpolicy *internalserviceconfig.BalancerConfig, odConfig outlierdetection.LBConfig) balancer.ClientConnState { discoveryMechanism := clusterresolver.DiscoveryMechanism{ Type: clusterresolver.DiscoveryMechanismTypeEDS, Cluster: service, @@ -421,7 +421,7 @@ func (s) TestHandleClusterUpdate(t *testing.T) { FailurePercentageMinimumHosts: 5, FailurePercentageRequestVolume: 50, }}, - wantCCS: edsCCS(serviceName, nil, false, nil, &outlierdetection.LBConfig{ + wantCCS: edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{ Interval: 10 * time.Second, BaseEjectionTime: 30 * time.Second, MaxEjectionTime: 300 * time.Second, @@ -506,7 +506,7 @@ func (s) TestHandleClusterUpdateError(t *testing.T) { // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName} - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { t.Fatal(err) } @@ -591,7 +591,7 @@ func (s) TestResolverError(t *testing.T) { // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName} - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { t.Fatal(err) } @@ -640,7 +640,7 @@ func (s) TestUpdateSubConnState(t *testing.T) { // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName} - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { @@ -675,7 +675,7 @@ func (s) TestCircuitBreaking(t *testing.T) { // the service's counter with the new max requests. var maxRequests uint32 = 1 cdsUpdate := xdsresource.ClusterUpdate{ClusterName: clusterName, MaxRequests: &maxRequests} - wantCCS := edsCCS(clusterName, &maxRequests, false, nil, nil) + wantCCS := edsCCS(clusterName, &maxRequests, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { @@ -708,7 +708,7 @@ func (s) TestClose(t *testing.T) { // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName} - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { @@ -779,7 +779,7 @@ func (s) TestExitIdle(t *testing.T) { // returned to the CDS balancer, because we have overridden the // newChildBalancer function as part of test setup. cdsUpdate := xdsresource.ClusterUpdate{ClusterName: serviceName} - wantCCS := edsCCS(serviceName, nil, false, nil, nil) + wantCCS := edsCCS(serviceName, nil, false, nil, outlierdetection.LBConfig{}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() if err := invokeWatchCbAndWait(ctx, xdsC, cdsWatchInfo{cdsUpdate, nil}, wantCCS, edsB); err != nil { @@ -846,7 +846,7 @@ func (s) TestOutlierDetectionToConfig(t *testing.T) { tests := []struct { name string od *xdsresource.OutlierDetection - odLBCfgWant *outlierdetection.LBConfig + odLBCfgWant outlierdetection.LBConfig }{ // "if the outlier_detection field is not set in the Cluster resource, // a "no-op" outlier_detection config will be generated in the @@ -876,7 +876,7 @@ func (s) TestOutlierDetectionToConfig(t *testing.T) { FailurePercentageMinimumHosts: 5, FailurePercentageRequestVolume: 50, }, - odLBCfgWant: &outlierdetection.LBConfig{ + odLBCfgWant: outlierdetection.LBConfig{ Interval: 10 * time.Second, BaseEjectionTime: 30 * time.Second, MaxEjectionTime: 300 * time.Second, @@ -909,7 +909,7 @@ func (s) TestOutlierDetectionToConfig(t *testing.T) { FailurePercentageMinimumHosts: 5, FailurePercentageRequestVolume: 50, }, - odLBCfgWant: &outlierdetection.LBConfig{ + odLBCfgWant: outlierdetection.LBConfig{ Interval: 10 * time.Second, BaseEjectionTime: 30 * time.Second, MaxEjectionTime: 300 * time.Second, @@ -939,7 +939,7 @@ func (s) TestOutlierDetectionToConfig(t *testing.T) { FailurePercentageMinimumHosts: 5, FailurePercentageRequestVolume: 50, }, - odLBCfgWant: &outlierdetection.LBConfig{ + odLBCfgWant: outlierdetection.LBConfig{ Interval: 10 * time.Second, BaseEjectionTime: 30 * time.Second, MaxEjectionTime: 300 * time.Second, diff --git a/xds/internal/balancer/clusterresolver/clusterresolver_test.go b/xds/internal/balancer/clusterresolver/clusterresolver_test.go index f0ec72b6b63..5876ca013bc 100644 --- a/xds/internal/balancer/clusterresolver/clusterresolver_test.go +++ b/xds/internal/balancer/clusterresolver/clusterresolver_test.go @@ -522,7 +522,7 @@ func newLBConfigWithOneEDS(edsServiceName string) *LBConfig { } } -func newLBConfigWithOneEDSAndOutlierDetection(edsServiceName string, odCfg *outlierdetection.LBConfig) *LBConfig { +func newLBConfigWithOneEDSAndOutlierDetection(edsServiceName string, odCfg outlierdetection.LBConfig) *LBConfig { lbCfg := newLBConfigWithOneEDS(edsServiceName) lbCfg.DiscoveryMechanisms[0].OutlierDetection = odCfg return lbCfg diff --git a/xds/internal/balancer/clusterresolver/config.go b/xds/internal/balancer/clusterresolver/config.go index 26e2812d2f6..cb870027a4e 100644 --- a/xds/internal/balancer/clusterresolver/config.go +++ b/xds/internal/balancer/clusterresolver/config.go @@ -105,7 +105,7 @@ type DiscoveryMechanism struct { DNSHostname string `json:"dnsHostname,omitempty"` // OutlierDetection is the Outlier Detection LB configuration for this // priority. - OutlierDetection *outlierdetection.LBConfig `json:"outlierDetection,omitempty"` + OutlierDetection outlierdetection.LBConfig `json:"outlierDetection,omitempty"` } // Equal returns whether the DiscoveryMechanism is the same with the parameter. @@ -121,7 +121,7 @@ func (dm DiscoveryMechanism) Equal(b DiscoveryMechanism) bool { return false case dm.DNSHostname != b.DNSHostname: return false - case !dm.OutlierDetection.EqualIgnoringChildPolicy(b.OutlierDetection): + case !dm.OutlierDetection.EqualIgnoringChildPolicy(&b.OutlierDetection): return false } diff --git a/xds/internal/balancer/clusterresolver/configbuilder.go b/xds/internal/balancer/clusterresolver/configbuilder.go index ddb3cbfdb3e..dc91d7fbd13 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder.go +++ b/xds/internal/balancer/clusterresolver/configbuilder.go @@ -158,7 +158,7 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi retAddrs = append(retAddrs, addrs...) var odCfg *outlierdetection.LBConfig if envconfig.XDSOutlierDetection { - odCfg = makeClusterImplOutlierDetectionChild(*config, p.mechanism.OutlierDetection) + odCfg = makeClusterImplOutlierDetectionChild(config, p.mechanism.OutlierDetection) retConfig.Children[name] = &priority.Child{ Config: &internalserviceconfig.BalancerConfig{Name: outlierdetection.Name, Config: odCfg}, // Not ignore re-resolution from DNS children, they will trigger @@ -178,21 +178,18 @@ func buildPriorityConfig(priorities []priorityConfig, xdsLBPolicy *internalservi return retConfig, retAddrs, nil } -func convertClusterImplMapToOutlierDetection(ciCfgs map[string]*clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) map[string]*outlierdetection.LBConfig { +func convertClusterImplMapToOutlierDetection(ciCfgs map[string]*clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) map[string]*outlierdetection.LBConfig { odCfgs := make(map[string]*outlierdetection.LBConfig, len(ciCfgs)) for n, c := range ciCfgs { - odCfgs[n] = makeClusterImplOutlierDetectionChild(*c, odCfg) + odCfgs[n] = makeClusterImplOutlierDetectionChild(c, odCfg) } return odCfgs } -func makeClusterImplOutlierDetectionChild(ciCfg clusterimpl.LBConfig, odCfg *outlierdetection.LBConfig) *outlierdetection.LBConfig { - odCfgRet := &outlierdetection.LBConfig{} - if odCfg != nil { - *odCfgRet = *odCfg - } - odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: &ciCfg} - return odCfgRet +func makeClusterImplOutlierDetectionChild(ciCfg *clusterimpl.LBConfig, odCfg outlierdetection.LBConfig) *outlierdetection.LBConfig { + odCfgRet := odCfg + odCfgRet.ChildPolicy = &internalserviceconfig.BalancerConfig{Name: clusterimpl.Name, Config: ciCfg} + return &odCfgRet } func buildClusterImplConfigForDNS(g *nameGenerator, addrStrs []string, mechanism DiscoveryMechanism) (string, *clusterimpl.LBConfig, []resolver.Address) { diff --git a/xds/internal/balancer/clusterresolver/configbuilder_test.go b/xds/internal/balancer/clusterresolver/configbuilder_test.go index 95962ac0116..cfe7de65d1a 100644 --- a/xds/internal/balancer/clusterresolver/configbuilder_test.go +++ b/xds/internal/balancer/clusterresolver/configbuilder_test.go @@ -71,7 +71,7 @@ var ( return out })} - noopODCfg = &outlierdetection.LBConfig{ + noopODCfg = outlierdetection.LBConfig{ Interval: 1<<63 - 1, } ) @@ -1122,7 +1122,7 @@ func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { tests := []struct { name string ciCfgsMap map[string]*clusterimpl.LBConfig - odCfg *outlierdetection.LBConfig + odCfg outlierdetection.LBConfig wantODCfgs map[string]*outlierdetection.LBConfig }{ { @@ -1132,7 +1132,7 @@ func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { Cluster: "cluster1", }, }, - odCfg: &outlierdetection.LBConfig{ + odCfg: outlierdetection.LBConfig{ Interval: 1<<63 - 1, }, wantODCfgs: map[string]*outlierdetection.LBConfig{ @@ -1157,7 +1157,7 @@ func TestConvertClusterImplMapToOutlierDetection(t *testing.T) { Cluster: "cluster2", }, }, - odCfg: &outlierdetection.LBConfig{ + odCfg: outlierdetection.LBConfig{ Interval: 1<<63 - 1, }, wantODCfgs: map[string]*outlierdetection.LBConfig{ From 457288009d8ba09c72ed3c20b1aa8bc35f358e00 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Thu, 16 Jun 2022 21:13:17 -0400 Subject: [PATCH 6/6] Responded to Easwar's comment --- xds/internal/balancer/clusterresolver/clusterresolver_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xds/internal/balancer/clusterresolver/clusterresolver_test.go b/xds/internal/balancer/clusterresolver/clusterresolver_test.go index 5876ca013bc..6e96f2e31f8 100644 --- a/xds/internal/balancer/clusterresolver/clusterresolver_test.go +++ b/xds/internal/balancer/clusterresolver/clusterresolver_test.go @@ -145,7 +145,7 @@ func (f *fakeChildBalancer) waitForClientConnStateChangeVerifyBalancerConfig(ctx return err } gotCCS := ccs.(balancer.ClientConnState) - if diff := cmp.Diff(gotCCS, wantCCS, cmpopts.IgnoreFields(resolver.State{}, "Addresses", "ServiceConfig", "Attributes")); diff != "" { + if diff := cmp.Diff(gotCCS, wantCCS, cmpopts.IgnoreFields(balancer.ClientConnState{}, "ResolverState")); diff != "" { return fmt.Errorf("received unexpected ClientConnState, diff (-got +want): %v", diff) } return nil