From 168fc9c67c3f774109f31da1c173dea39a2b799e Mon Sep 17 00:00:00 2001 From: Elina Akhmanova Date: Mon, 26 Sep 2022 22:15:58 +0200 Subject: [PATCH] Change l4 healthcheck timeout According to l4 healthcheck constants the default set up has timeout = 8 seconds and threshold = 3 times. As result it can lead to 24 (8*3) seconds of lost requests. This change is supposed to decrease this time to only 6 seconds for sharedHc=false. Signed-off-by: Elina Akhmanova --- pkg/healthchecksl4/healthchecksl4.go | 34 ++- pkg/healthchecksl4/healthchecksl4_test.go | 311 ++++++++++++++++++---- 2 files changed, 291 insertions(+), 54 deletions(-) diff --git a/pkg/healthchecksl4/healthchecksl4.go b/pkg/healthchecksl4/healthchecksl4.go index 1f0992c8d0..bb038464b6 100644 --- a/pkg/healthchecksl4/healthchecksl4.go +++ b/pkg/healthchecksl4/healthchecksl4.go @@ -38,12 +38,13 @@ import ( const ( // L4 Load Balancer parameters - gceHcCheckIntervalSeconds = int64(8) - gceHcTimeoutSeconds = int64(1) + gceSharedHcCheckIntervalSeconds = int64(8) // Shared Health check Interval + gceLocalHcCheckIntervalSeconds = int64(3) // Local Health check Interval + gceHcTimeoutSeconds = int64(1) // Start sending requests as soon as one healthcheck succeeds. - gceHcHealthyThreshold = int64(1) - // Defaults to 3 * 8 = 24 seconds before the LB will steer traffic away. - gceHcUnhealthyThreshold = int64(3) + gceHcHealthyThreshold = int64(1) + gceSharedHcUnhealthyThreshold = int64(3) // 3 * 8 = 24 seconds before the LB will steer traffic away + gceLocalHcUnhealthyThreshold = int64(2) // 2 * 3 = 6 seconds before the LB will steer traffic away ) var ( @@ -95,6 +96,24 @@ func GetInstance() *l4HealthChecks { return instance } +// Returns an interval constant based of shared/local status +func healthcheckInterval(isShared bool) int64 { + if isShared { + return gceSharedHcCheckIntervalSeconds + } else { + return gceLocalHcCheckIntervalSeconds + } +} + +// Returns a threshold for unhealthy instance based of shared/local status +func healthcheckUnhealthyThreshold(isShared bool) int64 { + if isShared { + return gceSharedHcUnhealthyThreshold + } else { + return gceLocalHcUnhealthyThreshold + } +} + // EnsureHealthCheckWithFirewall exist for the L4 // LoadBalancer Service. // @@ -302,12 +321,13 @@ func newL4HealthCheck(name string, svcName types.NamespacedName, shared bool, pa if err != nil { klog.Warningf("Failed to generate description for L4HealthCheck %s, err %v", name, err) } + return &composite.HealthCheck{ Name: name, - CheckIntervalSec: gceHcCheckIntervalSeconds, + CheckIntervalSec: healthcheckInterval(shared), TimeoutSec: gceHcTimeoutSeconds, HealthyThreshold: gceHcHealthyThreshold, - UnhealthyThreshold: gceHcUnhealthyThreshold, + UnhealthyThreshold: healthcheckUnhealthyThreshold(shared), HttpHealthCheck: &httpSettings, Type: "HTTP", Description: desc, diff --git a/pkg/healthchecksl4/healthchecksl4_test.go b/pkg/healthchecksl4/healthchecksl4_test.go index 448736c8cc..b732846156 100644 --- a/pkg/healthchecksl4/healthchecksl4_test.go +++ b/pkg/healthchecksl4/healthchecksl4_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" + "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/types" "k8s.io/ingress-gce/pkg/composite" "k8s.io/ingress-gce/pkg/utils" @@ -27,7 +28,7 @@ import ( func TestMergeHealthChecks(t *testing.T) { t.Parallel() - for _, tc := range []struct { + testCases := []struct { desc string checkIntervalSec int64 timeoutSec int64 @@ -38,16 +39,107 @@ func TestMergeHealthChecks(t *testing.T) { wantHealthyThreshold int64 wantUnhealthyThreshold int64 }{ - {"unchanged", gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold}, - {"interval - too small - should reconcile", gceHcCheckIntervalSeconds - 1, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold}, - {"timeout - too small - should reconcile", gceHcCheckIntervalSeconds, gceHcTimeoutSeconds - 1, gceHcHealthyThreshold, gceHcUnhealthyThreshold, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold}, - {"healthy threshold - too small - should reconcile", gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold - 1, gceHcUnhealthyThreshold, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold}, - {"unhealthy threshold - too small - should reconcile", gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold - 1, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold}, - {"interval - user configured - should keep", gceHcCheckIntervalSeconds + 1, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold, gceHcCheckIntervalSeconds + 1, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold}, - {"timeout - user configured - should keep", gceHcCheckIntervalSeconds, gceHcTimeoutSeconds + 1, gceHcHealthyThreshold, gceHcUnhealthyThreshold, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds + 1, gceHcHealthyThreshold, gceHcUnhealthyThreshold}, - {"healthy threshold - user configured - should keep", gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold + 1, gceHcUnhealthyThreshold, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold + 1, gceHcUnhealthyThreshold}, - {"unhealthy threshold - user configured - should keep", gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold + 1, gceHcCheckIntervalSeconds, gceHcTimeoutSeconds, gceHcHealthyThreshold, gceHcUnhealthyThreshold + 1}, - } { + { + desc: "unchanged", + checkIntervalSec: gceLocalHcCheckIntervalSeconds, + timeoutSec: gceHcTimeoutSeconds, + healthyThreshold: gceHcHealthyThreshold, + unhealthyThreshold: gceLocalHcUnhealthyThreshold, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "interval - too small - should reconcile", + checkIntervalSec: gceLocalHcCheckIntervalSeconds - 1, + timeoutSec: gceHcTimeoutSeconds, + healthyThreshold: gceHcHealthyThreshold, + unhealthyThreshold: gceLocalHcUnhealthyThreshold, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "timeout - too small - should reconcile", + checkIntervalSec: gceLocalHcCheckIntervalSeconds, + timeoutSec: gceHcTimeoutSeconds - 1, + healthyThreshold: gceHcHealthyThreshold, + unhealthyThreshold: gceLocalHcUnhealthyThreshold, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "healthy threshold - too small - should reconcil", + checkIntervalSec: gceLocalHcCheckIntervalSeconds, + timeoutSec: gceHcTimeoutSeconds, + healthyThreshold: gceHcHealthyThreshold - 1, + unhealthyThreshold: gceLocalHcUnhealthyThreshold, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "unhealthy threshold - too small - should reconcile", + checkIntervalSec: gceLocalHcCheckIntervalSeconds, + timeoutSec: gceHcTimeoutSeconds, + healthyThreshold: gceHcHealthyThreshold, + unhealthyThreshold: gceLocalHcUnhealthyThreshold - 1, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "interval - user configured - should keep", + checkIntervalSec: gceLocalHcCheckIntervalSeconds + 1, + timeoutSec: gceHcTimeoutSeconds, + healthyThreshold: gceHcHealthyThreshold, + unhealthyThreshold: gceLocalHcUnhealthyThreshold, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds + 1, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "timeout - user configured - should keep", + checkIntervalSec: gceLocalHcCheckIntervalSeconds, + timeoutSec: gceHcTimeoutSeconds + 1, + healthyThreshold: gceHcHealthyThreshold, + unhealthyThreshold: gceLocalHcUnhealthyThreshold, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds + 1, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "healthy threshold - user configured - should keep", + checkIntervalSec: gceLocalHcCheckIntervalSeconds, + timeoutSec: gceHcTimeoutSeconds, + healthyThreshold: gceHcHealthyThreshold + 1, + unhealthyThreshold: gceLocalHcUnhealthyThreshold, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold + 1, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + { + desc: "unhealthy threshold - user configured - should keep", + checkIntervalSec: gceLocalHcCheckIntervalSeconds, + timeoutSec: gceHcTimeoutSeconds, + healthyThreshold: gceHcHealthyThreshold, + unhealthyThreshold: gceLocalHcUnhealthyThreshold + 1, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantTimeoutSec: gceHcTimeoutSeconds, + wantHealthyThreshold: gceHcHealthyThreshold, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold + 1, + }, + } + for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { // healthcheck intervals and thresholds are common for Global and Regional healthchecks. Hence testing only Global case. wantHC := newL4HealthCheck("hc", types.NamespacedName{Name: "svc", Namespace: "default"}, false, "/", 12345, utils.ILB, meta.Global, "") @@ -58,43 +150,88 @@ func TestMergeHealthChecks(t *testing.T) { UnhealthyThreshold: tc.unhealthyThreshold, } mergeHealthChecks(hc, wantHC) - if wantHC.CheckIntervalSec != tc.wantCheckIntervalSec { - t.Errorf("wantHC.CheckIntervalSec = %d; want %d", wantHC.CheckIntervalSec, tc.checkIntervalSec) - } - if wantHC.TimeoutSec != tc.wantTimeoutSec { - t.Errorf("wantHC.TimeoutSec = %d; want %d", wantHC.TimeoutSec, tc.timeoutSec) - } - if wantHC.HealthyThreshold != tc.wantHealthyThreshold { - t.Errorf("wantHC.HealthyThreshold = %d; want %d", wantHC.HealthyThreshold, tc.healthyThreshold) - } - if wantHC.UnhealthyThreshold != tc.wantUnhealthyThreshold { - t.Errorf("wantHC.UnhealthyThreshold = %d; want %d", wantHC.UnhealthyThreshold, tc.unhealthyThreshold) - } + assert.Equalf(t, wantHC.CheckIntervalSec, tc.wantCheckIntervalSec, "wantHC.CheckIntervalSec = %d; want %d", wantHC.CheckIntervalSec, tc.checkIntervalSec) + assert.Equalf(t, wantHC.TimeoutSec, tc.wantTimeoutSec, "wantHC.TimeoutSec = %d; want %d", wantHC.TimeoutSec, tc.timeoutSec) + assert.Equalf(t, wantHC.HealthyThreshold, tc.wantHealthyThreshold, "wantHC.HealthyThreshold = %d; want %d", wantHC.HealthyThreshold, tc.healthyThreshold) + assert.Equalf(t, wantHC.UnhealthyThreshold, tc.wantUnhealthyThreshold, "wantHC.UnhealthyThreshold = %d; want %d", wantHC.UnhealthyThreshold, tc.unhealthyThreshold) }) } } func TestCompareHealthChecks(t *testing.T) { t.Parallel() - for _, tc := range []struct { + testCases := []struct { desc string modifier func(*composite.HealthCheck) wantChanged bool }{ - {"unchanged", nil, false}, - {"nil HttpHealthCheck", func(hc *composite.HealthCheck) { hc.HttpHealthCheck = nil }, true}, - {"desc does not match", func(hc *composite.HealthCheck) { hc.Description = "bad-desc" }, true}, - {"port does not match", func(hc *composite.HealthCheck) { hc.HttpHealthCheck.Port = 54321 }, true}, - {"requestPath does not match", func(hc *composite.HealthCheck) { hc.HttpHealthCheck.RequestPath = "/anotherone" }, true}, - {"interval needs update", func(hc *composite.HealthCheck) { hc.CheckIntervalSec = gceHcCheckIntervalSeconds - 1 }, true}, - {"timeout needs update", func(hc *composite.HealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds - 1 }, true}, - {"healthy threshold needs update", func(hc *composite.HealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold - 1 }, true}, - {"unhealthy threshold needs update", func(hc *composite.HealthCheck) { hc.UnhealthyThreshold = gceHcUnhealthyThreshold - 1 }, true}, - {"interval does not need update", func(hc *composite.HealthCheck) { hc.CheckIntervalSec = gceHcCheckIntervalSeconds + 1 }, false}, - {"timeout does not need update", func(hc *composite.HealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds + 1 }, false}, - {"healthy threshold does not need update", func(hc *composite.HealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold + 1 }, false}, - {"unhealthy threshold does not need update", func(hc *composite.HealthCheck) { hc.UnhealthyThreshold = gceHcUnhealthyThreshold + 1 }, false}, - } { + { + desc: "unchanged", + modifier: nil, + wantChanged: false, + }, + { + desc: "nil HttpHealthCheck", + modifier: func(hc *composite.HealthCheck) { hc.HttpHealthCheck = nil }, + wantChanged: true, + }, + { + desc: "desc does not match", + modifier: func(hc *composite.HealthCheck) { hc.Description = "bad-desc" }, + wantChanged: true, + }, + { + desc: "port does not match", + modifier: func(hc *composite.HealthCheck) { hc.HttpHealthCheck.Port = 54321 }, + wantChanged: true, + }, + { + desc: "requestPath does not match", + modifier: func(hc *composite.HealthCheck) { hc.HttpHealthCheck.RequestPath = "/anotherone" }, + wantChanged: true, + }, + { + desc: "interval needs update", + modifier: func(hc *composite.HealthCheck) { hc.CheckIntervalSec = gceLocalHcCheckIntervalSeconds - 1 }, + wantChanged: true, + }, + { + desc: "timeout needs update", + modifier: func(hc *composite.HealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds - 1 }, + wantChanged: true, + }, + { + desc: "healthy threshold needs update", + modifier: func(hc *composite.HealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold - 1 }, + wantChanged: true, + }, + { + desc: "unhealthy threshold needs update", + modifier: func(hc *composite.HealthCheck) { hc.UnhealthyThreshold = gceLocalHcUnhealthyThreshold - 1 }, + wantChanged: true, + }, + { + desc: "interval does not need update", + modifier: func(hc *composite.HealthCheck) { hc.CheckIntervalSec = gceLocalHcCheckIntervalSeconds + 1 }, + wantChanged: false, + }, + { + desc: "timeout does not need update", + modifier: func(hc *composite.HealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds + 1 }, + wantChanged: false, + }, + { + desc: "healthy threshold does not need update", + modifier: func(hc *composite.HealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold + 1 }, + wantChanged: false, + }, + { + desc: "unhealthy threshold does not need update", + modifier: func(hc *composite.HealthCheck) { hc.UnhealthyThreshold = gceLocalHcUnhealthyThreshold + 1 }, + wantChanged: false, + }, + } + for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { // healthcheck intervals and thresholds are common for Global and Regional healthchecks. Hence testing only Global case. hc := newL4HealthCheck("hc", types.NamespacedName{Name: "svc", Namespace: "default"}, false, "/", 12345, utils.ILB, meta.Global, "") @@ -102,9 +239,93 @@ func TestCompareHealthChecks(t *testing.T) { if tc.modifier != nil { tc.modifier(hc) } - if gotChanged := needToUpdateHealthChecks(hc, wantHC); gotChanged != tc.wantChanged { - t.Errorf("needToUpdateHealthChecks(%#v, %#v) = %t; want changed = %t", hc, wantHC, gotChanged, tc.wantChanged) - } + gotChanged := needToUpdateHealthChecks(hc, wantHC) + assert.Equalf(t, gotChanged, tc.wantChanged, "needToUpdateHealthChecks(%#v, %#v) = %t; want changed = %t", hc, wantHC, gotChanged, tc.wantChanged) + }) + } +} + +func TestHealthcheckInterval(t *testing.T) { + t.Parallel() + testCases := []struct { + desc string + shared bool + wantCheckIntervalSec int64 + }{ + { + desc: "shared - check interval function", + shared: true, + wantCheckIntervalSec: gceSharedHcCheckIntervalSeconds, + }, + { + desc: "local - check interval function", + shared: false, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + }, + } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + gotInterval := healthcheckInterval(tc.shared) + assert.Equalf(t, gotInterval, tc.wantCheckIntervalSec, "got Health Check Interval: %d, want: %d", gotInterval, tc.wantCheckIntervalSec) + }) + } +} + +func TestHealthcheckUnhealthyThreshold(t *testing.T) { + t.Parallel() + testCases := []struct { + desc string + shared bool + wantUnhealthyThreshold int64 + }{ + { + desc: "shared - check unhealthy threshold function", + shared: true, + wantUnhealthyThreshold: gceSharedHcUnhealthyThreshold, + }, + { + desc: "local - check unhealthy threshold function", + shared: false, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + } + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + gotUnhealthyThreshold := healthcheckUnhealthyThreshold(tc.shared) + assert.Equalf(t, gotUnhealthyThreshold, tc.wantUnhealthyThreshold, "got Unhealthy Threshold: %d, want: %d", gotUnhealthyThreshold, tc.wantUnhealthyThreshold) + }) + } +} + +// Checks that correct health check constants used for different traffic policies types (local/shared) +func TestSharedHealthChecks(t *testing.T) { + t.Parallel() + testCases := []struct { + desc string + shared bool + wantCheckIntervalSec int64 + wantUnhealthyThreshold int64 + }{ + { + desc: "shared - check interval and unhealthy threshold", + shared: true, + wantCheckIntervalSec: gceSharedHcCheckIntervalSeconds, + wantUnhealthyThreshold: gceSharedHcUnhealthyThreshold, + }, + { + desc: "local - check interval and unhealthy threshold", + shared: false, + wantCheckIntervalSec: gceLocalHcCheckIntervalSeconds, + wantUnhealthyThreshold: gceLocalHcUnhealthyThreshold, + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + // healthcheck intervals and thresholds are common for Global and Regional healthchecks. Hence testing only Global case. + gotHC := newL4HealthCheck("hc", types.NamespacedName{Name: "svc", Namespace: "default"}, tc.shared, "/", 12345, utils.ILB, meta.Global, "") + assert.Equalf(t, gotHC.CheckIntervalSec, tc.wantCheckIntervalSec, "gotHC.CheckIntervalSec = %d; want %d", gotHC.CheckIntervalSec, tc.wantCheckIntervalSec) + assert.Equalf(t, gotHC.UnhealthyThreshold, tc.wantUnhealthyThreshold, "gotHC.UnhealthyThreshold = %d; want %d", gotHC.UnhealthyThreshold, tc.wantUnhealthyThreshold) }) } } @@ -121,12 +342,8 @@ func TestNewHealthCheck(t *testing.T) { {meta.Regional, "us-central1"}, } { hc := newL4HealthCheck("hc", namespaceName, false, "/", 12345, utils.ILB, v.scope, v.region) - if hc.Region != v.region { - t.Errorf("HealthCheck Region mismatch! %v != %v", hc.Region, v.region) - } - if hc.Scope != v.scope { - t.Errorf("HealthCheck Scope mismatch! %v != %v", hc.Scope, v.scope) - } + assert.Equalf(t, hc.Region, v.region, "HealthCheck Region mismatch! %v != %v", hc.Region, v.region) + assert.Equalf(t, hc.Scope, v.scope, "HealthCheck Scope mismatch! %v != %v", hc.Scope, v.scope) } }