Merge pull request #70099 from grayluck/five-sec-hc

Change GCE LB health check interval from 2s to 8s

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.
+	// Configure 8 second period for external health checks.
-	gceHcCheckIntervalSeconds = int64(2)
+	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
+	// Defaults to 3 * 8 = 24 seconds before the LB will steer traffic away.
-	gceHcUnhealthyThreshold = int64(5)
+	gceHcUnhealthyThreshold = int64(3)
 
 	gceComputeAPIEndpoint     = "https://www.googleapis.com/compute/v1/"
 	gceComputeAPIEndpointBeta = "https://www.googleapis.com/compute/beta/"

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)
+	if needToUpdateHttpHealthChecks(hc, newHC) {
-	drift = drift || hc.CheckIntervalSec != gceHcCheckIntervalSeconds || hc.TimeoutSec != gceHcTimeoutSeconds
-	drift = drift || hc.UnhealthyThreshold != gceHcUnhealthyThreshold || hc.HealthyThreshold != gceHcHealthyThreshold
-	if drift {
 		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
 }

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)
+			}
+		})
+	}
+}

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) {
+	if needToUpdateHealthChecks(hc, expectedHC) {
-		return hc, nil
-	}
-
 		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
+		}
+	}
 	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 {
+// mergeHealthChecks reconciles HealthCheck configures to be no smaller than
-	return a.HttpHealthCheck != nil && b.HttpHealthCheck != nil &&
+// the default values.
-		a.HttpHealthCheck.Port == b.HttpHealthCheck.Port &&
+// E.g. old health check interval is 2s, new default is 8.
-		a.HttpHealthCheck.RequestPath == b.HttpHealthCheck.RequestPath &&
+// The HC interval will be reconciled to 8 seconds.
-		a.Description == b.Description &&
+// If the existing health check is larger than the default interval,
-		a.CheckIntervalSec == b.CheckIntervalSec &&
+// the configuration will be kept.
-		a.TimeoutSec == b.TimeoutSec &&
+func mergeHealthChecks(hc, newHC *compute.HealthCheck) *compute.HealthCheck {
-		a.UnhealthyThreshold == b.UnhealthyThreshold &&
+	if hc.CheckIntervalSec > newHC.CheckIntervalSec {
-		a.HealthyThreshold == b.HealthyThreshold
+		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

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)
+			}
+		})
+	}
+}

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