-
Notifications
You must be signed in to change notification settings - Fork 32
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
Oracle-bm-timeout-fix #456
Conversation
c8f275a
to
499cfae
Compare
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.
🖖
no idea what is broken with the ci-integration-tests job |
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.
I'm ok with it as-is, but I have a suggestion inline.
3323e72
to
9856837
Compare
Raising a timeout exception instead of a generic exception allows for consumers of pycloudlib to retry when instance launching fails due to timing out. This is important when trying to launch BM instances which can sometimes fail to provision even when the Oracle API reports their being sufficient capacity.
11d5061
to
a24fc02
Compare
Add new class variable `ready_timeout` that can be customizable per cloud. Changed the default to 10m but kept EC2's at 40m. Also, updated the public function instance.wait() to explicitly accept an optional ready_timeout arg. If not given, will use the default value defined for that instance class.
a24fc02
to
b2ea654
Compare
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.
One more question inline.
@@ -15,6 +15,7 @@ class EC2Instance(BaseInstance): | |||
"""EC2 backed instance.""" | |||
|
|||
_type = "ec2" | |||
ready_timeout = 40 # To keep backwards compatibility (will lower soon) |
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.
ready_timeout = 40 # To keep backwards compatibility (will lower soon) | |
ready_timeout = 40 # To keep backwards compatibility (will lower soon™) |
(this is a joke, don't actually apply this)
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.
LGTM!
do not squash merge, rebase merge to keep the two separate commits please!