Fix deleteEndpointSlice when there are multiple EndpointSlices (linke…

…rd#10370) When processing a `delete` event for an EndpointSlice, regardless of the outcome (whether there are still addresses/endpoints alive or whether we have no endpoints left) we make a call to `noEndpoints` on the subscriber. This will send a `Destination.Get` update to the listeners to advertise a `NoEndpoints` event. If there are still addresses left, NoEndpoints { exists: true } will be sent (to signal the service still exists). Until an update is registered (i.e a new add) there will be no available endpoints -- this is incorrect, since other EndpointSlices may exist. This change fixes the problem by handling `noEndpoints` in a more specialized way for EndpointSlices. Signed-off-by: Yannick Utard <[email protected]>
jandersen-plaid · Jul 28, 2023 · 4586ff5 · 4586ff5
1 parent 4e91cd6
commit 4586ff5
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 26 deletions.
diff --git a/controller/api/destination/watcher/endpoints_watcher.go b/controller/api/destination/watcher/endpoints_watcher.go
@@ -973,8 +973,14 @@ func (pp *portPublisher) deleteEndpointSlice(es *discovery.EndpointSlice) {
 		listener.Remove(addrSet)
 	}
 
-	svcExists := len(pp.addresses.Addresses) > 0
-	pp.noEndpoints(svcExists)
+	if len(pp.addresses.Addresses) == 0 {
+		pp.noEndpoints(false)
+	} else {
+		pp.exists = true
+		pp.metrics.incUpdates()
+		pp.metrics.setPods(len(pp.addresses.Addresses))
+		pp.metrics.setExists(true)
+	}
 }
 
 func (pp *portPublisher) noEndpoints(exists bool) {

diff --git a/controller/api/destination/watcher/endpoints_watcher_test.go b/controller/api/destination/watcher/endpoints_watcher_test.go
@@ -1485,33 +1485,77 @@ status:
   phase: Running
   podIP: 172.17.0.12`}
 
+	k8sConfigWithMultipleES := append(k8sConfigsWithES, []string{`
+addressType: IPv4
+apiVersion: discovery.k8s.io/v1
+endpoints:
+- addresses:
+  - 172.17.0.13
+  conditions:
+    ready: true
+  targetRef:
+    kind: Pod
+    name: name1-2
+    namespace: ns
+  topology:
+    kubernetes.io/hostname: node-1
+kind: EndpointSlice
+metadata:
+  labels:
+    kubernetes.io/service-name: name1
+  name: name1-live
+  namespace: ns
+ports:
+- name: ""
+  port: 8989`, `apiVersion: v1
+kind: Pod
+metadata:
+  name: name1-2
+  namespace: ns
+status:
+  phase: Running
+  podIP: 172.17.0.13`}...)
+
 	for _, tt := range []struct {
-		serviceType      string
-		k8sConfigs       []string
-		id               ServiceID
-		hostname         string
-		port             Port
-		objectToDelete   interface{}
-		deletingServices bool
-		hasSliceAccess   bool
+		serviceType       string
+		k8sConfigs        []string
+		id                ServiceID
+		hostname          string
+		port              Port
+		objectToDelete    interface{}
+		deletingServices  bool
+		hasSliceAccess    bool
+		noEndpointsCalled bool
 	}{
 		{
-			serviceType:    "can delete EndpointSlices",
-			k8sConfigs:     k8sConfigsWithES,
-			id:             ServiceID{Name: "name1", Namespace: "ns"},
-			port:           8989,
-			hostname:       "name1-1",
-			objectToDelete: createTestEndpointSlice(),
-			hasSliceAccess: true,
+			serviceType:       "can delete an EndpointSlice",
+			k8sConfigs:        k8sConfigsWithES,
+			id:                ServiceID{Name: "name1", Namespace: "ns"},
+			port:              8989,
+			hostname:          "name1-1",
+			objectToDelete:    createTestEndpointSlice(),
+			hasSliceAccess:    true,
+			noEndpointsCalled: true,
 		},
 		{
-			serviceType:    "can delete EndpointSlices when wrapped in a DeletedFinalStateUnknown",
-			k8sConfigs:     k8sConfigsWithES,
-			id:             ServiceID{Name: "name1", Namespace: "ns"},
-			port:           8989,
-			hostname:       "name1-1",
-			objectToDelete: createTestEndpointSlice(),
-			hasSliceAccess: true,
+			serviceType:       "can delete an EndpointSlice when wrapped in a DeletedFinalStateUnknown",
+			k8sConfigs:        k8sConfigsWithES,
+			id:                ServiceID{Name: "name1", Namespace: "ns"},
+			port:              8989,
+			hostname:          "name1-1",
+			objectToDelete:    createTestEndpointSlice(),
+			hasSliceAccess:    true,
+			noEndpointsCalled: true,
+		},
+		{
+			serviceType:       "can delete an EndpointSlice when there are multiple ones",
+			k8sConfigs:        k8sConfigWithMultipleES,
+			id:                ServiceID{Name: "name1", Namespace: "ns"},
+			port:              8989,
+			hostname:          "name1-1",
+			objectToDelete:    createTestEndpointSlice(),
+			hasSliceAccess:    true,
+			noEndpointsCalled: false,
 		},
 	} {
 		tt := tt // pin
@@ -1534,8 +1578,9 @@ status:
 
 			watcher.deleteEndpointSlice(tt.objectToDelete)
 
-			if !listener.endpointsAreNotCalled() {
-				t.Fatal("Expected NoEndpoints to be Called")
+			if listener.endpointsAreNotCalled() != tt.noEndpointsCalled {
+				t.Fatalf("Expected noEndpointsCalled to be [%t], got [%t]",
+					tt.noEndpointsCalled, listener.endpointsAreNotCalled())
 			}
 		})
 	}