Add unit test for determineBestPEGToFastpath and some fixes

Author: yonizxz

cluster-autoscaler/estimator/binpacking_estimator.go

@@ -109,13 +109,15 @@ func (e *BinpackingNodeEstimator) Estimate(
 
 	podsEquivalenceGroups = e.podOrderer.Order(podsEquivalenceGroups, nodeTemplate, nodeGroup)
 
+	useFastpathOnLastPEG := false
 	if e.fastpathBinpackingEnabled {
 		bestFastpathPEGindex := determineBestPEGToFastpath(podsEquivalenceGroups, nodeTemplate)
 		if bestFastpathPEGindex != -1 {
 			bestFastpathPEG := podsEquivalenceGroups[bestFastpathPEGindex]
 			pegsWithoutBestForFastpath := append(podsEquivalenceGroups[:bestFastpathPEGindex], podsEquivalenceGroups[bestFastpathPEGindex+1:]...)
 			// We'll put at the end the PEG which will benefit the most from fastpath binpacking, since it runs only on the last PEG
 			podsEquivalenceGroups = append(pegsWithoutBestForFastpath, bestFastpathPEG)
+			useFastpathOnLastPEG = true
 		}
 	}
 
@@ -137,8 +139,8 @@ func (e *BinpackingNodeEstimator) Estimate(
 		}
 
 		if newNodesAvailable {
-			// Since fastpath binpacking adds just one node to the snapshot, it will cause unaccurate simulations on subsequent loops, therefore we only use it on the last group
-			if e.fastpathBinpackingEnabled && i == len(podsEquivalenceGroups)-1 && shouldUseFastPath(remainingPods) {
+			// Since fastpath binpacking adds just one node to the snapshot, it will cause inaccurate simulations on subsequent loops, therefore we only use it on the last group
+			if i == len(podsEquivalenceGroups)-1 && useFastpathOnLastPEG {
 				newNodesAvailable, err = e.tryFastPath(estimationState, nodeTemplate, remainingPods)
 			} else {
 				newNodesAvailable, err = e.tryToScheduleOnNewNodes(estimationState, nodeTemplate, remainingPods)
@@ -264,6 +266,9 @@ func (e *BinpackingNodeEstimator) tryToScheduleOnNewNodes(
 	return true, nil
 }
 
+// An optimized version of tryToScheduleOnNewNodes
+// It attempts to pack as much pods as possible on a single node, and then estimates the total
+// amount of nodes by simple arithmetic: EstimatedNodes = ceil(TotalPods / PodsFittingOnASingleNode)
 func (e *BinpackingNodeEstimator) tryFastPath(
 	estimationState *estimationState,
 	nodeTemplate *framework.NodeInfo,
@@ -389,18 +394,6 @@ func observeBinpackingHeterogeneity(podsEquivalenceGroups []PodEquivalenceGroup,
 	metrics.ObserveBinpackingHeterogeneity(instanceType, cpuCount, nsCountBucket, len(podsEquivalenceGroups))
 }
 
-func hasNonHostnameTopologyConstraint(pod *apiv1.Pod) bool {
-	if pod.Spec.TopologySpreadConstraints == nil {
-		return false
-	}
-	for _, constraint := range pod.Spec.TopologySpreadConstraints {
-		if constraint.TopologyKey != apiv1.LabelHostname {
-			return true
-		}
-	}
-	return false
-}
-
 func hasNonHostnamePodAntiAffinity(pod *apiv1.Pod) bool {
 	if pod.Spec.Affinity == nil || pod.Spec.Affinity.PodAntiAffinity == nil || pod.Spec.Affinity.PodAntiAffinity.RequiredDuringSchedulingIgnoredDuringExecution == nil {
 		return false
@@ -418,15 +411,19 @@ func shouldUseFastPath(pods []*apiv1.Pod) bool {
 		return false
 	}
 
-	if hasNonHostnameTopologyConstraint(pods[0]) || hasNonHostnamePodAntiAffinity(pods[0]) {
+	// Fastpath assumes that once a pod failed to schedule on a node, it will never be possible to schedule any more pods on this node.
+	// This assumption is not correct in cases of topology spread constraints.
+	// Fastpath also assumes that if it was able to schedule pods on a new node, it will be able to then schedule an equal amount of pods on an additional identical new node,
+	// which isn't correct in cases of zonal/regional pod anti affinity.
+	if pods[0].Spec.TopologySpreadConstraints != nil || hasNonHostnamePodAntiAffinity(pods[0]) {
 		return false
 	}
 
 	return true
 }
 
 // determineBestPEGToFastpath returns the index of the pod equivalence group that would benefit the most from fastpath binpacking (= largest expected number of (nodes * pods))
-// returns -1 if it wasn't possible to determine such group
+// returns -1 if none of the groups support fastpath binpacking
 func determineBestPEGToFastpath(podsEquivalenceGroups []PodEquivalenceGroup, nodeTemplate *framework.NodeInfo) int {
 	maxNodesTimesPods := 0
 	bestPEGIndex := -1
@@ -445,8 +442,8 @@ func determineBestPEGToFastpath(podsEquivalenceGroups []PodEquivalenceGroup, nod
 				}
 			}
 		}
-		if peg.Exemplar().Spec.Resources != nil && peg.Exemplar().Spec.Resources.Requests != nil {
-			resourcesRequests := peg.Exemplar().Spec.Resources.Requests
+		if len(peg.Exemplar().Spec.Containers) > 0 && peg.Exemplar().Spec.Containers[0].Resources.Requests != nil {
+			resourcesRequests := peg.Exemplar().Spec.Containers[0].Resources.Requests
 			if resourcesRequests.Cpu() != nil {
 				numNodesByCpu = int(math.Ceil(float64(len(peg.Pods)) * resourcesRequests.Cpu().AsApproximateFloat64() / nodeTemplate.Node().Status.Capacity.Cpu().AsApproximateFloat64()))
 			}
@@ -455,7 +452,8 @@ func determineBestPEGToFastpath(podsEquivalenceGroups []PodEquivalenceGroup, nod
 			}
 		}
 		nodesTimesPods := max(numNodesByAntiAffinity, numNodesByCpu, numNodesByMemory) * len(peg.Pods)
-		if nodesTimesPods > maxNodesTimesPods && shouldUseFastPath(peg.Pods) {
+		// In case the (nodes*pods) score is equal, we still want to take the latest pod group to remain as close to the original pod ordering as possible
+		if nodesTimesPods >= maxNodesTimesPods && shouldUseFastPath(peg.Pods) {
 			bestPEGIndex = i
 			maxNodesTimesPods = nodesTimesPods
 		}

cluster-autoscaler/estimator/binpacking_estimator_test.go

@@ -312,3 +312,60 @@ func BenchmarkBinpackingEstimate(b *testing.B) {
 		assert.Equal(b, expectPodCount, len(estimatedPods))
 	}
 }
+
+func TestDetermineBestPodEquivalenceGroupToFastpath(t *testing.T) {
+	testCases := []struct {
+		name                  string
+		podsEquivalenceGroups []PodEquivalenceGroup
+		expectResult          int
+	}{
+		{
+			name: "both groups 1 node, take group with more pods",
+			podsEquivalenceGroups: []PodEquivalenceGroup{
+				makePodEquivalenceGroup(BuildTestPod("estimatee1", 5, 5), 4), // 1 node * 4 pods = 4
+				makePodEquivalenceGroup(BuildTestPod("estimatee2", 5, 6), 3), // 1 node * 3 pods = 3
+			},
+			expectResult: 0,
+		},
+		{
+			name: "take group with largest pods*nodes, even if it has less pods",
+			podsEquivalenceGroups: []PodEquivalenceGroup{
+				makePodEquivalenceGroup(BuildTestPod("estimatee1", 5, 5), 4),   // 1 node  * 4 pods = 4
+				makePodEquivalenceGroup(BuildTestPod("estimatee2", 80, 80), 3), // 3 nodes * 3 pods = 9
+			},
+			expectResult: 1,
+		},
+		{
+			name: "take group with largest pods*nodes, even if it has less nodes",
+			podsEquivalenceGroups: []PodEquivalenceGroup{
+				makePodEquivalenceGroup(BuildTestPod("estimatee1", 5, 5), 10),  // 1 node  * 10 pods = 10
+				makePodEquivalenceGroup(BuildTestPod("estimatee2", 80, 80), 3), // 3 nodes * 3  pods = 9
+			},
+			expectResult: 0,
+		},
+		{
+			name: "no fastpath supporting pod groups",
+			podsEquivalenceGroups: []PodEquivalenceGroup{
+				makePodEquivalenceGroup(BuildTestPod("estimatee1", 5, 5, WithMaxSkew(2, "kubernetes.io/hostname", 1)), 10),
+				makePodEquivalenceGroup(BuildTestPod("estimatee2", 8, 8, WithMaxSkew(2, "kubernetes.io/hostname", 1)), 3),
+			},
+			expectResult: -1,
+		},
+		{
+			name: "no resource requests defaults to last group, still valid fastpath group",
+			podsEquivalenceGroups: []PodEquivalenceGroup{
+				makePodEquivalenceGroup(&apiv1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "estimatee1"}, Spec: apiv1.PodSpec{Containers: []apiv1.Container{}}}, 2),
+				makePodEquivalenceGroup(&apiv1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "estimatee2"}, Spec: apiv1.PodSpec{Containers: []apiv1.Container{}}}, 4),
+			},
+			expectResult: 1,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			nodeInfo := framework.NewTestNodeInfo(makeNode(100, 100, 10, "oldnode", "zone-jupiter"))
+			result := determineBestPEGToFastpath(tc.podsEquivalenceGroups, nodeInfo)
+			assert.Equal(t, tc.expectResult, result)
+		})
+	}
+}