squash

DataDog · Jun 18, 2024 · c6af2c9 · c6af2c9
1 parent 841717b
commit c6af2c9
Show file tree

Hide file tree

Showing 8 changed files with 199 additions and 115 deletions.
diff --git a/cluster-autoscaler/config/autoscaling_options.go b/cluster-autoscaler/config/autoscaling_options.go
@@ -121,8 +121,6 @@ type AutoscalingOptions struct {
 	GRPCExpanderCert string
 	// GRPCExpanderURL is the url of the gRPC server when using the gRPC expander
 	GRPCExpanderURL string
-	// IncludeSimilarNodegroupsInExpanderOptions is whether CA will include similar nodegroups in it's request to the gRPC expander
-	IncludeSimilarNodegroupsInExpanderOptions bool
 	// IgnoreMirrorPodsUtilization is whether CA will ignore Mirror pods when calculating resource utilization for scaling down
 	IgnoreMirrorPodsUtilization bool
 	// MaxGracefulTerminationSec is maximum number of seconds scale down waits for pods to terminate before

diff --git a/cluster-autoscaler/core/scaleup/orchestrator/orchestrator.go b/cluster-autoscaler/core/scaleup/orchestrator/orchestrator.go
@@ -174,7 +174,6 @@ func (o *ScaleUpOrchestrator) ScaleUp(
 	}
 
 	// Pick some expansion option.
-	// We have to be aware of the flag here?
 	bestOption := o.autoscalingContext.ExpanderStrategy.BestOption(options, nodeInfos)
 	if bestOption == nil || bestOption.NodeCount <= 0 {
 		return &status.ScaleUpStatus{

diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client.go b/cluster-autoscaler/expander/grpcplugin/grpc_client.go
@@ -22,6 +22,7 @@ import (
 	"time"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
 	"k8s.io/autoscaler/cluster-autoscaler/expander"
 	"k8s.io/autoscaler/cluster-autoscaler/expander/grpcplugin/protos"
 	"k8s.io/klog/v2"
@@ -37,8 +38,7 @@ 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
@@ -80,7 +80,7 @@ func (g *grpcclientstrategy) BestOptions(expansionOptions []expander.Option, nod
 	}
 
 	// Transform inputs to gRPC inputs
-	grpcOptionsSlice, nodeGroupIDOptionMap := populateOptionsForGRPC(expansionOptions, false)
+	grpcOptionsSlice, nodeGroupIDOptionMap := populateOptionsForGRPC(expansionOptions)
 	grpcNodeMap := populateNodeInfoForGRPC(nodeInfo)
 
 	// call gRPC server to get BestOption
@@ -107,27 +107,23 @@ 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 {
-		similarNodegroupIds := []string{}
 		nodeGroupIDOptionMap[option.NodeGroup.Id()] = option
-		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 {
-	var similarNodegroupIds []string
+func getSimilarNodeGroupIds(option expander.Option) []string {
+	var similarNodeGroupIds []string
 	for _, sng := range option.SimilarNodeGroups {
-		similarNodegroupIds = append(similarNodegroupIds, sng.Id())
+		similarNodeGroupIds = append(similarNodeGroupIds, sng.Id())
 	}
-	return similarNodegroupIds
+	return similarNodeGroupIds
 }
 
 // populateNodeInfoForGRPC looks at the corresponding v1.Node object per NodeInfo object, and populates the grpcNodeInfoMap with these to pass over grpc
@@ -147,7 +143,9 @@ func transformAndSanitizeOptionsFromGRPC(bestOptionsResponseOptions []*protos.Op
 			continue
 		}
 		if _, ok := nodeGroupIDOptionMap[option.NodeGroupId]; ok {
-			options = append(options, nodeGroupIDOptionMap[option.NodeGroupId])
+			expanderOption := nodeGroupIDOptionMap[option.NodeGroupId]
+			expanderOption.SimilarNodeGroups = getRetainedSimilarNodegroups(option, expanderOption)
+			options = append(options, expanderOption)
 		} else {
 			klog.Errorf("GRPC server returned invalid nodeGroup ID: ", option.NodeGroupId)
 			continue
@@ -156,6 +154,25 @@ func transformAndSanitizeOptionsFromGRPC(bestOptionsResponseOptions []*protos.Op
 	return options
 }
 
-func newOptionMessage(nodeGroupId string, nodeCount int32, debug string, pods []*v1.Pod, similarNodegroupIds []string) *protos.Option {
-	return &protos.Option{NodeGroupId: nodeGroupId, NodeCount: nodeCount, Debug: debug, Pod: pods, SimilarNodegroupIds: similarNodegroupIds}
+// Any similar options that were not included in the original grpcExpander request, but were added
+// as part of the response, will be ignored
+func getRetainedSimilarNodegroups(grpcOption *protos.Option, expanderOption expander.Option) []cloudprovider.NodeGroup {
+	var retainedSimilarNodeGroups []cloudprovider.NodeGroup
+	for _, sng := range expanderOption.SimilarNodeGroups {
+		retained := false
+		for _, id := range grpcOption.SimilarNodeGroupIds {
+			if sng.Id() == id {
+				retained = true
+				continue
+			}
+		}
+		if retained {
+			retainedSimilarNodeGroups = append(retainedSimilarNodeGroups, sng)
+		}
+	}
+	return retainedSimilarNodeGroups
+}
+
+func newOptionMessage(nodeGroupId string, nodeCount int32, debug string, pods []*v1.Pod, similarNodeGroupIds []string) *protos.Option {
+	return &protos.Option{NodeGroupId: nodeGroupId, NodeCount: nodeCount, Debug: debug, Pod: pods, SimilarNodeGroupIds: similarNodeGroupIds}
 }
diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go b/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go
@@ -23,6 +23,7 @@ import (
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/autoscaler/cluster-autoscaler/cloudprovider"
 	"k8s.io/autoscaler/cluster-autoscaler/expander/grpcplugin/protos"
 	"k8s.io/autoscaler/cluster-autoscaler/expander/mocks"
 	. "k8s.io/autoscaler/cluster-autoscaler/utils/test"
@@ -58,6 +59,11 @@ var (
 		Debug:     "m4.4xlarge",
 		NodeGroup: test.NewTestNodeGroup("my-asg.m4.4xlarge", 10, 1, 1, true, false, "m4.4xlarge", nil, nil),
 	}
+	eoT2MicroWithSimilar = expander.Option{
+		Debug:             "t2.micro",
+		NodeGroup:         test.NewTestNodeGroup("my-asg.t2.micro", 10, 1, 1, true, false, "t2.micro", nil, nil),
+		SimilarNodeGroups: []cloudprovider.NodeGroup{test.NewTestNodeGroup("my-similar-asg.t2.micro", 10, 1, 1, true, false, "t2.micro", nil, nil)},
+	}
 	options = []expander.Option{eoT2Micro, eoT2Large, eoT3Large, eoM44XLarge}
 
 	grpcEoT2Micro = protos.Option{
@@ -84,6 +90,20 @@ var (
 		Debug:       eoM44XLarge.Debug,
 		Pod:         eoM44XLarge.Pods,
 	}
+	grpcEoT2MicroWithSimilar = protos.Option{
+		NodeGroupId:         eoT2Micro.NodeGroup.Id(),
+		NodeCount:           int32(eoT2Micro.NodeCount),
+		Debug:               eoT2Micro.Debug,
+		Pod:                 eoT2Micro.Pods,
+		SimilarNodeGroupIds: []string{eoT2MicroWithSimilar.SimilarNodeGroups[0].Id()},
+	}
+	grpcEoT2MicroWithSimilarWithExtraOptions = protos.Option{
+		NodeGroupId:         eoT2Micro.NodeGroup.Id(),
+		NodeCount:           int32(eoT2Micro.NodeCount),
+		Debug:               eoT2Micro.Debug,
+		Pod:                 eoT2Micro.Pods,
+		SimilarNodeGroupIds: []string{eoT2MicroWithSimilar.SimilarNodeGroups[0].Id(), "extra-ng-id"},
+	}
 )
 
 func TestPopulateOptionsForGrpc(t *testing.T) {
@@ -116,9 +136,15 @@ func TestPopulateOptionsForGrpc(t *testing.T) {
 				eoM44XLarge.NodeGroup.Id(): eoM44XLarge,
 			},
 		},
+		{
+			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, false)
+		grpcOptionsSlice, nodeGroupIDOptionMap := populateOptionsForGRPC(tc.opts)
 		assert.Equal(t, tc.expectedOpts, grpcOptionsSlice)
 		assert.Equal(t, tc.expectedMap, nodeGroupIDOptionMap)
 	}
@@ -146,18 +172,44 @@ func TestPopulateNodeInfoForGRPC(t *testing.T) {
 }
 
 func TestValidTransformAndSanitizeOptionsFromGRPC(t *testing.T) {
-	responseOptionsSlice := []*protos.Option{&grpcEoT2Micro, &grpcEoT3Large, &grpcEoM44XLarge}
-	nodeGroupIDOptionMap := map[string]expander.Option{
-		eoT2Micro.NodeGroup.Id():   eoT2Micro,
-		eoT2Large.NodeGroup.Id():   eoT2Large,
-		eoT3Large.NodeGroup.Id():   eoT3Large,
-		eoM44XLarge.NodeGroup.Id(): eoM44XLarge,
+	testCases := []struct {
+		desc                  string
+		responseOptions       []*protos.Option
+		expectedOptions       []expander.Option
+		nodegroupIDOptionaMap map[string]expander.Option
+	}{
+		{
+			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,
+				eoT3Large.NodeGroup.Id():   eoT3Large,
+				eoM44XLarge.NodeGroup.Id(): eoM44XLarge,
+			},
+			expectedOptions: []expander.Option{eoT2Micro, eoT3Large, eoM44XLarge},
+		},
+		{
+			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:            "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)
+		assert.Equal(t, tc.expectedOptions, ret)
 	}
-
-	expectedOptions := []expander.Option{eoT2Micro, eoT3Large, eoM44XLarge}
-
-	ret := transformAndSanitizeOptionsFromGRPC(responseOptionsSlice, nodeGroupIDOptionMap)
-	assert.Equal(t, expectedOptions, ret)
 }
 
 func TestAnInvalidTransformAndSanitizeOptionsFromGRPC(t *testing.T) {
@@ -176,7 +228,7 @@ func TestBestOptionsValid(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 	mockClient := mocks.NewMockExpanderClient(ctrl)
-	g := &grpcclientstrategy{grpcClient: mockClient}
+	g := &grpcclientstrategy{mockClient}
 
 	nodeInfos := makeFakeNodeInfos()
 	grpcNodeInfoMap := make(map[string]*v1.Node)
@@ -202,7 +254,7 @@ func TestBestOptionsErrors(t *testing.T) {
 	ctrl := gomock.NewController(t)
 	defer ctrl.Finish()
 	mockClient := mocks.NewMockExpanderClient(ctrl)
-	g := grpcclientstrategy{grpcClient: mockClient}
+	g := grpcclientstrategy{mockClient}
 
 	badProtosOption := protos.Option{
 		NodeGroupId: "badID",
@@ -220,7 +272,7 @@ func TestBestOptionsErrors(t *testing.T) {
 	}{
 		{
 			desc:         "Bad gRPC client config",
-			client:       grpcclientstrategy{grpcClient: nil},
+			client:       grpcclientstrategy{nil},
 			nodeInfo:     makeFakeNodeInfos(),
 			mockResponse: protos.BestOptionsResponse{},
 			errResponse:  nil,

diff --git a/cluster-autoscaler/expander/grpcplugin/protos/expander.pb.go b/cluster-autoscaler/expander/grpcplugin/protos/expander.pb.go
@@ -1,17 +1,28 @@
-// Code generated by protoc-gen-go. DO NOT EDIT.
-// versions:
-// 	protoc-gen-go v1.28.1
-// 	protoc        v5.26.1
-// source: expander.proto
+/*
+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.
+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 protos
 
 import (
+	reflect "reflect"
+	sync "sync"
+
 	protoreflect "google.golang.org/protobuf/reflect/protoreflect"
 	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
 	v1 "k8s.io/api/core/v1"
-	reflect "reflect"
-	sync "sync"
 )
 
 const (
@@ -134,7 +145,7 @@ type Option struct {
 	NodeCount           int32     `protobuf:"varint,2,opt,name=nodeCount,proto3" json:"nodeCount,omitempty"`
 	Debug               string    `protobuf:"bytes,3,opt,name=debug,proto3" json:"debug,omitempty"`
 	Pod                 []*v1.Pod `protobuf:"bytes,4,rep,name=pod,proto3" json:"pod,omitempty"`
-	SimilarNodegroupIds []string  `protobuf:"bytes,5,rep,name=similarNodegroupIds,proto3" json:"similarNodegroupIds,omitempty"`
+	SimilarNodeGroupIds []string  `protobuf:"bytes,5,rep,name=similarNodeGroupIds,proto3" json:"similarNodeGroupIds,omitempty"`
 }
 
 func (x *Option) Reset() {
@@ -197,9 +208,9 @@ func (x *Option) GetPod() []*v1.Pod {
 	return nil
 }
 
-func (x *Option) GetSimilarNodegroupIds() []string {
+func (x *Option) GetSimilarNodeGroupIds() []string {
 	if x != nil {
-		return x.SimilarNodegroupIds
+		return x.SimilarNodeGroupIds
 	}
 	return nil
 }
@@ -239,8 +250,8 @@ var file_expander_proto_rawDesc = []byte{
 	0x04, 0x20, 0x03, 0x28, 0x0b, 0x32, 0x17, 0x2e, 0x6b, 0x38, 0x73, 0x2e, 0x69, 0x6f, 0x2e, 0x61,
 	0x70, 0x69, 0x2e, 0x63, 0x6f, 0x72, 0x65, 0x2e, 0x76, 0x31, 0x2e, 0x50, 0x6f, 0x64, 0x52, 0x03,
 	0x70, 0x6f, 0x64, 0x12, 0x30, 0x0a, 0x13, 0x73, 0x69, 0x6d, 0x69, 0x6c, 0x61, 0x72, 0x4e, 0x6f,
-	0x64, 0x65, 0x67, 0x72, 0x6f, 0x75, 0x70, 0x49, 0x64, 0x73, 0x18, 0x05, 0x20, 0x03, 0x28, 0x09,
-	0x52, 0x13, 0x73, 0x69, 0x6d, 0x69, 0x6c, 0x61, 0x72, 0x4e, 0x6f, 0x64, 0x65, 0x67, 0x72, 0x6f,
+	0x64, 0x65, 0x47, 0x72, 0x6f, 0x75, 0x70, 0x49, 0x64, 0x73, 0x18, 0x05, 0x20, 0x03, 0x28, 0x09,
+	0x52, 0x13, 0x73, 0x69, 0x6d, 0x69, 0x6c, 0x61, 0x72, 0x4e, 0x6f, 0x64, 0x65, 0x47, 0x72, 0x6f,
 	0x75, 0x70, 0x49, 0x64, 0x73, 0x32, 0x5c, 0x0a, 0x08, 0x45, 0x78, 0x70, 0x61, 0x6e, 0x64, 0x65,
 	0x72, 0x12, 0x50, 0x0a, 0x0b, 0x42, 0x65, 0x73, 0x74, 0x4f, 0x70, 0x74, 0x69, 0x6f, 0x6e, 0x73,
 	0x12, 0x1e, 0x2e, 0x67, 0x72, 0x70, 0x63, 0x70, 0x6c, 0x75, 0x67, 0x69, 0x6e, 0x2e, 0x42, 0x65,

diff --git a/cluster-autoscaler/expander/grpcplugin/protos/expander.proto b/cluster-autoscaler/expander/grpcplugin/protos/expander.proto
@@ -27,5 +27,5 @@ message Option {
   int32 nodeCount = 2;
   string debug = 3;
   repeated k8s.io.api.core.v1.Pod pod = 4;
-  repeated string similarNodegroupIds = 5;
+  repeated string similarNodeGroupIds = 5;
 }
diff --git a/cluster-autoscaler/expander/grpcplugin/protos/expander_grpc.pb.go b/cluster-autoscaler/expander/grpcplugin/protos/expander_grpc.pb.go
@@ -1,13 +1,24 @@
-// Code generated by protoc-gen-go-grpc. DO NOT EDIT.
-// versions:
-// - protoc-gen-go-grpc v1.2.0
-// - protoc             v5.26.1
-// source: expander.proto
+/*
+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.
+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 protos
 
 import (
 	context "context"
+
 	grpc "google.golang.org/grpc"
 	codes "google.golang.org/grpc/codes"
 	status "google.golang.org/grpc/status"
@@ -43,21 +54,19 @@ func (c *expanderClient) BestOptions(ctx context.Context, in *BestOptionsRequest
 }
 
 // ExpanderServer is the server API for Expander service.
-// All implementations must embed UnimplementedExpanderServer
+// All implementations should embed UnimplementedExpanderServer
 // for forward compatibility
 type ExpanderServer interface {
 	BestOptions(context.Context, *BestOptionsRequest) (*BestOptionsResponse, error)
-	mustEmbedUnimplementedExpanderServer()
 }
 
-// UnimplementedExpanderServer must be embedded to have forward compatible implementations.
+// UnimplementedExpanderServer should be embedded to have forward compatible implementations.
 type UnimplementedExpanderServer struct {
 }
 
 func (UnimplementedExpanderServer) BestOptions(context.Context, *BestOptionsRequest) (*BestOptionsResponse, error) {
 	return nil, status.Errorf(codes.Unimplemented, "method BestOptions not implemented")
 }
-func (UnimplementedExpanderServer) mustEmbedUnimplementedExpanderServer() {}
 
 // UnsafeExpanderServer may be embedded to opt out of forward compatibility for this service.
 // Use of this interface is not recommended, as added methods to ExpanderServer will