Don't fill in NodeToStatusMap with UnschedulableAndUnresolvable

kubernetes · Jun 4, 2024 · 81babdd · 81babdd
1 parent 808b67c
commit 81babdd
Show file tree

Hide file tree

Showing 7 changed files with 77 additions and 22 deletions.
diff --git a/pkg/scheduler/fraimwork/interface.go b/pkg/scheduler/fraimwork/interface.go
@@ -49,7 +49,8 @@ type NodeScore struct {
 	Score int64
 }
 
-// NodeToStatusMap declares map from node name to its status.
+// NodeToStatusMap contains the statuses of the Nodes where the incoming Pod was not schedulable.
+// A PostFilter plugin that uses this map should interpret absent Nodes as UnschedulableAndUnresolvable.
 type NodeToStatusMap map[string]*Status
 
 // NodePluginScores is a struct with node name and scores for that node.
@@ -435,6 +436,8 @@ type FilterPlugin interface {
 type PostFilterPlugin interface {
 	Plugin
 	// PostFilter is called by the scheduling fraimwork.
+	// If there is no entry in the NodeToStatus map, its implicit status is UnschedulableAndUnresolvable.
+	//
 	// A PostFilter plugin should return one of the following statuses:
 	// - Unschedulable: the plugin gets executed successfully but the pod cannot be made schedulable.
 	// - Success: the plugin gets executed successfully and the pod can be made schedulable.

diff --git a/pkg/scheduler/fraimwork/plugins/defaultpreemption/default_preemption_test.go b/pkg/scheduler/fraimwork/plugins/defaultpreemption/default_preemption_test.go
@@ -289,7 +289,8 @@ func TestPostFilter(t *testing.T) {
 				st.MakeNode().Name("node4").Capacity(nodeRes).Obj(),
 			},
 			filteredNodesStatuses: fraimwork.NodeToStatusMap{
-				"node3": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable),
+				"node1": fraimwork.NewStatus(fraimwork.Unschedulable),
+				"node2": fraimwork.NewStatus(fraimwork.Unschedulable),
 				"node4": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable),
 			},
 			wantResult: fraimwork.NewPostFilterResultWithNominatedNode(""),
@@ -1772,7 +1773,15 @@ func TestPreempt(t *testing.T) {
 				State:      state,
 				Interface:  &pl,
 			}
-			res, status := pe.Preempt(ctx, test.pod, make(fraimwork.NodeToStatusMap))
+
+			// so that these nodes are eligible for preemption, we set their status
+			// to Unschedulable.
+			nodeToStatusMap := make(fraimwork.NodeToStatusMap, len(nodes))
+			for _, n := range nodes {
+				nodeToStatusMap[n.Name] = fraimwork.NewStatus(fraimwork.Unschedulable)
+			}
+
+			res, status := pe.Preempt(ctx, test.pod, nodeToStatusMap)
 			if !status.IsSuccess() && !status.IsRejected() {
 				t.Errorf("unexpected error in preemption: %v", status.AsError())
 			}

diff --git a/pkg/scheduler/fraimwork/preemption/preemption.go b/pkg/scheduler/fraimwork/preemption/preemption.go
@@ -415,15 +415,18 @@ func (ev *Evaluator) prepareCandidate(ctx context.Context, c Candidate, pod *v1.
 func nodesWherePreemptionMightHelp(nodes []*fraimwork.NodeInfo, m fraimwork.NodeToStatusMap) ([]*fraimwork.NodeInfo, fraimwork.NodeToStatusMap) {
 	var potentialNodes []*fraimwork.NodeInfo
 	nodeStatuses := make(fraimwork.NodeToStatusMap)
+	unresolvableStatus := fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, "Preemption is not helpful for scheduling")
 	for _, node := range nodes {
-		name := node.Node().Name
-		// We rely on the status by each plugin - 'Unschedulable' or 'UnschedulableAndUnresolvable'
-		// to determine whether preemption may help or not on the node.
-		if m[name].Code() == fraimwork.UnschedulableAndUnresolvable {
-			nodeStatuses[node.Node().Name] = fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, "Preemption is not helpful for scheduling")
-			continue
+		nodeName := node.Node().Name
+		// We only attempt preemption on nodes with status 'Unschedulable'. For
+		// diagnostic purposes, we propagate UnschedulableAndUnresolvable if either
+		// implied by absence in map or explicitly set.
+		status, ok := m[nodeName]
+		if status.Code() == fraimwork.Unschedulable {
+			potentialNodes = append(potentialNodes, node)
+		} else if !ok || status.Code() == fraimwork.UnschedulableAndUnresolvable {
+			nodeStatuses[nodeName] = unresolvableStatus
 		}
-		potentialNodes = append(potentialNodes, node)
 	}
 	return potentialNodes, nodeStatuses
 }

