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.

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

Configure Retryer and Max Retry to avoid throttling error #3578

Closed
wants to merge 1 commit into from

Conversation

Twsouza
Copy link
Contributor

@Twsouza Twsouza commented Sep 14, 2022

Use MaxRetry on aws.Config and set custom timers on DefaultRetryer.

Also, updates Go to 1.17 as golang.org/x/sys only allows 2 versions behind the latest one. Go 1.19 was released last month.

@codecov
Copy link

codecov bot commented Sep 14, 2022

Codecov Report

Base: 37.13% // Head: 37.07% // Decreases project coverage by -0.06% ⚠️

Coverage data is based on head (335f39f) compared to base (20c6dc2).
Patch coverage: 26.82% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3578      +/-   ##
==========================================
- Coverage   37.13%   37.07%   -0.07%     
==========================================
  Files         136      136              
  Lines       16643    16703      +60     
==========================================
+ Hits         6181     6192      +11     
- Misses       9425     9463      +38     
- Partials     1037     1048      +11     
Impacted Files Coverage Δ
pkg/cli/rack.go 66.54% <0.00%> (ø)
provider/aws/system.go 32.57% <0.00%> (-0.91%) ⬇️
provider/aws/aws.go 25.53% <34.37%> (-0.90%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@heronrs heronrs left a comment

Choose a reason for hiding this comment

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

It looks good but i don't think we need the wrapper around the DescribeStack operation.
We can configure the retry logic on all existing session objects.
To make it a bit more DRY you could have a helper function, like
helpers.NewSession() so you don't need to pass the same config over and over

@@ -379,14 +379,8 @@ func RackSync(rack sdk.Interface, c *stdcli.Context) error {

if c.String("name") != "" {
rname = c.String("name")
s, err := ss.NewSession(&aws.Config{})
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just configure the Retry/Throttling logic in this session instead?

@@ -71,12 +72,12 @@ func AwsErrorCode(err error) string {
return ""
}

func CloudformationDescribe(cf cloudformationiface.CloudFormationAPI, stack string) (*cloudformation.Stack, error) {
func CloudformationDescribe(stack string) (*cloudformation.Stack, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, looks like cf cloudformationiface.CloudFormationAPI is the provider.Cloudformation struct. We can change the session there.
https://github.com/convox/rack/blob/aws-retryer-throttling/provider/kaws/system.go#L271
https://github.com/convox/rack/blob/master/provider/kaws/kaws.go#L159

It's a good practice to have a retry/throtling logic so it's ok that we configure for the other services too. (in the initializeAwsServices function)

@@ -400,7 +401,7 @@ func (p *Provider) cleanup(app *structs.App) error {
shouldRetry := true

for i := 0; i < 60; i++ {
res, err := p.cloudformation().DescribeStacks(&cloudformation.DescribeStacksInput{
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, configure session in p;cloudformation().

provider/aws/lambda/autoscale/handler.go Show resolved Hide resolved
@@ -286,7 +287,7 @@ func (p *Provider) SystemInstall(w io.Writer, opts structs.SystemInstallOptions)
bar.Finish()
}

dres, err := cf.DescribeStacks(&cloudformation.DescribeStacksInput{
dres, err := pkgcf.DescribeStacks(&cloudformation.DescribeStacksInput{
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, use session config from L255

@@ -455,7 +456,9 @@ func (p *Provider) SystemUninstall(name string, w io.Writer, opts structs.System

cf := cloudformation.New(session.New(&aws.Config{}))
Copy link
Contributor

Choose a reason for hiding this comment

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

configure session here.

@@ -663,7 +666,7 @@ func cloudformationProgress(stack, token string, template []byte, w io.Writer) e
}
}

dres, err := cf.DescribeStacks(&cloudformation.DescribeStacksInput{
dres, err := pkgcf.DescribeStacks(&cloudformation.DescribeStacksInput{
Copy link
Contributor

Choose a reason for hiding this comment

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

configure session on L623

@@ -690,14 +693,12 @@ func cloudformationProgress(stack, token string, template []byte, w io.Writer) e
}

func rackDependencies(name string) ([]string, error) {
cf := cloudformation.New(session.New(&aws.Config{}))
Copy link
Contributor

Choose a reason for hiding this comment

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

configure session here.

@Twsouza Twsouza force-pushed the aws-retryer-throttling branch from 371937c to 850994f Compare September 15, 2022 15:44
@Twsouza Twsouza requested a review from heronrs September 15, 2022 15:45
Configure and use the session on each new session
@Twsouza Twsouza force-pushed the aws-retryer-throttling branch from 850994f to 335f39f Compare September 15, 2022 15:51
Copy link
Contributor

@heronrs heronrs left a comment

Choose a reason for hiding this comment

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

💯

@heronrs heronrs added this to the 20220914 milestone Sep 15, 2022
Twsouza added a commit that referenced this pull request Sep 15, 2022
Use MaxRetry on aws.Config and set custom timers on DefaultRetryer.

Also, updates Go to 1.17 as `golang.org/x/sys` only allows 2 versions behind the latest one. Go 1.19 was released last month.
@Twsouza Twsouza mentioned this pull request Sep 15, 2022
10 tasks
heronrs added a commit that referenced this pull request Sep 15, 2022
Use MaxRetry on aws.Config and set custom timers on DefaultRetryer.

Also, updates Go to 1.17 as `golang.org/x/sys` only allows 2 versions behind the latest one. Go 1.19 was released last month.
Twsouza added a commit that referenced this pull request Sep 15, 2022
Use MaxRetry on aws.Config and set custom timers on DefaultRetryer.

Also, updates Go to 1.17 as `golang.org/x/sys` only allows 2 versions behind the latest one. Go 1.19 was released last month.
Twsouza added a commit that referenced this pull request Sep 15, 2022
Use MaxRetry on aws.Config and set custom timers on DefaultRetryer.

Also, updates Go to 1.17 as `golang.org/x/sys` only allows 2 versions behind the latest one. Go 1.19 was released last month.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants