diff --git a/pkg/controller/endpointslice/endpointslice_controller_test.go b/pkg/controller/endpointslice/endpointslice_controller_test.go index a8fda60bba321..59daac8594907 100644 --- a/pkg/controller/endpointslice/endpointslice_controller_test.go +++ b/pkg/controller/endpointslice/endpointslice_controller_test.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/wait" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/informers" @@ -61,7 +62,8 @@ type endpointSliceController struct { } func newController(nodeNames []string, batchPeriod time.Duration) (*fake.Clientset, *endpointSliceController) { - client := newClientset() + client := fake.NewSimpleClientset() + informerFactory := informers.NewSharedInformerFactory(client, controller.NoResyncPeriodFunc()) nodeInformer := informerFactory.Core().V1().Nodes() indexer := nodeInformer.Informer().GetIndexer() @@ -69,11 +71,36 @@ func newController(nodeNames []string, batchPeriod time.Duration) (*fake.Clients indexer.Add(&v1.Node{ObjectMeta: metav1.ObjectMeta{Name: nodeName}}) } + esInformer := informerFactory.Discovery().V1().EndpointSlices() + esIndexer := esInformer.Informer().GetIndexer() + + // These reactors are required to mock functionality that would be covered + // automatically if we weren't using the fake client. + client.PrependReactor("create", "endpointslices", k8stesting.ReactionFunc(func(action k8stesting.Action) (bool, runtime.Object, error) { + endpointSlice := action.(k8stesting.CreateAction).GetObject().(*discovery.EndpointSlice) + + if endpointSlice.ObjectMeta.GenerateName != "" { + endpointSlice.ObjectMeta.Name = fmt.Sprintf("%s-%s", endpointSlice.ObjectMeta.GenerateName, rand.String(8)) + endpointSlice.ObjectMeta.GenerateName = "" + } + endpointSlice.Generation = 1 + esIndexer.Add(endpointSlice) + + return false, endpointSlice, nil + })) + client.PrependReactor("update", "endpointslices", k8stesting.ReactionFunc(func(action k8stesting.Action) (bool, runtime.Object, error) { + endpointSlice := action.(k8stesting.CreateAction).GetObject().(*discovery.EndpointSlice) + endpointSlice.Generation++ + esIndexer.Update(endpointSlice) + + return false, endpointSlice, nil + })) + esController := NewController( informerFactory.Core().V1().Pods(), informerFactory.Core().V1().Services(), nodeInformer, - informerFactory.Discovery().V1().EndpointSlices(), + esInformer, int32(100), client, batchPeriod) diff --git a/pkg/controller/endpointslice/endpointslice_tracker.go b/pkg/controller/endpointslice/endpointslice_tracker.go index 0c213698b3d81..1c411b0a0f827 100644 --- a/pkg/controller/endpointslice/endpointslice_tracker.go +++ b/pkg/controller/endpointslice/endpointslice_tracker.go @@ -80,9 +80,12 @@ func (est *endpointSliceTracker) ShouldSync(endpointSlice *discovery.EndpointSli return !ok || endpointSlice.Generation > g } -// StaleSlices returns true if one or more of the provided EndpointSlices -// have older generations than the corresponding tracked ones or if the tracker -// is expecting one or more of the provided EndpointSlices to be deleted. +// StaleSlices returns true if any of the following are true: +// 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. +// 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() defer est.lock.Unlock() @@ -92,12 +95,23 @@ func (est *endpointSliceTracker) StaleSlices(service *v1.Service, endpointSlices if !ok { return false } + providedSlices := map[types.UID]int64{} for _, endpointSlice := range endpointSlices { + providedSlices[endpointSlice.UID] = endpointSlice.Generation g, ok := gfs[endpointSlice.UID] if ok && (g == deletionExpected || g > endpointSlice.Generation) { return true } } + for uid, generation := range gfs { + if generation == deletionExpected { + continue + } + _, ok := providedSlices[uid] + if !ok { + return true + } + } return false } diff --git a/pkg/controller/endpointslice/endpointslice_tracker_test.go b/pkg/controller/endpointslice/endpointslice_tracker_test.go index c34a2c9c3c722..845b64db3cddd 100644 --- a/pkg/controller/endpointslice/endpointslice_tracker_test.go +++ b/pkg/controller/endpointslice/endpointslice_tracker_test.go @@ -184,6 +184,30 @@ func TestEndpointSliceTrackerStaleSlices(t *testing.T) { serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, slicesParam: []*discovery.EndpointSlice{epSlice1NewerGen}, expectNewer: false, + }, { + name: "slice in params is expected to be deleted", + tracker: &endpointSliceTracker{ + generationsByService: map[types.NamespacedName]generationsBySlice{ + {Name: "svc1", Namespace: "ns1"}: { + epSlice1.UID: deletionExpected, + }, + }, + }, + serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, + slicesParam: []*discovery.EndpointSlice{epSlice1}, + expectNewer: true, + }, { + name: "slice in tracker but not in params", + 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{}, + expectNewer: true, }} for _, tc := range testCases { diff --git a/pkg/controller/endpointslicemirroring/endpointslice_tracker.go b/pkg/controller/endpointslicemirroring/endpointslice_tracker.go index 8fb440e7bfd4f..365c30d612b23 100644 --- a/pkg/controller/endpointslicemirroring/endpointslice_tracker.go +++ b/pkg/controller/endpointslicemirroring/endpointslice_tracker.go @@ -80,9 +80,12 @@ func (est *endpointSliceTracker) ShouldSync(endpointSlice *discovery.EndpointSli return !ok || endpointSlice.Generation > g } -// StaleSlices returns true if one or more of the provided EndpointSlices -// have older generations than the corresponding tracked ones or if the tracker -// is expecting one or more of the provided EndpointSlices to be deleted. +// StaleSlices returns true if any of the following are true: +// 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. +// 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() defer est.lock.Unlock() @@ -92,12 +95,23 @@ func (est *endpointSliceTracker) StaleSlices(service *v1.Service, endpointSlices if !ok { return false } + providedSlices := map[types.UID]int64{} for _, endpointSlice := range endpointSlices { + providedSlices[endpointSlice.UID] = endpointSlice.Generation g, ok := gfs[endpointSlice.UID] if ok && (g == deletionExpected || g > endpointSlice.Generation) { return true } } + for uid, generation := range gfs { + if generation == deletionExpected { + continue + } + _, ok := providedSlices[uid] + if !ok { + return true + } + } return false } diff --git a/pkg/controller/endpointslicemirroring/endpointslice_tracker_test.go b/pkg/controller/endpointslicemirroring/endpointslice_tracker_test.go index 64ba4b3c4da49..40c1c8e06e6ce 100644 --- a/pkg/controller/endpointslicemirroring/endpointslice_tracker_test.go +++ b/pkg/controller/endpointslicemirroring/endpointslice_tracker_test.go @@ -184,6 +184,30 @@ func TestEndpointSliceTrackerStaleSlices(t *testing.T) { serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, slicesParam: []*discovery.EndpointSlice{epSlice1NewerGen}, expectNewer: false, + }, { + name: "slice in params is expected to be deleted", + tracker: &endpointSliceTracker{ + generationsByService: map[types.NamespacedName]generationsBySlice{ + {Name: "svc1", Namespace: "ns1"}: { + epSlice1.UID: deletionExpected, + }, + }, + }, + serviceParam: &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "svc1", Namespace: "ns1"}}, + slicesParam: []*discovery.EndpointSlice{epSlice1}, + expectNewer: true, + }, { + name: "slice in tracker but not in params", + 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{}, + expectNewer: true, }} for _, tc := range testCases {