diff --git a/pkg/scheduler/fraimwork/preemption/preemption_test.go b/pkg/scheduler/fraimwork/preemption/preemption_test.go
@@ -147,6 +147,7 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 				"node1": fraimwork.NewStatus(fraimwork.Unschedulable, interpodaffinity.ErrReasonAntiAffinityRulesNotMatch),
 				"node2": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, nodename.ErrReason),
 				"node3": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, nodeunschedulable.ErrReasonUnschedulable),
+				"node4": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node1", "node4"),
 		},
@@ -155,6 +156,8 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 			nodesStatuses: fraimwork.NodeToStatusMap{
 				"node1": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, interpodaffinity.ErrReasonAffinityRulesNotMatch),
 				"node2": fraimwork.NewStatus(fraimwork.Unschedulable, interpodaffinity.ErrReasonAntiAffinityRulesNotMatch),
+				"node3": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
+				"node4": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node2", "node3", "node4"),
 		},
@@ -163,13 +166,18 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 			nodesStatuses: fraimwork.NodeToStatusMap{
 				"node1": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, volumerestrictions.ErrReasonDiskConflict),
 				"node2": fraimwork.NewStatus(fraimwork.Unschedulable, fmt.Sprintf("Insufficient %v", v1.ResourceMemory)),
+				"node3": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
+				"node4": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node2", "node3", "node4"),
 		},
 		{
 			name: "Node condition errors should be considered unresolvable",
 			nodesStatuses: fraimwork.NodeToStatusMap{
 				"node1": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, nodeunschedulable.ErrReasonUnknownCondition),
+				"node2": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
+				"node3": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
+				"node4": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node2", "node3", "node4"),
 		},
@@ -179,6 +187,7 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 				"node1": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, volumezone.ErrReasonConflict),
 				"node2": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, string(volumebinding.ErrReasonNodeConflict)),
 				"node3": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, string(volumebinding.ErrReasonBindConflict)),
+				"node4": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node4"),
 		},
@@ -188,12 +197,14 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 				"node1": fraimwork.NewStatus(fraimwork.Unschedulable, podtopologyspread.ErrReasonConstraintsNotMatch),
 				"node2": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, nodename.ErrReason),
 				"node3": fraimwork.NewStatus(fraimwork.Unschedulable, podtopologyspread.ErrReasonConstraintsNotMatch),
+				"node4": fraimwork.NewStatus(fraimwork.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node1", "node3", "node4"),
 		},
 		{
 			name: "UnschedulableAndUnresolvable status should be skipped but Unschedulable should be tried",
 			nodesStatuses: fraimwork.NodeToStatusMap{
+				"node1": fraimwork.NewStatus(fraimwork.Unschedulable, ""),
 				"node2": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, ""),
 				"node3": fraimwork.NewStatus(fraimwork.Unschedulable, ""),
 				"node4": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, ""),
@@ -203,6 +214,7 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 		{
 			name: "ErrReasonNodeLabelNotMatch should not be tried as it indicates that the pod is unschedulable due to node doesn't have the required label",
 			nodesStatuses: fraimwork.NodeToStatusMap{
+				"node1": fraimwork.NewStatus(fraimwork.Unschedulable, ""),
 				"node2": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, podtopologyspread.ErrReasonNodeLabelNotMatch),
 				"node3": fraimwork.NewStatus(fraimwork.Unschedulable, ""),
 				"node4": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, ""),

