Fix CreateGenericNodeInfoLabelComparator and added tests.

cluster-autoscaler/main.go

@@ -196,6 +196,11 @@ func createAutoscalingOptions() config.AutoscalingOptions {
 	if err != nil {
 		klog.Fatalf("Failed to parse flags: %v", err)
 	}
+
+	err = validateBalancingLabelFlags(*balancingLabelsFlag, *balancingIgnoreLabelsFlag)
+	if err != nil {
+		klog.Fatalf("Failed to parse flags: %v", err)
+	}
 	return config.AutoscalingOptions{
 		CloudConfig:                        *cloudConfig,
 		CloudProviderName:                  *cloudProviderFlag,
@@ -548,3 +553,10 @@ func parseSingleGpuLimit(limits string) (config.GpuLimits, error) {
 	}
 	return parsedGpuLimits, nil
 }
+
+func validateBalancingLabelFlags(balancingLabelsFlag, balancingIgnoreLabelsFlag []string) error {
+	if len(balancingLabelsFlag) > 0 && len(balancingIgnoreLabelsFlag) > 0 {
+		return fmt.Errorf("cannot set --balancing-labels and --balancing-ignored-labels at the same time")
+	}
+	return nil
+}

cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go

@@ -78,7 +78,7 @@ func compareLabels(nodes []*schedulerframework.NodeInfo, explicitLabels, ignored
 		for label, value := range node.Node().ObjectMeta.Labels {
 			ignore, _ := ignoredLabels[label]
 			include, _ := explicitLabels[label]
-			if ( include || !ignore ) {
+			if ( include || ( len(explicitLabels) == 0 && !ignore ) ) {
 				labels[label] = append(labels[label], value)
 			}
 		}
@@ -91,44 +91,10 @@ func compareLabels(nodes []*schedulerframework.NodeInfo, explicitLabels, ignored
 	return true
 }
 
-// CreateGenericNodeInfoComparator returns a generic comparator that checks for node group similarity
-func CreateGenericNodeInfoComparator(extraIgnoredLabels []string) NodeInfoComparator {
-	genericIgnoredLabels := make(map[string]bool)
-	for k, v := range BasicIgnoredLabels {
-		genericIgnoredLabels[k] = v
-	}
-	for _, k := range extraIgnoredLabels {
-		genericIgnoredLabels[k] = true
-	}
-
-	return func(n1, n2 *schedulerframework.NodeInfo) bool {
-		return IsCloudProviderNodeInfoSimilar(n1, n2, genericIgnoredLabels)
-	}
-}
-
-func CreateGenericNodeInfoLabelComparator(labels []string) NodeInfoComparator {
-	return func(n1, n2 *schedulerframework.NodeInfo) bool {
-		includedLabels := make(map[string]bool)
-		for _, l := range labels {
-			includedLabels[l] = true
-		}
-		if !compareLabels([]*schedulerframework.NodeInfo{n1, n2}, includedLabels, make(map[string]bool)) {
-			return false
-		}
-		return IsCloudProviderNodeInfoSimilar(n1, n2, make(map[string]bool))
-	}
-}
-
-// 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 set of labels passed in to be ignored like hostname or zone).
-func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulerframework.NodeInfo, ignoredLabels map[string]bool) bool {
+func isCloudProviderNodeTypeSimilar(nodes []*schedulerframework.NodeInfo) bool {
 	capacity := make(map[apiv1.ResourceName][]resource.Quantity)
 	allocatable := make(map[apiv1.ResourceName][]resource.Quantity)
 	free := make(map[apiv1.ResourceName][]resource.Quantity)
-	nodes := []*schedulerframework.NodeInfo{n1, n2}
 	for _, node := range nodes {
 		for res, quantity := range node.Node().Status.Capacity {
 			capacity[res] = append(capacity[res], quantity)
@@ -170,6 +136,54 @@ func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulerframework.NodeInfo, ignored
 		return false
 	}
 
+	return true
+}
+
+// CreateGenericNodeInfoComparator returns a generic comparator that checks for node group similarity
+func CreateGenericNodeInfoComparator(extraIgnoredLabels []string) NodeInfoComparator {
+	genericIgnoredLabels := make(map[string]bool)
+	for k, v := range BasicIgnoredLabels {
+		genericIgnoredLabels[k] = v
+	}
+	for _, k := range extraIgnoredLabels {
+		genericIgnoredLabels[k] = true
+	}
+
+	return func(n1, n2 *schedulerframework.NodeInfo) bool {
+		return IsCloudProviderNodeInfoSimilar(n1, n2, genericIgnoredLabels)
+	}
+}
+
+// CreateGenericNodeInfoLabelComparator returns a generic comparator that checks for node with matching labels alongside node group similarity
+func CreateGenericNodeInfoLabelComparator(labels []string) NodeInfoComparator {
+	return func(n1, n2 *schedulerframework.NodeInfo) bool {
+		includedLabels := make(map[string]bool)
+		for _, l := range labels {
+			includedLabels[l] = true
+		}
+
+		if !compareLabels([]*schedulerframework.NodeInfo{n1, n2}, includedLabels, make(map[string]bool)) {
+			return false
+		}
+
+		if !isCloudProviderNodeTypeSimilar([]*schedulerframework.NodeInfo{n1, n2}) {
+			return false
+		}
+		return true
+	}
+}
+
+// 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 set of labels passed in to be ignored like hostname or zone).
+func IsCloudProviderNodeInfoSimilar(n1, n2 *schedulerframework.NodeInfo, ignoredLabels map[string]bool) bool {
+	nodes := []*schedulerframework.NodeInfo{n1, n2}
+
+	if !isCloudProviderNodeTypeSimilar(nodes) {
+		return false
+	}
 	if !compareLabels(nodes, make(map[string]bool), ignoredLabels) {
 		return false
 	}

cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go

@@ -197,3 +197,32 @@ func TestNodesSimilarVariousLabels(t *testing.T) {
 	n2.ObjectMeta.Labels["example.com/ready"] = "false"
 	checkNodesSimilar(t, n1, n2, comparator, true)
 }
+
+func TestNodesWithVariousExplicitLabels(t *testing.T) {
+	comparator := CreateGenericNodeInfoLabelComparator([]string{"test-label", "foo"})
+	n1 := BuildTestNode("node1", 1000, 2000)
+	n1.ObjectMeta.Labels["test-label"] = "test-value"
+	n1.ObjectMeta.Labels["character"] = "winnie the pooh"
+
+	n2 := BuildTestNode("node2", 1000, 2000)
+	n2.ObjectMeta.Labels["test-label"] = "test-value"
+
+	n3 := BuildTestNode("node3", 1000, 1000)
+	n3.ObjectMeta.Labels["test-label"] = "test-value"
+
+	// Matching explicit balancing labels
+	checkNodesSimilar(t, n1, n2, comparator, true)
+
+	// Matching explicit balancing labels but dissimilar nodes
+	checkNodesSimilar(t, n1, n3, comparator, false)
+
+	n2.ObjectMeta.Labels["test-label"] = "something-else"
+
+	// Mismatch in one of explicit balancing labels in the nodes
+	checkNodesSimilar(t, n1, n2, comparator, false)
+
+	// Balancing label present on one node, but missing on another
+	comparator = CreateGenericNodeInfoLabelComparator([]string{"test-label", "character", "foo"})
+
+	checkNodesSimilar(t, n1, n2, comparator, false)
+}