From d82684e691d9da030d81b468d5f79fdd136b6045 Mon Sep 17 00:00:00 2001
From: utam0k <k0ma@utam0k.jp>
Date: Sat, 29 Apr 2023 09:17:13 +0000
Subject: [PATCH] Add check to skip PodTopologySpread PreFilter if no
 constraints are specified

This commit adds a check in the PodTopologySpread PreFilter function to
return a Skip status if there are no topology spread constraints specified
This prevents unnecessary processing and filtering for pods that don't have any topology spread
constraints.
This change is a part of the work for issue #114399.

Signed-off-by: utam0k <k0ma@utam0k.jp>
---
 .../plugins/podtopologyspread/filtering.go    | 13 ++++++---
 .../podtopologyspread/filtering_test.go       | 27 ++++++++++++++-----
 2 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go
index bd0d5a92f30..ce1725ac8e1 100644
--- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go
+++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering.go
@@ -152,7 +152,10 @@ func (pl *PodTopologySpread) PreFilter(ctx context.Context, cycleState *framewor
 	s, err := pl.calPreFilterState(ctx, pod)
 	if err != nil {
 		return nil, framework.AsStatus(err)
+	} else if s != nil && len(s.Constraints) == 0 {
+		return nil, framework.NewStatus(framework.Skip)
 	}
+
 	cycleState.Write(preFilterStateKey, s)
 	return nil, nil
 }
@@ -236,11 +239,8 @@ func getPreFilterState(cycleState *framework.CycleState) (*preFilterState, error
 
 // calPreFilterState computes preFilterState describing how pods are spread on topologies.
 func (pl *PodTopologySpread) calPreFilterState(ctx context.Context, pod *v1.Pod) (*preFilterState, error) {
-	allNodes, err := pl.sharedLister.NodeInfos().List()
-	if err != nil {
-		return nil, fmt.Errorf("listing NodeInfos: %w", err)
-	}
 	var constraints []topologySpreadConstraint
+	var err error
 	if len(pod.Spec.TopologySpreadConstraints) > 0 {
 		// We have feature gating in APIServer to strip the spec
 		// so don't need to re-check feature gate, just check length of Constraints.
@@ -262,6 +262,11 @@ func (pl *PodTopologySpread) calPreFilterState(ctx context.Context, pod *v1.Pod)
 		return &preFilterState{}, nil
 	}
 
+	allNodes, err := pl.sharedLister.NodeInfos().List()
+	if err != nil {
+		return nil, fmt.Errorf("listing NodeInfos: %w", err)
+	}
+
 	s := preFilterState{
 		Constraints:          constraints,
 		TpKeyToCriticalPaths: make(map[string]*criticalPaths, len(constraints)),
diff --git a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go
index d6ea005c919..cd411cb95bd 100644
--- a/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go
+++ b/pkg/scheduler/framework/plugins/podtopologyspread/filtering_test.go
@@ -76,6 +76,7 @@ func TestPreFilterState(t *testing.T) {
 		objs                      []runtime.Object
 		defaultConstraints        []v1.TopologySpreadConstraint
 		want                      *preFilterState
+		wantPrefilterStatus       *framework.Status
 		enableMinDomains          bool
 		enableNodeInclusionPolicy bool
 		enableMatchLabelKeys      bool
@@ -573,7 +574,7 @@ func TestPreFilterState(t *testing.T) {
 			objs: []runtime.Object{
 				&v1.Service{Spec: v1.ServiceSpec{Selector: map[string]string{"baz": "kep"}}},
 			},
-			want: &preFilterState{},
+			wantPrefilterStatus: framework.NewStatus(framework.Skip),
 		},
 		{
 			name: "default constraints and a service, but pod has constraints",
@@ -613,7 +614,7 @@ func TestPreFilterState(t *testing.T) {
 			objs: []runtime.Object{
 				&v1.Service{Spec: v1.ServiceSpec{Selector: map[string]string{"foo": "bar"}}},
 			},
-			want: &preFilterState{},
+			wantPrefilterStatus: framework.NewStatus(framework.Skip),
 		},
 		{
 			name: "TpKeyToDomainsNum is calculated when MinDomains is enabled",
@@ -1453,6 +1454,14 @@ func TestPreFilterState(t *testing.T) {
 			},
 			enableMatchLabelKeys: true,
 		},
+		{
+			name: "skip if not specified",
+			pod:  st.MakePod().Name("p").Label("foo", "").Obj(),
+			nodes: []*v1.Node{
+				st.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
+			},
+			wantPrefilterStatus: framework.NewStatus(framework.Skip),
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1469,15 +1478,21 @@ func TestPreFilterState(t *testing.T) {
 			p.(*PodTopologySpread).enableMatchLabelKeysInPodTopologySpread = tt.enableMatchLabelKeys
 
 			cs := framework.NewCycleState()
-			if _, s := p.(*PodTopologySpread).PreFilter(ctx, cs, tt.pod); !s.IsSuccess() {
-				t.Fatal(s.AsError())
+			_, s := p.(*PodTopologySpread).PreFilter(ctx, cs, tt.pod)
+			if !tt.wantPrefilterStatus.Equal(s) {
+				t.Errorf("PodTopologySpread#PreFilter() returned unexpected status. got: %v, want: %v", s, tt.wantPrefilterStatus)
 			}
+
+			if !s.IsSuccess() {
+				return
+			}
+
 			got, err := getPreFilterState(cs)
 			if err != nil {
-				t.Fatal(err)
+				t.Fatalf("failed to get PreFilterState from cyclestate: %v", err)
 			}
 			if diff := cmp.Diff(tt.want, got, cmpOpts...); diff != "" {
-				t.Errorf("PodTopologySpread#PreFilter() returned diff (-want,+got):\n%s", diff)
+				t.Errorf("PodTopologySpread#PreFilter() returned unexpected prefilter status: diff (-want,+got):\n%s", diff)
 			}
 		})
 	}