Skip to content

Fix the batched describe insance method for instance not found case #1260

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

Merged
k8s-ci-robot merged 1 commit into from
Sep 24, 2025
Merged

Fix the batched describe insance method for instance not found case #1260

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
14 changes: 11 additions & 3 deletions pkg/providers/v1/describe_instance_batch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ func (b *describeInstanceBatcher) DescribeInstances(ctx context.Context, input *
return nil, fmt.Errorf("expected to receive a single instance only, found %d", len(input.InstanceIds))
}
result := b.batcher.Add(ctx, input)
if result.Output == nil {
Copy link
Copy Markdown
Member

@yue9944882 yue9944882 Sep 23, 2025

Choose a reason for hiding this comment

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

is if result.Err != nil a better check here? i wonder whether we're actually getting an error e.g. 404 not found wen describing a non-existent instance, it's trickier when e.g. we batch two describe instance IDs where one exists and the other is non-existent, are we getting an error in this case?

return nil, result.Err
}
return []*ec2types.Instance{result.Output}, result.Err
}

Expand All @@ -74,7 +77,8 @@ func describeInstanceHasher(ctx context.Context, input *ec2.DescribeInstancesInp
func execDescribeInstanceBatch(ec2api iface.EC2) batcher.BatchExecutor[ec2.DescribeInstancesInput, ec2types.Instance] {
return func(ctx context.Context, inputs []*ec2.DescribeInstancesInput) []batcher.Result[ec2types.Instance] {
results := make([]batcher.Result[ec2types.Instance], len(inputs))
firstInput := inputs[0]

firstInput := *inputs[0]
Copy link
Copy Markdown
Contributor

@sarthakkothari sarthakkothari Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

why did we translate this from a pointer? can potentially cause another panic?

Copy link
Copy Markdown
Member Author

@kmala kmala Sep 23, 2025

Choose a reason for hiding this comment

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

if we don't translate we are overriding it in the below for loop as firstInput would just be a pointer to the first item item in slice. inputs should never have nil items for the panic to happen and there should always be atleast 1 item in the inputs.

Copy link
Copy Markdown
Contributor

@gargipanatula gargipanatula Sep 23, 2025

Choose a reason for hiding this comment

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

Should we still have a len check on inputs before this line to catch the case where inputs is empty? Unless we have a hard guarantee somewhere that inputs is not empty?

Copy link
Copy Markdown
Member Author

@kmala kmala Sep 23, 2025

Choose a reason for hiding this comment

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

yes, we are guaranteed to not have empty list as this gets called only when we make a describe instance call and there it verifies the instance count.

// aggregate instanceIDs into 1 input
for _, input := range inputs[1:] {
firstInput.InstanceIds = append(firstInput.InstanceIds, input.InstanceIds...)
Expand All @@ -92,7 +96,7 @@ func execDescribeInstanceBatch(ec2api iface.EC2) batcher.BatchExecutor[ec2.Descr
go func(input *ec2.DescribeInstancesInput) {
defer wg.Done()
out, err := ec2api.DescribeInstances(ctx, input)
if err != nil {
if err != nil || len(out) == 0 {
results[idx] = batcher.Result[ec2types.Instance]{Output: nil, Err: err}
return
}
Expand All @@ -104,7 +108,11 @@ func execDescribeInstanceBatch(ec2api iface.EC2) batcher.BatchExecutor[ec2.Descr
instanceIDToOutputMap := map[string]ec2types.Instance{}
lo.ForEach(output, func(o ec2types.Instance, _ int) { instanceIDToOutputMap[lo.FromPtr(o.InstanceId)] = o })
for idx, input := range inputs {
o := instanceIDToOutputMap[input.InstanceIds[0]]
o, ok := instanceIDToOutputMap[input.InstanceIds[0]]
if !ok {
results[idx] = batcher.Result[ec2types.Instance]{Output: nil}
continue
}
results[idx] = batcher.Result[ec2types.Instance]{Output: &o}
}
}
Expand Down
136 changes: 136 additions & 0 deletions pkg/providers/v1/instances_v2_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,142 @@ func TestDescribeInstanceBatching(t *testing.T) {
mockedEC2API.AssertNumberOfCalls(t, "DescribeInstances", 1)
}

// TestDescribeInstanceBatchingWithInstanceDoesntExist will test where one of the instance doesn't exist
func TestDescribeInstanceBatchingWithInstanceDoesntExist(t *testing.T) {
mockedEC2API := newMockedEC2API()
batcher := newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})

mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{
Reservations: []ec2types.Reservation{
{
Instances: []ec2types.Instance{
{
InstanceId: aws.String("Test-1"),
},
{
InstanceId: aws.String("Test-2"),
},
{
InstanceId: aws.String("Test-3"),
},
},
},
},
}, nil)

type result struct {
input string
output []*ec2types.Instance
err error
isInstanceNotFound bool
}

// Add extra space to channel so that we can ensure there were only 3 responses
resCh := make(chan result, 5)
helper := func(wg *sync.WaitGroup, input string, isInstanceNotFound bool) {
defer wg.Done()
res, err := batcher.DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{InstanceIds: []string{input}})
resCh <- result{input: input, output: res, err: err, isInstanceNotFound: isInstanceNotFound}
}

wg := sync.WaitGroup{}
wg.Add(3)
go helper(&wg, "Test-1", false)
go helper(&wg, "Test-2", false)
go helper(&wg, "Test-4", true)
wg.Wait()
close(resCh)

assert.Len(t, resCh, 3)
for res := range resCh {
assert.NoError(t, res.err)
if !res.isInstanceNotFound {
assert.Len(t, res.output, 1)
assert.Equal(t, res.input, *res.output[0].InstanceId)
} else {
assert.Len(t, res.output, 0)
}
}

mockedEC2API.AssertNumberOfCalls(t, "DescribeInstances", 1)
}

// TestDescribeInstanceBatchingWithBatchedRequestFail will test where batched request fails but individual request succeeds
func TestDescribeInstanceBatchingWithBatchedRequestFail(t *testing.T) {
mockedEC2API := newMockedEC2API()
batcher := newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})
var nilResp *ec2.DescribeInstancesOutput
mockedEC2API.On("DescribeInstances", &ec2.DescribeInstancesInput{
InstanceIds: []string{"Test-1", "Test-2"},
}).Return(nilResp, errors.New("instances not found"))
mockedEC2API.On("DescribeInstances", &ec2.DescribeInstancesInput{
InstanceIds: []string{"Test-2", "Test-1"},
}).Return(nilResp, errors.New("instances not found"))
mockedEC2API.On("DescribeInstances",
&ec2.DescribeInstancesInput{
InstanceIds: []string{"Test-1"},
},
).Return(
&ec2.DescribeInstancesOutput{
Reservations: []ec2types.Reservation{
{
Instances: []ec2types.Instance{
{
InstanceId: aws.String("Test-1"),
},
},
},
},
},
nil,
)
mockedEC2API.On("DescribeInstances",
&ec2.DescribeInstancesInput{
InstanceIds: []string{"Test-2"},
},
).Return(
&ec2.DescribeInstancesOutput{
Reservations: []ec2types.Reservation{},
},
nil,
)

type result struct {
input string
output []*ec2types.Instance
err error
isInstanceNotFound bool
}

// Add extra space to channel so that we can ensure there were only 3 responses
resCh := make(chan result, 5)
helper := func(wg *sync.WaitGroup, input string, isInstanceNotFound bool) {
defer wg.Done()
res, err := batcher.DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{InstanceIds: []string{input}})
resCh <- result{input: input, output: res, err: err, isInstanceNotFound: isInstanceNotFound}
}

wg := sync.WaitGroup{}
wg.Add(2)
go helper(&wg, "Test-1", false)
go helper(&wg, "Test-2", true)
wg.Wait()
close(resCh)

assert.Len(t, resCh, 2)
for res := range resCh {
assert.NoError(t, res.err)
if !res.isInstanceNotFound {
assert.Len(t, res.output, 1)
assert.Equal(t, res.input, *res.output[0].InstanceId)
} else {
assert.Len(t, res.output, 0)
}
}

mockedEC2API.AssertNumberOfCalls(t, "DescribeInstances", 3)
}

func getCloudWithMockedDescribeInstances(instanceExists bool, instanceState ec2types.InstanceStateName, instanceID string) *Cloud {
mockedEC2API := newMockedEC2API()
c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})}
Expand Down
Loading