diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client.go b/cluster-autoscaler/expander/grpcplugin/grpc_client.go index f208ea763986..95a200991b1c 100644 --- a/cluster-autoscaler/expander/grpcplugin/grpc_client.go +++ b/cluster-autoscaler/expander/grpcplugin/grpc_client.go @@ -92,15 +92,11 @@ func (g *grpcclientstrategy) BestOptions(expansionOptions []expander.Option, nod return expansionOptions } - if bestOptionsResponse == nil || bestOptionsResponse.Options == nil { - klog.V(4).Info("GRPC returned nil bestOptions, no options filtered") - return expansionOptions - } - if len(bestOptionsResponse.Options) == 0 { - // best options is intentionally empty + if bestOptionsResponse == nil || len(bestOptionsResponse.Options) == 0 { klog.V(4).Info("GRPC returned no bestOptions") return nil } + // Transform back options slice options := transformAndSanitizeOptionsFromGRPC(bestOptionsResponse.Options, nodeGroupIDOptionMap) if options == nil { diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go b/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go index 5c0c800c4de6..cb6882b79fc6 100644 --- a/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go +++ b/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go @@ -201,25 +201,37 @@ func TestBestOptionsEmpty(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockClient := mocks.NewMockExpanderClient(ctrl) - g := &grpcclientstrategy{mockClient} + g := grpcclientstrategy{mockClient} - nodeInfos := makeFakeNodeInfos() - grpcNodeInfoMap := make(map[string]*v1.Node) - for i, opt := range options { - grpcNodeInfoMap[opt.NodeGroup.Id()] = nodes[i] - } - expectedBestOptionsReq := &protos.BestOptionsRequest{ - Options: []*protos.Option{&grpcEoT2Micro, &grpcEoT2Large, &grpcEoT3Large, &grpcEoM44XLarge}, - NodeMap: grpcNodeInfoMap, + testCases := []struct { + desc string + mockResponse protos.BestOptionsResponse + }{ + { + desc: "empty bestOptions response", + mockResponse: protos.BestOptionsResponse{}, + }, + { + desc: "empty bestOptions response, options nil", + mockResponse: protos.BestOptionsResponse{Options: nil}, + }, + { + desc: "empty bestOptions response, empty options slice", + mockResponse: protos.BestOptionsResponse{Options: []*protos.Option{}}, + }, } + for _, tc := range testCases { + grpcNodeInfoMap := populateNodeInfoForGRPC(makeFakeNodeInfos()) + mockClient.EXPECT().BestOptions( + gomock.Any(), gomock.Eq( + &protos.BestOptionsRequest{ + Options: []*protos.Option{&grpcEoT2Micro, &grpcEoT2Large, &grpcEoT3Large, &grpcEoM44XLarge}, + NodeMap: grpcNodeInfoMap, + })).Return(&tc.mockResponse, nil) + resp := g.BestOptions(options, makeFakeNodeInfos()) - mockClient.EXPECT().BestOptions( - gomock.Any(), gomock.Eq(expectedBestOptionsReq), - ).Return(&protos.BestOptionsResponse{Options: []*protos.Option{}}, nil) - - resp := g.BestOptions(options, nodeInfos) - - assert.Empty(t, resp) + assert.Nil(t, resp) + } } // All test cases should error, and no options should be filtered @@ -257,20 +269,6 @@ func TestBestOptionsErrors(t *testing.T) { mockResponse: protos.BestOptionsResponse{}, errResponse: errors.New("timeout error"), }, - { - desc: "bad bestOptions response", - client: g, - nodeInfo: makeFakeNodeInfos(), - mockResponse: protos.BestOptionsResponse{}, - errResponse: nil, - }, - { - desc: "bad bestOptions response, options nil", - client: g, - nodeInfo: makeFakeNodeInfos(), - mockResponse: protos.BestOptionsResponse{Options: nil}, - errResponse: nil, - }, { desc: "bad bestOptions response, options invalid - nil", client: g, @@ -288,13 +286,15 @@ func TestBestOptionsErrors(t *testing.T) { } for _, tc := range testCases { grpcNodeInfoMap := populateNodeInfoForGRPC(tc.nodeInfo) - mockClient.EXPECT().BestOptions( - gomock.Any(), gomock.Eq( - &protos.BestOptionsRequest{ - Options: []*protos.Option{&grpcEoT2Micro, &grpcEoT2Large, &grpcEoT3Large, &grpcEoM44XLarge}, - NodeMap: grpcNodeInfoMap, - })).Return(&tc.mockResponse, tc.errResponse) - resp := g.BestOptions(options, tc.nodeInfo) + if tc.client.grpcClient != nil { + mockClient.EXPECT().BestOptions( + gomock.Any(), gomock.Eq( + &protos.BestOptionsRequest{ + Options: []*protos.Option{&grpcEoT2Micro, &grpcEoT2Large, &grpcEoT3Large, &grpcEoM44XLarge}, + NodeMap: grpcNodeInfoMap, + })).Return(&tc.mockResponse, tc.errResponse) + } + resp := tc.client.BestOptions(options, tc.nodeInfo) assert.Equal(t, resp, options) }