From 390fe6a6309996b040f419aef713231ff5b8c069 Mon Sep 17 00:00:00 2001 From: Zach Reyes Date: Fri, 8 Apr 2022 15:55:35 -0400 Subject: [PATCH] Added branching logic for Outlier Detection configuration in config builder --- .../clusterresolver/clusterresolver_test.go | 131 ++++++++++- .../balancer/clusterresolver/config_test.go | 8 +- .../balancer/clusterresolver/configbuilder.go | 49 +++- .../clusterresolver/configbuilder_test.go | 220 +++++++++++++++++- .../balancer/clusterresolver/eds_impl_test.go | 2 +- .../clusterresolver/resource_resolver_test.go | 18 +- .../balancer/outlierdetection/balancer.go | 121 ++++++++++ .../outlierdetection/balancer_test.go | 185 +++++++++++++++ 8 files changed, 709 insertions(+), 25 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 3b0843f6807e..80ac7b2d9e72 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" @@ -41,7 +49,7 @@ import ( const ( defaultTestTimeout = 1 * time.Second defaultTestShortTimeout = 10 * time.Millisecond - testEDSServcie = "test-eds-service-name" + testEDSService = "test-eds-service-name" testClusterName = "test-cluster-name" testClusterName2 = "google_cfe_some-name" ) @@ -99,7 +107,7 @@ func (t *noopTestClientConn) NewSubConn([]resolver.Address, balancer.NewSubConnO return nil, nil } -func (noopTestClientConn) Target() string { return testEDSServcie } +func (noopTestClientConn) Target() string { return testEDSService } type scStateChange struct { sc balancer.SubConn @@ -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 { @@ -217,7 +237,7 @@ func (s) TestSubConnStateChange(t *testing.T) { if err := edsB.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{}, xdsC), - BalancerConfig: newLBConfigWithOneEDS(testEDSServcie), + BalancerConfig: newLBConfigWithOneEDS(testEDSService), }); err != nil { t.Fatalf("edsB.UpdateClientConnState() failed: %v", err) } @@ -265,7 +285,7 @@ func (s) TestErrorFromXDSClientUpdate(t *testing.T) { defer cancel() if err := edsB.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{}, xdsC), - BalancerConfig: newLBConfigWithOneEDS(testEDSServcie), + BalancerConfig: newLBConfigWithOneEDS(testEDSService), }); err != nil { t.Fatal(err) } @@ -319,7 +339,7 @@ func (s) TestErrorFromXDSClientUpdate(t *testing.T) { // An update with the same service name should not trigger a new watch. if err := edsB.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{}, xdsC), - BalancerConfig: newLBConfigWithOneEDS(testEDSServcie), + BalancerConfig: newLBConfigWithOneEDS(testEDSService), }); err != nil { t.Fatal(err) } @@ -353,7 +373,7 @@ func (s) TestErrorFromResolver(t *testing.T) { defer cancel() if err := edsB.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{}, xdsC), - BalancerConfig: newLBConfigWithOneEDS(testEDSServcie), + BalancerConfig: newLBConfigWithOneEDS(testEDSService), }); err != nil { t.Fatal(err) } @@ -404,7 +424,7 @@ func (s) TestErrorFromResolver(t *testing.T) { // the previous watch was canceled. if err := edsB.UpdateClientConnState(balancer.ClientConnState{ ResolverState: xdsclient.SetClient(resolver.State{}, xdsC), - BalancerConfig: newLBConfigWithOneEDS(testEDSServcie), + BalancerConfig: newLBConfigWithOneEDS(testEDSService), }); err != nil { t.Fatal(err) } @@ -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(testEDSService, 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-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: "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-1"}, + } + + 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/config_test.go b/xds/internal/balancer/clusterresolver/config_test.go index fb859e75ba4b..70bf3da87422 100644 --- a/xds/internal/balancer/clusterresolver/config_test.go +++ b/xds/internal/balancer/clusterresolver/config_test.go @@ -195,7 +195,7 @@ func TestParseConfig(t *testing.T) { LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }, }, XDSLBPolicy: nil, @@ -212,7 +212,7 @@ func TestParseConfig(t *testing.T) { LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }, { Type: DiscoveryMechanismTypeLogicalDNS, @@ -232,7 +232,7 @@ func TestParseConfig(t *testing.T) { LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }, }, XDSLBPolicy: &internalserviceconfig.BalancerConfig{ @@ -252,7 +252,7 @@ func TestParseConfig(t *testing.T) { LoadReportingServer: testLRSServerConfig, MaxConcurrentRequests: newUint32(testMaxRequests), Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }, }, XDSLBPolicy: &internalserviceconfig.BalancerConfig{ diff --git a/xds/internal/balancer/clusterresolver/configbuilder.go b/xds/internal/balancer/clusterresolver/configbuilder.go index 4cce16ff9a3d..eada7bed20cd 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" @@ -125,30 +127,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(i, 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(parentPriority int, 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 607f7b222419..a4d7fbdcd1ee 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() { @@ -308,6 +314,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], + }, + }, + }, + { + // This OD config should wrap the Logical DNS priorities balancer. + mechanism: DiscoveryMechanism{ + Cluster: testClusterName2, + Type: DiscoveryMechanismTypeLogicalDNS, + OutlierDetection: noopODCfg, + }, + addresses: testAddressStrs[4], + }, + }, 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) + } + // TODO: combine this into a t-test, with a knob on whether OD was specified or not, which this bool could also gate + // checking equality of addresses or not? +} + func TestBuildClusterImplConfigForDNS(t *testing.T) { gotName, gotConfig, gotAddrs := buildClusterImplConfigForDNS(3, testAddressStrs[0], DiscoveryMechanism{Cluster: testClusterName2, Type: DiscoveryMechanismTypeLogicalDNS}) wantName := "priority-3" @@ -598,12 +740,12 @@ func TestPriorityLocalitiesToClusterImpl(t *testing.T) { mechanism: DiscoveryMechanism{ Cluster: testClusterName, Type: DiscoveryMechanismTypeEDS, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }, // lrsServer is nil, so LRS policy will not be used. wantConfig: &clusterimpl.LBConfig{ Cluster: testClusterName, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, ChildPolicy: &internalserviceconfig.BalancerConfig{ Name: weightedtarget.Name, Config: &weightedtarget.LBConfig{ @@ -980,3 +1122,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/clusterresolver/eds_impl_test.go b/xds/internal/balancer/clusterresolver/eds_impl_test.go index 7f2bfa8a75d1..af20fc22bb94 100644 --- a/xds/internal/balancer/clusterresolver/eds_impl_test.go +++ b/xds/internal/balancer/clusterresolver/eds_impl_test.go @@ -62,7 +62,7 @@ func setupTestEDS(t *testing.T, initChild *internalserviceconfig.BalancerConfig) xdsC := fakeclient.NewClientWithName(testBalancerNameFooBar) cc := testutils.NewTestClientConn(t) builder := balancer.Get(Name) - edsb := builder.Build(cc, balancer.BuildOptions{Target: resolver.Target{Endpoint: testEDSServcie}}) + edsb := builder.Build(cc, balancer.BuildOptions{Target: resolver.Target{Endpoint: testEDSService}}) if edsb == nil { t.Fatalf("builder.Build(%s) failed and returned nil", Name) } diff --git a/xds/internal/balancer/clusterresolver/resource_resolver_test.go b/xds/internal/balancer/clusterresolver/resource_resolver_test.go index 432fdd9ceb65..54597b6c6dc5 100644 --- a/xds/internal/balancer/clusterresolver/resource_resolver_test.go +++ b/xds/internal/balancer/clusterresolver/resource_resolver_test.go @@ -59,14 +59,14 @@ func (s) TestResourceResolverOneEDSResource(t *testing.T) { }{ {name: "watch EDS", clusterName: testClusterName, - edsName: testEDSServcie, - wantName: testEDSServcie, + edsName: testEDSService, + wantName: testEDSService, edsUpdate: testEDSUpdates[0], want: []priorityConfig{{ mechanism: DiscoveryMechanism{ Type: DiscoveryMechanismTypeEDS, Cluster: testClusterName, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }, edsResp: testEDSUpdates[0], }}, @@ -120,7 +120,7 @@ func (s) TestResourceResolverOneEDSResource(t *testing.T) { t.Fatalf("xdsClient.CancelCDS failed with error: %v", err) } if edsNameCanceled != test.wantName { - t.Fatalf("xdsClient.CancelEDS called for %v, want: %v", edsNameCanceled, testEDSServcie) + t.Fatalf("xdsClient.CancelEDS called for %v, want: %v", edsNameCanceled, testEDSService) } }) } @@ -221,7 +221,7 @@ func (s) TestResourceResolverChangeEDSName(t *testing.T) { rr.updateMechanisms([]DiscoveryMechanism{{ Type: DiscoveryMechanismTypeEDS, Cluster: testClusterName, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }}) ctx, ctxCancel := context.WithTimeout(context.Background(), defaultTestTimeout) defer ctxCancel() @@ -229,8 +229,8 @@ func (s) TestResourceResolverChangeEDSName(t *testing.T) { if err != nil { t.Fatalf("xdsClient.WatchCDS failed with error: %v", err) } - if gotEDSName1 != testEDSServcie { - t.Fatalf("xdsClient.WatchEDS called for cluster: %v, want: %v", gotEDSName1, testEDSServcie) + if gotEDSName1 != testEDSService { + t.Fatalf("xdsClient.WatchEDS called for cluster: %v, want: %v", gotEDSName1, testEDSService) } // Invoke callback, should get an update. @@ -241,7 +241,7 @@ func (s) TestResourceResolverChangeEDSName(t *testing.T) { mechanism: DiscoveryMechanism{ Type: DiscoveryMechanismTypeEDS, Cluster: testClusterName, - EDSServiceName: testEDSServcie, + EDSServiceName: testEDSService, }, edsResp: testEDSUpdates[0], }}, cmp.AllowUnexported(priorityConfig{})); diff != "" { @@ -261,7 +261,7 @@ func (s) TestResourceResolverChangeEDSName(t *testing.T) { t.Fatalf("xdsClient.CancelCDS failed with error: %v", err) } if edsNameCanceled1 != gotEDSName1 { - t.Fatalf("xdsClient.CancelEDS called for %v, want: %v", edsNameCanceled1, testEDSServcie) + t.Fatalf("xdsClient.CancelEDS called for %v, want: %v", edsNameCanceled1, testEDSService) } gotEDSName2, err := fakeClient.WaitForWatchEDS(ctx) if err != nil { diff --git a/xds/internal/balancer/outlierdetection/balancer.go b/xds/internal/balancer/outlierdetection/balancer.go new file mode 100644 index 000000000000..4a50578bc92a --- /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 000000000000..52ee7fdf7ac5 --- /dev/null +++ b/xds/internal/balancer/outlierdetection/balancer_test.go @@ -0,0 +1,185 @@ +/* + * + * 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) + } + }) + } +}