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

Don't hardcode max_attempts, as it leads to errors when AMI builds lo… #194

Merged

Conversation

stefanhorning
Copy link
Contributor

@stefanhorning stefanhorning commented Nov 11, 2020

…ng and timeout setting won't have any effect.

SUMMARY

For AMIs that will take a little longer you will get a

botocore.exceptions.WaiterError: Waiter ImageAvailable failed: Max attempts exceeded

Increasing the wait_timeout module param won't help you, as this module - for some reason - has a hardcoded max_attempts value which is not relative to the wait_timeout value (as most other modules do it).

So I adjusted that section to follow the same pattern as other AWS modules, such as ec2_ami_copy, ec2_instance etc.

Also while at it I increased the default wait time to 1200 as this won't make it necessary for the user to set the wait_timeout param in most cases and it doesn't hurt to have a high value here as we have retries every 15 seconds anyway.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ec2_ami module

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to submit this PR,

As far as I can tell the original logic would still have worked, it tweaked the time between retries rather than the number of retries. So you'd get 30 retries spread over X minutes (retrying every X2 seconds), instead of X2 retries (retrying every 30 seconds)
The advantage of your logic over the original logic is that someone who says "You can wait for 3 hours" doesn't have to wait for 6 minutes before it retries the first time. As such I'm +1 to the logic change, but please add a changelog entry.

@stefanhorning
Copy link
Contributor Author

Well, all I can say that the module reproducibly failed on bigger AMIs and with this fix it works. Also it is the same fix that has been added after a similar issue with ec2_ami_copy, see ansible/ansible#37111 and PR https://github.com/ansible/ansible/pull/37680/files also is streamlines the waiting logic with other AWS modules, such as ec2_instance https://github.com/ansible-collections/community.aws/blob/main/plugins/modules/ec2_instance.py#L1307 and potentially others. However I am not too familiar with the // function in python, so you might be right.

In any case I will add a changelog entry, so we can get this merged.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Thanks for that change. Minor niggle on the formatting/wording, but once the tests pass I think we'll be good to merge.

@jillr
Copy link
Collaborator

jillr commented Nov 16, 2020

Also lgtm, with the proposed updated changelog verbiage - thanks @stefanhorning

plugins/modules/ec2_ami.py Outdated Show resolved Hide resolved
…ng and timeout setting won't have any effect.
@stefanhorning
Copy link
Contributor Author

@tremble Rebased and fixed now.

@tremble tremble merged commit a94cb4c into ansible-collections:main Nov 20, 2020
@tremble
Copy link
Contributor

tremble commented Nov 20, 2020

@stefanhorning :
Thanks for bearing with us and taking the time to submit this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants