From d26853fb66fcf49b12ed04102af28691641814f1 Mon Sep 17 00:00:00 2001
From: Joel Speed <joel.speed@hotmail.co.uk>
Date: Thu, 7 May 2020 18:03:29 +0100
Subject: [PATCH] UPSTREAM: 3124: openshift: Use quantities for memory capacity
 differences

This allows developers to better interpet how the calculations are being
done by leaving the values as "Quantities". For example, the max
difference is now a string converted to a quantity which will be easier
to reason about and update if needed in the future.
Also adds tests that match real values from a real set of nodes that
would be expected to be the same
---
 .../nodegroupset/compare_nodegroups.go        | 27 +++++++++++++---
 .../nodegroupset/compare_nodegroups_test.go   | 32 ++++++++++++++++---
 2 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go b/cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go
index 5623bcfbed5..e6d89570d01 100644
--- a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go
+++ b/cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go
@@ -31,9 +31,11 @@ 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
-	// capacity can differ but still be considered equal.
-	MaxMemoryDifferenceInKiloBytes = 256000
+)
+
+var (
+	// MaxMemoryDifference describes how much memory capacity can differ but still be considered equal.
+	MaxMemoryDifference = resource.MustParse("256Mi")
 )
 
 // BasicIgnoredLabels define a set of basic labels that should be ignored when comparing the similarity
@@ -124,8 +126,10 @@ func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo, ignoredL
 		switch kind {
 		case apiv1.ResourceMemory:
 			// For memory capacity we allow a small tolerance
-			memoryDifference := math.Abs(float64(qtyList[0].Value()) - float64(qtyList[1].Value()))
-			if memoryDifference > MaxMemoryDifferenceInKiloBytes {
+			difference := absSub(qtyList[0], qtyList[1])
+
+			// Cmp compares two quantities and returns -1 for less than, 0 for equal, 1 for greater than
+			if difference.Cmp(MaxMemoryDifference) > 0 {
 				return false
 			}
 		default:
@@ -152,3 +156,16 @@ func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo, ignoredL
 
 	return true
 }
+
+// absSub returns the absolute value of a - b
+func absSub(a, b resource.Quantity) *resource.Quantity {
+	q := &resource.Quantity{}
+	a.DeepCopyInto(q)
+	q.Sub(b)
+
+	if q.Sign() == -1 {
+		q.Neg()
+	}
+
+	return q
+}
diff --git a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go b/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go
index b36ed92b98d..e032829677b 100644
--- a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go
+++ b/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go
@@ -97,16 +97,38 @@ func TestNodesSimilarVariousRequirementsAndPods(t *testing.T) {
 }
 
 func TestNodesSimilarVariousMemoryRequirements(t *testing.T) {
-	n1 := BuildTestNode("node1", 1000, MaxMemoryDifferenceInKiloBytes)
+	n1 := BuildTestNode("node1", 1000, MaxMemoryDifference.Value())
 
 	// Different memory capacity within tolerance
-	n2 := BuildTestNode("node2", 1000, MaxMemoryDifferenceInKiloBytes)
-	n2.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifferenceInKiloBytes, resource.DecimalSI)
+	n2 := BuildTestNode("node2", 1000, MaxMemoryDifference.Value())
+	n2.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifference.Value(), resource.DecimalSI)
 	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
 
 	// Different memory capacity exceeds tolerance
-	n3 := BuildTestNode("node3", 1000, MaxMemoryDifferenceInKiloBytes)
-	n3.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifferenceInKiloBytes+1, resource.DecimalSI)
+	n3 := BuildTestNode("node3", 1000, MaxMemoryDifference.Value())
+	n3.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifference.Value()+1, resource.DecimalSI)
+	checkNodesSimilar(t, n1, n3, IsNodeInfoSimilar, false)
+}
+
+func TestNodesSimilarVariousLargeMemoryRequirements(t *testing.T) {
+	// 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, IsNodeInfoSimilar, true)
+
+	// Different memory capacity exceeds tolerance
+	// Same value as q1 but with MaxMemoryDifference added twice
+	q3 := resource.MustParse("16116152Ki")
+	q3.Add(MaxMemoryDifference)
+	q3.Add(MaxMemoryDifference)
+	n3 := BuildTestNode("node3", 1000, q3.Value())
 	checkNodesSimilar(t, n1, n3, IsNodeInfoSimilar, false)
 }