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

Check instance exists before getting status during provisioning #604

Merged

Conversation

A-Beck
Copy link
Contributor

@A-Beck A-Beck commented Mar 6, 2020

During AWS provisioning, I have been experiencing intermittent errors indicating invalid instance IDs ( longer trace attached ):

MIQ(ManageIQ::Providers::Amazon::CloudManager::Provision#provision_error) [[Aws::EC2::Errors::InvalidInstanceIDNotFound]: The instance ID 'i-xxxxxxxxxxxx' does not exist] encountered during phase [poll_clone_complete]

These requests did not include additional network interfaces. The solution posted to a similar issue - https://access.redhat.com/solutions/3638601 - did not help in my case.

This appears to be because the clone_task_check did not check if the instance existed before getting instance status. Since implementing this PR in my environment, I have not encountered errors during AWS provisioning.

aws-prov-trace-20200228-1258.log

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1830305

@djberg96
Copy link
Contributor

djberg96 commented Mar 6, 2020

👍

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

Nice catch! Couple of comments

status = instance.state.name.to_sym
return true if status == :running
return false, status
if instance.exists?
Copy link
Member

Choose a reason for hiding this comment

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

return false unless instance.exists?

status = instance.state.name.to_sym
return true if status == :running
return false, status

would look a little cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does look much cleaner - thanks for the suggestion! I will update the PR.

I still want to return a state, just so the log message here will continue to be useful.

https://github.com/ManageIQ/manageiq/blob/413d9e2c174052435e8c61834f626000fbeff83c/app/models/manageiq/providers/cloud_manager/provision/state_machine.rb#L46

Copy link
Member

Choose a reason for hiding this comment

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

👍 You're right return false, :pending if yadda yadda then

return true if status == :running
return false, status
if instance.exists?
status = instance.state.name.to_sym
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@miq-bot
Copy link
Member

miq-bot commented Mar 6, 2020

Checked commits A-Beck/manageiq-providers-amazon@195e03f~...ca0d2e0 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 👍

@agrare agrare self-assigned this Mar 6, 2020
@agrare agrare merged commit 262d51c into ManageIQ:master Mar 6, 2020
@agrare agrare added the bug label Mar 6, 2020
@agrare
Copy link
Member

agrare commented Mar 6, 2020

Thanks @A-Beck !

@A-Beck A-Beck deleted the check-instance-existance-before-getting-status branch March 6, 2020 18:15
@agrare
Copy link
Member

agrare commented May 1, 2020

simaishi pushed a commit that referenced this pull request May 1, 2020
…etting-status

Check instance exists before getting status during provisioning

(cherry picked from commit 262d51c)

https://bugzilla.redhat.com/show_bug.cgi?id=1830305
@simaishi
Copy link
Contributor

simaishi commented May 1, 2020

Jansa backport details:

$ git log -1
commit 0ef7a2c2611723eab527ebf04fad3747f0168beb
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Mar 6 13:14:32 2020 -0500

    Merge pull request #604 from A-Beck/check-instance-existance-before-getting-status

    Check instance exists before getting status during provisioning

    (cherry picked from commit 262d51c7ce5d41e061960618b816255bf5282e8b)

    https://bugzilla.redhat.com/show_bug.cgi?id=1830305

simaishi pushed a commit that referenced this pull request May 1, 2020
…etting-status

Check instance exists before getting status during provisioning

(cherry picked from commit 262d51c)

https://bugzilla.redhat.com/show_bug.cgi?id=1830305
@simaishi
Copy link
Contributor

simaishi commented May 1, 2020

Ivanchuk backport details:

$ git log -1
commit 3ddfa00cddfa93722d33801fc7c80bfd433ffb4d
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Mar 6 13:14:32 2020 -0500

    Merge pull request #604 from A-Beck/check-instance-existance-before-getting-status

    Check instance exists before getting status during provisioning

    (cherry picked from commit 262d51c7ce5d41e061960618b816255bf5282e8b)

    https://bugzilla.redhat.com/show_bug.cgi?id=1830305

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

Successfully merging this pull request may close these issues.

6 participants