From 40ab479f06d061106581173b8543a49d086fb710 Mon Sep 17 00:00:00 2001 From: yankaiz Date: Mon, 22 Oct 2018 14:21:45 -0700 Subject: [PATCH] Change GCE LB health check interval from 2s to 8s, unhealthyThreashold to 3 Force ELB to ensureHealthCheck when target pool exists. Add e2e test to ensure that HC interval will be reconciled when kube-controller-manager restarts. Health checks with bigger thresholds and larger intervals will not be reconciled. Add unittest for ILB and ELB to ensure HC reconciles and is configurable. --- pkg/cloudprovider/providers/gce/gce.go | 8 +- .../gce/gce_loadbalancer_external.go | 49 +++++- .../gce/gce_loadbalancer_external_test.go | 151 ++++++++++++++++++ .../gce/gce_loadbalancer_internal.go | 61 ++++--- .../gce/gce_loadbalancer_internal_test.go | 112 ++++++++++++- test/e2e/network/service.go | 73 +++++++++ 6 files changed, 427 insertions(+), 27 deletions(-) diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index 794bf85efd3..a514dc07288 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -80,13 +80,13 @@ const ( maxTargetPoolCreateInstances = 200 // HTTP Load Balancer parameters - // Configure 2 second period for external health checks. - gceHcCheckIntervalSeconds = int64(2) + // Configure 8 second period for external health checks. + gceHcCheckIntervalSeconds = int64(8) gceHcTimeoutSeconds = int64(1) // Start sending requests as soon as a pod is found on the node. gceHcHealthyThreshold = int64(1) - // Defaults to 5 * 2 = 10 seconds before the LB will steer traffic away - gceHcUnhealthyThreshold = int64(5) + // Defaults to 3 * 8 = 24 seconds before the LB will steer traffic away. + gceHcUnhealthyThreshold = int64(3) gceComputeAPIEndpoint = "https://www.googleapis.com/compute/v1/" gceComputeAPIEndpointBeta = "https://www.googleapis.com/compute/beta/" diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go index 9687393b453..4efea8ab974 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external.go @@ -505,6 +505,14 @@ func (gce *GCECloud) ensureTargetPoolAndHealthCheck(tpExists, tpNeedsRecreation return fmt.Errorf("failed to update target pool for load balancer (%s): %v", lbRefStr, err) } glog.Infof("ensureTargetPoolAndHealthCheck(%s): Updated target pool (with %d hosts).", lbRefStr, len(hosts)) + if hcToCreate != nil { + if hc, err := gce.ensureHttpHealthCheck(hcToCreate.Name, hcToCreate.RequestPath, int32(hcToCreate.Port)); err != nil || hc == nil { + return fmt.Errorf("Failed to ensure health check for %v port %d path %v: %v", loadBalancerName, hcToCreate.Port, hcToCreate.RequestPath, err) + } + } + } else { + // Panic worthy. + glog.Errorf("ensureTargetPoolAndHealthCheck(%s): target pool not exists and doesn't need to be created.", lbRefStr) } return nil } @@ -621,6 +629,37 @@ func makeHttpHealthCheck(name, path string, port int32) *compute.HttpHealthCheck } } +// mergeHttpHealthChecks reconciles HttpHealthCheck configures to be no smaller +// than the default values. +// E.g. old health check interval is 2s, new default is 8. +// The HC interval will be reconciled to 8 seconds. +// If the existing health check is larger than the default interval, +// the configuration will be kept. +func mergeHttpHealthChecks(hc, newHC *compute.HttpHealthCheck) *compute.HttpHealthCheck { + if hc.CheckIntervalSec > newHC.CheckIntervalSec { + newHC.CheckIntervalSec = hc.CheckIntervalSec + } + if hc.TimeoutSec > newHC.TimeoutSec { + newHC.TimeoutSec = hc.TimeoutSec + } + if hc.UnhealthyThreshold > newHC.UnhealthyThreshold { + newHC.UnhealthyThreshold = hc.UnhealthyThreshold + } + if hc.HealthyThreshold > newHC.HealthyThreshold { + newHC.HealthyThreshold = hc.HealthyThreshold + } + return newHC +} + +// needToUpdateHttpHealthChecks checks whether the http healthcheck needs to be +// updated. +func needToUpdateHttpHealthChecks(hc, newHC *compute.HttpHealthCheck) bool { + changed := hc.Port != newHC.Port || hc.RequestPath != newHC.RequestPath || hc.Description != newHC.Description + changed = changed || hc.CheckIntervalSec < newHC.CheckIntervalSec || hc.TimeoutSec < newHC.TimeoutSec + changed = changed || hc.UnhealthyThreshold < newHC.UnhealthyThreshold || hc.HealthyThreshold < newHC.HealthyThreshold + return changed +} + func (gce *GCECloud) ensureHttpHealthCheck(name, path string, port int32) (hc *compute.HttpHealthCheck, err error) { newHC := makeHttpHealthCheck(name, path, port) hc, err = gce.GetHttpHealthCheck(name) @@ -639,16 +678,18 @@ func (gce *GCECloud) ensureHttpHealthCheck(name, path string, port int32) (hc *c } // Validate health check fields glog.V(4).Infof("Checking http health check params %s", name) - drift := hc.Port != int64(port) || hc.RequestPath != path || hc.Description != makeHealthCheckDescription(name) - drift = drift || hc.CheckIntervalSec != gceHcCheckIntervalSeconds || hc.TimeoutSec != gceHcTimeoutSeconds - drift = drift || hc.UnhealthyThreshold != gceHcUnhealthyThreshold || hc.HealthyThreshold != gceHcHealthyThreshold - if drift { + if needToUpdateHttpHealthChecks(hc, newHC) { glog.Warningf("Health check %v exists but parameters have drifted - updating...", name) + newHC = mergeHttpHealthChecks(hc, newHC) if err := gce.UpdateHttpHealthCheck(newHC); err != nil { glog.Warningf("Failed to reconcile http health check %v parameters", name) return nil, err } glog.V(4).Infof("Corrected health check %v parameters successful", name) + hc, err = gce.GetHttpHealthCheck(name) + if err != nil { + return nil, err + } } return hc, nil } diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go index 18312857144..60838277fb1 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_external_test.go @@ -1032,3 +1032,154 @@ func TestEnsureExternalLoadBalancerErrors(t *testing.T) { }) } } + +func TestExternalLoadBalancerEnsureHttpHealthCheck(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + desc string + modifier func(*compute.HttpHealthCheck) *compute.HttpHealthCheck + wantEqual bool + }{ + {"should ensure HC", func(_ *compute.HttpHealthCheck) *compute.HttpHealthCheck { return nil }, false}, + { + "should reconcile HC interval", + func(hc *compute.HttpHealthCheck) *compute.HttpHealthCheck { + hc.CheckIntervalSec = gceHcCheckIntervalSeconds - 1 + return hc + }, + false, + }, + { + "should allow HC to be configurable to bigger intervals", + func(hc *compute.HttpHealthCheck) *compute.HttpHealthCheck { + hc.CheckIntervalSec = gceHcCheckIntervalSeconds * 10 + return hc + }, + true, + }, + { + "should allow HC to accept bigger intervals while applying default value to small thresholds", + func(hc *compute.HttpHealthCheck) *compute.HttpHealthCheck { + hc.CheckIntervalSec = gceHcCheckIntervalSeconds * 10 + hc.UnhealthyThreshold = gceHcUnhealthyThreshold - 1 + return hc + }, + false, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + + gce, err := fakeGCECloud(DefaultTestClusterValues()) + require.NoError(t, err) + c := gce.c.(*cloud.MockGCE) + c.MockHttpHealthChecks.UpdateHook = func(ctx context.Context, key *meta.Key, obj *ga.HttpHealthCheck, m *cloud.MockHttpHealthChecks) error { + m.Objects[*key] = &cloud.MockHttpHealthChecksObj{Obj: obj} + return nil + } + + hcName, hcPath, hcPort := "test-hc", "/healthz", int32(12345) + existingHC := makeHttpHealthCheck(hcName, hcPath, hcPort) + existingHC = tc.modifier(existingHC) + if existingHC != nil { + if err := gce.CreateHttpHealthCheck(existingHC); err != nil { + t.Fatalf("gce.CreateHttpHealthCheck(%#v) = %v; want err = nil", existingHC, err) + } + } + if _, err := gce.ensureHttpHealthCheck(hcName, hcPath, hcPort); err != nil { + t.Fatalf("gce.ensureHttpHealthCheck(%q, %q, %v) = _, %d; want err = nil", hcName, hcPath, hcPort, err) + } + if hc, err := gce.GetHttpHealthCheck(hcName); err != nil { + t.Fatalf("gce.GetHttpHealthCheck(%q) = _, %d; want err = nil", hcName, err) + } else { + if tc.wantEqual { + assert.Equal(t, hc, existingHC) + } else { + assert.NotEqual(t, hc, existingHC) + } + } + }) + } + +} + +func TestMergeHttpHealthChecks(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + desc string + checkIntervalSec int64 + timeoutSec int64 + healthyThreshold int64 + unhealthyThreshold int64 + wantCheckIntervalSec int64 + wantTimeoutSec int64 + 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}, + } { + t.Run(tc.desc, func(t *testing.T) { + wantHC := makeHttpHealthCheck("hc", "/", 12345) + hc := &compute.HttpHealthCheck{ + CheckIntervalSec: tc.checkIntervalSec, + TimeoutSec: tc.timeoutSec, + HealthyThreshold: tc.healthyThreshold, + UnhealthyThreshold: tc.unhealthyThreshold, + } + mergeHttpHealthChecks(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) + } + }) + } +} + +func TestNeedToUpdateHttpHealthChecks(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + desc string + modifier func(*compute.HttpHealthCheck) + wantChanged bool + }{ + {"unchanged", nil, false}, + {"desc does not match", func(hc *compute.HttpHealthCheck) { hc.Description = "bad-desc" }, true}, + {"port does not match", func(hc *compute.HttpHealthCheck) { hc.Port = 54321 }, true}, + {"requestPath does not match", func(hc *compute.HttpHealthCheck) { hc.RequestPath = "/anotherone" }, true}, + {"interval needs update", func(hc *compute.HttpHealthCheck) { hc.CheckIntervalSec = gceHcCheckIntervalSeconds - 1 }, true}, + {"timeout needs update", func(hc *compute.HttpHealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds - 1 }, true}, + {"healthy threshold needs update", func(hc *compute.HttpHealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold - 1 }, true}, + {"unhealthy threshold needs update", func(hc *compute.HttpHealthCheck) { hc.UnhealthyThreshold = gceHcUnhealthyThreshold - 1 }, true}, + {"interval does not need update", func(hc *compute.HttpHealthCheck) { hc.CheckIntervalSec = gceHcCheckIntervalSeconds + 1 }, false}, + {"timeout does not need update", func(hc *compute.HttpHealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds + 1 }, false}, + {"healthy threshold does not need update", func(hc *compute.HttpHealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold + 1 }, false}, + {"unhealthy threshold does not need update", func(hc *compute.HttpHealthCheck) { hc.UnhealthyThreshold = gceHcUnhealthyThreshold + 1 }, false}, + } { + t.Run(tc.desc, func(t *testing.T) { + hc := makeHttpHealthCheck("hc", "/", 12345) + wantHC := makeHttpHealthCheck("hc", "/", 12345) + if tc.modifier != nil { + tc.modifier(hc) + } + if gotChanged := needToUpdateHttpHealthChecks(hc, wantHC); gotChanged != tc.wantChanged { + t.Errorf("needToUpdateHttpHealthChecks(%#v, %#v) = %t; want changed = %t", hc, wantHC, gotChanged, tc.wantChanged) + } + }) + } +} diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go index 4542fa1232b..3bc83710b86 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal.go @@ -405,16 +405,19 @@ func (gce *GCECloud) ensureInternalHealthCheck(name string, svcName types.Namesp return hc, nil } - if healthChecksEqual(expectedHC, hc) { - return hc, nil + if needToUpdateHealthChecks(hc, expectedHC) { + glog.V(2).Infof("ensureInternalHealthCheck: health check %v exists but parameters have drifted - updating...", name) + expectedHC = mergeHealthChecks(hc, expectedHC) + if err := gce.UpdateHealthCheck(expectedHC); err != nil { + glog.Warningf("Failed to reconcile http health check %v parameters", name) + return nil, err + } + glog.V(2).Infof("ensureInternalHealthCheck: corrected health check %v parameters successful", name) + hc, err = gce.GetHealthCheck(name) + if err != nil { + return nil, err + } } - - glog.V(2).Infof("ensureInternalHealthCheck: health check %v exists but parameters have drifted - updating...", name) - if err := gce.UpdateHealthCheck(expectedHC); err != nil { - glog.Warningf("Failed to reconcile http health check %v parameters", name) - return nil, err - } - glog.V(2).Infof("ensureInternalHealthCheck: corrected health check %v parameters successful", name) return hc, nil } @@ -620,15 +623,37 @@ func firewallRuleEqual(a, b *compute.Firewall) bool { equalStringSets(a.TargetTags, b.TargetTags) } -func healthChecksEqual(a, b *compute.HealthCheck) bool { - return a.HttpHealthCheck != nil && b.HttpHealthCheck != nil && - a.HttpHealthCheck.Port == b.HttpHealthCheck.Port && - a.HttpHealthCheck.RequestPath == b.HttpHealthCheck.RequestPath && - a.Description == b.Description && - a.CheckIntervalSec == b.CheckIntervalSec && - a.TimeoutSec == b.TimeoutSec && - a.UnhealthyThreshold == b.UnhealthyThreshold && - a.HealthyThreshold == b.HealthyThreshold +// mergeHealthChecks reconciles HealthCheck configures to be no smaller than +// the default values. +// E.g. old health check interval is 2s, new default is 8. +// The HC interval will be reconciled to 8 seconds. +// If the existing health check is larger than the default interval, +// the configuration will be kept. +func mergeHealthChecks(hc, newHC *compute.HealthCheck) *compute.HealthCheck { + if hc.CheckIntervalSec > newHC.CheckIntervalSec { + newHC.CheckIntervalSec = hc.CheckIntervalSec + } + if hc.TimeoutSec > newHC.TimeoutSec { + newHC.TimeoutSec = hc.TimeoutSec + } + if hc.UnhealthyThreshold > newHC.UnhealthyThreshold { + newHC.UnhealthyThreshold = hc.UnhealthyThreshold + } + if hc.HealthyThreshold > newHC.HealthyThreshold { + newHC.HealthyThreshold = hc.HealthyThreshold + } + return newHC +} + +// needToUpdateHealthChecks checks whether the healthcheck needs to be updated. +func needToUpdateHealthChecks(hc, newHC *compute.HealthCheck) bool { + if hc.HttpHealthCheck == nil || newHC.HttpHealthCheck == nil { + return true + } + changed := hc.HttpHealthCheck.Port != newHC.HttpHealthCheck.Port || hc.HttpHealthCheck.RequestPath != newHC.HttpHealthCheck.RequestPath || hc.Description != newHC.Description + changed = changed || hc.CheckIntervalSec < newHC.CheckIntervalSec || hc.TimeoutSec < newHC.TimeoutSec + changed = changed || hc.UnhealthyThreshold < newHC.UnhealthyThreshold || hc.HealthyThreshold < newHC.HealthyThreshold + return changed } // backendsListEqual asserts that backend lists are equal by instance group link only diff --git a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go index 32808409102..e3910828de8 100644 --- a/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go +++ b/pkg/cloudprovider/providers/gce/gce_loadbalancer_internal_test.go @@ -231,7 +231,7 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { // Create a healthcheck with an incorrect threshold existingHC := newInternalLBHealthCheck(hcName, nm, sharedHealthCheck, hcPath, hcPort) - existingHC.HealthyThreshold = gceHcHealthyThreshold * 10 + existingHC.CheckIntervalSec = gceHcCheckIntervalSeconds - 1 gce.CreateHealthCheck(existingHC) // Create a backend Service that's missing Description and Backends @@ -268,6 +268,34 @@ func TestEnsureInternalLoadBalancerClearPreviousResources(t *testing.T) { assert.NotEqual(t, bs, existingBS) } +func TestEnsureInternalLoadBalancerHealthCheckConfigurable(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + + svc := fakeLoadbalancerService(string(LBTypeInternal)) + lbName := gce.GetLoadBalancerName(context.TODO(), "", svc) + + sharedHealthCheck := !v1_service.RequestsOnlyLocalTraffic(svc) + hcName := makeHealthCheckName(lbName, vals.ClusterID, sharedHealthCheck) + hcPath, hcPort := GetNodesHealthCheckPath(), GetNodesHealthCheckPort() + nm := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace} + + // Create a healthcheck with an incorrect threshold + existingHC := newInternalLBHealthCheck(hcName, nm, sharedHealthCheck, hcPath, hcPort) + existingHC.CheckIntervalSec = gceHcCheckIntervalSeconds * 10 + gce.CreateHealthCheck(existingHC) + + _, err = createInternalLoadBalancer(gce, svc, nil, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) + assert.NoError(t, err) + + healthcheck, err := gce.GetHealthCheck(hcName) + require.NoError(t, err) + assert.Equal(t, healthcheck, existingHC) +} + func TestUpdateInternalLoadBalancerBackendServices(t *testing.T) { t.Parallel() @@ -737,3 +765,85 @@ func TestEnsureInternalLoadBalancerErrors(t *testing.T) { }) } } + +func TestMergeHealthChecks(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + desc string + checkIntervalSec int64 + timeoutSec int64 + healthyThreshold int64 + unhealthyThreshold int64 + wantCheckIntervalSec int64 + wantTimeoutSec int64 + 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}, + } { + t.Run(tc.desc, func(t *testing.T) { + wantHC := newInternalLBHealthCheck("hc", types.NamespacedName{Name: "svc", Namespace: "default"}, false, "/", 12345) + hc := &compute.HealthCheck{ + CheckIntervalSec: tc.checkIntervalSec, + TimeoutSec: tc.timeoutSec, + HealthyThreshold: tc.healthyThreshold, + 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) + } + }) + } +} + +func TestCompareHealthChecks(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + desc string + modifier func(*compute.HealthCheck) + wantChanged bool + }{ + {"unchanged", nil, false}, + {"nil HttpHealthCheck", func(hc *compute.HealthCheck) { hc.HttpHealthCheck = nil }, true}, + {"desc does not match", func(hc *compute.HealthCheck) { hc.Description = "bad-desc" }, true}, + {"port does not match", func(hc *compute.HealthCheck) { hc.HttpHealthCheck.Port = 54321 }, true}, + {"requestPath does not match", func(hc *compute.HealthCheck) { hc.HttpHealthCheck.RequestPath = "/anotherone" }, true}, + {"interval needs update", func(hc *compute.HealthCheck) { hc.CheckIntervalSec = gceHcCheckIntervalSeconds - 1 }, true}, + {"timeout needs update", func(hc *compute.HealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds - 1 }, true}, + {"healthy threshold needs update", func(hc *compute.HealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold - 1 }, true}, + {"unhealthy threshold needs update", func(hc *compute.HealthCheck) { hc.UnhealthyThreshold = gceHcUnhealthyThreshold - 1 }, true}, + {"interval does not need update", func(hc *compute.HealthCheck) { hc.CheckIntervalSec = gceHcCheckIntervalSeconds + 1 }, false}, + {"timeout does not need update", func(hc *compute.HealthCheck) { hc.TimeoutSec = gceHcTimeoutSeconds + 1 }, false}, + {"healthy threshold does not need update", func(hc *compute.HealthCheck) { hc.HealthyThreshold = gceHcHealthyThreshold + 1 }, false}, + {"unhealthy threshold does not need update", func(hc *compute.HealthCheck) { hc.UnhealthyThreshold = gceHcUnhealthyThreshold + 1 }, false}, + } { + t.Run(tc.desc, func(t *testing.T) { + hc := newInternalLBHealthCheck("hc", types.NamespacedName{Name: "svc", Namespace: "default"}, false, "/", 12345) + wantHC := newInternalLBHealthCheck("hc", types.NamespacedName{Name: "svc", Namespace: "default"}, false, "/", 12345) + 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) + } + }) + } +} diff --git a/test/e2e/network/service.go b/test/e2e/network/service.go index 6e92643f91f..7fd188a364d 100644 --- a/test/e2e/network/service.go +++ b/test/e2e/network/service.go @@ -35,6 +35,7 @@ import ( clientset "k8s.io/client-go/kubernetes" cloudprovider "k8s.io/cloud-provider" "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" + gcecloud "k8s.io/kubernetes/pkg/cloudprovider/providers/gce" "k8s.io/kubernetes/pkg/controller/endpoint" "k8s.io/kubernetes/test/e2e/framework" "k8s.io/kubernetes/test/e2e/framework/providers/gce" @@ -1546,6 +1547,78 @@ var _ = SIGDescribe("Services", func() { jig.ChangeServiceType(svc.Namespace, svc.Name, v1.ServiceTypeClusterIP, createTimeout) }) + // This test creates a load balancer, make sure its health check interval + // equals to gceHcCheckIntervalSeconds. Then the interval is manipulated + // to be something else, see if the interval will be reconciled. + It("should reconcile LB health check interval [Slow][Serial]", func() { + const gceHcCheckIntervalSeconds = int64(8) + // This test is for clusters on GCE/GKE. + framework.SkipUnlessProviderIs("gce", "gke") + clusterID, err := gce.GetClusterID(cs) + if err != nil { + framework.Failf("framework.GetClusterID(cs) = _, %v; want nil", err) + } + gceCloud, err := gce.GetGCECloud() + if err != nil { + framework.Failf("framework.GetGCECloud() = _, %v; want nil", err) + } + + namespace := f.Namespace.Name + serviceName := "lb-hc-int" + jig := framework.NewServiceTestJig(cs, serviceName) + + By("create load balancer service") + // Create loadbalancer service with source range from node[0] and podAccept + svc := jig.CreateTCPServiceOrFail(namespace, func(svc *v1.Service) { + svc.Spec.Type = v1.ServiceTypeLoadBalancer + }) + + // Clean up loadbalancer service + defer func() { + jig.UpdateServiceOrFail(svc.Namespace, svc.Name, func(svc *v1.Service) { + svc.Spec.Type = v1.ServiceTypeNodePort + }) + Expect(cs.CoreV1().Services(svc.Namespace).Delete(svc.Name, nil)).NotTo(HaveOccurred()) + }() + + svc = jig.WaitForLoadBalancerOrFail(namespace, serviceName, framework.LoadBalancerCreateTimeoutDefault) + + hcName := gcecloud.MakeNodesHealthCheckName(clusterID) + hc, err := gceCloud.GetHttpHealthCheck(hcName) + if err != nil { + framework.Failf("gceCloud.GetHttpHealthCheck(%q) = _, %v; want nil", hcName, err) + } + Expect(hc.CheckIntervalSec).To(Equal(gceHcCheckIntervalSeconds)) + + By("modify the health check interval") + hc.CheckIntervalSec = gceHcCheckIntervalSeconds - 1 + if err = gceCloud.UpdateHttpHealthCheck(hc); err != nil { + framework.Failf("gcecloud.UpdateHttpHealthCheck(%#v) = %v; want nil", hc, err) + } + + By("restart kube-controller-manager") + if err := framework.RestartControllerManager(); err != nil { + framework.Failf("framework.RestartControllerManager() = %v; want nil", err) + } + if err := framework.WaitForControllerManagerUp(); err != nil { + framework.Failf("framework.WaitForControllerManagerUp() = %v; want nil", err) + } + + By("health check should be reconciled") + pollInterval := framework.Poll * 10 + if pollErr := wait.PollImmediate(pollInterval, framework.LoadBalancerCreateTimeoutDefault, func() (bool, error) { + hc, err := gceCloud.GetHttpHealthCheck(hcName) + if err != nil { + framework.Logf("Failed to get HttpHealthCheck(%q): %v", hcName, err) + return false, err + } + framework.Logf("hc.CheckIntervalSec = %v", hc.CheckIntervalSec) + return hc.CheckIntervalSec == gceHcCheckIntervalSeconds, nil + }); pollErr != nil { + framework.Failf("Health check %q does not reconcile its check interval to %d.", hcName, gceHcCheckIntervalSeconds) + } + }) + It("should have session affinity work for service with type clusterIP", func() { svc := getServeHostnameService("service") svc.Spec.Type = v1.ServiceTypeClusterIP