Merge pull request #3124 from JoelSpeed/memory-tolerance-quantity

Allow small tolerance on memory capacity when comparing nodegroups

cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go

@@ -31,9 +31,9 @@ const (
 	// MaxFreeDifferenceRatio describes how free resources (allocatable - daemon and system pods)
 	// can differ between groups in the same NodeGroupSet
 	MaxFreeDifferenceRatio = 0.05
-	// MaxMemoryDifferenceInKiloBytes describes how much memory
+	// MaxCapacityMemoryDifferenceRatio describes how Node.Status.Capacity.Memory can differ between
-	// capacity can differ but still be considered equal.
+	// groups in the same NodeGroupSet
-	MaxMemoryDifferenceInKiloBytes = 256000
+	MaxCapacityMemoryDifferenceRatio = 0.015
 )
 
 // BasicIgnoredLabels define a set of basic labels that should be ignored when comparing the similarity
@@ -53,19 +53,23 @@ var BasicIgnoredLabels = map[string]bool{
 // similar enough to be considered a part of a single NodeGroupSet.
 type NodeInfoComparator func(n1, n2 *schedulerframework.NodeInfo) bool
 
-func compareResourceMapsWithTolerance(resources map[apiv1.ResourceName][]resource.Quantity,
+func resourceMapsWithinTolerance(resources map[apiv1.ResourceName][]resource.Quantity,
 	maxDifferenceRatio float64) bool {
 	for _, qtyList := range resources {
+		if !resourceListWithinTolerance(qtyList, maxDifferenceRatio) {
+			return false
+		}
+	}
+	return true
+}
+
+func resourceListWithinTolerance(qtyList []resource.Quantity, maxDifferenceRatio float64) bool {
 	if len(qtyList) != 2 {
 		return false
 	}
 	larger := math.Max(float64(qtyList[0].MilliValue()), float64(qtyList[1].MilliValue()))
 	smaller := math.Min(float64(qtyList[0].MilliValue()), float64(qtyList[1].MilliValue()))
-		if larger-smaller > larger*maxDifferenceRatio {
+	return larger-smaller <= larger*maxDifferenceRatio
-			return false
-		}
-	}
-	return true
 }
 
 func compareLabels(nodes []*schedulerframework.NodeInfo, ignoredLabels map[string]bool) bool {
@@ -131,9 +135,7 @@ func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulerframework.NodeInfo, ignored
 		}
 		switch kind {
 		case apiv1.ResourceMemory:
-			// For memory capacity we allow a small tolerance
+			if !resourceListWithinTolerance(qtyList, MaxCapacityMemoryDifferenceRatio) {
-			memoryDifference := math.Abs(float64(qtyList[0].Value()) - float64(qtyList[1].Value()))
-			if memoryDifference > MaxMemoryDifferenceInKiloBytes {
 				return false
 			}
 		default:
@@ -147,10 +149,10 @@ func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulerframework.NodeInfo, ignored
 	}
 
 	// For allocatable and free we allow resource quantities to be within a few % of each other
-	if !compareResourceMapsWithTolerance(allocatable, MaxAllocatableDifferenceRatio) {
+	if !resourceMapsWithinTolerance(allocatable, MaxAllocatableDifferenceRatio) {
 		return false
 	}
-	if !compareResourceMapsWithTolerance(free, MaxFreeDifferenceRatio) {
+	if !resourceMapsWithinTolerance(free, MaxFreeDifferenceRatio) {
 		return false
 	}
 

cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go

@@ -101,16 +101,60 @@ func TestNodesSimilarVariousRequirementsAndPods(t *testing.T) {
 
 func TestNodesSimilarVariousMemoryRequirements(t *testing.T) {
 	comparator := CreateGenericNodeInfoComparator([]string{})
-	n1 := BuildTestNode("node1", 1000, MaxMemoryDifferenceInKiloBytes)
+	n1 := BuildTestNode("node1", 1000, 1000)
 
 	// Different memory capacity within tolerance
-	n2 := BuildTestNode("node2", 1000, MaxMemoryDifferenceInKiloBytes)
+	n2 := BuildTestNode("node2", 1000, 1000)
-	n2.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifferenceInKiloBytes, resource.DecimalSI)
+	n2.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(1000-(1000*MaxCapacityMemoryDifferenceRatio)+1, resource.DecimalSI)
 	checkNodesSimilar(t, n1, n2, comparator, true)
 
 	// Different memory capacity exceeds tolerance
-	n3 := BuildTestNode("node3", 1000, MaxMemoryDifferenceInKiloBytes)
+	n3 := BuildTestNode("node3", 1000, 1000)
-	n3.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifferenceInKiloBytes+1, resource.DecimalSI)
+	n3.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(1000-(1000*MaxCapacityMemoryDifferenceRatio)-1, resource.DecimalSI)
+	checkNodesSimilar(t, n1, n3, comparator, false)
+}
+
+func TestNodesSimilarVariousLargeMemoryRequirementsM5XLarge(t *testing.T) {
+	comparator := CreateGenericNodeInfoComparator([]string{})
+
+	// Use realistic memory capacity (taken from real nodes)
+	// 15944120 KB ~= 16GiB (m5.xLarge)
+	q1 := resource.MustParse("16116152Ki")
+	q2 := resource.MustParse("15944120Ki")
+
+	n1 := BuildTestNode("node1", 1000, q1.Value())
+
+	// Different memory capacity within tolerance
+	// Value taken from another m5.xLarge in a different zone
+	n2 := BuildTestNode("node2", 1000, q2.Value())
+	checkNodesSimilar(t, n1, n2, comparator, true)
+
+	// Different memory capacity exceeds tolerance
+	// Value of q1 * 1.02
+	q3 := resource.MustParse("16438475Ki")
+	n3 := BuildTestNode("node3", 1000, q3.Value())
+	checkNodesSimilar(t, n1, n3, comparator, false)
+}
+
+func TestNodesSimilarVariousLargeMemoryRequirementsM516XLarge(t *testing.T) {
+	comparator := CreateGenericNodeInfoComparator([]string{})
+
+	// Use realistic memory capacity (taken from real nodes)
+	// 257217528 KB ~= 256GiB (m5.16xLarge)
+	q1 := resource.MustParse("259970052Ki")
+	q2 := resource.MustParse("257217528Ki")
+
+	n1 := BuildTestNode("node1", 1000, q1.Value())
+
+	// Different memory capacity within tolerance
+	// Value taken from another m5.xLarge in a different zone
+	n2 := BuildTestNode("node2", 1000, q2.Value())
+	checkNodesSimilar(t, n1, n2, comparator, true)
+
+	// Different memory capacity exceeds tolerance
+	// Value of q1 * 1.02
+	q3 := resource.MustParse("265169453Ki")
+	n3 := BuildTestNode("node3", 1000, q3.Value())
 	checkNodesSimilar(t, n1, n3, comparator, false)
 }