EndpointSlice and Endpoints should treat terminating pods the same

Signed-off-by: Andrew Sy Kim <[email protected]>

Author: andrewsykim

pkg/controller/endpointslice/reconciler.go

@@ -86,30 +86,32 @@ func (r *reconciler) reconcile(service *corev1.Service, pods []*corev1.Pod, exis
 	numDesiredEndpoints := 0
 
 	for _, pod := range pods {
-		if endpointutil.ShouldPodBeInEndpoints(pod) {
-			endpointPorts := getEndpointPorts(service, pod)
-			epHash := endpointutil.NewPortMapKey(endpointPorts)
-			if _, ok := desiredEndpointsByPortMap[epHash]; !ok {
-				desiredEndpointsByPortMap[epHash] = endpointSet{}
-			}
+		if !endpointutil.ShouldPodBeInEndpoints(pod, service.Spec.PublishNotReadyAddresses) {
+			continue
+		}
 
-			if _, ok := desiredMetaByPortMap[epHash]; !ok {
-				desiredMetaByPortMap[epHash] = &endpointMeta{
-					AddressType: addressType,
-					Ports:       endpointPorts,
-				}
-			}
+		endpointPorts := getEndpointPorts(service, pod)
+		epHash := endpointutil.NewPortMapKey(endpointPorts)
+		if _, ok := desiredEndpointsByPortMap[epHash]; !ok {
+			desiredEndpointsByPortMap[epHash] = endpointSet{}
+		}
 
-			node, err := r.nodeLister.Get(pod.Spec.NodeName)
-			if err != nil {
-				return err
-			}
-			endpoint := podToEndpoint(pod, node, service)
-			if len(endpoint.Addresses) > 0 {
-				desiredEndpointsByPortMap[epHash].Insert(&endpoint)
-				numDesiredEndpoints++
+		if _, ok := desiredMetaByPortMap[epHash]; !ok {
+			desiredMetaByPortMap[epHash] = &endpointMeta{
+				AddressType: addressType,
+				Ports:       endpointPorts,
 			}
 		}
+
+		node, err := r.nodeLister.Get(pod.Spec.NodeName)
+		if err != nil {
+			return err
+		}
+		endpoint := podToEndpoint(pod, node, service)
+		if len(endpoint.Addresses) > 0 {
+			desiredEndpointsByPortMap[epHash].Insert(&endpoint)
+			numDesiredEndpoints++
+		}
 	}
 
 	spMetrics := metrics.NewServicePortCache()

pkg/controller/util/endpoint/controller_utils.go

@@ -126,12 +126,16 @@ func DeepHashObjectToString(objectToWrite interface{}) string {
 }
 
 // ShouldPodBeInEndpoints returns true if a specified pod should be in an
-// endpoints object.
-func ShouldPodBeInEndpoints(pod *v1.Pod) bool {
+// endpoints object. Terminating pods are only included if publishNotReady is true.
+func ShouldPodBeInEndpoints(pod *v1.Pod, publishNotReady bool) bool {
 	if len(pod.Status.PodIP) == 0 && len(pod.Status.PodIPs) == 0 {
 		return false
 	}
 
+	if !publishNotReady && pod.DeletionTimestamp != nil {
+		return false
+	}
+
 	if pod.Spec.RestartPolicy == v1.RestartPolicyNever {
 		return pod.Status.Phase != v1.PodFailed && pod.Status.Phase != v1.PodSucceeded
 	}

pkg/controller/util/endpoint/controller_utils_test.go

@@ -102,9 +102,10 @@ func TestDetermineNeededServiceUpdates(t *testing.T) {
 // 12 true cases.
 func TestShouldPodBeInEndpoints(t *testing.T) {
 	testCases := []struct {
-		name     string
-		pod      *v1.Pod
-		expected bool
+		name            string
+		pod             *v1.Pod
+		publishNotReady bool
+		expected        bool
 	}{
 		// Pod should not be in endpoints:
 		{
@@ -160,6 +161,23 @@ func TestShouldPodBeInEndpoints(t *testing.T) {
 			},
 			expected: false,
 		},
+		{
+			name: "Terminating Pod",
+			pod: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					DeletionTimestamp: &metav1.Time{
+						Time: time.Now(),
+					},
+				},
+				Spec: v1.PodSpec{},
+				Status: v1.PodStatus{
+					Phase: v1.PodRunning,
+					PodIP: "1.2.3.4",
+				},
+			},
+			publishNotReady: false,
+			expected:        false,
+		},
 		// Pod should be in endpoints:
 		{
 			name: "Failed pod with Always RestartPolicy",
@@ -226,11 +244,28 @@ func TestShouldPodBeInEndpoints(t *testing.T) {
 			},
 			expected: true,
 		},
+		{
+			name: "Terminating Pod with publish not ready",
+			pod: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					DeletionTimestamp: &metav1.Time{
+						Time: time.Now(),
+					},
+				},
+				Spec: v1.PodSpec{},
+				Status: v1.PodStatus{
+					Phase: v1.PodRunning,
+					PodIP: "1.2.3.4",
+				},
+			},
+			publishNotReady: true,
+			expected:        true,
+		},
 	}
 
 	for _, test := range testCases {
 		t.Run(test.name, func(t *testing.T) {
-			result := ShouldPodBeInEndpoints(test.pod)
+			result := ShouldPodBeInEndpoints(test.pod, test.publishNotReady)
 			if result != test.expected {
 				t.Errorf("expected: %t, got: %t", test.expected, result)
 			}