From 7d8048dd59244796c8af5104b347e3255900911b Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Sat, 23 Apr 2022 01:31:45 +0530 Subject: [PATCH 1/2] Ignore EndpointSlices that are already marked for deletion Signed-off-by: Sanskar Jaiswal --- .../util/endpointslice/endpointslice_tracker.go | 5 ++++- .../endpointslice/endpointslice_tracker_test.go | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/pkg/controller/util/endpointslice/endpointslice_tracker.go b/pkg/controller/util/endpointslice/endpointslice_tracker.go index 8138ca59b70..35b58f988d2 100644 --- a/pkg/controller/util/endpointslice/endpointslice_tracker.go +++ b/pkg/controller/util/endpointslice/endpointslice_tracker.go @@ -84,7 +84,7 @@ func (est *EndpointSliceTracker) ShouldSync(endpointSlice *discovery.EndpointSli // 1. One or more of the provided EndpointSlices have older generations than the // corresponding tracked ones. // 2. The tracker is expecting one or more of the provided EndpointSlices to be -// deleted. +// deleted. (EndpointSlices that have already been marked for deletion are ignored here.) // 3. The tracker is tracking EndpointSlices that have not been provided. func (est *EndpointSliceTracker) StaleSlices(service *v1.Service, endpointSlices []*discovery.EndpointSlice) bool { est.lock.Lock() @@ -100,6 +100,9 @@ func (est *EndpointSliceTracker) StaleSlices(service *v1.Service, endpointSlices providedSlices[endpointSlice.UID] = endpointSlice.Generation g, ok := gfs[endpointSlice.UID] if ok && (g == deletionExpected || g > endpointSlice.Generation) { + if endpointSlice.DeletionTimestamp != nil { + continue + } return true } } diff --git a/pkg/controller/util/endpointslice/endpointslice_tracker_test.go b/pkg/controller/util/endpointslice/endpointslice_tracker_test.go index ca73ed20978..b7b0d7fc1e7 100644 --- a/pkg/controller/util/endpointslice/endpointslice_tracker_test.go +++ b/pkg/controller/util/endpointslice/endpointslice_tracker_test.go @@ -124,6 +124,10 @@ func TestEndpointSliceTrackerStaleSlices(t *testing.T) { epSlice1NewerGen := epSlice1.DeepCopy() epSlice1NewerGen.Generation = 2 + epTerminatingSlice := epSlice1.DeepCopy() + now := metav1.Now() + epTerminatingSlice.DeletionTimestamp = &now + testCases := []struct { name string tracker *EndpointSliceTracker @@ -208,6 +212,18 @@ func TestEndpointSliceTrackerStaleSlices(t *testing.T) { serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, slicesParam: []*discovery.EndpointSlice{}, expectNewer: true, + }, { + name: "slice in params is has non nil deletion timestamp", + tracker: &EndpointSliceTracker{ + generationsByService: map[types.NamespacedName]GenerationsBySlice{ + {Name: "svc1", Namespace: "ns1"}: { + epSlice1.UID: epSlice1.Generation, + }, + }, + }, + serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, + slicesParam: []*discovery.EndpointSlice{epTerminatingSlice}, + expectNewer: false, }} for _, tc := range testCases { From 4314e58ae54832c3f1cecae7ca448e1f08b7c00d Mon Sep 17 00:00:00 2001 From: Sanskar Jaiswal Date: Tue, 26 Apr 2022 03:24:18 +0530 Subject: [PATCH 2/2] move the ignore logic higher up to the reconciler Signed-off-by: Sanskar Jaiswal --- .../endpointslice/endpointslice_controller.go | 14 +++ .../endpointslice_controller_test.go | 103 ++++++++++++++++++ .../endpointslice/endpointslice_tracker.go | 3 - .../endpointslice_tracker_test.go | 12 -- 4 files changed, 117 insertions(+), 15 deletions(-) diff --git a/pkg/controller/endpointslice/endpointslice_controller.go b/pkg/controller/endpointslice/endpointslice_controller.go index f91dc4de11d..c33ab5b393f 100644 --- a/pkg/controller/endpointslice/endpointslice_controller.go +++ b/pkg/controller/endpointslice/endpointslice_controller.go @@ -364,6 +364,9 @@ func (c *Controller) syncService(key string) error { return err } + // Drop EndpointSlices that have been marked for deletion to prevent the controller from getting stuck. + endpointSlices = dropEndpointSlicesPendingDeletion(endpointSlices) + if c.endpointSliceTracker.StaleSlices(service, endpointSlices) { return endpointsliceutil.NewStaleInformerCache("EndpointSlice informer cache is out of date") } @@ -557,3 +560,14 @@ func trackSync(err error) { } endpointslicemetrics.EndpointSliceSyncs.WithLabelValues(metricLabel).Inc() } + +func dropEndpointSlicesPendingDeletion(endpointSlices []*discovery.EndpointSlice) []*discovery.EndpointSlice { + n := 0 + for _, endpointSlice := range endpointSlices { + if endpointSlice.DeletionTimestamp == nil { + endpointSlices[n] = endpointSlice + n++ + } + } + return endpointSlices[:n] +} diff --git a/pkg/controller/endpointslice/endpointslice_controller_test.go b/pkg/controller/endpointslice/endpointslice_controller_test.go index f5f49603065..bd70f8a4b95 100644 --- a/pkg/controller/endpointslice/endpointslice_controller_test.go +++ b/pkg/controller/endpointslice/endpointslice_controller_test.go @@ -241,6 +241,52 @@ func TestSyncServicePodSelection(t *testing.T) { assert.EqualValues(t, endpoint.TargetRef, &v1.ObjectReference{Kind: "Pod", Namespace: ns, Name: pod1.Name}) } +func TestSyncServiceEndpointSlicePendingDeletion(t *testing.T) { + client, esController := newController([]string{"node-1"}, time.Duration(0)) + ns := metav1.NamespaceDefault + serviceName := "testing-1" + service := createService(t, esController, ns, serviceName) + err := esController.syncService(fmt.Sprintf("%s/%s", ns, serviceName)) + assert.Nil(t, err, "Expected no error syncing service") + + gvk := schema.GroupVersionKind{Version: "v1", Kind: "Service"} + ownerRef := metav1.NewControllerRef(service, gvk) + + deletedTs := metav1.Now() + endpointSlice := &discovery.EndpointSlice{ + ObjectMeta: metav1.ObjectMeta{ + Name: "epSlice-1", + Namespace: ns, + OwnerReferences: []metav1.OwnerReference{*ownerRef}, + Labels: map[string]string{ + discovery.LabelServiceName: serviceName, + discovery.LabelManagedBy: controllerName, + }, + DeletionTimestamp: &deletedTs, + }, + AddressType: discovery.AddressTypeIPv4, + } + err = esController.endpointSliceStore.Add(endpointSlice) + if err != nil { + t.Fatalf("Expected no error adding EndpointSlice: %v", err) + } + _, err = client.DiscoveryV1().EndpointSlices(ns).Create(context.TODO(), endpointSlice, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Expected no error creating EndpointSlice: %v", err) + } + + numActionsBefore := len(client.Actions()) + err = esController.syncService(fmt.Sprintf("%s/%s", ns, serviceName)) + 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 + // EndpointSlice removing the trigger time annotation.) + if len(client.Actions()) != numActionsBefore+1 { + t.Errorf("Expected 1 more action, got %d", len(client.Actions())-numActionsBefore) + } +} + // Ensure SyncService correctly selects and labels EndpointSlices. func TestSyncServiceEndpointSliceLabelSelection(t *testing.T) { client, esController := newController([]string{"node-1"}, time.Duration(0)) @@ -1808,3 +1854,60 @@ func (cmc *cacheMutationCheck) Check(t *testing.T) { } } } + +func Test_dropEndpointSlicesPendingDeletion(t *testing.T) { + now := metav1.Now() + endpointSlices := []*discovery.EndpointSlice{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "epSlice1", + DeletionTimestamp: &now, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "epSlice2", + }, + AddressType: discovery.AddressTypeIPv4, + Endpoints: []discovery.Endpoint{ + { + Addresses: []string{"172.18.0.2"}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "epSlice3", + }, + AddressType: discovery.AddressTypeIPv6, + Endpoints: []discovery.Endpoint{ + { + Addresses: []string{"3001:0da8:75a3:0000:0000:8a2e:0370:7334"}, + }, + }, + }, + } + + epSlice2 := endpointSlices[1] + epSlice3 := endpointSlices[2] + + result := dropEndpointSlicesPendingDeletion(endpointSlices) + + assert.Len(t, result, 2) + for _, epSlice := range result { + if epSlice.Name == "epSlice1" { + t.Errorf("Expected EndpointSlice marked for deletion to be dropped.") + } + } + + // We don't use endpointSlices and instead check manually for equality, because + // `dropEndpointSlicesPendingDeletion` mutates the slice it receives, so it's easy + // to break this test later. This way, we can be absolutely sure that the result + // has exactly what we expect it to. + if !reflect.DeepEqual(epSlice2, result[0]) { + t.Errorf("EndpointSlice was unexpectedly mutated. Expected: %+v, Mutated: %+v", epSlice2, result[0]) + } + if !reflect.DeepEqual(epSlice3, result[1]) { + t.Errorf("EndpointSlice was unexpectedly mutated. Expected: %+v, Mutated: %+v", epSlice3, result[1]) + } +} diff --git a/pkg/controller/util/endpointslice/endpointslice_tracker.go b/pkg/controller/util/endpointslice/endpointslice_tracker.go index 35b58f988d2..2ab9dc1326a 100644 --- a/pkg/controller/util/endpointslice/endpointslice_tracker.go +++ b/pkg/controller/util/endpointslice/endpointslice_tracker.go @@ -100,9 +100,6 @@ func (est *EndpointSliceTracker) StaleSlices(service *v1.Service, endpointSlices providedSlices[endpointSlice.UID] = endpointSlice.Generation g, ok := gfs[endpointSlice.UID] if ok && (g == deletionExpected || g > endpointSlice.Generation) { - if endpointSlice.DeletionTimestamp != nil { - continue - } return true } } diff --git a/pkg/controller/util/endpointslice/endpointslice_tracker_test.go b/pkg/controller/util/endpointslice/endpointslice_tracker_test.go index b7b0d7fc1e7..efd9ba07499 100644 --- a/pkg/controller/util/endpointslice/endpointslice_tracker_test.go +++ b/pkg/controller/util/endpointslice/endpointslice_tracker_test.go @@ -212,18 +212,6 @@ func TestEndpointSliceTrackerStaleSlices(t *testing.T) { serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, slicesParam: []*discovery.EndpointSlice{}, expectNewer: true, - }, { - name: "slice in params is has non nil deletion timestamp", - tracker: &EndpointSliceTracker{ - generationsByService: map[types.NamespacedName]GenerationsBySlice{ - {Name: "svc1", Namespace: "ns1"}: { - epSlice1.UID: epSlice1.Generation, - }, - }, - }, - serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, - slicesParam: []*discovery.EndpointSlice{epTerminatingSlice}, - expectNewer: false, }} for _, tc := range testCases {