-
Notifications
You must be signed in to change notification settings - Fork 396
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 jittered_backoff to the iam_policy & iam_policy_info modules to handle AWS rate limiting #324
Conversation
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.
Hi @marknet15,
Thanks for working on this. Rather than exponential backoff I'd recommend using the jittered backoff: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/
Now that we're consistently using AnsibleAWSModule there's also a much cleaner option for adding the decorator: it can be passed as an option to module.client, and then you just need to add "aws_retry=True" to the various client calls. See e2051ae for an example, which is also using the jittered backoff.
Finally, please also add a changelog entry details can be found at https://docs.ansible.com/ansible/latest/reference_appendices/release_and_maintenance.html#changelogs
Mark
p.s. because the tests are closely coupled, it'd be really nice if you added the decorator to the iam_policy_info module too, but this is very much a "nice to have" rather than a must.
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.
Many thanks, I'll let the various test suites run through and then I think we're good to get this merged.
Hi @tremble Thanks very much for the review and help ! Sounds good :) |
Thanks for your work on this. For what it's worth I'm a big fan of getting the retries more widely applied (it really helps with CI flakes). Feel free to CC me (or ping me on IRC) on any more PRs like this. |
No problem at all 👍🏻 And thanks will do! |
…andle AWS rate limiting (ansible-collections#324) * Add jittered_backoff to handle AWS rate limiting * Fix for failing test * Add changelog fragment
…andle AWS rate limiting (ansible-collections#324) * Add jittered_backoff to handle AWS rate limiting * Fix for failing test * Add changelog fragment
iam_role_info jittered backoff SUMMARY Sometimes some rate limiting exceptions on the lookups still cause failures, I've switched the iam_role_info to jittered_backoff in a similar vein to: #324 ISSUE TYPE Bugfix Pull Request COMPONENT NAME iam_role_info Reviewed-by: Mark Chappell <None> Reviewed-by: None <None>
…andle AWS rate limiting (ansible-collections#324) * Add jittered_backoff to handle AWS rate limiting * Fix for failing test * Add changelog fragment
…nsible-collections#323)" (ansible-collections#324) Revert "WIP: aws_ec2 inventory add includes_entries_matching options (ansible-collections#323)" Reviewed-by: https://github.com/apps/ansible-zuul
…andle AWS rate limiting (ansible-collections#324) * Add jittered_backoff to handle AWS rate limiting * Fix for failing test * Add changelog fragment This commit was initially merged in https://github.com/ansible-collections/community.aws See: ansible-collections@be0042c
SUMMARY
Add jittered_backoff to the
iam_policy
&iam_policy_info
modules to handle AWS rate limiting to help avoid fatal errors in which no objects get retrieved or updated.ISSUE TYPE
COMPONENT NAME
iam_policy
,iam_policy_info
ADDITIONAL INFORMATION
Most other modules include some form of the AWS backoff / exponential_backoff logic to handle rate limiting however this particular module isn't currently.