EvenPodsSpread: Supports Preemption (removePod)

- add removePod() for podSpreadMap

pkg/scheduler/algorithm/predicates/metadata.go

@@ -374,6 +374,31 @@ func (m *topologyPairsMaps) clone() *topologyPairsMaps {
 	return copy
 }
 
+func (m *topologyPairsPodSpreadMap) removePod(deletedPod *v1.Pod) {
+	if m == nil || deletedPod == nil {
+		return
+	}
+
+	deletedPodFullName := schedutil.GetPodFullName(deletedPod)
+	pairSet, ok := m.podToTopologyPairs[deletedPodFullName]
+	if !ok {
+		return
+	}
+	topologyPairToPods := m.topologyPairToPods
+	for pair := range pairSet {
+		delete(topologyPairToPods[pair], deletedPod)
+		// if topologyPairToPods[pair] is empty after deletion
+		// don't clean it up as that topology counts as a match now
+
+		// removal of the deletedPod would probably genereate a smaller matching number
+		// so re-calculate minMatch to a smaller value if possible
+		if l := int32(len(topologyPairToPods[pair])); l < m.topologyKeyToMinPodsMap[pair.key] {
+			m.topologyKeyToMinPodsMap[pair.key] = l
+		}
+	}
+	delete(m.podToTopologyPairs, deletedPodFullName)
+}
+
 func (m *topologyPairsPodSpreadMap) clone() *topologyPairsPodSpreadMap {
 	// m could be nil when EvenPodsSpread feature is disabled
 	if m == nil {
@@ -400,6 +425,8 @@ func (meta *predicateMetadata) RemovePod(deletedPod *v1.Pod) error {
 	// Delete pod from the matching affinity or anti-affinity topology pairs maps.
 	meta.topologyPairsPotentialAffinityPods.removePod(deletedPod)
 	meta.topologyPairsPotentialAntiAffinityPods.removePod(deletedPod)
+	// Delete pod from the pod spread topology maps.
+	meta.topologyPairsPodSpreadMap.removePod(deletedPod)
 	// All pods in the serviceAffinityMatchingPodList are in the same namespace.
 	// So, if the namespace of the first one is not the same as the namespace of the
 	// deletedPod, we don't need to check the list, as deletedPod isn't in the list.

pkg/scheduler/algorithm/predicates/metadata_test.go

@@ -1243,6 +1243,226 @@ func TestGetTPMapMatchingSpreadConstraints(t *testing.T) {
 	}
 }
 
+func TestPodSpreadMap_RemovePod(t *testing.T) {
+	tests := []struct {
+		name              string
+		preemptor         *v1.Pod // preemptor pod
+		nodes             []*v1.Node
+		existingPods      []*v1.Pod
+		deletedPodIdx     int     // need to reuse *Pod of exsitingPods[i]
+		deletedPod        *v1.Pod // if deletedPodIdx is invalid, this field is bypassed
+		injectPodPointers map[topologyPair][]int
+		want              *topologyPairsPodSpreadMap
+	}{
+		{
+			// A high priority pod may not be scheduled due to node taints or resource shortage.
+			// So preemption is triggered.
+			name: "one spreadConstraint on zone, minMatchMap unchanged",
+			preemptor: u.MakePod().Name("p").Label("foo", "").
+				SpreadConstraint(1, "zone", hardSpread, u.MakeLabelSelector().Exists("foo").Obj()).
+				Obj(),
+			nodes: []*v1.Node{
+				u.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
+				u.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
+				u.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
+			},
+			existingPods: []*v1.Pod{
+				u.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
+				u.MakePod().Name("p-b1").Node("node-b").Label("foo", "").Obj(),
+				u.MakePod().Name("p-x1").Node("node-x").Label("foo", "").Obj(),
+			},
+			deletedPodIdx: 0, // remove pod "p-a1"
+			injectPodPointers: map[topologyPair][]int{
+				{key: "zone", value: "zone1"}: {1},
+				{key: "zone", value: "zone2"}: {2},
+			},
+			want: &topologyPairsPodSpreadMap{
+				// minMatchMap actually doesn't change
+				minMatchMap: map[string]int32{"zone": 1},
+				topologyPairsMaps: &topologyPairsMaps{
+					podToTopologyPairs: map[string]topologyPairSet{
+						"p-b1_": newPairSet("zone", "zone1"),
+						"p-x1_": newPairSet("zone", "zone2"),
+					},
+				},
+			},
+		},
+		{
+			name: "one spreadConstraint on node, minMatchMap changed",
+			preemptor: u.MakePod().Name("p").Label("foo", "").
+				SpreadConstraint(1, "zone", hardSpread, u.MakeLabelSelector().Exists("foo").Obj()).
+				Obj(),
+			nodes: []*v1.Node{
+				u.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
+				u.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
+				u.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
+				u.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
+			},
+			existingPods: []*v1.Pod{
+				u.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
+				u.MakePod().Name("p-b1").Node("node-b").Label("foo", "").Obj(),
+				u.MakePod().Name("p-x1").Node("node-x").Label("foo", "").Obj(),
+				u.MakePod().Name("p-y1").Node("node-y").Label("foo", "").Obj(),
+			},
+			deletedPodIdx: 0, // remove pod "p-a1"
+			injectPodPointers: map[topologyPair][]int{
+				{key: "zone", value: "zone1"}: {1},
+				{key: "zone", value: "zone2"}: {2, 3},
+			},
+			want: &topologyPairsPodSpreadMap{
+				// minMatchMap is expected to be re-calculated from {"zone": 2}
+				// to {"zone": 1}
+				minMatchMap: map[string]int32{"zone": 1},
+				topologyPairsMaps: &topologyPairsMaps{
+					podToTopologyPairs: map[string]topologyPairSet{
+						"p-b1_": newPairSet("zone", "zone1"),
+						"p-x1_": newPairSet("zone", "zone2"),
+						"p-y1_": newPairSet("zone", "zone2"),
+					},
+				},
+			},
+		},
+		{
+			name: "delete an irrelevant pod won't help",
+			preemptor: u.MakePod().Name("p").Label("foo", "").
+				SpreadConstraint(1, "zone", hardSpread, u.MakeLabelSelector().Exists("foo").Obj()).
+				Obj(),
+			nodes: []*v1.Node{
+				u.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
+				u.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
+				u.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
+				u.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
+			},
+			existingPods: []*v1.Pod{
+				u.MakePod().Name("p-a0").Node("node-a").Label("bar", "").Obj(),
+				u.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
+				u.MakePod().Name("p-b1").Node("node-b").Label("foo", "").Obj(),
+				u.MakePod().Name("p-x1").Node("node-x").Label("foo", "").Obj(),
+				u.MakePod().Name("p-y1").Node("node-y").Label("foo", "").Obj(),
+			},
+			deletedPodIdx: 0, // remove pod "p-a0"
+			injectPodPointers: map[topologyPair][]int{
+				{key: "zone", value: "zone1"}: {1, 2},
+				{key: "zone", value: "zone2"}: {3, 4},
+			},
+			want: &topologyPairsPodSpreadMap{
+				// minMatchMap is unchanged
+				minMatchMap: map[string]int32{"zone": 2},
+				topologyPairsMaps: &topologyPairsMaps{
+					podToTopologyPairs: map[string]topologyPairSet{
+						"p-a1_": newPairSet("zone", "zone1"),
+						"p-b1_": newPairSet("zone", "zone1"),
+						"p-x1_": newPairSet("zone", "zone2"),
+						"p-y1_": newPairSet("zone", "zone2"),
+					},
+				},
+			},
+		},
+		{
+			name: "delete a non-existing pod won't help",
+			preemptor: u.MakePod().Name("p").Label("foo", "").
+				SpreadConstraint(1, "zone", hardSpread, u.MakeLabelSelector().Exists("foo").Obj()).
+				Obj(),
+			nodes: []*v1.Node{
+				u.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
+				u.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
+				u.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
+				u.MakeNode().Name("node-y").Label("zone", "zone2").Label("node", "node-y").Obj(),
+			},
+			existingPods: []*v1.Pod{
+				u.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
+				u.MakePod().Name("p-b1").Node("node-b").Label("foo", "").Obj(),
+				u.MakePod().Name("p-x1").Node("node-x").Label("foo", "").Obj(),
+				u.MakePod().Name("p-y1").Node("node-y").Label("foo", "").Obj(),
+			},
+			deletedPodIdx: -1,
+			deletedPod:    u.MakePod().Name("p-a0").Node("node-a").Label("bar", "").Obj(),
+			injectPodPointers: map[topologyPair][]int{
+				{key: "zone", value: "zone1"}: {0, 1},
+				{key: "zone", value: "zone2"}: {2, 3},
+			},
+			want: &topologyPairsPodSpreadMap{
+				// minMatchMap is unchanged
+				minMatchMap: map[string]int32{"zone": 2},
+				topologyPairsMaps: &topologyPairsMaps{
+					podToTopologyPairs: map[string]topologyPairSet{
+						"p-a1_": newPairSet("zone", "zone1"),
+						"p-b1_": newPairSet("zone", "zone1"),
+						"p-x1_": newPairSet("zone", "zone2"),
+						"p-y1_": newPairSet("zone", "zone2"),
+					},
+				},
+			},
+		},
+		{
+			name: "two spreadConstraints",
+			preemptor: u.MakePod().Name("p").Label("foo", "").
+				SpreadConstraint(1, "zone", hardSpread, u.MakeLabelSelector().Exists("foo").Obj()).
+				SpreadConstraint(1, "node", hardSpread, u.MakeLabelSelector().Exists("foo").Obj()).
+				Obj(),
+			nodes: []*v1.Node{
+				u.MakeNode().Name("node-a").Label("zone", "zone1").Label("node", "node-a").Obj(),
+				u.MakeNode().Name("node-b").Label("zone", "zone1").Label("node", "node-b").Obj(),
+				u.MakeNode().Name("node-x").Label("zone", "zone2").Label("node", "node-x").Obj(),
+			},
+			existingPods: []*v1.Pod{
+				u.MakePod().Name("p-a1").Node("node-a").Label("foo", "").Obj(),
+				u.MakePod().Name("p-a2").Node("node-a").Label("foo", "").Obj(),
+				u.MakePod().Name("p-b1").Node("node-b").Label("foo", "").Obj(),
+				u.MakePod().Name("p-x1").Node("node-x").Label("foo", "").Obj(),
+				u.MakePod().Name("p-x2").Node("node-x").Label("foo", "").Obj(),
+			},
+			deletedPodIdx: 3, // remove pod "p-x1"
+			injectPodPointers: map[topologyPair][]int{
+				{key: "zone", value: "zone1"}:  {0, 1, 2},
+				{key: "zone", value: "zone2"}:  {4},
+				{key: "node", value: "node-a"}: {0, 1},
+				{key: "node", value: "node-b"}: {2},
+				{key: "node", value: "node-x"}: {4},
+			},
+			want: &topologyPairsPodSpreadMap{
+				// minMatchMap is expected to be re-calculated from {"zone": 2, "node": 1}
+				// to {"zone": 1, "node": 1}
+				minMatchMap: map[string]int32{"zone": 1, "node": 1},
+				topologyPairsMaps: &topologyPairsMaps{
+					podToTopologyPairs: map[string]topologyPairSet{
+						"p-a1_": newPairSet("zone", "zone1", "node", "node-a"),
+						"p-a2_": newPairSet("zone", "zone1", "node", "node-a"),
+						"p-b1_": newPairSet("zone", "zone1", "node", "node-b"),
+						"p-x2_": newPairSet("zone", "zone2", "node", "node-x"),
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			tt.want.topologyPairToPods = make(map[topologyPair]podSet)
+			for pair, indice := range tt.injectPodPointers {
+				pSet := make(podSet)
+				for _, i := range indice {
+					pSet[tt.existingPods[i]] = struct{}{}
+				}
+				tt.want.topologyPairToPods[pair] = pSet
+			}
+
+			nodeInfoMap := schedulernodeinfo.CreateNodeNameToInfoMap(tt.existingPods, tt.nodes)
+			podSpreadMap := getTPMapMatchingSpreadConstraints(tt.preemptor, nodeInfoMap)
+
+			var deletedPod *v1.Pod
+			if tt.deletedPodIdx < len(tt.existingPods) && tt.deletedPodIdx >= 0 {
+				deletedPod = tt.existingPods[tt.deletedPodIdx]
+			} else {
+				deletedPod = tt.deletedPod
+			}
+			podSpreadMap.removePod(deletedPod)
+			if !reflect.DeepEqual(podSpreadMap, tt.want) {
+				t.Errorf("podSpreadMap#removePod() = %v, want %v", podSpreadMap, tt.want)
+			}
+		})
+	}
+}
+
 var (
 	hardSpread = v1.DoNotSchedule
 	softSpread = v1.ScheduleAnyway

pkg/scheduler/algorithm/predicates/predicates.go

@@ -1722,7 +1722,6 @@ func EvenPodsSpreadPredicate(pod *v1.Pod, meta PredicateMetadata, nodeInfo *sche
 	if node == nil {
 		return false, nil, fmt.Errorf("node not found")
 	}
-
 	constraints := getHardTopologySpreadConstraints(pod)
 	if len(constraints) == 0 {
 		return true, nil, nil