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

Report accurate status on inactive VM #18181

Merged

Conversation

AparnaKarve
Copy link
Contributor

Currently in v2v, the VM Validator method in the CSV mode, provides incorrect status on an inactive VM -- the status reported is not_exist (as in, the VM does not exist)

This used to work prior to the refactor done in #17364

In this PR, the VM Validator method was fixed to get the accurate inactive status of the VM

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

screen shot 2018-11-08 at 2 46 59 pm

@AparnaKarve
Copy link
Contributor Author

/cc @lfu @michaelkro

@miq-bot add_label bug,transformation,hammer/yes

@@ -37,7 +37,7 @@ def identify_vms
conflict_list = []

vm_names = @vm_list.collect { |row| row['name'] }
vm_objects = Vm.where(:name => vm_names, :ems_cluster => mapped_clusters).includes(:lans, :storages, :host, :ext_management_system)
vm_objects = Vm.where(:name => vm_names)
Copy link
Member

Choose a reason for hiding this comment

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

For better performance, we want to do the SQL query once, then look at the in-memory VM objects for selection.
Should we have here:

 vm_objects = Vm.where(:name => vm_names).includes(:ems_cluster, :lans, :storages, :host, :ext_management_system)

Then later on line 55, we do:

vms = vm_objects.select { |vm| mapped_clusters.include?(vm.ems_cluster) }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lfu - made the above change

@JPrause
Copy link
Member

JPrause commented Nov 13, 2018

@miq-bot add_label blocker

This master list will have no other clauses on selection

apply the selection criteria just before VM validation
That being done, we don't need the `vm.active?` check in `vm_migration_status` again

https://bugzilla.redhat.com/show_bug.cgi?id=1639239
@AparnaKarve AparnaKarve force-pushed the bz1639239_report_status_on_inactive_vm branch from 0d8fdaf to 2791ee0 Compare November 13, 2018 22:32
@miq-bot
Copy link
Member

miq-bot commented Nov 13, 2018

Checked commits AparnaKarve/manageiq@7fda4a3~...2791ee0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@lfu lfu left a comment

Choose a reason for hiding this comment

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

LGTM

@JPrause
Copy link
Member

JPrause commented Nov 19, 2018

@AparnaKarve was there anything else needed before this can be merged.

@AparnaKarve
Copy link
Contributor Author

@JPrause This is good to go and can be merged.

@JPrause
Copy link
Member

JPrause commented Nov 27, 2018

@AparnaKarve thanks! Who can merge? Can you assign or ping someone.

@AparnaKarve
Copy link
Contributor Author

@roliveri ? ... since it's v2v related?

@roliveri roliveri merged commit 76df3db into ManageIQ:master Nov 29, 2018
@roliveri roliveri added this to the Sprint 100 Ending Dec 3, 2018 milestone Nov 29, 2018
@AparnaKarve AparnaKarve deleted the bz1639239_report_status_on_inactive_vm branch November 29, 2018 15:25
simaishi pushed a commit that referenced this pull request Nov 30, 2018
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 42687860cb869703bb464640c722fcac06a0ce10
Author: Richard Oliveri <oliveri.richard.github@gmail.com>
Date:   Thu Nov 29 09:58:03 2018 -0500

    Merge pull request #18181 from AparnaKarve/bz1639239_report_status_on_inactive_vm
    
    Report accurate status on inactive VM
    
    (cherry picked from commit 76df3dba524c59b7ab6a257dbb2770cffafe9aca)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1639239

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