overhaul proxy healthchecks

The existing healthcheck lib was pretty complicated and was hiding some
bugs (like the count always being 1),  This is a reboot of the interface
and implementation to be significantly simpler and better tested.

pkg/proxy/iptables/proxier_test.go

@@ -30,7 +30,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/service"
 	"k8s.io/kubernetes/pkg/proxy"
@@ -357,21 +356,25 @@ func (f *fakePortOpener) OpenLocalPort(lp *localPort) (closeable, error) {
 }
 
 type fakeHealthChecker struct {
-	endpoints map[types.NamespacedName]sets.String
+	services  map[types.NamespacedName]uint16
+	endpoints map[types.NamespacedName]int
 }
 
 func newFakeHealthChecker() *fakeHealthChecker {
 	return &fakeHealthChecker{
-		endpoints: map[types.NamespacedName]sets.String{},
+		services:  map[types.NamespacedName]uint16{},
+		endpoints: map[types.NamespacedName]int{},
 	}
 }
 
-func (fake *fakeHealthChecker) UpdateEndpoints(serviceName types.NamespacedName, endpointUIDs sets.String) {
-	if len(endpointUIDs) == 0 {
-		delete(fake.endpoints, serviceName)
-	} else {
-		fake.endpoints[serviceName] = endpointUIDs
-	}
+func (fake *fakeHealthChecker) SyncServices(newServices map[types.NamespacedName]uint16) error {
+	fake.services = newServices
+	return nil
+}
+
+func (fake *fakeHealthChecker) SyncEndpoints(newEndpoints map[types.NamespacedName]int) error {
+	fake.endpoints = newEndpoints
+	return nil
 }
 
 const testHostname = "test-hostname"
@@ -941,30 +944,18 @@ func TestBuildServiceMapAddRemove(t *testing.T) {
 		}),
 	}
 
-	serviceMap, hcAdd, hcDel, staleUDPServices := buildNewServiceMap(services, make(proxyServiceMap))
+	serviceMap, hcPorts, staleUDPServices := buildNewServiceMap(services, make(proxyServiceMap))
 	if len(serviceMap) != 8 {
 		t.Errorf("expected service map length 8, got %v", serviceMap)
 	}
 
 	// The only-local-loadbalancer ones get added
