EndpointSlice and Endpoints should treat terminating pods the same

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>

pkg/controller/endpointslice/reconciler.go

@@ -86,7 +86,10 @@ func (r *reconciler) reconcile(service *corev1.Service, pods []*corev1.Pod, exis
 	numDesiredEndpoints := 0
 
 	for _, pod := range pods {
-		if endpointutil.ShouldPodBeInEndpoints(pod) {
+		if !endpointutil.ShouldPodBeInEndpoints(pod, service.Spec.PublishNotReadyAddresses) {
+			continue
+		}
+
 		endpointPorts := getEndpointPorts(service, pod)
 		epHash := endpointutil.NewPortMapKey(endpointPorts)
 		if _, ok := desiredEndpointsByPortMap[epHash]; !ok {
@@ -110,7 +113,6 @@ func (r *reconciler) reconcile(service *corev1.Service, pods []*corev1.Pod, exis
 			numDesiredEndpoints++
 		}
 	}
-	}
 
 	spMetrics := metrics.NewServicePortCache()
 	totalAdded := 0

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

@@ -104,6 +104,7 @@ func TestShouldPodBeInEndpoints(t *testing.T) {
 	testCases := []struct {
 		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)
 			}