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

Provide better error message when migrating to the same host #14155

Merged
merged 1 commit into from
Mar 22, 2017

Conversation

jelkosz
Copy link
Contributor

@jelkosz jelkosz commented Mar 3, 2017

In case the VM is migrated to the same host it is already running on, a
misleading error message is shown in the Requests:
"There are no hosts to use. Check that the cluster contains at least one host
in Up state"

Fixed by failing the migration with the better message:
"Error: The VM 'vm name' can not be migrated to the same host it is already
running on."

The host is not filtered out from the "Migrate Virtual Machine" screen since this
screen can be used to migrate more VMs at once and what is a good host for one
VM is not for the other.

@jelkosz jelkosz force-pushed the fail-migration-with-better-error branch from 25f9fdf to af195ce Compare March 3, 2017 10:02
@jelkosz
Copy link
Contributor Author

jelkosz commented Mar 3, 2017

@miq-bot assign @borod108

@borod108
Copy link

borod108 commented Mar 6, 2017

@jelkosz is there a test checking this functionality? I guess there are non for the error at least...

@isimluk
Copy link
Member

isimluk commented Mar 10, 2017

What if we put this check to VmOrTemplate::Operations::Relocation.raw_migrate ?

There are already similar sanity checks. What do you think?

@jelkosz jelkosz force-pushed the fail-migration-with-better-error branch 2 times, most recently from 590b1d0 to 39d48dc Compare March 17, 2017 13:54
@jelkosz
Copy link
Contributor Author

jelkosz commented Mar 17, 2017

@isimluk I find it a bit too abstract place for it - I would anyway have to delegate this check to the specific provider to parse out the data they need for the check.

@jelkosz jelkosz force-pushed the fail-migration-with-better-error branch from 39d48dc to 90fc9b4 Compare March 21, 2017 12:56
@jelkosz
Copy link
Contributor Author

jelkosz commented Mar 21, 2017

@isimluk you are right, it is a better place, done

current_host_id = Host.where(:id => host_id).pluck(:uid_ems).first
destination_host_id = host.uid_ems

if current_host_id == destination_host_id
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: do we need to use uid_ems?

If we do just if host_id == host.id # current host equals destination host we could avoid extra select from db.

In case the VM is migrated to the same host it is already running on, a
misleading error message is shown in the Requests:
"There are no hosts to use. Check that the cluster contains at least one host
in Up state"

Fixed by failing the migration with the better message:
"Error: The VM 'vm name' can not be migrated to the same host it is already
running on."

The host is not filtered out from the "Migrate Virtual Machine" screen since this
screen can be used to migrate more VMs at once and what is a good host for one
VM is not for the other.
@jelkosz jelkosz force-pushed the fail-migration-with-better-error branch from 90fc9b4 to f4bc176 Compare March 21, 2017 16:23
@jelkosz
Copy link
Contributor Author

jelkosz commented Mar 21, 2017

@isimluk right, done

@miq-bot
Copy link
Member

miq-bot commented Mar 21, 2017

Checked commit jelkosz@f4bc176 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

Copy link
Member

@isimluk isimluk left a comment

Choose a reason for hiding this comment

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

👍 Thanks @jelkosz ! This is perfect.

@isimluk isimluk self-assigned this Mar 22, 2017
@isimluk isimluk added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 22, 2017
@isimluk isimluk merged commit 9764860 into ManageIQ:master Mar 22, 2017
simaishi pushed a commit that referenced this pull request Apr 13, 2017
Provide better error message when migrating to the same host
(cherry picked from commit 9764860)

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

Euwe backport details:

$ git log -1
commit 22bf18ab11e337d6de0354ab93f4785c7cccbef7
Author: Šimon Lukašík <isimluk@fedoraproject.org>
Date:   Wed Mar 22 10:58:11 2017 +0100

    Merge pull request #14155 from jelkosz/fail-migration-with-better-error
    
    Provide better error message when migrating to the same host
    (cherry picked from commit 976486006c16f51478718409200b818d509c7a14)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1436222

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