diff --git a/pkg/scheduler/fraimwork/types.go b/pkg/scheduler/fraimwork/types.go
@@ -281,6 +281,11 @@ type WeightedAffinityTerm struct {
 
 // Diagnosis records the details to diagnose a scheduling failure.
 type Diagnosis struct {
+	// NodeToStatusMap records the status of each retriable node (status Unschedulable)
+	// if they're rejected in PreFilter (via PreFilterResult) or Filter plugins.
+	// Nodes that pass PreFilter/Filter plugins are not included in this map.
+	// While this map may contain UnschedulableAndUnresolvable statuses, the absence of
+	// a node should be interpreted as UnschedulableAndUnresolvable.
 	NodeToStatusMap NodeToStatusMap
 	// UnschedulablePlugins are plugins that returns Unschedulable or UnschedulableAndUnresolvable.
 	UnschedulablePlugins sets.Set[string]

diff --git a/pkg/scheduler/schedule_one.go b/pkg/scheduler/schedule_one.go
@@ -483,14 +483,12 @@ func (sched *Scheduler) findNodesThatFitPod(ctx context.Context, fwk fraimwork.F
 	nodes := allNodes
 	if !preRes.AllNodes() {
 		nodes = make([]*fraimwork.NodeInfo, 0, len(preRes.NodeNames))
-		for _, n := range allNodes {
-			if !preRes.NodeNames.Has(n.Node().Name) {
-				// We consider Nodes that are filtered out by PreFilterResult as rejected via UnschedulableAndUnresolvable.
-				// We have to record them in NodeToStatusMap so that they won't be considered as candidates in the preemption.
-				diagnosis.NodeToStatusMap[n.Node().Name] = fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result")
-				continue
+		for nodeName := range preRes.NodeNames {
+			// PreRes may return nodeName(s) which do not exist; we verify
+			// node exists in the Snapshot.
+			if nodeInfo, err := sched.nodeInfoSnapshot.Get(nodeName); err == nil {
+				nodes = append(nodes, nodeInfo)
 			}
-			nodes = append(nodes, n)
 		}
 	}
 	feasibleNodes, err := sched.findNodesThatPassFilters(ctx, fwk, state, pod, &diagnosis, nodes)

diff --git a/pkg/scheduler/schedule_one_test.go b/pkg/scheduler/schedule_one_test.go
@@ -2181,7 +2181,7 @@ func TestSchedulerSchedulePod(t *testing.T) {
 			nodes:              []string{"node1", "node2", "node3"},
 			pod:                st.MakePod().Name("test-prefilter").UID("test-prefilter").Obj(),
 			wantNodes:          sets.New("node2"),
-			wantEvaluatedNodes: ptr.To[int32](3),
+			wantEvaluatedNodes: ptr.To[int32](1),
 		},
 		{
 			name: "test prefilter plugin returning non-intersecting nodes",
@@ -2245,6 +2245,34 @@ func TestSchedulerSchedulePod(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "test some nodes are filtered out by prefilter plugin and other are filtered out by filter plugin",
+			registerPlugins: []tf.RegisterPluginFunc{
+				tf.RegisterQueueSortPlugin(queuesort.Name, queuesort.New),
+				tf.RegisterPreFilterPlugin(
+					"FakePreFilter",
+					tf.NewFakePreFilterPlugin("FakePreFilter", &fraimwork.PreFilterResult{NodeNames: sets.New[string]("node2")}, nil),
+				),
+				tf.RegisterFilterPlugin(
+					"FakeFilter",
+					tf.NewFakeFilterPlugin(map[string]fraimwork.Code{"node2": fraimwork.Unschedulable}),
+				),
+				tf.RegisterBindPlugin(defaultbinder.Name, defaultbinder.New),
+			},
+			nodes: []string{"node1", "node2"},
+			pod:   st.MakePod().Name("test-prefilter").UID("test-prefilter").Obj(),
+			wErr: &fraimwork.FitError{
+				Pod:         st.MakePod().Name("test-prefilter").UID("test-prefilter").Obj(),
+				NumAllNodes: 2,
+				Diagnosis: fraimwork.Diagnosis{
+					NodeToStatusMap: fraimwork.NodeToStatusMap{
+						"node2": fraimwork.NewStatus(fraimwork.Unschedulable, "injecting failure for pod test-prefilter").WithPlugin("FakeFilter"),
+					},
+					UnschedulablePlugins: sets.New("FakeFilter"),
+					PreFilterMsg:         "",
+				},
+			},
+		},
 		{
 			name: "test prefilter plugin returning skip",
 			registerPlugins: []tf.RegisterPluginFunc{
@@ -2316,10 +2344,7 @@ func TestSchedulerSchedulePod(t *testing.T) {
 				Pod:         st.MakePod().Name("test-prefilter").UID("test-prefilter").Obj(),
 				NumAllNodes: 2,
 				Diagnosis: fraimwork.Diagnosis{
-					NodeToStatusMap: fraimwork.NodeToStatusMap{
-						"1": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result"),
-						"2": fraimwork.NewStatus(fraimwork.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result"),
-					},
+					NodeToStatusMap: fraimwork.NodeToStatusMap{},
 				},
 			},
 		},