Merge pull request #1452 from a7i/defaultevictor-reinit

fix: indexer cache error when default evictor is re-initialized

pkg/framework/plugins/defaultevictor/defaultevictor.go

@@ -244,6 +244,15 @@ func (d *DefaultEvictor) Filter(pod *v1.Pod) bool {
 
 func getPodIndexerByOwnerRefs(indexName string, handle frameworktypes.Handle) (cache.Indexer, error) {
 	podInformer := handle.SharedInformerFactory().Core().V1().Pods().Informer()
+	indexer := podInformer.GetIndexer()
+
+	// do not reinitialize the indexer, if it's been defined already
+	for name := range indexer.GetIndexers() {
+		if name == indexName {
+			return indexer, nil
+		}
+	}
+
 	if err := podInformer.AddIndexers(cache.Indexers{
 		indexName: func(obj interface{}) ([]string, error) {
 			pod, ok := obj.(*v1.Pod)
@@ -257,6 +266,5 @@ func getPodIndexerByOwnerRefs(indexName string, handle frameworktypes.Handle) (c
 		return nil, err
 	}
 
-	indexer := podInformer.GetIndexer()
 	return indexer, nil
 }

pkg/framework/plugins/defaultevictor/defaultevictor_test.go

@@ -15,6 +15,7 @@ package defaultevictor
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	v1 "k8s.io/api/core/v1"
@@ -31,6 +32,19 @@ import (
 	"sigs.k8s.io/descheduler/test"
 )
 
+type testCase struct {
+	description             string
+	pods                    []*v1.Pod
+	nodes                   []*v1.Node
+	evictFailedBarePods     bool
+	evictLocalStoragePods   bool
+	evictSystemCriticalPods bool
+	priorityThreshold       *int32
+	nodeFit                 bool
+	minReplicas             uint
+	result                  bool
+}
+
 func TestDefaultEvictorPreEvictionFilter(t *testing.T) {
 	n1 := test.BuildTestNode("node1", 1000, 2000, 13, nil)
 
@@ -39,17 +53,6 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) {
 
 	nodeLabelKey := "datacenter"
 	nodeLabelValue := "east"
-	type testCase struct {
-		description             string
-		pods                    []*v1.Pod
-		nodes                   []*v1.Node
-		evictFailedBarePods     bool
-		evictLocalStoragePods   bool
-		evictSystemCriticalPods bool
-		priorityThreshold       *int32
-		nodeFit                 bool
-		result                  bool
-	}
 
 	testCases := []testCase{
 		{
@@ -305,45 +308,7 @@ func TestDefaultEvictorPreEvictionFilter(t *testing.T) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 
-			var objs []runtime.Object
+			evictorPlugin, err := initializePlugin(ctx, test)
-			for _, node := range test.nodes {
-				objs = append(objs, node)
-			}
-			for _, pod := range test.pods {
-				objs = append(objs, pod)
-			}
-
-			fakeClient := fake.NewSimpleClientset(objs...)
-
-			sharedInformerFactory := informers.NewSharedInformerFactory(fakeClient, 0)
-			podInformer := sharedInformerFactory.Core().V1().Pods().Informer()
-
-			getPodsAssignedToNode, err := podutil.BuildGetPodsAssignedToNodeFunc(podInformer)
-			if err != nil {
-				t.Errorf("Build get pods assigned to node function error: %v", err)
-			}
-
-			sharedInformerFactory.Start(ctx.Done())
-			sharedInformerFactory.WaitForCacheSync(ctx.Done())
-
-			defaultEvictorArgs := &DefaultEvictorArgs{
-				EvictLocalStoragePods:   test.evictLocalStoragePods,
-				EvictSystemCriticalPods: test.evictSystemCriticalPods,
-				IgnorePvcPods:           false,
-				EvictFailedBarePods:     test.evictFailedBarePods,
-				PriorityThreshold: &api.PriorityThreshold{
-					Value: test.priorityThreshold,
-				},
-				NodeFit: test.nodeFit,
-			}
-
-			evictorPlugin, err := New(
-				defaultEvictorArgs,
-				&frameworkfake.HandleImpl{
-					ClientsetImpl:                 fakeClient,
-					GetPodsAssignedToNodeFuncImpl: getPodsAssignedToNode,
-					SharedInformerFactoryImpl:     sharedInformerFactory,
-				})
 			if err != nil {
 				t.Fatalf("Unable to initialize the plugin: %v", err)
 			}
@@ -366,19 +331,6 @@ func TestDefaultEvictorFilter(t *testing.T) {
 
 	ownerRefUUID := uuid.NewUUID()
 
-	type testCase struct {
-		description             string
-		pods                    []*v1.Pod
-		nodes                   []*v1.Node
-		evictFailedBarePods     bool
-		evictLocalStoragePods   bool
-		evictSystemCriticalPods bool
-		priorityThreshold       *int32
-		nodeFit                 bool
-		minReplicas             uint
-		result                  bool
-	}
-
 	testCases := []testCase{
 		{
 			description: "Failed pod eviction with no ownerRefs",
@@ -757,46 +709,7 @@ func TestDefaultEvictorFilter(t *testing.T) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 
-			var objs []runtime.Object
+			evictorPlugin, err := initializePlugin(ctx, test)
-			for _, node := range test.nodes {
-				objs = append(objs, node)
-			}
-			for _, pod := range test.pods {
-				objs = append(objs, pod)
-			}
-
-			fakeClient := fake.NewSimpleClientset(objs...)
-
-			sharedInformerFactory := informers.NewSharedInformerFactory(fakeClient, 0)
-			podInformer := sharedInformerFactory.Core().V1().Pods().Informer()
-
-			getPodsAssignedToNode, err := podutil.BuildGetPodsAssignedToNodeFunc(podInformer)
-			if err != nil {
-				t.Errorf("Build get pods assigned to node function error: %v", err)
-			}
-
-			sharedInformerFactory.Start(ctx.Done())
-			sharedInformerFactory.WaitForCacheSync(ctx.Done())
-
-			defaultEvictorArgs := &DefaultEvictorArgs{
-				EvictLocalStoragePods:   test.evictLocalStoragePods,
-				EvictSystemCriticalPods: test.evictSystemCriticalPods,
-				IgnorePvcPods:           false,
-				EvictFailedBarePods:     test.evictFailedBarePods,
-				PriorityThreshold: &api.PriorityThreshold{
-					Value: test.priorityThreshold,
-				},
-				NodeFit:     test.nodeFit,
-				MinReplicas: test.minReplicas,
-			}
-
-			evictorPlugin, err := New(
-				defaultEvictorArgs,
-				&frameworkfake.HandleImpl{
-					ClientsetImpl:                 fakeClient,
-					GetPodsAssignedToNodeFuncImpl: getPodsAssignedToNode,
-					SharedInformerFactoryImpl:     sharedInformerFactory,
-				})
 			if err != nil {
 				t.Fatalf("Unable to initialize the plugin: %v", err)
 			}
@@ -808,3 +721,94 @@ func TestDefaultEvictorFilter(t *testing.T) {
 		})
 	}
 }
+
+func TestReinitialization(t *testing.T) {
+	n1 := test.BuildTestNode("node1", 1000, 2000, 13, nil)
+	ownerRefUUID := uuid.NewUUID()
+
+	testCases := []testCase{
+		{
+			description: "minReplicas of 2, multiple owners, eviction",
+			pods: []*v1.Pod{
+				test.BuildTestPod("p1", 1, 1, n1.Name, func(pod *v1.Pod) {
+					pod.ObjectMeta.OwnerReferences = append(test.GetNormalPodOwnerRefList(), test.GetNormalPodOwnerRefList()...)
+					pod.ObjectMeta.OwnerReferences[0].UID = ownerRefUUID
+				}),
+				test.BuildTestPod("p2", 1, 1, n1.Name, func(pod *v1.Pod) {
+					pod.ObjectMeta.OwnerReferences = test.GetNormalPodOwnerRefList()
+				}),
+			},
+			minReplicas: 2,
+			result:      true,
+		},
+	}
+
+	for _, test := range testCases {
+		t.Run(test.description, func(t *testing.T) {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			evictorPlugin, err := initializePlugin(ctx, test)
+			if err != nil {
+				t.Fatalf("Unable to initialize the plugin: %v", err)
+			}
+
+			defaultEvictor, ok := evictorPlugin.(*DefaultEvictor)
+			if !ok {
+				t.Fatalf("Unable to initialize as a DefaultEvictor plugin")
+			}
+			_, err = New(defaultEvictor.args, defaultEvictor.handle)
+			if err != nil {
+				t.Fatalf("Unable to reinitialize the plugin: %v", err)
+			}
+		})
+	}
+}
+
+func initializePlugin(ctx context.Context, test testCase) (frameworktypes.Plugin, error) {
+	var objs []runtime.Object
+	for _, node := range test.nodes {
+		objs = append(objs, node)
+	}
+	for _, pod := range test.pods {
+		objs = append(objs, pod)
+	}
+
+	fakeClient := fake.NewSimpleClientset(objs...)
+
+	sharedInformerFactory := informers.NewSharedInformerFactory(fakeClient, 0)
+	podInformer := sharedInformerFactory.Core().V1().Pods().Informer()
+
+	getPodsAssignedToNode, err := podutil.BuildGetPodsAssignedToNodeFunc(podInformer)
+	if err != nil {
+		return nil, fmt.Errorf("build get pods assigned to node function error: %v", err)
+	}
+
+	sharedInformerFactory.Start(ctx.Done())
+	sharedInformerFactory.WaitForCacheSync(ctx.Done())
+
+	defaultEvictorArgs := &DefaultEvictorArgs{
+		EvictLocalStoragePods:   test.evictLocalStoragePods,
+		EvictSystemCriticalPods: test.evictSystemCriticalPods,
+		IgnorePvcPods:           false,
+		EvictFailedBarePods:     test.evictFailedBarePods,
+		PriorityThreshold: &api.PriorityThreshold{
+			Value: test.priorityThreshold,
+		},
+		NodeFit:     test.nodeFit,
+		MinReplicas: test.minReplicas,
+	}
+
+	evictorPlugin, err := New(
+		defaultEvictorArgs,
+		&frameworkfake.HandleImpl{
+			ClientsetImpl:                 fakeClient,
+			GetPodsAssignedToNodeFuncImpl: getPodsAssignedToNode,
+			SharedInformerFactoryImpl:     sharedInformerFactory,
+		})
+	if err != nil {
+		return nil, fmt.Errorf("unable to initialize the plugin: %v", err)
+	}
+
+	return evictorPlugin, nil
+}