From 833f92113df9339a64dd5ef20ab6e0bd7fbeada4 Mon Sep 17 00:00:00 2001 From: aattuluri <44482891+aattuluri@users.noreply.github.com> Date: Mon, 2 May 2022 09:41:20 -0700 Subject: [PATCH] Tune default outlier detection parameters for remote endpoints (#207) Signed-off-by: sa --- admiral/pkg/clusters/handler.go | 37 ++++++--- admiral/pkg/clusters/handler_test.go | 113 +++++++++++++++++++++++++-- admiral/pkg/clusters/serviceentry.go | 4 +- 3 files changed, 135 insertions(+), 19 deletions(-) diff --git a/admiral/pkg/clusters/handler.go b/admiral/pkg/clusters/handler.go index 4fab356f..454235c6 100644 --- a/admiral/pkg/clusters/handler.go +++ b/admiral/pkg/clusters/handler.go @@ -3,6 +3,7 @@ package clusters import ( "bytes" "fmt" + "net" "reflect" "sort" "strings" @@ -61,20 +62,15 @@ func getIstioResourceName(host string, suffix string) string { return strings.ToLower(host) + suffix } -func getDestinationRule(host string, locality string, gtpTrafficPolicy *model.TrafficPolicy) *v1alpha32.DestinationRule { +func getDestinationRule(se *v1alpha32.ServiceEntry, locality string, gtpTrafficPolicy *model.TrafficPolicy) *v1alpha32.DestinationRule { var dr = &v1alpha32.DestinationRule{} - dr.Host = host + dr.Host = se.Hosts[0] dr.TrafficPolicy = &v1alpha32.TrafficPolicy{Tls: &v1alpha32.TLSSettings{Mode: v1alpha32.TLSSettings_ISTIO_MUTUAL}} processGtp := true if len(locality) == 0 { - log.Warnf(LogErrFormat, "Process", "GlobalTrafficPolicy", host, "", "Skipping gtp processing, locality of the cluster nodes cannot be determined. Is this minikube?") + log.Warnf(LogErrFormat, "Process", "GlobalTrafficPolicy", dr.Host, "", "Skipping gtp processing, locality of the cluster nodes cannot be determined. Is this minikube?") processGtp = false } - outlierDetection := &v1alpha32.OutlierDetection{ - BaseEjectionTime: &types.Duration{Seconds: 300}, - Consecutive_5XxErrors: &types.UInt32Value{Value: uint32(10)}, - Interval: &types.Duration{Seconds: 60}, - } if gtpTrafficPolicy != nil && processGtp { var loadBalancerSettings = &v1alpha32.LoadBalancerSettings{ LbPolicy: &v1alpha32.LoadBalancerSettings_Simple{Simple: v1alpha32.LoadBalancerSettings_ROUND_ROBIN}, @@ -103,10 +99,33 @@ func getDestinationRule(host string, locality string, gtpTrafficPolicy *model.Tr dr.TrafficPolicy.LoadBalancer = loadBalancerSettings } } - dr.TrafficPolicy.OutlierDetection = outlierDetection + dr.TrafficPolicy.OutlierDetection = getOutlierDetection(se, locality, gtpTrafficPolicy) return dr } +func getOutlierDetection(se *v1alpha32.ServiceEntry, locality string, gtpTrafficPolicy *model.TrafficPolicy) *v1alpha32.OutlierDetection { + + outlierDetection := &v1alpha32.OutlierDetection{ + BaseEjectionTime: &types.Duration{Seconds: 300}, + ConsecutiveGatewayErrors: &types.UInt32Value{Value: uint32(50)}, + Interval: &types.Duration{Seconds: 60}, + } + + //Scenario 1: Only one endpoint present and is local service (ends in svc.cluster.local) - no outlier detection (optimize this for headless services in future?) + if len(se.Endpoints) == 1 && (strings.Contains(se.Endpoints[0].Address, common.DotLocalDomainSuffix) || net.ParseIP(se.Endpoints[0].Address).To4() != nil) { + return nil + } else if len(se.Endpoints) == 1 { + //Scenario 2: Only one endpoint present and is remote - outlier detection with 34% ejection (protection against zone specific issues) + outlierDetection.MaxEjectionPercent = 34 + } else { + //Scenario 3: Two endpoints present each with different locality and both remote - outlier detection with 100% ejection + //Scenario 4: Two endpoints present each with different locality with one local and other remote - outlier detection with 100% ejection + //for service entries with more than 2 endpoints eject 100% to failover to other endpoint within or outside the same region + outlierDetection.MaxEjectionPercent = 100 + } + return outlierDetection +} + func (se *ServiceEntryHandler) Added(obj *v1alpha3.ServiceEntry) { if IgnoreIstioResource(obj.Spec.ExportTo, obj.Annotations, obj.Namespace) { log.Infof(LogFormat, "Add", "ServiceEntry", obj.Name, se.ClusterID, "Skipping resource from namespace="+obj.Namespace) diff --git a/admiral/pkg/clusters/handler_test.go b/admiral/pkg/clusters/handler_test.go index 0089be30..29dcfe8a 100644 --- a/admiral/pkg/clusters/handler_test.go +++ b/admiral/pkg/clusters/handler_test.go @@ -176,10 +176,16 @@ func TestGetDestinationRule(t *testing.T) { //Do setup here outlierDetection := &v1alpha3.OutlierDetection{ BaseEjectionTime: &types.Duration{Seconds: 300}, - Consecutive_5XxErrors: &types.UInt32Value{Value: 10}, - Interval: &types.Duration{Seconds: 60}} + ConsecutiveGatewayErrors: &types.UInt32Value{Value: 50}, + Interval: &types.Duration{Seconds: 60}, + MaxEjectionPercent: 100, + } mTLS := &v1alpha3.TrafficPolicy{Tls: &v1alpha3.TLSSettings{Mode: v1alpha3.TLSSettings_ISTIO_MUTUAL}, OutlierDetection: outlierDetection} + + se := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{ + {Address: "east.com", Locality: "us-east-2"}, {Address: "west.com", Locality: "us-west-2"}, + }} noGtpDr := v1alpha3.DestinationRule{ Host: "qa.myservice.global", TrafficPolicy: mTLS, @@ -243,35 +249,35 @@ func TestGetDestinationRule(t *testing.T) { //Struct of test case info. Name is required. testCases := []struct { name string - host string + se *v1alpha3.ServiceEntry locality string gtpPolicy *model.TrafficPolicy destinationRule *v1alpha3.DestinationRule }{ { name: "Should handle a nil GTP", - host: "qa.myservice.global", + se: se, locality: "uswest2", gtpPolicy: nil, destinationRule: &noGtpDr, }, { name: "Should return default DR with empty locality", - host: "qa.myservice.global", + se: se, locality: "", gtpPolicy: failoverGTPPolicy, destinationRule: &noGtpDr, }, { name: "Should handle a topology GTP", - host: "qa.myservice.global", + se: se, locality: "uswest2", gtpPolicy: topologyGTPPolicy, destinationRule: &basicGtpDr, }, { name: "Should handle a failover GTP", - host: "qa.myservice.global", + se: se, locality: "uswest2", gtpPolicy: failoverGTPPolicy, destinationRule: &failoverGtpDr, @@ -281,7 +287,7 @@ func TestGetDestinationRule(t *testing.T) { //Run the test for every provided case for _, c := range testCases { t.Run(c.name, func(t *testing.T) { - result := getDestinationRule(c.host, c.locality, c.gtpPolicy) + result := getDestinationRule(c.se, c.locality, c.gtpPolicy) if !cmp.Equal(result, c.destinationRule) { t.Fatalf("DestinationRule Mismatch. Diff: %v", cmp.Diff(result, c.destinationRule)) } @@ -289,6 +295,97 @@ func TestGetDestinationRule(t *testing.T) { } } +func TestGetOutlierDetection(t *testing.T) { + //Do setup here + outlierDetection := &v1alpha3.OutlierDetection{ + BaseEjectionTime: &types.Duration{Seconds: 300}, + ConsecutiveGatewayErrors: &types.UInt32Value{Value: 50}, + Interval: &types.Duration{Seconds: 60}, + MaxEjectionPercent: 100, + } + + outlierDetectionOneHostRemote := &v1alpha3.OutlierDetection{ + BaseEjectionTime: &types.Duration{Seconds: 300}, + ConsecutiveGatewayErrors: &types.UInt32Value{Value: 50}, + Interval: &types.Duration{Seconds: 60}, + MaxEjectionPercent: 34, + } + + topologyGTPPolicy := &model.TrafficPolicy{ + LbType: model.TrafficPolicy_TOPOLOGY, + Target: []*model.TrafficGroup{ + { + Region: "us-west-2", + Weight: 100, + }, + }, + } + + se := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{ + {Address: "east.com", Locality: "us-east-2"}, {Address: "west.com", Locality: "us-west-2"}, + }} + + seOneHostRemote := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{ + {Address: "east.com", Locality: "us-east-2"}, + }} + + seOneHostLocal := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{ + {Address: "hello.ns.svc.cluster.local", Locality: "us-east-2"}, + }} + + seOneHostRemoteIp := &v1alpha3.ServiceEntry{Hosts: []string{"qa.myservice.global"},Endpoints:[]*v1alpha3.ServiceEntry_Endpoint{ + {Address: "95.45.25.34", Locality: "us-east-2"}, + }} + + //Struct of test case info. Name is required. + testCases := []struct { + name string + se *v1alpha3.ServiceEntry + locality string + gtpPolicy *model.TrafficPolicy + outlierDetection *v1alpha3.OutlierDetection + }{ + { + name: "Should return nil for cluster local only endpoint", + se: seOneHostLocal, + locality: "uswest2", + gtpPolicy: topologyGTPPolicy, + outlierDetection: nil, + }, + { + name: "Should return nil for one IP endpoint", + se: seOneHostRemoteIp, + locality: "uswest2", + gtpPolicy: topologyGTPPolicy, + outlierDetection: nil, + }, + { + name: "Should return 34% ejection for remote endpoint with one entry", + se: seOneHostRemote, + locality: "uswest2", + gtpPolicy: topologyGTPPolicy, + outlierDetection: outlierDetectionOneHostRemote, + }, + { + name: "Should return 100% ejection for two remote endpoints", + se: se, + locality: "uswest2", + gtpPolicy: topologyGTPPolicy, + outlierDetection: outlierDetection, + }, + } + + //Run the test for every provided case + for _, c := range testCases { + t.Run(c.name, func(t *testing.T) { + result := getOutlierDetection(c.se, c.locality, c.gtpPolicy) + if !cmp.Equal(result, c.outlierDetection) { + t.Fatalf("OutlierDetection Mismatch. Diff: %v", cmp.Diff(result, c.outlierDetection)) + } + }) + } +} + func TestHandleVirtualServiceEvent(t *testing.T) { tooManyHosts := v1alpha32.VirtualService{ Spec: v1alpha3.VirtualService{ diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go index 8f6aaecd..ad36ede6 100644 --- a/admiral/pkg/clusters/serviceentry.go +++ b/admiral/pkg/clusters/serviceentry.go @@ -469,7 +469,7 @@ func createSeAndDrSetFromGtp(env, region string, se *networking.ServiceEntry, gl var seDr = &SeDrTuple{ DrName: drName, SeName: seName, - DestinationRule: getDestinationRule(host, region, gtpTrafficPolicy), + DestinationRule: getDestinationRule(modifiedSe, region, gtpTrafficPolicy), ServiceEntry: modifiedSe, } seDrSet[host] = seDr @@ -480,7 +480,7 @@ func createSeAndDrSetFromGtp(env, region string, se *networking.ServiceEntry, gl var seDr = &SeDrTuple{ DrName: defaultDrName, SeName: defaultSeName, - DestinationRule: getDestinationRule(se.Hosts[0], region, nil), + DestinationRule: getDestinationRule(se, region, nil), ServiceEntry: se, } seDrSet[se.Hosts[0]] = seDr