-
Notifications
You must be signed in to change notification settings - Fork 60
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
Default to StandardError if a connection cannot be made #278
Conversation
@@ -50,7 +50,7 @@ def connection_rescue_block | |||
raise # Raise before falling into catch-all block below | |||
rescue StandardError => err | |||
_log.error("Error Class=#{err.class.name}, Message=#{err.message}, Backtrace=#{err.backtrace}") | |||
raise MiqException::MiqInvalidCredentialsError, _("Unexpected response returned from system: %{error_message}") % {:error_message => err.message} | |||
raise MiqException::MiqUnreachableError, _("Unexpected response returned from system: %{error_message}") % {:error_message => err.message} |
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.
Perhaps it makes sense to just re-raise err because we're not really sure it's unreachable at this point. We treat StandardError
exceptions the same as MiqUnreachableError
: we retry more frequently with an exponential backoff. The problem was that we were explicitly raising MiqInvalidCredentialsError
, an exception we don't retry as frequently.
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.
@jrafanie Fair enough, I'll change it.
Fall back to just using StandardError on connection failure.
Checked commit https://github.com/djberg96/manageiq-providers-azure/commit/79b6e737fb90140ab51ab4a77837ec29b195f9cf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.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.
LGTM
@djberg96 Can this be |
@simaishi Yes, tagged. |
Default to StandardError if a connection cannot be made (cherry picked from commit 745543c) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1601589
Gaprindashvili backport details:
|
Currently the
connection_rescue_block
is defaulting to aMiqCredentialsError
as a last resort when an unexpected connection error occurs. As per @jrafanie's advice, this should be changed to re-raiseStandardError
so that more graceful handling will occur via theAuthenticationMixin
module from the core repo, since it is now a "valid" error.It's also just more sensical since by the time we fall into this bucket, since it's probably not an actual credentials error.
While we obviously cannot solve problems on Azure's end (which is typically when this error will be reached), we can handle it more gracefully with this change.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1600968