Use builder methods to create NodeInfoComparator functions

Author: adammw

cluster-autoscaler/main.go

@@ -293,10 +293,10 @@ func buildAutoscaler() (core.Autoscaler, error) {
 	processors.PodListProcessor = core.NewFilterOutSchedulablePodListProcessor()
 	if autoscalingOptions.CloudProviderName == cloudprovider.AzureProviderName {
 		processors.NodeGroupSetProcessor = &nodegroupset.BalancingNodeGroupSetProcessor{
-			Comparator: nodegroupset.IsAzureNodeInfoSimilar}
+			Comparator: nodegroupset.CreateAzureNodeInfoComparator()}
 	} else if autoscalingOptions.CloudProviderName == cloudprovider.AwsProviderName {
 		processors.NodeGroupSetProcessor = &nodegroupset.BalancingNodeGroupSetProcessor{
-			Comparator: nodegroupset.IsAwsNodeInfoSimilar}
+			Comparator: nodegroupset.CreateAwsNodeInfoComparator()}
 	}
 
 	opts := core.AutoscalerOptions{

cluster-autoscaler/processors/nodegroupset/aws_nodegroups.go

@@ -20,8 +20,7 @@ import (
 	schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
 )
 
-// IsAwsNodeInfoSimilar adds AWS specific node labels to the list of ignored labels.
-func IsAwsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
+func CreateAwsNodeInfoComparator() NodeInfoComparator {
 	awsIgnoredLabels := map[string]bool{
 		"alpha.eksctl.io/instance-id":    true, // this is a label used by eksctl to identify instances.
 		"alpha.eksctl.io/nodegroup-name": true, // this is a label used by eksctl to identify "node group" names.
@@ -33,5 +32,8 @@ func IsAwsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
 	for k, v := range BasicIgnoredLabels {
 		awsIgnoredLabels[k] = v
 	}
-	return IsCloudProviderNodeInfoSimilar(n1, n2, awsIgnoredLabels)
+
+	return func(n1, n2 *schedulernodeinfo.NodeInfo) bool {
+		return IsCloudProviderNodeInfoSimilar(n1, n2, awsIgnoredLabels)
+	}
 }

cluster-autoscaler/processors/nodegroupset/azure_nodegroups.go

@@ -29,17 +29,20 @@ func nodesFromSameAzureNodePool(n1, n2 *schedulernodeinfo.NodeInfo) bool {
 	return n1AzureNodePool != "" && n1AzureNodePool == n2AzureNodePool
 }
 
-// IsAzureNodeInfoSimilar compares if two nodes should be considered part of the
+// Returned NodeInfoComparator compares if two nodes should be considered part of the
 // same NodeGroupSet. This is true if they either belong to the same Azure agentpool
-// or match usual conditions checked by IsAzureNodeInfoSimilar, even if they have different agentpool labels.
-func IsAzureNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
-	if nodesFromSameAzureNodePool(n1, n2) {
-		return true
-	}
+// or match usual conditions checked by IsCloudProviderNodeInfoSimilar, even if they have different agentpool labels.
+func CreateAzureNodeInfoComparator() NodeInfoComparator {
 	azureIgnoredLabels := make(map[string]bool)
 	for k, v := range BasicIgnoredLabels {
 		azureIgnoredLabels[k] = v
 	}
 	azureIgnoredLabels[AzureNodepoolLabel] = true
-	return IsCloudProviderNodeInfoSimilar(n1, n2, azureIgnoredLabels)
+
+	return func(n1, n2 *schedulernodeinfo.NodeInfo) bool {
+		if nodesFromSameAzureNodePool(n1, n2) {
+			return true
+		}
+		return IsCloudProviderNodeInfoSimilar(n1, n2, azureIgnoredLabels)
+	}
 }

cluster-autoscaler/processors/nodegroupset/azure_nodegroups_test.go

@@ -29,47 +29,48 @@ import (
 )
 
 func TestIsAzureNodeInfoSimilar(t *testing.T) {
+	comparator := CreateAzureNodeInfoComparator()
 	n1 := BuildTestNode("node1", 1000, 2000)
 	n1.ObjectMeta.Labels["test-label"] = "test-value"
 	n1.ObjectMeta.Labels["character"] = "thing"
 	n2 := BuildTestNode("node2", 1000, 2000)
 	n2.ObjectMeta.Labels["test-label"] = "test-value"
 	// No node-pool labels.
-	checkNodesSimilar(t, n1, n2, IsAzureNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n2, comparator, false)
 	// Empty agentpool labels
 	n1.ObjectMeta.Labels["agentpool"] = ""
 	n2.ObjectMeta.Labels["agentpool"] = ""
-	checkNodesSimilar(t, n1, n2, IsAzureNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n2, comparator, false)
 	// Only one non empty
 	n1.ObjectMeta.Labels["agentpool"] = ""
 	n2.ObjectMeta.Labels["agentpool"] = "foo"
-	checkNodesSimilar(t, n1, n2, IsAzureNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n2, comparator, false)
 	// Only one present
 	delete(n1.ObjectMeta.Labels, "agentpool")
 	n2.ObjectMeta.Labels["agentpool"] = "foo"
-	checkNodesSimilar(t, n1, n2, IsAzureNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n2, comparator, false)
 	// Different vales
 	n1.ObjectMeta.Labels["agentpool"] = "foo1"
 	n2.ObjectMeta.Labels["agentpool"] = "foo2"
-	checkNodesSimilar(t, n1, n2, IsAzureNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n2, comparator, false)
 	// Same values
 	n1.ObjectMeta.Labels["agentpool"] = "foo"
 	n2.ObjectMeta.Labels["agentpool"] = "foo"
-	checkNodesSimilar(t, n1, n2, IsAzureNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 	// Same labels except for agentpool
 	delete(n1.ObjectMeta.Labels, "character")
 	n1.ObjectMeta.Labels["agentpool"] = "foo"
 	n2.ObjectMeta.Labels["agentpool"] = "bar"
-	checkNodesSimilar(t, n1, n2, IsAzureNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 }
 
 func TestFindSimilarNodeGroupsAzureBasic(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{Comparator: IsAzureNodeInfoSimilar}
+	processor := &BalancingNodeGroupSetProcessor{Comparator: CreateAzureNodeInfoComparator()}
 	basicSimilarNodeGroupsTest(t, processor)
 }
 
 func TestFindSimilarNodeGroupsAzureByLabel(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{Comparator: IsAzureNodeInfoSimilar}
+	processor := &BalancingNodeGroupSetProcessor{Comparator: CreateAzureNodeInfoComparator()}
 	context := &context.AutoscalingContext{}
 
 	n1 := BuildTestNode("n1", 1000, 1000)

cluster-autoscaler/processors/nodegroupset/balancing_processor.go

@@ -32,8 +32,8 @@ type BalancingNodeGroupSetProcessor struct {
 	Comparator NodeInfoComparator
 }
 
-// FindSimilarNodeGroups returns a list of NodeGroups similar to the given one.
-// Two groups are similar if the NodeInfos for them compare equal using IsNodeInfoSimilar.
+// FindSimilarNodeGroups returns a list of NodeGroups similar to the given one using the
+// BalancingNodeGroupSetProcessor's comparator function.
 func (b *BalancingNodeGroupSetProcessor) FindSimilarNodeGroups(context *context.AutoscalingContext, nodeGroup cloudprovider.NodeGroup,
 	nodeInfosForGroups map[string]*schedulernodeinfo.NodeInfo) ([]cloudprovider.NodeGroup, errors.AutoscalerError) {
 
@@ -58,7 +58,7 @@ func (b *BalancingNodeGroupSetProcessor) FindSimilarNodeGroups(context *context.
 		}
 		comparator := b.Comparator
 		if comparator == nil {
-			comparator = IsNodeInfoSimilar
+			panic("BalancingNodeGroupSetProcessor comparator not set")
 		}
 		if comparator(nodeInfo, ngNodeInfo) {
 			result = append(result, ng)

cluster-autoscaler/processors/nodegroupset/balancing_processor_test.go

@@ -72,12 +72,12 @@ func basicSimilarNodeGroupsTest(t *testing.T, processor NodeGroupSetProcessor) {
 }
 
 func TestFindSimilarNodeGroups(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
+	processor := NewDefaultNodeGroupSetProcessor()
 	basicSimilarNodeGroupsTest(t, processor)
 }
 
 func TestBalanceSingleGroup(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
+	processor := NewDefaultNodeGroupSetProcessor()
 	context := &context.AutoscalingContext{}
 
 	provider := testprovider.NewTestCloudProvider(nil, nil)
@@ -97,7 +97,7 @@ func TestBalanceSingleGroup(t *testing.T) {
 }
 
 func TestBalanceUnderMaxSize(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
+	processor := NewDefaultNodeGroupSetProcessor()
 	context := &context.AutoscalingContext{}
 
 	provider := testprovider.NewTestCloudProvider(nil, nil)
@@ -147,7 +147,7 @@ func TestBalanceUnderMaxSize(t *testing.T) {
 }
 
 func TestBalanceHittingMaxSize(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
+	processor := NewDefaultNodeGroupSetProcessor()
 	context := &context.AutoscalingContext{}
 
 	provider := testprovider.NewTestCloudProvider(nil, nil)

cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go

@@ -85,18 +85,19 @@ func compareLabels(nodes []*schedulernodeinfo.NodeInfo, ignoredLabels map[string
 	}
 	return true
 }
+// CreateGenericNodeInfoComparator returns a generic comparator that checks for node group similarity
+// based on a standard set of widely-applicable ignore labels
+func CreateGenericNodeInfoComparator() NodeInfoComparator {
+	return func(n1, n2 *schedulernodeinfo.NodeInfo) bool {
+		return IsCloudProviderNodeInfoSimilar(n1, n2, BasicIgnoredLabels)
+	}
+}
 
-// IsNodeInfoSimilar returns true if two NodeInfos are similar enough to consider
+// IsCloudProviderNodeInfoSimilar returns true if two NodeInfos are similar enough to consider
 // that the NodeGroups they come from are part of the same NodeGroupSet. The criteria are
 // somewhat arbitrary, but generally we check if resources provided by both nodes
 // are similar enough to likely be the same type of machine and if the set of labels
-// is the same (except for a pre-defined set of labels like hostname or zone).
-func IsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
-	return IsCloudProviderNodeInfoSimilar(n1, n2, BasicIgnoredLabels)
-}
-
-// IsCloudProviderNodeInfoSimilar remains the same logic of IsNodeInfoSimilar with the
-// customized set of labels that should be ignored when comparing the similarity of two NodeInfos.
+// is the same (except for a set of labels passed in to be ignored like hostname or zone).
 func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo, ignoredLabels map[string]bool) bool {
 	capacity := make(map[apiv1.ResourceName][]resource.Quantity)
 	allocatable := make(map[apiv1.ResourceName][]resource.Quantity)

cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go

@@ -41,37 +41,40 @@ func checkNodesSimilarWithPods(t *testing.T, n1, n2 *apiv1.Node, pods1, pods2 []
 }
 
 func TestIdenticalNodesSimilar(t *testing.T) {
+	comparator := CreateGenericNodeInfoComparator()
 	n1 := BuildTestNode("node1", 1000, 2000)
 	n2 := BuildTestNode("node2", 1000, 2000)
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 }
 
 func TestNodesSimilarVariousRequirements(t *testing.T) {
+	comparator := CreateGenericNodeInfoComparator()
 	n1 := BuildTestNode("node1", 1000, 2000)
 
 	// Different CPU capacity
 	n2 := BuildTestNode("node2", 1000, 2000)
 	n2.Status.Capacity[apiv1.ResourceCPU] = *resource.NewMilliQuantity(1001, resource.DecimalSI)
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n2, comparator, false)
 
 	// Same CPU capacity, but slightly different allocatable
 	n3 := BuildTestNode("node3", 1000, 2000)
 	n3.Status.Allocatable[apiv1.ResourceCPU] = *resource.NewMilliQuantity(999, resource.DecimalSI)
-	checkNodesSimilar(t, n1, n3, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n3, comparator, true)
 
 	// Same CPU capacity, significantly different allocatable
 	n4 := BuildTestNode("node4", 1000, 2000)
 	n4.Status.Allocatable[apiv1.ResourceCPU] = *resource.NewMilliQuantity(500, resource.DecimalSI)
-	checkNodesSimilar(t, n1, n4, IsNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n4, comparator, false)
 
 	// One with GPU, one without
 	n5 := BuildTestNode("node5", 1000, 2000)
 	n5.Status.Capacity[gpu.ResourceNvidiaGPU] = *resource.NewQuantity(1, resource.DecimalSI)
 	n5.Status.Allocatable[gpu.ResourceNvidiaGPU] = n5.Status.Capacity[gpu.ResourceNvidiaGPU]
-	checkNodesSimilar(t, n1, n5, IsNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n5, comparator, false)
 }
 
 func TestNodesSimilarVariousRequirementsAndPods(t *testing.T) {
+	comparator := CreateGenericNodeInfoComparator()
 	n1 := BuildTestNode("node1", 1000, 2000)
 	p1 := BuildTestPod("pod1", 500, 1000)
 	p1.Spec.NodeName = "node1"
@@ -80,37 +83,39 @@ func TestNodesSimilarVariousRequirementsAndPods(t *testing.T) {
 	n2 := BuildTestNode("node2", 1000, 2000)
 	n2.Status.Allocatable[apiv1.ResourceCPU] = *resource.NewMilliQuantity(500, resource.DecimalSI)
 	n2.Status.Allocatable[apiv1.ResourceMemory] = *resource.NewQuantity(1000, resource.DecimalSI)
-	checkNodesSimilarWithPods(t, n1, n2, []*apiv1.Pod{p1}, []*apiv1.Pod{}, IsNodeInfoSimilar, false)
+	checkNodesSimilarWithPods(t, n1, n2, []*apiv1.Pod{p1}, []*apiv1.Pod{}, comparator, false)
 
 	// Same requests of pods
 	n3 := BuildTestNode("node3", 1000, 2000)
 	p3 := BuildTestPod("pod3", 500, 1000)
 	p3.Spec.NodeName = "node3"
-	checkNodesSimilarWithPods(t, n1, n3, []*apiv1.Pod{p1}, []*apiv1.Pod{p3}, IsNodeInfoSimilar, true)
+	checkNodesSimilarWithPods(t, n1, n3, []*apiv1.Pod{p1}, []*apiv1.Pod{p3}, comparator, true)
 
 	// Similar allocatable, similar pods
 	n4 := BuildTestNode("node4", 1000, 2000)
 	n4.Status.Allocatable[apiv1.ResourceCPU] = *resource.NewMilliQuantity(999, resource.DecimalSI)
 	p4 := BuildTestPod("pod4", 501, 1001)
 	p4.Spec.NodeName = "node4"
-	checkNodesSimilarWithPods(t, n1, n4, []*apiv1.Pod{p1}, []*apiv1.Pod{p4}, IsNodeInfoSimilar, true)
+	checkNodesSimilarWithPods(t, n1, n4, []*apiv1.Pod{p1}, []*apiv1.Pod{p4}, comparator, true)
 }
 
 func TestNodesSimilarVariousMemoryRequirements(t *testing.T) {
+	comparator := CreateGenericNodeInfoComparator()
 	n1 := BuildTestNode("node1", 1000, MaxMemoryDifferenceInKiloBytes)
 
 	// Different memory capacity within tolerance
 	n2 := BuildTestNode("node2", 1000, MaxMemoryDifferenceInKiloBytes)
 	n2.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifferenceInKiloBytes, resource.DecimalSI)
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 
 	// Different memory capacity exceeds tolerance
 	n3 := BuildTestNode("node3", 1000, MaxMemoryDifferenceInKiloBytes)
 	n3.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(2*MaxMemoryDifferenceInKiloBytes+1, resource.DecimalSI)
-	checkNodesSimilar(t, n1, n3, IsNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n3, comparator, false)
 }
 
 func TestNodesSimilarVariousLabels(t *testing.T) {
+	comparator := CreateGenericNodeInfoComparator()
 	n1 := BuildTestNode("node1", 1000, 2000)
 	n1.ObjectMeta.Labels["test-label"] = "test-value"
 	n1.ObjectMeta.Labels["character"] = "winnie the pooh"
@@ -119,27 +124,27 @@ func TestNodesSimilarVariousLabels(t *testing.T) {
 	n2.ObjectMeta.Labels["test-label"] = "test-value"
 
 	// Missing character label
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, false)
+	checkNodesSimilar(t, n1, n2, comparator, false)
 
 	n2.ObjectMeta.Labels["character"] = "winnie the pooh"
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 
 	// Different hostname labels shouldn't matter
 	n1.ObjectMeta.Labels[apiv1.LabelHostname] = "node1"
 	n2.ObjectMeta.Labels[apiv1.LabelHostname] = "node2"
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 
 	// Different zone shouldn't matter either
 	n1.ObjectMeta.Labels[apiv1.LabelZoneFailureDomain] = "mars-olympus-mons1-b"
 	n2.ObjectMeta.Labels[apiv1.LabelZoneFailureDomain] = "us-houston1-a"
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 
 	// Different beta.kubernetes.io/fluentd-ds-ready should not matter
 	n1.ObjectMeta.Labels["beta.kubernetes.io/fluentd-ds-ready"] = "true"
 	n2.ObjectMeta.Labels["beta.kubernetes.io/fluentd-ds-ready"] = "false"
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 
 	n1.ObjectMeta.Labels["beta.kubernetes.io/fluentd-ds-ready"] = "true"
 	delete(n2.ObjectMeta.Labels, "beta.kubernetes.io/fluentd-ds-ready")
-	checkNodesSimilar(t, n1, n2, IsNodeInfoSimilar, true)
+	checkNodesSimilar(t, n1, n2, comparator, true)
 }

cluster-autoscaler/processors/nodegroupset/nodegroup_set_processor.go

@@ -71,5 +71,7 @@ func (n *NoOpNodeGroupSetProcessor) CleanUp() {}
 
 // NewDefaultNodeGroupSetProcessor creates an instance of NodeGroupSetProcessor.
 func NewDefaultNodeGroupSetProcessor() NodeGroupSetProcessor {
-	return &BalancingNodeGroupSetProcessor{}
+	return &BalancingNodeGroupSetProcessor{
+		Comparator: CreateGenericNodeInfoComparator(),
+	}
 }