-	if len(hcAdd) != 2 {
-		t.Errorf("expected healthcheck add length 2, got %v", hcAdd)
+	if len(hcPorts) != 1 {
+		t.Errorf("expected 1 healthcheck port, got %v", hcPorts)
 	} else {
-		for _, hc := range hcAdd {
-			if hc.namespace.Namespace != "somewhere" || hc.namespace.Name != "only-local-load-balancer" {
-				t.Errorf("unexpected healthcheck listener added: %v", hc)
-			}
-		}
-	}
-
-	// All the rest get deleted
-	if len(hcDel) != 6 {
-		t.Errorf("expected healthcheck del length 6, got %v", hcDel)
-	} else {
-		for _, hc := range hcDel {
-			if hc.namespace.Namespace == "somewhere" && hc.namespace.Name == "only-local-load-balancer" {
-				t.Errorf("unexpected healthcheck listener deleted: %v", hc)
-			}
+		nsn := makeNSN("somewhere", "only-local-load-balancer")
+		if port, found := hcPorts[nsn]; !found || port != 345 {
+			t.Errorf("expected healthcheck port [%q]=345: got %v", nsn, hcPorts)
 		}
 	}
 
@@ -976,27 +967,13 @@ func TestBuildServiceMapAddRemove(t *testing.T) {
 	// Remove some stuff
 	services = []*api.Service{services[0]}
 	services[0].Spec.Ports = []api.ServicePort{services[0].Spec.Ports[1]}
-	serviceMap, hcAdd, hcDel, staleUDPServices = buildNewServiceMap(services, serviceMap)
+	serviceMap, hcPorts, staleUDPServices = buildNewServiceMap(services, serviceMap)
 	if len(serviceMap) != 1 {
 		t.Errorf("expected service map length 1, got %v", serviceMap)
 	}
 
-	if len(hcAdd) != 0 {
-		t.Errorf("expected healthcheck add length 1, got %v", hcAdd)
-	}
-
-	// The only OnlyLocal annotation was removed above, so we expect a delete now.
-	// FIXME: Since the BetaAnnotationHealthCheckNodePort is the same for all
-	// ServicePorts, we'll get one delete per ServicePort, even though they all
-	// contain the same information
-	if len(hcDel) != 2 {
-		t.Errorf("expected healthcheck del length 2, got %v", hcDel)
-	} else {
-		for _, hc := range hcDel {
-			if hc.namespace.Namespace != "somewhere" || hc.namespace.Name != "only-local-load-balancer" {
-				t.Errorf("unexpected healthcheck listener deleted: %v", hc)
-			}
-		}
+	if len(hcPorts) != 0 {
+		t.Errorf("expected healthcheck ports length 1, got %v", hcPorts)
 	}
 
 	// All services but one were deleted. While you'd expect only the ClusterIPs
@@ -1023,17 +1000,14 @@ func TestBuildServiceMapServiceHeadless(t *testing.T) {
 	}
 
 	// Headless service should be ignored
-	serviceMap, hcAdd, hcDel, staleUDPServices := buildNewServiceMap(services, make(proxyServiceMap))
+	serviceMap, hcPorts, staleUDPServices := buildNewServiceMap(services, make(proxyServiceMap))
 	if len(serviceMap) != 0 {
 		t.Errorf("expected service map length 0, got %d", len(serviceMap))
 	}
 
 	// No proxied services, so no healthchecks
-	if len(hcAdd) != 0 {
-		t.Errorf("expected healthcheck add length 0, got %d", len(hcAdd))
-	}
-	if len(hcDel) != 0 {
-		t.Errorf("expected healthcheck del length 0, got %d", len(hcDel))
+	if len(hcPorts) != 0 {
+		t.Errorf("expected healthcheck ports length 0, got %d", len(hcPorts))
 	}
 
 	if len(staleUDPServices) != 0 {
@@ -1051,16 +1025,13 @@ func TestBuildServiceMapServiceTypeExternalName(t *testing.T) {
 		}),
 	}
 
-	serviceMap, hcAdd, hcDel, staleUDPServices := buildNewServiceMap(services, make(proxyServiceMap))
+	serviceMap, hcPorts, staleUDPServices := buildNewServiceMap(services, make(proxyServiceMap))
 	if len(serviceMap) != 0 {
 		t.Errorf("expected service map length 0, got %v", serviceMap)
 	}
 	// No proxied services, so no healthchecks
-	if len(hcAdd) != 0 {
-		t.Errorf("expected healthcheck add length 0, got %v", hcAdd)
-	}
-	if len(hcDel) != 0 {
-		t.Errorf("expected healthcheck del length 0, got %v", hcDel)
+	if len(hcPorts) != 0 {
+		t.Errorf("expected healthcheck ports length 0, got %v", hcPorts)
 	}
 	if len(staleUDPServices) != 0 {
 		t.Errorf("expected stale UDP services length 0, got %v", staleUDPServices)
@@ -1096,15 +1067,12 @@ func TestBuildServiceMapServiceUpdate(t *testing.T) {
 		}),
 	}
 
-	serviceMap, hcAdd, hcDel, staleUDPServices := buildNewServiceMap(first, make(proxyServiceMap))
+	serviceMap, hcPorts, staleUDPServices := buildNewServiceMap(first, make(proxyServiceMap))
 	if len(serviceMap) != 2 {
 		t.Errorf("expected service map length 2, got %v", serviceMap)
 	}
-	if len(hcAdd) != 0 {
-		t.Errorf("expected healthcheck add length 0, got %v", hcAdd)
-	}
-	if len(hcDel) != 2 {
-		t.Errorf("expected healthcheck del length 2, got %v", hcDel)
+	if len(hcPorts) != 0 {
+		t.Errorf("expected healthcheck ports length 0, got %v", hcPorts)
 	}
 	if len(staleUDPServices) != 0 {
 		// Services only added, so nothing stale yet
@@ -1112,15 +1080,12 @@ func TestBuildServiceMapServiceUpdate(t *testing.T) {
 	}
 
 	// Change service to load-balancer
-	serviceMap, hcAdd, hcDel, staleUDPServices = buildNewServiceMap(second, serviceMap)
+	serviceMap, hcPorts, staleUDPServices = buildNewServiceMap(second, serviceMap)
 	if len(serviceMap) != 2 {
 		t.Errorf("expected service map length 2, got %v", serviceMap)
 	}
-	if len(hcAdd) != 2 {
-		t.Errorf("expected healthcheck add length 2, got %v", hcAdd)
-	}
-	if len(hcDel) != 0 {
-		t.Errorf("expected healthcheck add length 2, got %v", hcDel)
+	if len(hcPorts) != 1 {
+		t.Errorf("expected healthcheck ports length 1, got %v", hcPorts)
 	}
 	if len(staleUDPServices) != 0 {
 		t.Errorf("expected stale UDP services length 0, got %v", staleUDPServices.List())
@@ -1128,30 +1093,24 @@ func TestBuildServiceMapServiceUpdate(t *testing.T) {
 
 	// No change; make sure the service map stays the same and there are
 	// no health-check changes
-	serviceMap, hcAdd, hcDel, staleUDPServices = buildNewServiceMap(second, serviceMap)
+	serviceMap, hcPorts, staleUDPServices = buildNewServiceMap(second, serviceMap)
 	if len(serviceMap) != 2 {
 		t.Errorf("expected service map length 2, got %v", serviceMap)
 	}
-	if len(hcAdd) != 0 {
-		t.Errorf("expected healthcheck add length 0, got %v", hcAdd)
-	}
-	if len(hcDel) != 0 {
-		t.Errorf("expected healthcheck add length 2, got %v", hcDel)
+	if len(hcPorts) != 1 {
+		t.Errorf("expected healthcheck ports length 1, got %v", hcPorts)
 	}
 	if len(staleUDPServices) != 0 {
 		t.Errorf("expected stale UDP services length 0, got %v", staleUDPServices.List())
 	}
 
 	// And back to ClusterIP
-	serviceMap, hcAdd, hcDel, staleUDPServices = buildNewServiceMap(first, serviceMap)
+	serviceMap, hcPorts, staleUDPServices = buildNewServiceMap(first, serviceMap)
 	if len(serviceMap) != 2 {
 		t.Errorf("expected service map length 2, got %v", serviceMap)
 	}
-	if len(hcAdd) != 0 {
-		t.Errorf("expected healthcheck add length 0, got %v", hcAdd)
-	}
-	if len(hcDel) != 2 {
-		t.Errorf("expected healthcheck del length 2, got %v", hcDel)
+	if len(hcPorts) != 0 {
+		t.Errorf("expected healthcheck ports length 0, got %v", hcPorts)
 	}
 	if len(staleUDPServices) != 0 {
 		// Services only added, so nothing stale yet
@@ -1401,13 +1360,14 @@ func makeTestEndpoints(namespace, name string, eptFunc func(*api.Endpoints)) *ap
 	return ept
 }
 
+func makeNSN(namespace, name string) types.NamespacedName {
+	return types.NamespacedName{Namespace: namespace, Name: name}
+}
+
 func makeServicePortName(ns, name, port string) proxy.ServicePortName {
 	return proxy.ServicePortName{
-		NamespacedName: types.NamespacedName{
-			Namespace: ns,
-			Name:      name,
-		},
-		Port: port,
+		NamespacedName: makeNSN(ns, name),
+		Port:           port,
 	}
 }
 
@@ -1419,14 +1379,14 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 		oldEndpoints         map[proxy.ServicePortName][]*endpointsInfo
 		expectedResult       map[proxy.ServicePortName][]*endpointsInfo
 		expectedStale        []endpointServicePair
-		expectedHealthchecks map[types.NamespacedName]sets.String
+		expectedHealthchecks map[types.NamespacedName]int
 	}{{
 		// Case[0]: nothing
 		newEndpoints:         []*api.Endpoints{},
 		oldEndpoints:         map[proxy.ServicePortName][]*endpointsInfo{},
 		expectedResult:       map[proxy.ServicePortName][]*endpointsInfo{},
 		expectedStale:        []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[1]: no change, unnamed port
 		newEndpoints: []*api.Endpoints{
@@ -1452,7 +1412,7 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale:        []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[2]: no change, named port, local
 		newEndpoints: []*api.Endpoints{
@@ -1480,8 +1440,8 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale: []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{
-			types.NamespacedName{Namespace: "ns1", Name: "ep1"}: sets.NewString("ns1/ep1"),
+		expectedHealthchecks: map[types.NamespacedName]int{
+			makeNSN("ns1", "ep1"): 1,
 		},
 	}, {
 		// Case[3]: no change, multiple subsets
@@ -1523,7 +1483,7 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale:        []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[4]: no change, multiple subsets, multiple ports, local
 		newEndpoints: []*api.Endpoints{
@@ -1574,8 +1534,8 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale: []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{
-			types.NamespacedName{Namespace: "ns1", Name: "ep1"}: sets.NewString("ns1/ep1"),
+		expectedHealthchecks: map[types.NamespacedName]int{
+			makeNSN("ns1", "ep1"): 1,
 		},
 	}, {
 		// Case[5]: no change, multiple endpoints, subsets, IPs, and ports
@@ -1682,9 +1642,9 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale: []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{
-			types.NamespacedName{Namespace: "ns1", Name: "ep1"}: sets.NewString("ns1/ep1"),
-			types.NamespacedName{Namespace: "ns2", Name: "ep2"}: sets.NewString("ns2/ep2"),
+		expectedHealthchecks: map[types.NamespacedName]int{
+			makeNSN("ns1", "ep1"): 2,
+			makeNSN("ns2", "ep2"): 1,
 		},
 	}, {
 		// Case[6]: add an Endpoints
@@ -1708,8 +1668,8 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale: []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{
-			types.NamespacedName{Namespace: "ns1", Name: "ep1"}: sets.NewString("ns1/ep1"),
+		expectedHealthchecks: map[types.NamespacedName]int{
+			makeNSN("ns1", "ep1"): 1,
 		},
 	}, {
 		// Case[7]: remove an Endpoints
@@ -1724,7 +1684,7 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			endpoint:        "1.1.1.1:11",
 			servicePortName: makeServicePortName("ns1", "ep1", ""),
 		}},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[8]: add an IP and port
 		newEndpoints: []*api.Endpoints{
@@ -1762,8 +1722,8 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale: []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{
-			types.NamespacedName{Namespace: "ns1", Name: "ep1"}: sets.NewString("ns1/ep1"),
+		expectedHealthchecks: map[types.NamespacedName]int{
+			makeNSN("ns1", "ep1"): 1,
 		},
 	}, {
 		// Case[9]: remove an IP and port
@@ -1805,7 +1765,7 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			endpoint:        "1.1.1.2:12",
 			servicePortName: makeServicePortName("ns1", "ep1", "p12"),
 		}},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[10]: add a subset
 		newEndpoints: []*api.Endpoints{
@@ -1844,8 +1804,8 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			},
 		},
 		expectedStale: []endpointServicePair{},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{
-			types.NamespacedName{Namespace: "ns1", Name: "ep1"}: sets.NewString("ns1/ep1"),
+		expectedHealthchecks: map[types.NamespacedName]int{
+			makeNSN("ns1", "ep1"): 1,
 		},
 	}, {
 		// Case[11]: remove a subset
@@ -1879,7 +1839,7 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			endpoint:        "2.2.2.2:22",
 			servicePortName: makeServicePortName("ns1", "ep1", "p22"),
 		}},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[12]: rename a port
 		newEndpoints: []*api.Endpoints{
@@ -1909,7 +1869,7 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			endpoint:        "1.1.1.1:11",
 			servicePortName: makeServicePortName("ns1", "ep1", "p11"),
 		}},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[13]: renumber a port
 		newEndpoints: []*api.Endpoints{
@@ -1939,7 +1899,7 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			endpoint:        "1.1.1.1:11",
 			servicePortName: makeServicePortName("ns1", "ep1", "p11"),
 		}},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{},
+		expectedHealthchecks: map[types.NamespacedName]int{},
 	}, {
 		// Case[14]: complex add and remove
 		newEndpoints: []*api.Endpoints{
@@ -2044,8 +2004,8 @@ func Test_buildNewEndpointsMap(t *testing.T) {
 			endpoint:        "4.4.4.6:45",
 			servicePortName: makeServicePortName("ns4", "ep4", "p45"),
 		}},
-		expectedHealthchecks: map[types.NamespacedName]sets.String{
-			types.NamespacedName{Namespace: "ns4", Name: "ep4"}: sets.NewString("ns4/ep4"),
+		expectedHealthchecks: map[types.NamespacedName]int{
+			makeNSN("ns4", "ep4"): 1,
 		},
 	}}