-
Notifications
You must be signed in to change notification settings - Fork 169
retry non-duplicate ec2 register_image errors #274
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
base: master
Are you sure you want to change the base?
Conversation
|
Aminator currently only retries |
aminator/plugins/cloud/ec2.py
Outdated
| return False | ||
| # This could be a retryable error, such as ec2 api throttling | ||
| _tries -= 1 | ||
| if (tries > 0): |
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.
should this be
| if (tries > 0): | |
| if (_tries > 0): |
instead?
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.
yep, thanks!
bmoyles
left a comment
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.
Still bummed that Amazon doesn't support tag creation at image registration time like it does for other objects (volumes, snapshots, etc) as it would be nice to be able to get rid of the tagging step altogether and just do it in-line with registration.
@asher one thing I might offer up -- we could make failing on tag failure optional... We'll still have some identifying metadata embedded in the AMI name and the AMI description fields which gives the app, build number, the commit, the base AMI name, ID, and version. We would potentially lose one (or all) of:
base_ami_version(redundant, inancestor_versionindescription)creator(which is not always accurate, anyways, depending on where, when, and how the bake was triggered)creation_time(which, givencreationDateexists on the AMI, is redundant -- ourcreation_timetag reflects the time at which tags were added,creationDatereflects registration time if I'm not mistaken, and looking at some recent bakes, they differ by seconds at most)appversion-- this might be more annoying to miss as I know there are some internal tools that use it. All of the information inappversionis also available fromdescriptionEXCEPT the build job name
Given we have enough metadata in the name and description, seems like we could make tag failure optionally fatal and worst case, run something out-of-process later on that adds tags back to the image if they're missing...
| else: | ||
| log.critical("Unable to retry register_image due to ClientError: %s", e) | ||
| return False | ||
| # This could be a retryable error, such as ec2 api throttling |
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.
Are the current crop of retry-able errors identifiable by either a specific exception or message in the exception?
What this effectively does is add an alternative path for ClientError exceptions wherein exception['Error']['Code'] does not match InvalidAMIName.Duplicate that catches all other forms, so if you're just looking to retry all exceptions from ClientError I might consider getting rid of the specific foo around checking for InvalidAMIName.Duplicate in the error code and just always retry ClientError exceptions (which is basically what your else: block does).
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.
The bigger deal is if the exceptions thrown during registration are of some other type than ClientError...
No description provided.