Merge pull request #110365 from spencerhance/epslice-recycle-bug

Fix unnecessary recreation of placeholder EndpointSlice

pkg/controller/endpointslice/endpointslice_controller_test.go

@@ -280,10 +280,9 @@ func TestSyncServiceEndpointSlicePendingDeletion(t *testing.T) {
 	assert.Nil(t, err, "Expected no error syncing service")
 
 	// The EndpointSlice marked for deletion should be ignored by the controller, and thus
-	// should not result in more than one action from the client (an update to the non-terminating
+	// should not result in any action.
-	// EndpointSlice removing the trigger time annotation.)
+	if len(client.Actions()) != numActionsBefore {
-	if len(client.Actions()) != numActionsBefore+1 {
+		t.Errorf("Expected 0 more actions, got %d", len(client.Actions())-numActionsBefore)
-		t.Errorf("Expected 1 more action, got %d", len(client.Actions())-numActionsBefore)
 	}
 }
 

pkg/controller/endpointslice/reconciler.go

@@ -26,6 +26,7 @@ import (
 	discovery "k8s.io/api/discovery/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/conversion"
 	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -215,12 +216,18 @@ func (r *reconciler) reconcileByAddressType(service *corev1.Service, pods []*cor
 
 	// When no endpoint slices would usually exist, we need to add a placeholder.
 	if len(existingSlices) == len(slicesToDelete) && len(slicesToCreate) < 1 {
+		// Check for existing placeholder slice outside of the core control flow
 		placeholderSlice := newEndpointSlice(service, &endpointMeta{Ports: []discovery.EndpointPort{}, AddressType: addressType})
-		slicesToCreate = append(slicesToCreate, placeholderSlice)
+		if len(slicesToDelete) == 1 && placeholderSliceCompare.DeepEqual(slicesToDelete[0], placeholderSlice) {
-		spMetrics.Set(endpointutil.NewPortMapKey(placeholderSlice.Ports), metrics.EfficiencyInfo{
+			// We are about to unnecessarily delete/recreate the placeholder, remove it now.
-			Endpoints: 0,
+			slicesToDelete = slicesToDelete[:0]
-			Slices:    1,
+		} else {
-		})
+			slicesToCreate = append(slicesToCreate, placeholderSlice)
+			spMetrics.Set(endpointutil.NewPortMapKey(placeholderSlice.Ports), metrics.EfficiencyInfo{
+				Endpoints: 0,
+				Slices:    1,
+			})
+		}
 	}
 
 	metrics.EndpointsAddedPerSync.WithLabelValues().Observe(float64(totalAdded))
@@ -251,6 +258,30 @@ func (r *reconciler) reconcileByAddressType(service *corev1.Service, pods []*cor
 	return r.finalize(service, slicesToCreate, slicesToUpdate, slicesToDelete, triggerTime)
 }
 
+// placeholderSliceCompare is a conversion func for comparing two placeholder endpoint slices.
+// It only compares the specific fields we care about.
+var placeholderSliceCompare = conversion.EqualitiesOrDie(
+	func(a, b metav1.OwnerReference) bool {
+		return a.String() == b.String()
+	},
+	func(a, b metav1.ObjectMeta) bool {
+		if a.Namespace != b.Namespace {
+			return false
+		}
+		for k, v := range a.Labels {
+			if b.Labels[k] != v {
+				return false
+			}
+		}
+		for k, v := range b.Labels {
+			if a.Labels[k] != v {
+				return false
+			}
+		}
+		return true
+	},
+)
+
 // finalize creates, updates, and deletes slices as specified
 func (r *reconciler) finalize(
 	service *corev1.Service,

pkg/controller/endpointslice/reconciler_test.go

@@ -25,7 +25,6 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
-
 	corev1 "k8s.io/api/core/v1"
 	discovery "k8s.io/api/discovery/v1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
@@ -460,34 +459,154 @@ func TestReconcile1Pod(t *testing.T) {
 	}
 }
 
-// given an existing endpoint slice and no pods matching the service, the existing
+// given an existing placeholder endpoint slice and no pods matching the service, the existing
-// slice should be updated to a placeholder (not deleted)
+// slice should not change the placeholder
 func TestReconcile1EndpointSlice(t *testing.T) {
-	client := newClientset()
-	setupMetrics()
 	namespace := "test"
-	svc, endpointMeta := newServiceAndEndpointMeta("foo", namespace)
+	svc, epMeta := newServiceAndEndpointMeta("foo", namespace)
-	endpointSlice1 := newEmptyEndpointSlice(1, namespace, endpointMeta, svc)
+	emptySlice := newEmptyEndpointSlice(1, namespace, epMeta, svc)
+	emptySlice.ObjectMeta.Labels = map[string]string{"bar": "baz"}
 
-	_, createErr := client.DiscoveryV1().EndpointSlices(namespace).Create(context.TODO(), endpointSlice1, metav1.CreateOptions{})
+	testCases := []struct {
-	assert.Nil(t, createErr, "Expected no error creating endpoint slice")
+		desc        string
+		existing    *discovery.EndpointSlice
+		wantUpdate  bool
+		wantMetrics expectedMetrics
+	}{
+		{
+			desc:        "No existing placeholder",
+			wantUpdate:  true,
+			wantMetrics: expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 0, addedPerSync: 0, removedPerSync: 0, numCreated: 1, numUpdated: 0, numDeleted: 0, slicesChangedPerSync: 1},
+		},
+		{
+			desc:        "Existing placeholder that's the same",
+			existing:    newEndpointSlice(&svc, &endpointMeta{Ports: []discovery.EndpointPort{}, AddressType: discovery.AddressTypeIPv4}),
+			wantMetrics: expectedMetrics{desiredSlices: 0, actualSlices: 0, desiredEndpoints: 0, addedPerSync: 0, removedPerSync: 0, numCreated: 0, numUpdated: 0, numDeleted: 0, slicesChangedPerSync: 0},
+		},
+		{
+			desc:        "Existing placeholder that's different",
+			existing:    emptySlice,
+			wantUpdate:  true,
+			wantMetrics: expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 0, addedPerSync: 0, removedPerSync: 0, numCreated: 0, numUpdated: 1, numDeleted: 0, slicesChangedPerSync: 1},
+		},
+	}
 
-	numActionsBefore := len(client.Actions())
+	for _, tc := range testCases {
-	r := newReconciler(client, []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}}, defaultMaxEndpointsPerSlice)
+		t.Run(tc.desc, func(t *testing.T) {
-	reconcileHelper(t, r, &svc, []*corev1.Pod{}, []*discovery.EndpointSlice{endpointSlice1}, time.Now())
+			client := newClientset()
-	assert.Len(t, client.Actions(), numActionsBefore+1, "Expected 1 additional clientset action")
+			setupMetrics()
-	actions := client.Actions()
-	assert.True(t, actions[numActionsBefore].Matches("update", "endpointslices"), "Action should be update endpoint slice")
 
-	slices := fetchEndpointSlices(t, client, namespace)
+			existingSlices := []*discovery.EndpointSlice{}
-	assert.Len(t, slices, 1, "Expected 1 endpoint slices")
+			if tc.existing != nil {
+				existingSlices = append(existingSlices, tc.existing)
+				_, createErr := client.DiscoveryV1().EndpointSlices(namespace).Create(context.TODO(), tc.existing, metav1.CreateOptions{})
+				assert.Nil(t, createErr, "Expected no error creating endpoint slice")
+			}
 
-	assert.Regexp(t, "^"+svc.Name, slices[0].Name)
+			numActionsBefore := len(client.Actions())
-	assert.Equal(t, svc.Name, slices[0].Labels[discovery.LabelServiceName])
+			r := newReconciler(client, []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}}, defaultMaxEndpointsPerSlice)
-	assert.EqualValues(t, []discovery.EndpointPort{}, slices[0].Ports)
+			reconcileHelper(t, r, &svc, []*corev1.Pod{}, existingSlices, time.Now())
-	assert.EqualValues(t, []discovery.Endpoint{}, slices[0].Endpoints)
+
-	expectTrackedGeneration(t, r.endpointSliceTracker, &slices[0], 1)
+			var numUpdates int
-	expectMetrics(t, expectedMetrics{desiredSlices: 1, actualSlices: 1, desiredEndpoints: 0, addedPerSync: 0, removedPerSync: 0, numCreated: 0, numUpdated: 1, numDeleted: 0, slicesChangedPerSync: 1})
+			if tc.wantUpdate {
+				numUpdates = 1
+			}
+			wantActions := numActionsBefore + numUpdates
+			assert.Len(t, client.Actions(), wantActions, "Expected %d additional clientset actions", numUpdates)
+
+			slices := fetchEndpointSlices(t, client, namespace)
+			assert.Len(t, slices, 1, "Expected 1 endpoint slices")
+
+			if !tc.wantUpdate {
+				assert.Regexp(t, "^"+svc.Name, slices[0].Name)
+				assert.Equal(t, svc.Name, slices[0].Labels[discovery.LabelServiceName])
+				assert.EqualValues(t, []discovery.EndpointPort{}, slices[0].Ports)
+				assert.EqualValues(t, []discovery.Endpoint{}, slices[0].Endpoints)
+				if tc.existing == nil {
+					expectTrackedGeneration(t, r.endpointSliceTracker, &slices[0], 1)
+				}
+			}
+			expectMetrics(t, tc.wantMetrics)
+		})
+	}
+}
+
+func TestPlaceHolderSliceCompare(t *testing.T) {
+	testCases := []struct {
+		desc string
+		x    *discovery.EndpointSlice
+		y    *discovery.EndpointSlice
+		want bool
+	}{
+		{
+			desc: "Both nil",
+			want: true,
+		},
+		{
+			desc: "Y is nil",
+			x:    &discovery.EndpointSlice{},
+			want: false,
+		},
+		{
+			desc: "X is nil",
+			y:    &discovery.EndpointSlice{},
+			want: false,
+		},
+		{
+			desc: "Both are empty and non-nil",
+			x:    &discovery.EndpointSlice{},
+			y:    &discovery.EndpointSlice{},
+			want: true,
+		},
+		{
+			desc: "Only ObjectMeta.Name has diff",
+			x: &discovery.EndpointSlice{ObjectMeta: metav1.ObjectMeta{
+				Name: "foo",
+			}},
+			y: &discovery.EndpointSlice{ObjectMeta: metav1.ObjectMeta{
+				Name: "bar",
+			}},
+			want: true,
+		},
+		{
+			desc: "Only ObjectMeta.Labels has diff",
+			x: &discovery.EndpointSlice{ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"foo": "true",
+				},
+			}},
+			y: &discovery.EndpointSlice{ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"bar": "true",
+				},
+			}},
+			want: false,
+		},
+		{
+			desc: "Creation time is different",
+			x: &discovery.EndpointSlice{ObjectMeta: metav1.ObjectMeta{
+				CreationTimestamp: metav1.Unix(1, 0),
+			}},
+			y: &discovery.EndpointSlice{ObjectMeta: metav1.ObjectMeta{
+				CreationTimestamp: metav1.Unix(2, 0),
+			}},
+			want: true,
+		},
+		{
+			desc: "Different except for ObjectMeta",
+			x:    &discovery.EndpointSlice{AddressType: discovery.AddressTypeIPv4},
+			y:    &discovery.EndpointSlice{AddressType: discovery.AddressTypeIPv6},
+			want: false,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			got := placeholderSliceCompare.DeepEqual(tc.x, tc.y)
+			if got != tc.want {
+				t.Errorf("sliceEqual(%v, %v) = %t, want %t", tc.x, tc.y, got, tc.want)
+			}
+		})
+	}
 }
 
 // when a Service has PublishNotReadyAddresses set to true, corresponding