Handle intentionally empty grpc BestOptions

DataDog · Oct 15, 2024 · ae475b3 · ae475b3
1 parent 13c90d8
commit ae475b3
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 24 deletions.
diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client.go b/cluster-autoscaler/expander/grpcplugin/grpc_client.go
@@ -92,9 +92,9 @@ 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 bestOptionsResponse == nil || len(bestOptionsResponse.Options) == 0 {
+		klog.V(4).Info("GRPC returned nil bestOptions")
+		return nil
 	}
 	// Transform back options slice
 	options := transformAndSanitizeOptionsFromGRPC(bestOptionsResponse.Options, nodeGroupIDOptionMap)

diff --git a/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go b/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go
@@ -197,6 +197,43 @@ func TestBestOptionsValid(t *testing.T) {
 	assert.Equal(t, resp, []expander.Option{eoT3Large})
 }
 
+func TestBestOptionsEmpty(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	mockClient := mocks.NewMockExpanderClient(ctrl)
+	g := grpcclientstrategy{mockClient}
+
+	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())
+
+		assert.Nil(t, resp)
+	}
+}
+
 // All test cases should error, and no options should be filtered
 func TestBestOptionsErrors(t *testing.T) {
 	ctrl := gomock.NewController(t)
@@ -232,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,
@@ -263,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)
 	}