Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change l4 healthcheck timeout #1828

Merged
merged 1 commit into from
Oct 6, 2022
Merged

Change l4 healthcheck timeout #1828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions pkg/healthchecksl4/healthchecksl4.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -79,6 +80,24 @@ func Fake(cloud *gce.Cloud, recorder record.EventRecorder) *l4HealthChecks {
}
}

// 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.
//
Expand Down Expand Up @@ -285,12 +304,16 @@ 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)
}
// Get constant values based on shared/local status
interval := healthcheckInterval(shared)
unhealthyThreshold := healthcheckUnhealthyThreshold(shared)

return &composite.HealthCheck{
Name: name,
CheckIntervalSec: gceHcCheckIntervalSeconds,
CheckIntervalSec: interval,
TimeoutSec: gceHcTimeoutSeconds,
HealthyThreshold: gceHcHealthyThreshold,
UnhealthyThreshold: gceHcUnhealthyThreshold,
UnhealthyThreshold: unhealthyThreshold,
HttpHealthCheck: &httpSettings,
Type: "HTTP",
Description: desc,
Expand Down
290 changes: 264 additions & 26 deletions pkg/healthchecksl4/healthchecksl4_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

func TestMergeHealthChecks(t *testing.T) {
t.Parallel()
for _, tc := range []struct {
testCases := []struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we add a shared bool var here we could this new dimension

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me do that, seems a low hanging fruit I can use to participate here

desc string
checkIntervalSec int64
timeoutSec int64
Expand All @@ -38,16 +38,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, "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't exercise shared/local that is the thing we just changed,no?

Expand Down Expand Up @@ -76,25 +167,78 @@ func TestMergeHealthChecks(t *testing.T) {

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, "")
Expand All @@ -109,6 +253,100 @@ func TestCompareHealthChecks(t *testing.T) {
}
}

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)
if gotInterval != tc.wantCheckIntervalSec {
t.Errorf("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)
if gotUnhealthyThreshold != tc.wantUnhealthyThreshold {
t.Errorf("got Unhealthy Threshold: %d, want: %d", gotUnhealthyThreshold, tc.wantUnhealthyThreshold)
}
})
}
}

// Checks that newL4HealthCheck() returns correct CheckInterval and UnhealthyThreshold
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, "")
if gotHC.CheckIntervalSec != tc.wantCheckIntervalSec {
t.Errorf("gotHC.CheckIntervalSec = %d; want %d", gotHC.CheckIntervalSec, tc.wantCheckIntervalSec)
}
if gotHC.UnhealthyThreshold != tc.wantUnhealthyThreshold {
t.Errorf("gotHC.UnhealthyThreshold = %d; want %d", gotHC.UnhealthyThreshold, tc.wantUnhealthyThreshold)
}
})
}
}

func TestNewHealthCheck(t *testing.T) {
t.Parallel()
namespaceName := types.NamespacedName{Name: "svc", Namespace: "default"}
Expand Down