fix

DataDog · Oct 4, 2024 · 237a527 · 237a527
1 parent a4ef710
commit 237a527
Show file tree

Hide file tree

Showing 2 changed files with 39 additions and 43 deletions.
diff --git 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
@@ -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)
 	}