Skip to content

Commit 6c1ebf1

Browse files
author
Hannah Lyons
committed
Fix route-controller by handling multi-ID inputs in the batcher
The describeInstanceBatcher currently rejects DescribeInstances inputs with anything other than a single instance ID, which breaks every caller of getInstancesByIDs that legitimately passes multiple IDs (notably the route controller's UpdateRoutes path). See #1351. Move the fan-out logic into the batcher itself: multi-ID inputs are split into one batcher submission per ID so the batcher can coalesce them with other in-flight requests, and the aggregated results (and joined errors, if any) are returned to the caller. The caller in getInstancesByIDs is unchanged. Adds a regression test exercising a multi-ID input through the batcher.
1 parent 3b7a391 commit 6c1ebf1

2 files changed

Lines changed: 78 additions & 7 deletions

File tree

pkg/providers/v1/describe_instance_batch.go

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package aws
1818

1919
import (
2020
"context"
21+
"errors"
2122
"fmt"
2223
"sync"
2324
"time"
@@ -51,16 +52,50 @@ func newdescribeInstanceBatcher(ctx context.Context, ec2api iface.EC2) *describe
5152
return &describeInstanceBatcher{batcher: batcher.NewBatcher(ctx, options)}
5253
}
5354

54-
// DescribeInstances adds describe instances input to batcher
55+
// DescribeInstances adds describe instances input to batcher. Inputs with
56+
// multiple instance IDs are split into one batcher submission per ID so the
57+
// batcher can coalesce them with other in-flight requests; a single
58+
// aggregated slice (and joined error, if any) is returned to the caller.
5559
func (b *describeInstanceBatcher) DescribeInstances(ctx context.Context, input *ec2.DescribeInstancesInput) ([]*ec2types.Instance, error) {
56-
if len(input.InstanceIds) != 1 {
57-
return nil, fmt.Errorf("expected to receive a single instance only, found %d", len(input.InstanceIds))
60+
if len(input.InstanceIds) == 0 {
61+
return nil, fmt.Errorf("expected at least one instance ID, found 0")
5862
}
59-
result := b.batcher.Add(ctx, input)
60-
if result.Output == nil {
61-
return nil, result.Err
63+
if len(input.InstanceIds) == 1 {
64+
result := b.batcher.Add(ctx, input)
65+
if result.Output == nil {
66+
return nil, result.Err
67+
}
68+
return []*ec2types.Instance{result.Output}, result.Err
69+
}
70+
71+
var (
72+
mu sync.Mutex
73+
wg sync.WaitGroup
74+
instances []*ec2types.Instance
75+
errs []error
76+
)
77+
for _, id := range input.InstanceIds {
78+
wg.Add(1)
79+
go func(id string) {
80+
defer wg.Done()
81+
subInput := *input
82+
subInput.InstanceIds = []string{id}
83+
result := b.batcher.Add(ctx, &subInput)
84+
mu.Lock()
85+
defer mu.Unlock()
86+
if result.Err != nil {
87+
errs = append(errs, result.Err)
88+
}
89+
if result.Output != nil {
90+
instances = append(instances, result.Output)
91+
}
92+
}(id)
93+
}
94+
wg.Wait()
95+
if len(errs) > 0 {
96+
return instances, errors.Join(errs...)
6297
}
63-
return []*ec2types.Instance{result.Output}, result.Err
98+
return instances, nil
6499
}
65100

66101
// DescribeInstanceHasher generates hash for different describe instances inputs

pkg/providers/v1/instances_v2_test.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,42 @@ func TestDescribeInstanceBatchingWithBatchedRequestFail(t *testing.T) {
506506
mockedEC2API.AssertNumberOfCalls(t, "DescribeInstances", 3)
507507
}
508508

509+
// TestDescribeInstanceBatchingWithMultipleInstanceIDs verifies that an input
510+
// containing multiple instance IDs is fanned out through the batcher and
511+
// returns aggregated results. Regression test for #1351.
512+
func TestDescribeInstanceBatchingWithMultipleInstanceIDs(t *testing.T) {
513+
mockedEC2API := newMockedEC2API()
514+
batcher := newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})
515+
516+
mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{
517+
Reservations: []ec2types.Reservation{
518+
{
519+
Instances: []ec2types.Instance{
520+
{InstanceId: aws.String("Test-1")},
521+
{InstanceId: aws.String("Test-2")},
522+
{InstanceId: aws.String("Test-3")},
523+
},
524+
},
525+
},
526+
}, nil)
527+
528+
res, err := batcher.DescribeInstances(context.Background(), &ec2.DescribeInstancesInput{
529+
InstanceIds: []string{"Test-1", "Test-2", "Test-3"},
530+
})
531+
assert.NoError(t, err)
532+
assert.Len(t, res, 3)
533+
534+
gotIDs := map[string]bool{}
535+
for _, instance := range res {
536+
gotIDs[aws.ToString(instance.InstanceId)] = true
537+
}
538+
assert.True(t, gotIDs["Test-1"])
539+
assert.True(t, gotIDs["Test-2"])
540+
assert.True(t, gotIDs["Test-3"])
541+
542+
mockedEC2API.AssertNumberOfCalls(t, "DescribeInstances", 1)
543+
}
544+
509545
func getCloudWithMockedDescribeInstances(instanceExists bool, instanceState ec2types.InstanceStateName, instanceID string) *Cloud {
510546
mockedEC2API := newMockedEC2API()
511547
c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})}

0 commit comments

Comments
 (0)