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

Add retries for AppScaling policies throttling exceptions #1430

Merged
merged 1 commit into from
Aug 30, 2017

Conversation

s-maj
Copy link
Contributor

@s-maj s-maj commented Aug 16, 2017

Fix for #1428, allows retry on DescribeScalingPolices throttling error.

TF_ACC=1 go test ./aws -v -run=TestAccAWSAppautoScalingPolicy -timeout 120m
=== RUN   TestAccAWSAppautoScalingPolicy_basic
--- PASS: TestAccAWSAppautoScalingPolicy_basic (128.68s)
=== RUN   TestAccAWSAppautoScalingPolicy_spotFleetRequest
--- PASS: TestAccAWSAppautoScalingPolicy_spotFleetRequest (84.73s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	213.435s

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 17, 2017
@s-maj
Copy link
Contributor Author

s-maj commented Aug 21, 2017

Hi @Ninir! This is a showstopper for folks that are using Application Scaling. Default TPS limit for Cloudwatch Describe action is 9 so it's quite low. If have 35 App Scaling policies, which is quite common when you're using ECS, 3 on 5 plan/apply will fail. In other words, this is seriously affecting ECS users that want to utilize AppScaling.

What I am trying to say, it's more bug than enhancement.

@Ninir
Copy link
Contributor

Ninir commented Aug 21, 2017

@s-maj Got it :)

Do you think you could provide a TF configuration (acceptance test) including a lot of alarms, etc, so that we can reproduce it and validate please?

Would help a lot for the review :)

Thanks!

@Ninir Ninir added bug Addresses a defect in current functionality. waiting-response Maintainers are waiting on response from community or contributor. and removed enhancement Requests to existing resources that expand the functionality or scope. labels Aug 21, 2017
@s-maj
Copy link
Contributor Author

s-maj commented Aug 22, 2017

Sure thing, but first I would like to add some extra background. Initially, a very similar issue was reported aws/aws-sdk-go#1376 but since AWS API is quirky, this is working as designed (client is responsible for handling this, not SDK). Then #1085 was submitted and merged.
I asked a question aws/aws-sdk-go#1472 and looking at the answer this is also working as expected. I had a chat with AWS support they confirmed that this also should be implemented on the client-side.
Now, testing can be quirky too:
If you have a really slow Internet connection/lack of CPU, this error will never occur to you.
If you have increased cloudwatch DescribeAlarms limit (yep, it's per account setting, see: http://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/cloudwatch_limits.html) it will affect how this issue is triggered.

@s-maj
Copy link
Contributor Author

s-maj commented Aug 22, 2017

You can test it by using something like this: https://gist.github.com/s-maj/0bf81fef71274d31605d4caf43631a2c. Apply and then refresh or try to destroy it :)

Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Hi @s-maj

Thanks for the config you provided, was able to replicate the issue & validate that the suggested change is ok!

Ran tests just "in-case" 😄

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppautoScaling'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAppautoScaling -timeout 120m
=== RUN   TestAccAWSAppautoScalingPolicy_basic
--- PASS: TestAccAWSAppautoScalingPolicy_basic (124.82s)
=== RUN   TestAccAWSAppautoScalingPolicy_spotFleetRequest
--- PASS: TestAccAWSAppautoScalingPolicy_spotFleetRequest (85.28s)
=== RUN   TestAccAWSAppautoScalingTarget_basic
--- PASS: TestAccAWSAppautoScalingTarget_basic (116.06s)
=== RUN   TestAccAWSAppautoScalingTarget_spotFleetRequest
--- PASS: TestAccAWSAppautoScalingTarget_spotFleetRequest (84.49s)
=== RUN   TestAccAWSAppautoScalingTarget_emrCluster
--- PASS: TestAccAWSAppautoScalingTarget_emrCluster (602.44s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1013.147s

Thanks a LOT for the work here :)

@Ninir Ninir merged commit 4166b7a into hashicorp:master Aug 30, 2017
nbaztec pushed a commit to nbaztec/terraform-provider-aws that referenced this pull request Sep 26, 2017
Add retries for AppScaling policies throttling exceptions
@ghost
Copy link

ghost commented Apr 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 11, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants