Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle intentionally empty grpc BestOptions #7351

Merged
merged 1 commit into from
Oct 14, 2024
Merged

Handle intentionally empty grpc BestOptions #7351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cluster-autoscaler/expander/grpcplugin/grpc_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
67 changes: 46 additions & 21 deletions cluster-autoscaler/expander/grpcplugin/grpc_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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,
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? It seems like if this happen, the test case haven't been properly initialized, in which case it should fail and not pass silently

Copy link
Contributor Author

@rrangith rrangith Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaciekPytel it was for this test case

autoscaler/cluster-autoscaler/expander/grpcplugin/grpc_client_test.go

Lines 259 to 263 in 29de500

desc: "Bad gRPC client config",
client: grpcclientstrategy{nil},
nodeInfo: makeFakeNodeInfos(),
mockResponse: protos.BestOptionsResponse{},
errResponse: nil,
, prior to my change, the test was not executing the correct code path since tc.client was never being used, it was always g being used as the client https://github.com/kubernetes/autoscaler/pull/7351/files#diff-5392da3174a0a9693626803662fd3c6871211cfe739ec3b7d6a5efff38e1cae9L272

tc.client was not getting used at all prior to my change, but i fixed it to now get used

the code path being tested is

autoscaler/cluster-autoscaler/expander/grpcplugin/grpc_client.go

Lines 76 to 79 in 29de500

if g.grpcClient == nil {
klog.Errorf("Incorrect gRPC client config, filtering no options")
return expansionOptions
}
, which is unrelated to my change and I was just fixing the test case since it was broken

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)
}
Expand Down
Loading