include similar nodegroups by default

DataDog · Jun 18, 2024 · 9b368d7 · 9b368d7
1 parent db624a5
commit 9b368d7
Show file tree

Hide file tree

Showing 6 changed files with 93 additions and 124 deletions.
diff --git a/cluster-autoscaler/core/autoscaler.go b/cluster-autoscaler/core/autoscaler.go
@@ -110,7 +110,7 @@ func initializeDefaultOptions(opts *AutoscalerOptions) error {
 	}
 	if opts.ExpanderStrategy == nil {
 		expanderFactory := factory.NewFactory()
-		expanderFactory.RegisterDefaultExpanders(opts.CloudProvider, opts.AutoscalingKubeClients, opts.KubeClient, opts.ConfigNamespace, opts.GRPCExpanderCert, opts.GRPCExpanderURL, opts.IncludeSimilarNodegroupsInExpanderOptions)
+		expanderFactory.RegisterDefaultExpanders(opts.CloudProvider, opts.AutoscalingKubeClients, opts.KubeClient, opts.ConfigNamespace, opts.GRPCExpanderCert, opts.GRPCExpanderURL)
 		expanderStrategy, err := expanderFactory.Build(strings.Split(opts.ExpanderNames, ","))
 		if err != nil {
 			return err

diff --git a/cluster-autoscaler/expander/factory/expander_factory.go b/cluster-autoscaler/expander/factory/expander_factory.go
@@ -78,7 +78,7 @@ func (f *Factory) Build(names []string) (expander.Strategy, errors.AutoscalerErr
 }
 
 // RegisterDefaultExpanders is a convenience function, registering all known expanders in the Factory.
-func (f *Factory) RegisterDefaultExpanders(cloudProvider cloudprovider.CloudProvider, autoscalingKubeClients *context.AutoscalingKubeClients, kubeClient kube_client.Interface, configNamespace string, GRPCExpanderCert string, GRPCExpanderURL string, includeSimilarNodegroupsInExpanderOptions bool) {
+func (f *Factory) RegisterDefaultExpanders(cloudProvider cloudprovider.CloudProvider, autoscalingKubeClients *context.AutoscalingKubeClients, kubeClient kube_client.Interface, configNamespace string, GRPCExpanderCert string, GRPCExpanderURL string) {
 	f.RegisterFilter(expander.RandomExpanderName, random.NewFilter)
 	f.RegisterFilter(expander.MostPodsExpanderName, mostpods.NewFilter)
 	f.RegisterFilter(expander.LeastWasteExpanderName, waste.NewFilter)
@@ -96,6 +96,6 @@ func (f *Factory) RegisterDefaultExpanders(cloudProvider cloudprovider.CloudProv
 		return priority.NewFilter(lister.ConfigMaps(configNamespace), autoscalingKubeClients.Recorder)
 	})
 	f.RegisterFilter(expander.GRPCExpanderName, func() expander.Filter {
-		return grpcplugin.NewFilter(GRPCExpanderCert, GRPCExpanderURL, includeSimilarNodegroupsInExpanderOptions)
+		return grpcplugin.NewFilter(GRPCExpanderCert, GRPCExpanderURL)
 	})
 }
diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client.go b/cluster-autoscaler/expander/grpcplugin/grpc_client.go
@@ -39,17 +39,16 @@ const (
 )
 
 type grpcclientstrategy struct {
-	grpcClient               protos.ExpanderClient
-	includeSimilarNodegroups bool
+	grpcClient protos.ExpanderClient
 }
 
 // NewFilter returns an expansion filter that creates a gRPC client, and calls out to a gRPC server
-func NewFilter(expanderCert string, expanderUrl string, includeSimilarNodegroups bool) expander.Filter {
+func NewFilter(expanderCert string, expanderUrl string) expander.Filter {
 	client := createGRPCClient(expanderCert, expanderUrl)
 	if client == nil {
-		return &grpcclientstrategy{grpcClient: nil, includeSimilarNodegroups: includeSimilarNodegroups}
+		return &grpcclientstrategy{grpcClient: nil}
 	}
-	return &grpcclientstrategy{grpcClient: client, includeSimilarNodegroups: includeSimilarNodegroups}
+	return &grpcclientstrategy{grpcClient: client}
 }
 
 func createGRPCClient(expanderCert string, expanderUrl string) protos.ExpanderClient {
@@ -82,7 +81,7 @@ func (g *grpcclientstrategy) BestOptions(expansionOptions []expander.Option, nod
 	}
 
 	// Transform inputs to gRPC inputs
-	grpcOptionsSlice, nodeGroupIDOptionMap := populateOptionsForGRPC(expansionOptions, g.includeSimilarNodegroups)
+	grpcOptionsSlice, nodeGroupIDOptionMap := populateOptionsForGRPC(expansionOptions)
 	grpcNodeMap := populateNodeInfoForGRPC(nodeInfo)
 
 	// call gRPC server to get BestOption
@@ -100,7 +99,7 @@ func (g *grpcclientstrategy) BestOptions(expansionOptions []expander.Option, nod
 		return expansionOptions
 	}
 	// Transform back options slice
-	options := transformAndSanitizeOptionsFromGRPC(bestOptionsResponse.Options, nodeGroupIDOptionMap, g.includeSimilarNodegroups)
+	options := transformAndSanitizeOptionsFromGRPC(bestOptionsResponse.Options, nodeGroupIDOptionMap)
 	if options == nil {
 		klog.V(4).Info("Unable to sanitize GPRC returned bestOptions, no options filtered")
 		return expansionOptions
@@ -109,22 +108,18 @@ func (g *grpcclientstrategy) BestOptions(expansionOptions []expander.Option, nod
 }
 
 // populateOptionsForGRPC creates a map of nodegroup ID and options, as well as a slice of Options objects for the gRPC call
-func populateOptionsForGRPC(expansionOptions []expander.Option, includeSimilarNodegroups bool) ([]*protos.Option, map[string]expander.Option) {
+func populateOptionsForGRPC(expansionOptions []expander.Option) ([]*protos.Option, map[string]expander.Option) {
 	grpcOptionsSlice := []*protos.Option{}
 	nodeGroupIDOptionMap := make(map[string]expander.Option)
 	for _, option := range expansionOptions {
 		nodeGroupIDOptionMap[option.NodeGroup.Id()] = option
-		var similarNodegroupIds []string
-		if includeSimilarNodegroups {
-			similarNodegroupIds = getSimilarNodegroupIds(option)
-		}
-
-		grpcOptionsSlice = append(grpcOptionsSlice, newOptionMessage(option.NodeGroup.Id(), int32(option.NodeCount), option.Debug, option.Pods, similarNodegroupIds))
+		similarNodeGroupIds := getSimilarNodeGroupIds(option)
+		grpcOptionsSlice = append(grpcOptionsSlice, newOptionMessage(option.NodeGroup.Id(), int32(option.NodeCount), option.Debug, option.Pods, similarNodeGroupIds))
 	}
 	return grpcOptionsSlice, nodeGroupIDOptionMap
 }
 
-func getSimilarNodegroupIds(option expander.Option) []string {
+func getSimilarNodeGroupIds(option expander.Option) []string {
 	var similarNodegroupIds []string
 	for _, sng := range option.SimilarNodeGroups {
 		similarNodegroupIds = append(similarNodegroupIds, sng.Id())
@@ -141,7 +136,7 @@ func populateNodeInfoForGRPC(nodeInfos map[string]*schedulerframework.NodeInfo)
 	return grpcNodeInfoMap
 }
 
-func transformAndSanitizeOptionsFromGRPC(bestOptionsResponseOptions []*protos.Option, nodeGroupIDOptionMap map[string]expander.Option, similarNgsIncludedInOptions bool) []expander.Option {
+func transformAndSanitizeOptionsFromGRPC(bestOptionsResponseOptions []*protos.Option, nodeGroupIDOptionMap map[string]expander.Option) []expander.Option {
 	var options []expander.Option
 	for _, option := range bestOptionsResponseOptions {
 		if option == nil {
@@ -150,9 +145,7 @@ func transformAndSanitizeOptionsFromGRPC(bestOptionsResponseOptions []*protos.Op
 		}
 		if _, ok := nodeGroupIDOptionMap[option.NodeGroupId]; ok {
 			expanderOption := nodeGroupIDOptionMap[option.NodeGroupId]
-			if similarNgsIncludedInOptions {
-				expanderOption.SimilarNodeGroups = getRetainedSimilarNodegroups(option, expanderOption)
-			}
+			expanderOption.SimilarNodeGroups = getRetainedSimilarNodegroups(option, expanderOption)
 			options = append(options, expanderOption)
 		} else {
 			klog.Errorf("GRPC server returned invalid nodeGroup ID: ", option.NodeGroupId)

diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go b/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go
@@ -108,11 +108,10 @@ var (
 
 func TestPopulateOptionsForGrpc(t *testing.T) {
 	testCases := []struct {
-		desc                     string
-		includeSimilarNodegroups bool
-		opts                     []expander.Option
-		expectedOpts             []*protos.Option
-		expectedMap              map[string]expander.Option
+		desc         string
+		opts         []expander.Option
+		expectedOpts []*protos.Option
+		expectedMap  map[string]expander.Option
 	}{
 		{
 			desc:         "empty options",
@@ -138,22 +137,14 @@ func TestPopulateOptionsForGrpc(t *testing.T) {
 			},
 		},
 		{
-			desc:                     "similar nodegroups are excluded",
-			opts:                     []expander.Option{eoT2MicroWithSimilar},
-			includeSimilarNodegroups: false,
-			expectedOpts:             []*protos.Option{&grpcEoT2Micro},
-			expectedMap:              map[string]expander.Option{eoT2MicroWithSimilar.NodeGroup.Id(): eoT2MicroWithSimilar},
-		},
-		{
-			desc:                     "similar nodegroups are included",
-			opts:                     []expander.Option{eoT2MicroWithSimilar},
-			includeSimilarNodegroups: true,
-			expectedOpts:             []*protos.Option{&grpcEoT2MicroWithSimilar},
-			expectedMap:              map[string]expander.Option{eoT2MicroWithSimilar.NodeGroup.Id(): eoT2MicroWithSimilar},
+			desc:         "similar nodegroups are included",
+			opts:         []expander.Option{eoT2MicroWithSimilar},
+			expectedOpts: []*protos.Option{&grpcEoT2MicroWithSimilar},
+			expectedMap:  map[string]expander.Option{eoT2MicroWithSimilar.NodeGroup.Id(): eoT2MicroWithSimilar},
 		},
 	}
 	for _, tc := range testCases {
-		grpcOptionsSlice, nodeGroupIDOptionMap := populateOptionsForGRPC(tc.opts, tc.includeSimilarNodegroups)
+		grpcOptionsSlice, nodeGroupIDOptionMap := populateOptionsForGRPC(tc.opts)
 		assert.Equal(t, tc.expectedOpts, grpcOptionsSlice)
 		assert.Equal(t, tc.expectedMap, nodeGroupIDOptionMap)
 	}
@@ -182,16 +173,14 @@ func TestPopulateNodeInfoForGRPC(t *testing.T) {
 
 func TestValidTransformAndSanitizeOptionsFromGRPC(t *testing.T) {
 	testCases := []struct {
-		desc                        string
-		similarNgsIncludedInOptions bool
-		responseOptions             []*protos.Option
-		expectedOptions             []expander.Option
-		nodegroupIDOptionaMap       map[string]expander.Option
+		desc                  string
+		responseOptions       []*protos.Option
+		expectedOptions       []expander.Option
+		nodegroupIDOptionaMap map[string]expander.Option
 	}{
 		{
-			desc:                        "valid transform and sanitize options",
-			similarNgsIncludedInOptions: false,
-			responseOptions:             []*protos.Option{&grpcEoT2Micro, &grpcEoT3Large, &grpcEoM44XLarge},
+			desc:            "valid transform and sanitize options",
+			responseOptions: []*protos.Option{&grpcEoT2Micro, &grpcEoT3Large, &grpcEoM44XLarge},
 			nodegroupIDOptionaMap: map[string]expander.Option{
 				eoT2Micro.NodeGroup.Id():   eoT2Micro,
 				eoT2Large.NodeGroup.Id():   eoT2Large,
@@ -201,35 +190,24 @@ func TestValidTransformAndSanitizeOptionsFromGRPC(t *testing.T) {
 			expectedOptions: []expander.Option{eoT2Micro, eoT3Large, eoM44XLarge},
 		},
 		{
-			desc:                        "only similar ngs retained in proto options are retained when options include similar ngs",
-			similarNgsIncludedInOptions: true,
-			responseOptions:             []*protos.Option{&grpcEoT2Micro},
-			nodegroupIDOptionaMap: map[string]expander.Option{
-				eoT2MicroWithSimilar.NodeGroup.Id(): eoT2MicroWithSimilar,
-			},
-			expectedOptions: []expander.Option{eoT2Micro},
-		},
-		{
-			desc:                        "all similar ngs retained when options do not include similar ngs",
-			similarNgsIncludedInOptions: false,
-			responseOptions:             []*protos.Option{&grpcEoT2Micro},
+			desc:            "similar ngs are retained in proto options are retained",
+			responseOptions: []*protos.Option{&grpcEoT2MicroWithSimilar},
 			nodegroupIDOptionaMap: map[string]expander.Option{
 				eoT2MicroWithSimilar.NodeGroup.Id(): eoT2MicroWithSimilar,
 			},
 			expectedOptions: []expander.Option{eoT2MicroWithSimilar},
 		},
 		{
-			desc:                        "similar ngs added to expander response are ignored",
-			similarNgsIncludedInOptions: true,
-			responseOptions:             []*protos.Option{&grpcEoT2MicroWithSimilarWithExtraOptions},
+			desc:            "extra similar ngs added to expander response are ignored",
+			responseOptions: []*protos.Option{&grpcEoT2MicroWithSimilarWithExtraOptions},
 			nodegroupIDOptionaMap: map[string]expander.Option{
 				eoT2MicroWithSimilar.NodeGroup.Id(): eoT2MicroWithSimilar,
 			},
 			expectedOptions: []expander.Option{eoT2MicroWithSimilar},
 		},
 	}
 	for _, tc := range testCases {
-		ret := transformAndSanitizeOptionsFromGRPC(tc.responseOptions, tc.nodegroupIDOptionaMap, tc.similarNgsIncludedInOptions)
+		ret := transformAndSanitizeOptionsFromGRPC(tc.responseOptions, tc.nodegroupIDOptionaMap)
 		assert.Equal(t, tc.expectedOptions, ret)
 	}
 }
@@ -242,7 +220,7 @@ func TestAnInvalidTransformAndSanitizeOptionsFromGRPC(t *testing.T) {
 		eoT3Large.NodeGroup.Id(): eoT3Large,
 	}
 
-	ret := transformAndSanitizeOptionsFromGRPC(responseOptionsSlice, nodeGroupIDOptionMap, false)
+	ret := transformAndSanitizeOptionsFromGRPC(responseOptionsSlice, nodeGroupIDOptionMap)
 	assert.Equal(t, []expander.Option{eoT2Micro, eoT3Large}, ret)
 }
 

diff --git a/cluster-autoscaler/expander/grpcplugin/protos/expander.pb.go b/cluster-autoscaler/expander/grpcplugin/protos/expander.pb.go
@@ -5,7 +5,7 @@ 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
+    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,