From 44dcaa8cf3488c159d25b4bbc190371cc0280528 Mon Sep 17 00:00:00 2001 From: olagacek Date: Fri, 4 Oct 2024 12:54:22 +0200 Subject: [PATCH] Revert "CAS: cloudprovider-specific nodegroupset" --- cluster-autoscaler/main.go | 9 +- .../nodegroupset}/aws_nodegroups.go | 13 ++- .../nodegroupset}/aws_nodegroups_test.go | 15 ++- .../nodegroupset}/azure_nodegroups.go | 13 ++- .../nodegroupset}/azure_nodegroups_test.go | 43 ++++---- .../nodegroupset/balancing_processor_test.go | 65 ++++++++++-- .../nodegroupset/compare_nodegroups_test.go | 57 +++++++---- .../compare_nodegroups_test_helpers.go | 98 ------------------- .../nodegroupset}/gce_nodegroups.go | 11 +-- .../nodegroupset/label_nodegroups_test.go | 2 +- 10 files changed, 144 insertions(+), 182 deletions(-) rename cluster-autoscaler/{cloudprovider/aws => processors/nodegroupset}/aws_nodegroups.go (77%) rename cluster-autoscaler/{cloudprovider/aws => processors/nodegroupset}/aws_nodegroups_test.go (89%) rename cluster-autoscaler/{cloudprovider/azure => processors/nodegroupset}/azure_nodegroups.go (86%) rename cluster-autoscaler/{cloudprovider/azure => processors/nodegroupset}/azure_nodegroups_test.go (77%) delete mode 100644 cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test_helpers.go rename cluster-autoscaler/{cloudprovider/gce => processors/nodegroupset}/gce_nodegroups.go (78%) diff --git a/cluster-autoscaler/main.go b/cluster-autoscaler/main.go index c6f506b8ec4b..d56d581afb31 100644 --- a/cluster-autoscaler/main.go +++ b/cluster-autoscaler/main.go @@ -47,10 +47,7 @@ import ( "k8s.io/apiserver/pkg/server/routes" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" - "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/aws" - "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/azure" cloudBuilder "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/builder" - "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/gce" "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/gce/localssdsize" "k8s.io/autoscaler/cluster-autoscaler/config" "k8s.io/autoscaler/cluster-autoscaler/core" @@ -574,12 +571,12 @@ func buildAutoscaler(debuggingSnapshotter debuggingsnapshot.DebuggingSnapshotter } else { nodeInfoComparatorBuilder := nodegroupset.CreateGenericNodeInfoComparator if autoscalingOptions.CloudProviderName == cloudprovider.AzureProviderName { - nodeInfoComparatorBuilder = azure.CreateNodeInfoComparator + nodeInfoComparatorBuilder = nodegroupset.CreateAzureNodeInfoComparator } else if autoscalingOptions.CloudProviderName == cloudprovider.AwsProviderName { - nodeInfoComparatorBuilder = aws.CreateNodeInfoComparator + nodeInfoComparatorBuilder = nodegroupset.CreateAwsNodeInfoComparator opts.Processors.TemplateNodeInfoProvider = nodeinfosprovider.NewAsgTagResourceNodeInfoProvider(nodeInfoCacheExpireTime, *forceDaemonSets) } else if autoscalingOptions.CloudProviderName == cloudprovider.GceProviderName { - nodeInfoComparatorBuilder = gce.CreateGceNodeInfoComparator + nodeInfoComparatorBuilder = nodegroupset.CreateGceNodeInfoComparator opts.Processors.TemplateNodeInfoProvider = nodeinfosprovider.NewAnnotationNodeInfoProvider(nodeInfoCacheExpireTime, *forceDaemonSets) } nodeInfoComparator = nodeInfoComparatorBuilder(autoscalingOptions.BalancingExtraIgnoredLabels, autoscalingOptions.NodeGroupSetRatios) diff --git a/cluster-autoscaler/cloudprovider/aws/aws_nodegroups.go b/cluster-autoscaler/processors/nodegroupset/aws_nodegroups.go similarity index 77% rename from cluster-autoscaler/cloudprovider/aws/aws_nodegroups.go rename to cluster-autoscaler/processors/nodegroupset/aws_nodegroups.go index ef759d5c91a8..2c22f8280522 100644 --- a/cluster-autoscaler/cloudprovider/aws/aws_nodegroups.go +++ b/cluster-autoscaler/processors/nodegroupset/aws_nodegroups.go @@ -1,5 +1,5 @@ /* -Copyright 2024 The Kubernetes Authors. +Copyright 2019 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,18 +14,17 @@ See the License for the specific language governing permissions and limitations under the License. */ -package aws +package nodegroupset import ( "k8s.io/autoscaler/cluster-autoscaler/config" - "k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset" schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" ) -// CreateNodeInfoComparator returns a comparator that checks if two nodes should be considered +// 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 CreateNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.NodeGroupDifferenceRatios) nodegroupset.NodeInfoComparator { +func CreateAwsNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.NodeGroupDifferenceRatios) 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. @@ -35,7 +34,7 @@ func CreateNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.Node "topology.ebs.csi.aws.com/zone": true, // this is a label used by the AWS EBS CSI driver as a target for Persistent Volume Node Affinity } - for k, v := range nodegroupset.BasicIgnoredLabels { + for k, v := range BasicIgnoredLabels { awsIgnoredLabels[k] = v } @@ -44,6 +43,6 @@ func CreateNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.Node } return func(n1, n2 *schedulerframework.NodeInfo) bool { - return nodegroupset.IsCloudProviderNodeInfoSimilar(n1, n2, awsIgnoredLabels, ratioOpts) + return IsCloudProviderNodeInfoSimilar(n1, n2, awsIgnoredLabels, ratioOpts) } } diff --git a/cluster-autoscaler/cloudprovider/aws/aws_nodegroups_test.go b/cluster-autoscaler/processors/nodegroupset/aws_nodegroups_test.go similarity index 89% rename from cluster-autoscaler/cloudprovider/aws/aws_nodegroups_test.go rename to cluster-autoscaler/processors/nodegroupset/aws_nodegroups_test.go index 9b291fb0fa0d..334d9dc8d627 100644 --- a/cluster-autoscaler/cloudprovider/aws/aws_nodegroups_test.go +++ b/cluster-autoscaler/processors/nodegroupset/aws_nodegroups_test.go @@ -1,5 +1,5 @@ /* -Copyright 2024 The Kubernetes Authors. +Copyright 2021 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,19 +14,18 @@ See the License for the specific language governing permissions and limitations under the License. */ -package aws +package nodegroupset import ( "testing" "k8s.io/autoscaler/cluster-autoscaler/config" "k8s.io/autoscaler/cluster-autoscaler/context" - "k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset" . "k8s.io/autoscaler/cluster-autoscaler/utils/test" ) func TestIsAwsNodeInfoSimilar(t *testing.T) { - comparator := CreateNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{}) + comparator := CreateAwsNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{}) node1 := BuildTestNode("node1", 1000, 2000) node2 := BuildTestNode("node2", 1000, 2000) @@ -170,14 +169,14 @@ func TestIsAwsNodeInfoSimilar(t *testing.T) { if tc.removeOneLabel { delete(node2.ObjectMeta.Labels, tc.label) } - nodegroupset.CheckNodesSimilar(t, node1, node2, comparator, true) + checkNodesSimilar(t, node1, node2, comparator, true) }) } } func TestFindSimilarNodeGroupsAwsBasic(t *testing.T) { context := &context.AutoscalingContext{} - ni1, ni2, ni3 := nodegroupset.BuildBasicNodeGroups(context) - processor := &nodegroupset.BalancingNodeGroupSetProcessor{Comparator: CreateNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{})} - nodegroupset.BasicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) + ni1, ni2, ni3 := buildBasicNodeGroups(context) + processor := &BalancingNodeGroupSetProcessor{Comparator: CreateAwsNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{})} + basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) } diff --git a/cluster-autoscaler/cloudprovider/azure/azure_nodegroups.go b/cluster-autoscaler/processors/nodegroupset/azure_nodegroups.go similarity index 86% rename from cluster-autoscaler/cloudprovider/azure/azure_nodegroups.go rename to cluster-autoscaler/processors/nodegroupset/azure_nodegroups.go index 1fda177a135d..3b615a620675 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_nodegroups.go +++ b/cluster-autoscaler/processors/nodegroupset/azure_nodegroups.go @@ -1,5 +1,5 @@ /* -Copyright 2024 The Kubernetes Authors. +Copyright 2019 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,11 +14,10 @@ See the License for the specific language governing permissions and limitations under the License. */ -package azure +package nodegroupset import ( "k8s.io/autoscaler/cluster-autoscaler/config" - "k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset" schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" ) @@ -53,12 +52,12 @@ func nodesFromSameAzureNodePoolLegacy(n1, n2 *schedulerframework.NodeInfo) bool return n1AzureNodePool != "" && n1AzureNodePool == n2AzureNodePool } -// CreateNodeInfoComparator returns a comparator that checks if two nodes should be considered +// 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 CreateNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.NodeGroupDifferenceRatios) nodegroupset.NodeInfoComparator { +func CreateAzureNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.NodeGroupDifferenceRatios) NodeInfoComparator { azureIgnoredLabels := make(map[string]bool) - for k, v := range nodegroupset.BasicIgnoredLabels { + for k, v := range BasicIgnoredLabels { azureIgnoredLabels[k] = v } azureIgnoredLabels[AzureNodepoolLegacyLabel] = true @@ -79,6 +78,6 @@ func CreateNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.Node if nodesFromSameAzureNodePool(n1, n2) { return true } - return nodegroupset.IsCloudProviderNodeInfoSimilar(n1, n2, azureIgnoredLabels, ratioOpts) + return IsCloudProviderNodeInfoSimilar(n1, n2, azureIgnoredLabels, ratioOpts) } } diff --git a/cluster-autoscaler/cloudprovider/azure/azure_nodegroups_test.go b/cluster-autoscaler/processors/nodegroupset/azure_nodegroups_test.go similarity index 77% rename from cluster-autoscaler/cloudprovider/azure/azure_nodegroups_test.go rename to cluster-autoscaler/processors/nodegroupset/azure_nodegroups_test.go index 9b7e892d9d8e..8b73f36111a1 100644 --- a/cluster-autoscaler/cloudprovider/azure/azure_nodegroups_test.go +++ b/cluster-autoscaler/processors/nodegroupset/azure_nodegroups_test.go @@ -1,5 +1,5 @@ /* -Copyright 2024 The Kubernetes Authors. +Copyright 2019 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package azure +package nodegroupset import ( "testing" @@ -23,7 +23,6 @@ import ( testprovider "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/test" "k8s.io/autoscaler/cluster-autoscaler/config" "k8s.io/autoscaler/cluster-autoscaler/context" - "k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset" . "k8s.io/autoscaler/cluster-autoscaler/utils/test" schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" @@ -31,75 +30,75 @@ import ( ) func TestIsAzureNodeInfoSimilar(t *testing.T) { - comparator := CreateNodeInfoComparator([]string{"example.com/ready"}, config.NodeGroupDifferenceRatios{}) + comparator := CreateAzureNodeInfoComparator([]string{"example.com/ready"}, config.NodeGroupDifferenceRatios{}) 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. - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, false) + checkNodesSimilar(t, n1, n2, comparator, false) // Empty agentpool labels n1.ObjectMeta.Labels["agentpool"] = "" n2.ObjectMeta.Labels["agentpool"] = "" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, false) + checkNodesSimilar(t, n1, n2, comparator, false) // AKS agentpool labels n1.ObjectMeta.Labels["kubernetes.azure.com/agentpool"] = "foo" n2.ObjectMeta.Labels["kubernetes.azure.com/agentpool"] = "bar" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, false) + checkNodesSimilar(t, n1, n2, comparator, false) // Only one non empty n1.ObjectMeta.Labels["agentpool"] = "" n2.ObjectMeta.Labels["agentpool"] = "foo" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, false) + checkNodesSimilar(t, n1, n2, comparator, false) // Only one present delete(n1.ObjectMeta.Labels, "agentpool") n2.ObjectMeta.Labels["agentpool"] = "foo" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, false) + checkNodesSimilar(t, n1, n2, comparator, false) // Different vales n1.ObjectMeta.Labels["agentpool"] = "foo1" n2.ObjectMeta.Labels["agentpool"] = "foo2" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, false) + checkNodesSimilar(t, n1, n2, comparator, false) // Same values n1.ObjectMeta.Labels["agentpool"] = "foo" n2.ObjectMeta.Labels["agentpool"] = "foo" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, 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" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // Different creationSource n1.ObjectMeta.Labels["creationSource"] = "aks-aks-nodepool2-vmss" n2.ObjectMeta.Labels["creationSource"] = "aks-aks-nodepool3-vmss" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // Different node image version n1.ObjectMeta.Labels["kubernetes.azure.com/node-image-version"] = "AKSUbuntu-1804gen2-2021.01.28" n2.ObjectMeta.Labels["kubernetes.azure.com/node-image-version"] = "AKSUbuntu-1804gen2-2022.01.30" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // Custom label n1.ObjectMeta.Labels["example.com/ready"] = "true" n2.ObjectMeta.Labels["example.com/ready"] = "false" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // One node with aksConsolidatedAdditionalProperties label n1.ObjectMeta.Labels[aksConsolidatedAdditionalProperties] = "foo" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // Same aksConsolidatedAdditionalProperties n2.ObjectMeta.Labels[aksConsolidatedAdditionalProperties] = "foo" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // Different aksConsolidatedAdditionalProperties label n2.ObjectMeta.Labels[aksConsolidatedAdditionalProperties] = "bar" - nodegroupset.CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) } func TestFindSimilarNodeGroupsAzureBasic(t *testing.T) { context := &context.AutoscalingContext{} - ni1, ni2, ni3 := nodegroupset.BuildBasicNodeGroups(context) - processor := &nodegroupset.BalancingNodeGroupSetProcessor{Comparator: CreateNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{})} - nodegroupset.BasicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) + ni1, ni2, ni3 := buildBasicNodeGroups(context) + processor := &BalancingNodeGroupSetProcessor{Comparator: CreateAzureNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{})} + basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) } func TestFindSimilarNodeGroupsAzureByLabel(t *testing.T) { - processor := &nodegroupset.BalancingNodeGroupSetProcessor{Comparator: CreateNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{})} + processor := &BalancingNodeGroupSetProcessor{Comparator: CreateAzureNodeInfoComparator([]string{}, config.NodeGroupDifferenceRatios{})} context := &context.AutoscalingContext{} n1 := BuildTestNode("n1", 1000, 1000) diff --git a/cluster-autoscaler/processors/nodegroupset/balancing_processor_test.go b/cluster-autoscaler/processors/nodegroupset/balancing_processor_test.go index 6def3bd4f9de..9645cdb3997b 100644 --- a/cluster-autoscaler/processors/nodegroupset/balancing_processor_test.go +++ b/cluster-autoscaler/processors/nodegroupset/balancing_processor_test.go @@ -25,30 +25,83 @@ import ( testprovider "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/test" "k8s.io/autoscaler/cluster-autoscaler/config" "k8s.io/autoscaler/cluster-autoscaler/context" + . "k8s.io/autoscaler/cluster-autoscaler/utils/test" "github.com/stretchr/testify/assert" ) +func buildBasicNodeGroups(context *context.AutoscalingContext) (*schedulerframework.NodeInfo, *schedulerframework.NodeInfo, *schedulerframework.NodeInfo) { + n1 := BuildTestNode("n1", 1000, 1000) + n2 := BuildTestNode("n2", 1000, 1000) + n3 := BuildTestNode("n3", 2000, 2000) + provider := testprovider.NewTestCloudProvider(nil, nil) + provider.AddNodeGroup("ng1", 1, 10, 1) + provider.AddNodeGroup("ng2", 1, 10, 1) + provider.AddNodeGroup("ng3", 1, 10, 1) + provider.AddNode("ng1", n1) + provider.AddNode("ng2", n2) + provider.AddNode("ng3", n3) + + ni1 := schedulerframework.NewNodeInfo() + ni1.SetNode(n1) + ni2 := schedulerframework.NewNodeInfo() + ni2.SetNode(n2) + ni3 := schedulerframework.NewNodeInfo() + ni3.SetNode(n3) + + context.CloudProvider = provider + return ni1, ni2, ni3 +} + +func basicSimilarNodeGroupsTest( + t *testing.T, + context *context.AutoscalingContext, + processor NodeGroupSetProcessor, + ni1 *schedulerframework.NodeInfo, + ni2 *schedulerframework.NodeInfo, + ni3 *schedulerframework.NodeInfo, +) { + nodeInfosForGroups := map[string]*schedulerframework.NodeInfo{ + "ng1": ni1, "ng2": ni2, "ng3": ni3, + } + + 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, []cloudprovider.NodeGroup{ng2}, similar) + + similar, err = processor.FindSimilarNodeGroups(context, ng2, nodeInfosForGroups) + assert.NoError(t, err) + assert.Equal(t, []cloudprovider.NodeGroup{ng1}, similar) + + similar, err = processor.FindSimilarNodeGroups(context, ng3, nodeInfosForGroups) + assert.NoError(t, err) + assert.Equal(t, []cloudprovider.NodeGroup{}, similar) +} + func TestFindSimilarNodeGroups(t *testing.T) { context := &context.AutoscalingContext{} - ni1, ni2, ni3 := BuildBasicNodeGroups(context) + ni1, ni2, ni3 := buildBasicNodeGroups(context) processor := NewDefaultNodeGroupSetProcessor([]string{}, config.NodeGroupDifferenceRatios{}) - BasicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) + basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) } func TestFindSimilarNodeGroupsCustomLabels(t *testing.T) { context := &context.AutoscalingContext{} - ni1, ni2, ni3 := BuildBasicNodeGroups(context) + 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"}, config.NodeGroupDifferenceRatios{}) - BasicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) + basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) } func TestFindSimilarNodeGroupsCustomComparator(t *testing.T) { context := &context.AutoscalingContext{} - ni1, ni2, ni3 := BuildBasicNodeGroups(context) + ni1, ni2, ni3 := buildBasicNodeGroups(context) processor := &BalancingNodeGroupSetProcessor{ Comparator: func(n1, n2 *schedulerframework.NodeInfo) bool { @@ -56,7 +109,7 @@ func TestFindSimilarNodeGroupsCustomComparator(t *testing.T) { (n1.Node().Name == "n2" && n2.Node().Name == "n1") }, } - BasicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) + basicSimilarNodeGroupsTest(t, context, processor, ni1, ni2, ni3) } func TestBalanceSingleGroup(t *testing.T) { diff --git a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go b/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go index 990c90edd8c1..eae8a67090e0 100644 --- a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go +++ b/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test.go @@ -24,13 +24,28 @@ import ( "k8s.io/autoscaler/cluster-autoscaler/config" "k8s.io/autoscaler/cluster-autoscaler/utils/gpu" . "k8s.io/autoscaler/cluster-autoscaler/utils/test" + schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" + + "github.com/stretchr/testify/assert" ) +func checkNodesSimilar(t *testing.T, n1, n2 *apiv1.Node, comparator NodeInfoComparator, shouldEqual bool) { + checkNodesSimilarWithPods(t, n1, n2, []*apiv1.Pod{}, []*apiv1.Pod{}, comparator, shouldEqual) +} + +func checkNodesSimilarWithPods(t *testing.T, n1, n2 *apiv1.Node, pods1, pods2 []*apiv1.Pod, comparator NodeInfoComparator, shouldEqual bool) { + ni1 := schedulerframework.NewNodeInfo(pods1...) + ni1.SetNode(n1) + ni2 := schedulerframework.NewNodeInfo(pods2...) + ni2.SetNode(n2) + assert.Equal(t, shouldEqual, comparator(ni1, ni2)) +} + func TestIdenticalNodesSimilar(t *testing.T) { comparator := CreateGenericNodeInfoComparator([]string{}, config.NewDefaultNodeGroupDifferenceRatios()) n1 := BuildTestNode("node1", 1000, 2000) n2 := BuildTestNode("node2", 1000, 2000) - CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) } func TestNodesSimilarVariousRequirements(t *testing.T) { @@ -40,23 +55,23 @@ func TestNodesSimilarVariousRequirements(t *testing.T) { // Different CPU capacity n2 := BuildTestNode("node2", 1000, 2000) n2.Status.Capacity[apiv1.ResourceCPU] = *resource.NewMilliQuantity(1001, resource.DecimalSI) - CheckNodesSimilar(t, n1, n2, comparator, 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, comparator, 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, comparator, 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, comparator, false) + checkNodesSimilar(t, n1, n5, comparator, false) } func TestNodesSimilarVariousRequirementsAndPods(t *testing.T) { @@ -69,20 +84,20 @@ 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{}, comparator, 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}, comparator, 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}, comparator, true) + checkNodesSimilarWithPods(t, n1, n4, []*apiv1.Pod{p1}, []*apiv1.Pod{p4}, comparator, true) } func TestNodesSimilarVariousMemoryRequirements(t *testing.T) { @@ -92,12 +107,12 @@ func TestNodesSimilarVariousMemoryRequirements(t *testing.T) { // Different memory capacity within tolerance n2 := BuildTestNode("node2", 1000, 1000) n2.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(1000-(1000*config.DefaultMaxCapacityMemoryDifferenceRatio)+1, resource.DecimalSI) - CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // Different memory capacity exceeds tolerance n3 := BuildTestNode("node3", 1000, 1000) n3.Status.Capacity[apiv1.ResourceMemory] = *resource.NewQuantity(1000-(1000*config.DefaultMaxCapacityMemoryDifferenceRatio)-1, resource.DecimalSI) - CheckNodesSimilar(t, n1, n3, comparator, false) + checkNodesSimilar(t, n1, n3, comparator, false) } func TestNodesSimilarVariousLargeMemoryRequirementsM5XLarge(t *testing.T) { @@ -113,13 +128,13 @@ func TestNodesSimilarVariousLargeMemoryRequirementsM5XLarge(t *testing.T) { // 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) + 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) + checkNodesSimilar(t, n1, n3, comparator, false) } func TestNodesSimilarVariousLargeMemoryRequirementsM516XLarge(t *testing.T) { @@ -135,13 +150,13 @@ func TestNodesSimilarVariousLargeMemoryRequirementsM516XLarge(t *testing.T) { // 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) + 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) + checkNodesSimilar(t, n1, n3, comparator, false) } func TestNodesSimilarVariousLabels(t *testing.T) { @@ -154,32 +169,32 @@ func TestNodesSimilarVariousLabels(t *testing.T) { n2.ObjectMeta.Labels["test-label"] = "test-value" // Missing character label - CheckNodesSimilar(t, n1, n2, comparator, false) + checkNodesSimilar(t, n1, n2, comparator, false) n2.ObjectMeta.Labels["character"] = "winnie the pooh" - CheckNodesSimilar(t, n1, n2, comparator, 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, comparator, 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, comparator, 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, comparator, 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, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) // Different custom labels should not matter n1.ObjectMeta.Labels["example.com/ready"] = "true" n2.ObjectMeta.Labels["example.com/ready"] = "false" - CheckNodesSimilar(t, n1, n2, comparator, true) + checkNodesSimilar(t, n1, n2, comparator, true) } diff --git a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test_helpers.go b/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test_helpers.go deleted file mode 100644 index 4819569676fd..000000000000 --- a/cluster-autoscaler/processors/nodegroupset/compare_nodegroups_test_helpers.go +++ /dev/null @@ -1,98 +0,0 @@ -/* -Copyright 2024 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package nodegroupset - -import ( - "testing" - - apiv1 "k8s.io/api/core/v1" - "k8s.io/autoscaler/cluster-autoscaler/cloudprovider" - testprovider "k8s.io/autoscaler/cluster-autoscaler/cloudprovider/test" - "k8s.io/autoscaler/cluster-autoscaler/context" - testutils "k8s.io/autoscaler/cluster-autoscaler/utils/test" - schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" - - "github.com/stretchr/testify/assert" -) - -// CheckNodesSimilar is a helper func for tests to validate node comparison outcomes -func CheckNodesSimilar(t *testing.T, n1, n2 *apiv1.Node, comparator NodeInfoComparator, shouldEqual bool) { - CheckNodesSimilarWithPods(t, n1, n2, []*apiv1.Pod{}, []*apiv1.Pod{}, comparator, shouldEqual) -} - -// CheckNodesSimilarWithPods is a helper func for tests to validate nodes with pods comparison outcomes -func CheckNodesSimilarWithPods(t *testing.T, n1, n2 *apiv1.Node, pods1, pods2 []*apiv1.Pod, comparator NodeInfoComparator, shouldEqual bool) { - ni1 := schedulerframework.NewNodeInfo(pods1...) - ni1.SetNode(n1) - ni2 := schedulerframework.NewNodeInfo(pods2...) - ni2.SetNode(n2) - assert.Equal(t, shouldEqual, comparator(ni1, ni2)) -} - -// BuildBasicNodeGroups is a helper func for tests to get a set of NodeInfo objects -func BuildBasicNodeGroups(context *context.AutoscalingContext) (*schedulerframework.NodeInfo, *schedulerframework.NodeInfo, *schedulerframework.NodeInfo) { - n1 := testutils.BuildTestNode("n1", 1000, 1000) - n2 := testutils.BuildTestNode("n2", 1000, 1000) - n3 := testutils.BuildTestNode("n3", 2000, 2000) - provider := testprovider.NewTestCloudProvider(nil, nil) - provider.AddNodeGroup("ng1", 1, 10, 1) - provider.AddNodeGroup("ng2", 1, 10, 1) - provider.AddNodeGroup("ng3", 1, 10, 1) - provider.AddNode("ng1", n1) - provider.AddNode("ng2", n2) - provider.AddNode("ng3", n3) - - ni1 := schedulerframework.NewNodeInfo() - ni1.SetNode(n1) - ni2 := schedulerframework.NewNodeInfo() - ni2.SetNode(n2) - ni3 := schedulerframework.NewNodeInfo() - ni3.SetNode(n3) - - context.CloudProvider = provider - return ni1, ni2, ni3 -} - -// BasicSimilarNodeGroupsTest is a helper func for tests to assert node group similarity -func BasicSimilarNodeGroupsTest( - t *testing.T, - context *context.AutoscalingContext, - processor NodeGroupSetProcessor, - ni1 *schedulerframework.NodeInfo, - ni2 *schedulerframework.NodeInfo, - ni3 *schedulerframework.NodeInfo, -) { - nodeInfosForGroups := map[string]*schedulerframework.NodeInfo{ - "ng1": ni1, "ng2": ni2, "ng3": ni3, - } - - 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, []cloudprovider.NodeGroup{ng2}, similar) - - similar, err = processor.FindSimilarNodeGroups(context, ng2, nodeInfosForGroups) - assert.NoError(t, err) - assert.Equal(t, []cloudprovider.NodeGroup{ng1}, similar) - - similar, err = processor.FindSimilarNodeGroups(context, ng3, nodeInfosForGroups) - assert.NoError(t, err) - assert.Equal(t, []cloudprovider.NodeGroup{}, similar) -} diff --git a/cluster-autoscaler/cloudprovider/gce/gce_nodegroups.go b/cluster-autoscaler/processors/nodegroupset/gce_nodegroups.go similarity index 78% rename from cluster-autoscaler/cloudprovider/gce/gce_nodegroups.go rename to cluster-autoscaler/processors/nodegroupset/gce_nodegroups.go index 0667b95a1549..483f6f04f395 100644 --- a/cluster-autoscaler/cloudprovider/gce/gce_nodegroups.go +++ b/cluster-autoscaler/processors/nodegroupset/gce_nodegroups.go @@ -1,5 +1,5 @@ /* -Copyright 2024 The Kubernetes Authors. +Copyright 2019 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -14,23 +14,22 @@ See the License for the specific language governing permissions and limitations under the License. */ -package gce +package nodegroupset import ( "k8s.io/autoscaler/cluster-autoscaler/config" - "k8s.io/autoscaler/cluster-autoscaler/processors/nodegroupset" schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework" ) // CreateGceNodeInfoComparator 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 GCE-specific labels. -func CreateGceNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.NodeGroupDifferenceRatios) nodegroupset.NodeInfoComparator { +func CreateGceNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.NodeGroupDifferenceRatios) NodeInfoComparator { gceIgnoredLabels := map[string]bool{ "topology.gke.io/zone": true, } - for k, v := range nodegroupset.BasicIgnoredLabels { + for k, v := range BasicIgnoredLabels { gceIgnoredLabels[k] = v } @@ -39,6 +38,6 @@ func CreateGceNodeInfoComparator(extraIgnoredLabels []string, ratioOpts config.N } return func(n1, n2 *schedulerframework.NodeInfo) bool { - return nodegroupset.IsCloudProviderNodeInfoSimilar(n1, n2, gceIgnoredLabels, ratioOpts) + return IsCloudProviderNodeInfoSimilar(n1, n2, gceIgnoredLabels, ratioOpts) } } diff --git a/cluster-autoscaler/processors/nodegroupset/label_nodegroups_test.go b/cluster-autoscaler/processors/nodegroupset/label_nodegroups_test.go index afee2542f47a..aa3f7722ccbe 100644 --- a/cluster-autoscaler/processors/nodegroupset/label_nodegroups_test.go +++ b/cluster-autoscaler/processors/nodegroupset/label_nodegroups_test.go @@ -86,7 +86,7 @@ func TestNodeLabelComparison(t *testing.T) { t.Run(tc.description, func(t *testing.T) { node1.ObjectMeta.Labels = tc.labels1 node2.ObjectMeta.Labels = tc.labels2 - CheckNodesSimilar(t, node1, node2, comparator, tc.isSimilar) + checkNodesSimilar(t, node1, node2, comparator, tc.isSimilar) }) } }