-
Notifications
You must be signed in to change notification settings - Fork 367
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.
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
fix(ec2): Call describe-instance API concurrently #2047
fix(ec2): Call describe-instance API concurrently #2047
Conversation
2e917d8
to
18504de
Compare
Adding multi-threading generally increases the complexity of the code and makes it harder to debug. Since we have a bunch of other controllers (i.e. S3) where we make multiple, consecutive calls that cause no problem so far, I am very reluctant to merge this. Is there any issue that comes with the increased reconcile time for EC2 instances? |
Having thousands of instances, this allowed us to reduce queue size and period of a single instance reconciliation from 10+ minutes down to several minutes. |
descAttr := func(attr types.InstanceAttributeName) (*awsec2.DescribeInstanceAttributeOutput, error) { | ||
return e.client.DescribeInstanceAttribute(ctx, &awsec2.DescribeInstanceAttributeInput{ | ||
InstanceId: &instanceId, | ||
Attribute: attr, | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this could be done more elegantly by iterating over an array of attribute keys and start a Go routine in parallel. I would also prefer to use https://pkg.go.dev/golang.org/x/sync/errgroup instead of standard WaitGroup
.
Something like this:
func (e *external) describeInstanceAttributes(ctx context.Context, cr *svcapitypes.Instance) (*awsec2.DescribeInstanceAttributeOutput, error) {
eg := errgroup.Group{}
out := &awsec2.DescribeInstanceAttributeOutput{}
describeAttribute := func(attr types.InstanceAttributeName, onSuccess func(res *awsec2.DescribeInstanceAttributeOutput)) {
eg.Go(func() error {
res, err := e.client.DescribeInstanceAttribute(ctx, &awsec2.DescribeInstanceAttributeInput{
InstanceId: aws.String(meta.GetExternalName(cr)),
Attribute: attr,
})
if err != nil {
err = errors.Wrap(err, string(attr))
return err
}
onSuccess(res)
return nil
})
}
describeAttribute(types.InstanceAttributeNameDisableApiTermination, func(res *awsec2.DescribeInstanceAttributeOutput) {
out.DisableApiTermination = res.DisableApiTermination
})
describeAttribute(types.InstanceAttributeNameInstanceInitiatedShutdownBehavior, func(res *awsec2.DescribeInstanceAttributeOutput) {
out.InstanceInitiatedShutdownBehavior = res.InstanceInitiatedShutdownBehavior
})
describeAttribute(types.InstanceAttributeNameUserData, func(res *awsec2.DescribeInstanceAttributeOutput) {
out.UserData = res.UserData
})
err := eg.Wait()
return out, err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool. I didn't know about errgroup
. It looks much better now.
Signed-off-by: Max Melentyev <max.melentyev@reddit.com>
ca04245
to
9482248
Compare
Signed-off-by: Max Melentyev <max.melentyev@reddit.com>
fffb54a
to
6482b89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you very much @max-melentyev!
Description of your changes
Instance observation makes 4 AWS API calls. Running this calls concurrently reduces reconciliation time. Here is 0.99 quantile of reconciliation duration
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
with existing unit tests