Don't fill in NodeToStatusMap with UnschedulableAndUnresolvable

kubernetes · May 29, 2024 · 268919d · 268919d
1 parent 7ea3bf4
commit 268919d
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 19 deletions.
diff --git a/pkg/scheduler/framework/interface.go b/pkg/scheduler/framework/interface.go
@@ -49,7 +49,8 @@ type NodeScore struct {
 	Score int64
 }
 
-// NodeToStatusMap declares map from node name to its status.
+// NodeToStatusMap declares map from node name to its status. If a node is not
+// present in the map, its implicit Status is UnschedulableAndUnresolvable.
 type NodeToStatusMap map[string]*Status
 
 // NodePluginScores is a struct with node name and scores for that node.
@@ -448,6 +449,7 @@ type PostFilterPlugin interface {
 	// If this scheduling cycle failed at PreFilter, all Nodes have the status from the rejector PreFilter plugin in NodeToStatusMap.
 	// Note that the scheduling framework runs PostFilter plugins even when PreFilter returned UnschedulableAndUnresolvable.
 	// In that case, NodeToStatusMap contains all Nodes with UnschedulableAndUnresolvable.
+	// If there is no entry in the NodeToStatus map, its implicit status is UnschedulableAndUnresolvable.
 	//
 	// Also, ignoring Nodes with UnschedulableAndUnresolvable is the responsibility of each PostFilter plugin,
 	// meaning NodeToStatusMap obviously could have Nodes with UnschedulableAndUnresolvable

diff --git a/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go b/pkg/scheduler/framework/plugins/defaultpreemption/default_preemption_test.go
@@ -289,6 +289,8 @@ func TestPostFilter(t *testing.T) {
 				st.MakeNode().Name("node4").Capacity(nodeRes).Obj(),
 			},
 			filteredNodesStatuses: framework.NodeToStatusMap{
+				"node1": framework.NewStatus(framework.Unschedulable),
+				"node2": framework.NewStatus(framework.Unschedulable),
 				"node3": framework.NewStatus(framework.UnschedulableAndUnresolvable),
 				"node4": framework.NewStatus(framework.UnschedulableAndUnresolvable),
 			},
@@ -1776,7 +1778,15 @@ func TestPreempt(t *testing.T) {
 				State:      state,
 				Interface:  &pl,
 			}
-			res, status := pe.Preempt(ctx, test.pod, make(framework.NodeToStatusMap))
+
+			// so that these nodes are eligible for preemption, we set their status
+			// to Unschedulable.
+			nodeToStatusMap := make(framework.NodeToStatusMap)
+			for _, n := range nodes {
+				nodeToStatusMap[n.Name] = framework.NewStatus(framework.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/framework/preemption/preemption.go b/pkg/scheduler/framework/preemption/preemption.go
@@ -416,14 +416,17 @@ func nodesWherePreemptionMightHelp(nodes []*framework.NodeInfo, m framework.Node
 	var potentialNodes []*framework.NodeInfo
 	nodeStatuses := make(framework.NodeToStatusMap)
 	for _, node := range nodes {
-		name := node.Node().Name
+		nodeName := 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() == framework.UnschedulableAndUnresolvable {
-			nodeStatuses[node.Node().Name] = framework.NewStatus(framework.UnschedulableAndUnresolvable, "Preemption is not helpful for scheduling")
-			continue
+		// to determine whether preemption may help or not on the node. We first check that the entry
+		// is in the map, as a missing entry implies UnschedulableAndUnresolvable.
+		if status, ok := m[nodeName]; ok {
+			if status.Code() == framework.UnschedulableAndUnresolvable {
+				nodeStatuses[nodeName] = framework.NewStatus(framework.UnschedulableAndUnresolvable, "Preemption is not helpful for scheduling")
+				continue
+			}
+			potentialNodes = append(potentialNodes, node)
 		}
-		potentialNodes = append(potentialNodes, node)
 	}
 	return potentialNodes, nodeStatuses
 }

diff --git a/pkg/scheduler/framework/preemption/preemption_test.go b/pkg/scheduler/framework/preemption/preemption_test.go
@@ -147,6 +147,7 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 				"node1": framework.NewStatus(framework.Unschedulable, interpodaffinity.ErrReasonAntiAffinityRulesNotMatch),
 				"node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, nodename.ErrReason),
 				"node3": framework.NewStatus(framework.UnschedulableAndUnresolvable, nodeunschedulable.ErrReasonUnschedulable),
+				"node4": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node1", "node4"),
 		},
@@ -155,6 +156,8 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 			nodesStatuses: framework.NodeToStatusMap{
 				"node1": framework.NewStatus(framework.UnschedulableAndUnresolvable, interpodaffinity.ErrReasonAffinityRulesNotMatch),
 				"node2": framework.NewStatus(framework.Unschedulable, interpodaffinity.ErrReasonAntiAffinityRulesNotMatch),
+				"node3": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
+				"node4": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node2", "node3", "node4"),
 		},
@@ -163,13 +166,18 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 			nodesStatuses: framework.NodeToStatusMap{
 				"node1": framework.NewStatus(framework.UnschedulableAndUnresolvable, volumerestrictions.ErrReasonDiskConflict),
 				"node2": framework.NewStatus(framework.Unschedulable, fmt.Sprintf("Insufficient %v", v1.ResourceMemory)),
+				"node3": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
+				"node4": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node2", "node3", "node4"),
 		},
 		{
 			name: "Node condition errors should be considered unresolvable",
 			nodesStatuses: framework.NodeToStatusMap{
 				"node1": framework.NewStatus(framework.UnschedulableAndUnresolvable, nodeunschedulable.ErrReasonUnknownCondition),
+				"node2": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
+				"node3": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
+				"node4": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node2", "node3", "node4"),
 		},
@@ -179,6 +187,7 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 				"node1": framework.NewStatus(framework.UnschedulableAndUnresolvable, volumezone.ErrReasonConflict),
 				"node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, string(volumebinding.ErrReasonNodeConflict)),
 				"node3": framework.NewStatus(framework.UnschedulableAndUnresolvable, string(volumebinding.ErrReasonBindConflict)),
+				"node4": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node4"),
 		},
@@ -188,12 +197,14 @@ func TestNodesWherePreemptionMightHelp(t *testing.T) {
 				"node1": framework.NewStatus(framework.Unschedulable, podtopologyspread.ErrReasonConstraintsNotMatch),
 				"node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, nodename.ErrReason),
 				"node3": framework.NewStatus(framework.Unschedulable, podtopologyspread.ErrReasonConstraintsNotMatch),
+				"node4": framework.NewStatus(framework.Unschedulable, "Unschedulable"),
 			},
 			expected: sets.New("node1", "node3", "node4"),
 		},
 		{
 			name: "UnschedulableAndUnresolvable status should be skipped but Unschedulable should be tried",
 			nodesStatuses: framework.NodeToStatusMap{
+				"node1": framework.NewStatus(framework.Unschedulable, ""),
 				"node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, ""),
 				"node3": framework.NewStatus(framework.Unschedulable, ""),
 				"node4": framework.NewStatus(framework.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: framework.NodeToStatusMap{
+				"node1": framework.NewStatus(framework.Unschedulable, ""),
 				"node2": framework.NewStatus(framework.UnschedulableAndUnresolvable, podtopologyspread.ErrReasonNodeLabelNotMatch),
 				"node3": framework.NewStatus(framework.Unschedulable, ""),
 				"node4": framework.NewStatus(framework.UnschedulableAndUnresolvable, ""),

diff --git a/pkg/scheduler/framework/types.go b/pkg/scheduler/framework/types.go
@@ -325,9 +325,11 @@ const ExtenderName = "Extender"
 
 // Diagnosis records the details to diagnose a scheduling failure.
 type Diagnosis struct {
-	// NodeToStatusMap records the status of each node
+	// 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
@@ -485,14 +485,10 @@ func (sched *Scheduler) findNodesThatFitPod(ctx context.Context, fwk framework.F
 	nodes := allNodes
 	if !preRes.AllNodes() {
 		nodes = make([]*framework.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] = framework.NewStatus(framework.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result")
-				continue
+		for nodeName := range preRes.NodeNames {
+			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
@@ -2344,7 +2344,6 @@ func TestSchedulerSchedulePod(t *testing.T) {
 				NumAllNodes: 2,
 				Diagnosis: framework.Diagnosis{
 					NodeToStatusMap: framework.NodeToStatusMap{
-						"node1": framework.NewStatus(framework.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result"),
 						"node2": framework.NewStatus(framework.Unschedulable, "injecting failure for pod test-prefilter").WithPlugin("FakeFilter"),
 					},
 					UnschedulablePlugins: sets.New("FakeFilter"),
@@ -2454,8 +2453,6 @@ func TestSchedulerSchedulePod(t *testing.T) {
 				NumAllNodes: 2,
 				Diagnosis: framework.Diagnosis{
 					NodeToStatusMap: framework.NodeToStatusMap{
-						"1": framework.NewStatus(framework.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result"),
-						"2": framework.NewStatus(framework.UnschedulableAndUnresolvable, "node is filtered out by the prefilter result"),
 					},
 				},
 			},