Merge pull request kubernetes#2493 from adammw/adammw/custom-ignored-labels

Add support for passing custom ignored labels

Author: k8s-ci-robot

cluster-autoscaler/config/autoscaling_options.go

@@ -135,6 +135,9 @@ type AutoscalingOptions struct {
 	MaxBulkSoftTaintTime time.Duration
 	// IgnoredTaints is a list of taints to ignore when considering a node template for scheduling.
 	IgnoredTaints []string
+	// BalancingExtraIgnoredLabels is a list of labels to additionally ignore when comparing if two node groups are similar.
+	// Labels in BasicIgnoredLabels and the cloud provider-specific ignored labels are always ignored.
+	BalancingExtraIgnoredLabels []string
 	// AWSUseStaticInstanceList tells if AWS cloud provider use static instance type list or dynamically fetch from remote APIs.
 	AWSUseStaticInstanceList bool
 	// Path to kube configuration if available

cluster-autoscaler/core/scale_test_common.go

@@ -132,7 +132,7 @@ func NewTestProcessors() *processors.AutoscalingProcessors {
 	return &processors.AutoscalingProcessors{
 		PodListProcessor:       NewFilterOutSchedulablePodListProcessor(),
 		NodeGroupListProcessor: &nodegroups.NoOpNodeGroupListProcessor{},
-		NodeGroupSetProcessor:  &nodegroupset.BalancingNodeGroupSetProcessor{},
+		NodeGroupSetProcessor:  nodegroupset.NewDefaultNodeGroupSetProcessor([]string{}),
 		// TODO(bskiba): change scale up test so that this can be a NoOpProcessor
 		ScaleUpStatusProcessor:     &status.EventingScaleUpStatusProcessor{},
 		ScaleDownStatusProcessor:   &status.NoOpScaleDownStatusProcessor{},

cluster-autoscaler/main.go

@@ -168,9 +168,10 @@ var (
 	regional                      = flag.Bool("regional", false, "Cluster is regional.")
 	newPodScaleUpDelay            = flag.Duration("new-pod-scale-up-delay", 0*time.Second, "Pods less than this old will not be considered for scale-up.")
 
-	ignoreTaintsFlag         = multiStringFlag("ignore-taint", "Specifies a taint to ignore in node templates when considering to scale a node group")
-	awsUseStaticInstanceList = flag.Bool("aws-use-static-instance-list", false, "Should CA fetch instance types in runtime or use a static list. AWS only")
-	enableProfiling          = flag.Bool("profiling", false, "Is debug/pprof endpoint enabled")
+	ignoreTaintsFlag          = multiStringFlag("ignore-taint", "Specifies a taint to ignore in node templates when considering to scale a node group")
+	balancingIgnoreLabelsFlag = multiStringFlag("balancing-ignore-label", "Specifies a label to ignore in addition to the basic and cloud-provider set of labels when comparing if two node groups are similar")
+	awsUseStaticInstanceList  = flag.Bool("aws-use-static-instance-list", false, "Should CA fetch instance types in runtime or use a static list. AWS only")
+	enableProfiling           = flag.Bool("profiling", false, "Is debug/pprof endpoint enabled")
 )
 
 func createAutoscalingOptions() config.AutoscalingOptions {
@@ -235,6 +236,7 @@ func createAutoscalingOptions() config.AutoscalingOptions {
 		Regional:                         *regional,
 		NewPodScaleUpDelay:               *newPodScaleUpDelay,
 		IgnoredTaints:                    *ignoreTaintsFlag,
+		BalancingExtraIgnoredLabels:      *balancingIgnoreLabelsFlag,
 		KubeConfigPath:                   *kubeConfigFile,
 		NodeDeletionDelayTimeout:         *nodeDeletionDelayTimeout,
 		AWSUseStaticInstanceList:         *awsUseStaticInstanceList,
@@ -289,21 +291,24 @@ func buildAutoscaler() (core.Autoscaler, error) {
 	kubeClient := createKubeClient(getKubeConfig())
 	eventsKubeClient := createKubeClient(getKubeConfig())
 
-	processors := ca_processors.DefaultProcessors()
-	processors.PodListProcessor = core.NewFilterOutSchedulablePodListProcessor()
-	if autoscalingOptions.CloudProviderName == cloudprovider.AzureProviderName {
-		processors.NodeGroupSetProcessor = &nodegroupset.BalancingNodeGroupSetProcessor{
-			Comparator: nodegroupset.IsAzureNodeInfoSimilar}
-	} else if autoscalingOptions.CloudProviderName == cloudprovider.AwsProviderName {
-		processors.NodeGroupSetProcessor = &nodegroupset.BalancingNodeGroupSetProcessor{
-			Comparator: nodegroupset.IsAwsNodeInfoSimilar}
-	}
-
 	opts := core.AutoscalerOptions{
 		AutoscalingOptions: autoscalingOptions,
 		KubeClient:         kubeClient,
 		EventsKubeClient:   eventsKubeClient,
-		Processors:         processors,
+	}
+
+	opts.Processors = ca_processors.DefaultProcessors()
+	opts.Processors.PodListProcessor = core.NewFilterOutSchedulablePodListProcessor()
+
+	nodeInfoComparatorBuilder := nodegroupset.CreateGenericNodeInfoComparator
+	if autoscalingOptions.CloudProviderName == cloudprovider.AzureProviderName {
+		nodeInfoComparatorBuilder = nodegroupset.CreateAzureNodeInfoComparator
+	} else if autoscalingOptions.CloudProviderName == cloudprovider.AwsProviderName {
+		nodeInfoComparatorBuilder = nodegroupset.CreateAwsNodeInfoComparator
+	}
+
+	opts.Processors.NodeGroupSetProcessor = &nodegroupset.BalancingNodeGroupSetProcessor{
+		Comparator: nodeInfoComparatorBuilder(autoscalingOptions.BalancingExtraIgnoredLabels),
 	}
 
 	// This metric should be published only once.

cluster-autoscaler/processors/nodegroupset/aws_nodegroups.go

@@ -20,8 +20,10 @@ 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 {
+// CreateAwsNodeInfoComparator returns a comparator that checks if two nodes should be considered
+// part of the same NodeGroupSet. This is true if they match usual conditions checked by IsCloudProviderNodeInfoSimilar,
+// even if they have different AWS-specific labels.
+func CreateAwsNodeInfoComparator(extraIgnoredLabels []string) 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 +35,12 @@ func IsAwsNodeInfoSimilar(n1, n2 *schedulernodeinfo.NodeInfo) bool {
 	for k, v := range BasicIgnoredLabels {
 		awsIgnoredLabels[k] = v
 	}
-	return IsCloudProviderNodeInfoSimilar(n1, n2, awsIgnoredLabels)
+
+	for _, k := range extraIgnoredLabels {
+		awsIgnoredLabels[k] = true
+	}
+
+	return func(n1, n2 *schedulernodeinfo.NodeInfo) bool {
+		return IsCloudProviderNodeInfoSimilar(n1, n2, awsIgnoredLabels)
+	}
 }

cluster-autoscaler/processors/nodegroupset/azure_nodegroups.go

@@ -29,17 +29,23 @@ func nodesFromSameAzureNodePool(n1, n2 *schedulernodeinfo.NodeInfo) bool {
 	return n1AzureNodePool != "" && n1AzureNodePool == n2AzureNodePool
 }
 
-// IsAzureNodeInfoSimilar 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
-	}
+// CreateAzureNodeInfoComparator returns a comparator that checks 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 IsCloudProviderNodeInfoSimilar, even if they have different agentpool labels.
+func CreateAzureNodeInfoComparator(extraIgnoredLabels []string) NodeInfoComparator {
 	azureIgnoredLabels := make(map[string]bool)
 	for k, v := range BasicIgnoredLabels {
 		azureIgnoredLabels[k] = v
 	}
 	azureIgnoredLabels[AzureNodepoolLabel] = true
-	return IsCloudProviderNodeInfoSimilar(n1, n2, azureIgnoredLabels)
+	for _, k := range extraIgnoredLabels {
+		azureIgnoredLabels[k] = true
+	}
+
+	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,54 @@ import (
 )
 
 func TestIsAzureNodeInfoSimilar(t *testing.T) {
+	comparator := CreateAzureNodeInfoComparator([]string{"example.com/ready"})
 	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)
+	// Custom label
+	n1.ObjectMeta.Labels["example.com/ready"] = "true"
+	n2.ObjectMeta.Labels["example.com/ready"] = "false"
+	checkNodesSimilar(t, n1, n2, comparator, true)
 }
 
 func TestFindSimilarNodeGroupsAzureBasic(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{Comparator: IsAzureNodeInfoSimilar}
-	basicSimilarNodeGroupsTest(t, processor)
+	context := &context.AutoscalingContext{}
+	ni1, ni2, ni3 := buildBasicNodeGroups(context)
+	processor := &BalancingNodeGroupSetProcessor{Comparator: CreateAzureNodeInfoComparator([]string{})}
+	basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3)
 }
 
 func TestFindSimilarNodeGroupsAzureByLabel(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{Comparator: IsAzureNodeInfoSimilar}
+	processor := &BalancingNodeGroupSetProcessor{Comparator: CreateAzureNodeInfoComparator([]string{})}
 	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
+			klog.Fatal("BalancingNodeGroupSetProcessor comparator not set")
 		}
 		if comparator(nodeInfo, ngNodeInfo) {
 			result = append(result, ng)

cluster-autoscaler/processors/nodegroupset/balancing_processor_test.go

@@ -19,18 +19,17 @@ package nodegroupset
 import (
 	"testing"
 
+	schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
+
 	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
 	testprovider "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/test"
 	"k8s.io/autoscaler/cluster-autoscaler/context"
 	. "k8s.io/autoscaler/cluster-autoscaler/utils/test"
-	schedulernodeinfo "k8s.io/kubernetes/pkg/scheduler/nodeinfo"
 
 	"github.com/stretchr/testify/assert"
 )
 
-func basicSimilarNodeGroupsTest(t *testing.T, processor NodeGroupSetProcessor) {
-	context := &context.AutoscalingContext{}
-
+func buildBasicNodeGroups(context *context.AutoscalingContext) (*schedulernodeinfo.NodeInfo, *schedulernodeinfo.NodeInfo, *schedulernodeinfo.NodeInfo) {
 	n1 := BuildTestNode("n1", 1000, 1000)
 	n2 := BuildTestNode("n2", 1000, 1000)
 	n3 := BuildTestNode("n3", 2000, 2000)
@@ -49,35 +48,71 @@ func basicSimilarNodeGroupsTest(t *testing.T, processor NodeGroupSetProcessor) {
 	ni3 := schedulernodeinfo.NewNodeInfo()
 	ni3.SetNode(n3)
 
+	context.CloudProvider = provider
+	return ni1, ni2, ni3
+}
+
+func basicSimilarNodeGroupsTest(
+	t *testing.T,
+	context *context.AutoscalingContext,
+	processor NodeGroupSetProcessor,
+	ni1 *schedulernodeinfo.NodeInfo,
+	ni2 *schedulernodeinfo.NodeInfo,
+	ni3 *schedulernodeinfo.NodeInfo,
+) {
 	nodeInfosForGroups := map[string]*schedulernodeinfo.NodeInfo{
 		"ng1": ni1, "ng2": ni2, "ng3": ni3,
 	}
 
-	ng1, _ := provider.NodeGroupForNode(n1)
-	ng2, _ := provider.NodeGroupForNode(n2)
-	ng3, _ := provider.NodeGroupForNode(n3)
-	context.CloudProvider = provider
+	ng1, _ := context.CloudProvider.NodeGroupForNode(ni1.Node())
+	ng2, _ := context.CloudProvider.NodeGroupForNode(ni2.Node())
+	ng3, _ := context.CloudProvider.NodeGroupForNode(ni3.Node())
 
 	similar, err := processor.FindSimilarNodeGroups(context, ng1, nodeInfosForGroups)
 	assert.NoError(t, err)
-	assert.Equal(t, similar, []cloudprovider.NodeGroup{ng2})
+	assert.Equal(t, []cloudprovider.NodeGroup{ng2}, similar)
 
 	similar, err = processor.FindSimilarNodeGroups(context, ng2, nodeInfosForGroups)
 	assert.NoError(t, err)
-	assert.Equal(t, similar, []cloudprovider.NodeGroup{ng1})
+	assert.Equal(t, []cloudprovider.NodeGroup{ng1}, similar)
 
 	similar, err = processor.FindSimilarNodeGroups(context, ng3, nodeInfosForGroups)
 	assert.NoError(t, err)
-	assert.Equal(t, similar, []cloudprovider.NodeGroup{})
+	assert.Equal(t, []cloudprovider.NodeGroup{}, similar)
 }
 
 func TestFindSimilarNodeGroups(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
-	basicSimilarNodeGroupsTest(t, processor)
+	context := &context.AutoscalingContext{}
+	ni1, ni2, ni3 := buildBasicNodeGroups(context)
+	processor := NewDefaultNodeGroupSetProcessor([]string{})
+	basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3)
+}
+
+func TestFindSimilarNodeGroupsCustomLabels(t *testing.T) {
+	context := &context.AutoscalingContext{}
+	ni1, ni2, ni3 := buildBasicNodeGroups(context)
+	ni1.Node().Labels["example.com/ready"] = "true"
+	ni2.Node().Labels["example.com/ready"] = "false"
+
+	processor := NewDefaultNodeGroupSetProcessor([]string{"example.com/ready"})
+	basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3)
+}
+
+func TestFindSimilarNodeGroupsCustomComparator(t *testing.T) {
+	context := &context.AutoscalingContext{}
+	ni1, ni2, ni3 := buildBasicNodeGroups(context)
+
+	processor := &BalancingNodeGroupSetProcessor{
+		Comparator: func(n1, n2 *schedulernodeinfo.NodeInfo) bool {
+			return (n1.Node().Name == "n1" && n2.Node().Name == "n2") ||
+				(n1.Node().Name == "n2" && n2.Node().Name == "n1")
+		},
+	}
+	basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3)
 }
 
 func TestBalanceSingleGroup(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
+	processor := NewDefaultNodeGroupSetProcessor([]string{})
 	context := &context.AutoscalingContext{}
 
 	provider := testprovider.NewTestCloudProvider(nil, nil)
@@ -97,7 +132,7 @@ func TestBalanceSingleGroup(t *testing.T) {
 }
 
 func TestBalanceUnderMaxSize(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
+	processor := NewDefaultNodeGroupSetProcessor([]string{})
 	context := &context.AutoscalingContext{}
 
 	provider := testprovider.NewTestCloudProvider(nil, nil)
@@ -147,7 +182,7 @@ func TestBalanceUnderMaxSize(t *testing.T) {
 }
 
 func TestBalanceHittingMaxSize(t *testing.T) {
-	processor := &BalancingNodeGroupSetProcessor{}
+	processor := NewDefaultNodeGroupSetProcessor([]string{})
 	context := &context.AutoscalingContext{}
 
 	provider := testprovider.NewTestCloudProvider(nil, nil)

cluster-autoscaler/processors/nodegroupset/compare_nodegroups.go

@@ -86,17 +86,26 @@ func compareLabels(nodes []*schedulernodeinfo.NodeInfo, ignoredLabels map[string
 	return true
 }
 
-// IsNodeInfoSimilar returns true if two NodeInfos are similar enough to consider
+// 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 *schedulernodeinfo.NodeInfo) bool {
+		return IsCloudProviderNodeInfoSimilar(n1, n2, genericIgnoredLabels)
+	}
+}
+
+// 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)