Skip to content

Commit

Permalink
Tune default outlier detection parameters for remote endpoints (#207)
Browse files Browse the repository at this point in the history
  • Loading branch information
@aattuluri
aattuluri committed May 2, 2022
1 parent dd976ab commit ecc771e
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 19 deletions.
37 changes: 28 additions & 9 deletions admiral/pkg/clusters/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package clusters
import (
"bytes"
"fmt"
"net"
"reflect"
"sort"
"strings"
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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)
Expand Down
113 changes: 105 additions & 8 deletions admiral/pkg/clusters/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -281,14 +287,105 @@ 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))
}
})
}
}

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{
Expand Down
4 changes: 2 additions & 2 deletions admiral/pkg/clusters/serviceentry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit ecc771e

Please sign in to comment.