Fixes undefined method `name` error during CSV validation required for v2v migration #17650

AparnaKarve · 2018-06-28T14:26:29Z

Account for a case when there is no cluster or ext_management_system info available from the csv file.

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

Also requires v2v UI PR to show the new inactive VM state in the UI -
ManageIQ/manageiq-v2v#450

AparnaKarve · 2018-06-28T14:27:02Z

@bzwei Can you review this?

@miq-bot add_label bug

bzwei · 2018-06-28T14:53:05Z

app/models/transformation_mapping.rb

@@ -82,8 +82,8 @@ def describe_non_vm(vm_name)
  def describe_vm(vm, reason)
    {
      "name"           => vm.name,
-      "cluster"        => vm.ems_cluster.name,
-      "path"           => "#{vm.ext_management_system.name}/#{vm.parent_blue_folder_path(:exclude_non_display_folders => true)}",
+      "cluster"        => vm.ems_cluster.try(:name),


vm.ems_cluster.try(:name) || ''

bzwei · 2018-06-28T14:54:33Z

app/models/transformation_mapping.rb

-      "cluster"        => vm.ems_cluster.name,
-      "path"           => "#{vm.ext_management_system.name}/#{vm.parent_blue_folder_path(:exclude_non_display_folders => true)}",
+      "cluster"        => vm.ems_cluster.try(:name),
+      "path"           => vm.try(:ext_management_system) ? "#{vm.ext_management_system.name}/#{vm.parent_blue_folder_path(:exclude_non_display_folders => true)}" : "",


vm.ext_management_system ? "#{vm.ext_management_system.name}/#{vm.parent_blue_folder_path(:exclude_non_display_folders => true)}" : ''

bzwei · 2018-06-28T14:56:58Z

@miq-bot add_label transformation

AparnaKarve · 2018-06-28T15:02:43Z

Thanks @bzwei

chrispy1 · 2018-06-28T15:26:06Z

@miq-bot add_label blocker

AparnaKarve · 2018-06-28T15:29:36Z

@miq-bot add_label gaprindashvili/yes

AparnaKarve · 2018-06-28T18:04:42Z

@gmcculloug Can you please merge this? Thanks.

gmcculloug · 2018-06-28T18:47:33Z

@AparnaKarve I think this is the wrong fix. It looks like the vm is no longer valid (either orphaned or archived) so we should not be returning it in the list of available VMs.

Discuss with @bzwei about moving the VM into the invalid return set.

bzwei · 2018-06-28T18:50:15Z

@AparnaKarve in #validate_vm method we should check whether vm.ext_management_system exists. Return an error message if not.

AparnaKarve · 2018-06-28T18:55:38Z

@gmcculloug, I came across this issue after looking into a use case that @bthurber mentioned.

His csv file had the following entry (only the Name field)

Name
dev-windows-server-2012

@bthurber Can you confirm if the VM in your environment dev-windows-server-2012 is valid or orphaned or archived?

AparnaKarve · 2018-06-28T19:03:42Z

#validate_vm method we should check whether vm.ext_management_system exists. Return an error message if not.

@bzwei @gmcculloug That does make sense.

The other change is vm.ems_cluster.try(:name) || ''
For a VM, is is possible to have a cluster with no name?

gmcculloug · 2018-06-28T19:26:52Z

For the cluster issue, name is a column on the ems_clusters table so if you have a cluster the name properly will be available. My guess here is that the VM had no Provider and therefore was also disconnected from the cluster in the database.

AparnaKarve · 2018-06-28T19:46:56Z

The change in this PR was added in the describe_vm method

The VM in question may belong in the invalid list, but it still needs to be described to show the Cluster/Path details in the UI as shown below -

So to be able to describe, we do need all the try/nil checks that have been added in the PR regardless of the validity of the VM.
Hence I think the changes in the PR are valid.

Loicavenel · 2018-06-28T20:29:29Z

@bzwei can we review and merge ASAP?

bzwei · 2018-06-28T20:39:56Z

@AparnaKarve You're right. The changes are still needed to describe the VM. But we need to enhance #validate_vm method to raise an error message either when ext_management_system or ems_cluster is nil. It is good to have all the changes in one PR to make the fix complete.

I think you will need a BZ to back port the fix .

AparnaKarve · 2018-06-28T20:44:05Z

@bzwei Can you review 74c7efa?

bzwei · 2018-06-28T21:26:22Z

app/models/transformation_mapping.rb

@@ -5,6 +5,7 @@ class TransformationMapping < ApplicationRecord
  VM_MIGRATED = "migrated".freeze
  VM_NOT_EXIST = "not_exist".freeze
  VM_VALID = "ok".freeze
+  VM_ORPHANED_OR_ARCHIVED = "orphaned".freeze


change to VM_INACTIVE = "inactive".freeze

bzwei · 2018-06-28T21:29:28Z

app/models/transformation_mapping.rb

@@ -94,6 +95,11 @@ def validate_vm(vm, quick = true)
    # a valid vm must find all resources in the mapping and has never been migrated
    invalid_list = []

+    if vm.ext_management_system.nil?


remove Ln 98 - 102.
Add following to https://github.com/ManageIQ/manageiq/blob/master/app/models/vm.rb#L125

return TransformationMapping::VM_INACTIVE unless active?

With this change, the return reason I get is undefined and not VM_INACTIVE because the return happens from here - https://github.com/ManageIQ/manageiq/blob/master/app/models/transformation_mapping.rb#L99

and not from validate_v2v_migration

I think the active? check needs to happen before we call this block

unless valid_cluster?(vm) invalid_list << "cluster: %{name}" % {:name => vm.ems_cluster.name} return no_mapping_msg(invalid_list) if quick end

since the return is intercepted by the above block, because of which our return reason is undefined

@bzwei I had to go with the implementation in c2b8ec7, for the reason mentioned above.

bzwei · 2018-06-28T21:38:03Z

@AparnaKarve Please add a spec case for an inactive VM in the context of https://github.com/ManageIQ/manageiq/blob/master/spec/models/transformation_mapping_spec.rb#L46

bzwei · 2018-06-28T23:45:19Z

app/models/transformation_mapping.rb

@@ -94,6 +95,8 @@ def validate_vm(vm, quick = true)
    # a valid vm must find all resources in the mapping and has never been migrated
    invalid_list = []

+    return VM_INACTIVE unless vm.active?


Move this line into #validate_v2v_migration and move vm.validate_v2v_migration to the beginning of this method.

if I move vm.validate_v2v_migration to the beginning of this method, it would have to be a return call as shown below in order to avoid executing the unless valid_cluster?(vm) block

return vm.validate_v2v_migration

which won't work for other cases.

I understand that you are trying not to use the VM_INACTIVE constant in the TransformationMapping methods, but that does not seem to be easily doable.

@bzwei any suggestions for Aparna. We need this PR merged asap.

def validate_vm(vm, quick = true) validate_error = vm.validate_v2v_migration return validate_error if validate_error invalid_list = [] ... # remove vm.validate_v2v_migration

@AparnaKarve VM_VALID is a contant in TransformationMapping.

VM_INACTIVE is also a new constant in TransformationMapping added in this PR

@lfu just trying to understand why this approach is preferable compared to the approach taken in the PR

We are not trying to avoid using constants in this class. Logically active? check belongs to method #validate_v2v_transformation.
You will still need to add

return TransformationMapping::VM_INACTIVE unless active?

in Vm#validate_v2v_migration.
Lucy has a refactoring PR to restructure the constants and methods. For now this is the solution.

In the interest of time, I can just go ahead and make the above changes now
(although I am not sure how this approach is better than the PR approach)

@bzwei Does the above code look OK? If yes, I can push an update shortly.
Thanks.

New update pushed. deacfdc

bzwei · 2018-06-29T17:38:43Z

app/models/transformation_mapping.rb

    # a valid vm must find all resources in the mapping and has never been migrated
    invalid_list = []

+    return VM_INACTIVE unless vm.active?


remove this line return VM_INACTIVE unless vm.active?. Looks good else.

Yes, agree that should have been removed. Thanks

miq-bot · 2018-06-29T17:53:02Z

Checked commits AparnaKarve/manageiq@a1d9804~...04d8b3e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

JPrause · 2018-06-29T18:57:37Z

@AparnaKarve @bzwei are reviews/edits complete? If yes, who can merge.

bzwei · 2018-06-29T19:04:50Z

LGTM. But I can't merge. @mkanoor please help.

JPrause · 2018-06-29T19:15:58Z

Thanks all! Team effort 👍

Fixes undefined method `name` error during CSV validation required for v2v migration (cherry picked from commit 5ff11a6) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596850

simaishi · 2018-06-29T19:20:11Z

Gaprindashvili backport details:

$ git log -1
commit 27c37f1f3e9ca64db05dfbe348bb06b31e19c6d8
Author: Madhu Kanoor <[email protected]>
Date:   Fri Jun 29 15:12:11 2018 -0400

    Merge pull request #17650 from AparnaKarve/fix_csv_describe_vm
    
    Fixes undefined method `name` error during CSV validation required for v2v migration
    (cherry picked from commit 5ff11a6f899bd9fac9f4146fbbb23b9869995fb9)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596850

miq-bot added the bug label Jun 28, 2018

bzwei reviewed Jun 28, 2018

View reviewed changes

miq-bot added the v2v label Jun 28, 2018

Accounts for a case when there is no cluster or ems

a1d9804

AparnaKarve force-pushed the fix_csv_describe_vm branch from eb044f4 to a1d9804 Compare June 28, 2018 15:02

miq-bot added the blocker label Jun 28, 2018

miq-bot added the gaprindashvili/yes label Jun 28, 2018

bzwei approved these changes Jun 28, 2018

View reviewed changes

AparnaKarve mentioned this pull request Jun 28, 2018

VMs in conflict could be valid hence make them selectable with a warning ManageIQ/manageiq-v2v#450

Merged

bzwei reviewed Jun 28, 2018

View reviewed changes

AparnaKarve added 2 commits June 28, 2018 15:39

Add inactive case for VM

c2b8ec7

spec for inactive vm

9206cc0

AparnaKarve force-pushed the fix_csv_describe_vm branch from 74c7efa to 9206cc0 Compare June 28, 2018 22:41

bzwei reviewed Jun 28, 2018

View reviewed changes

bzwei reviewed Jun 29, 2018

View reviewed changes

address PR feedback

04d8b3e

AparnaKarve force-pushed the fix_csv_describe_vm branch from deacfdc to 04d8b3e Compare June 29, 2018 17:41

mkanoor merged commit 5ff11a6 into ManageIQ:master Jun 29, 2018

mkanoor added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 29, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jun 29, 2018

AparnaKarve deleted the fix_csv_describe_vm branch June 29, 2018 20:07

Fixes undefined method name error during CSV validation required for v2v migration #17650

Fixes undefined method name error during CSV validation required for v2v migration #17650

Conversation

AparnaKarve commented Jun 28, 2018 • edited Loading

AparnaKarve commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Jun 28, 2018

AparnaKarve commented Jun 28, 2018

chrispy1 commented Jun 28, 2018

AparnaKarve commented Jun 28, 2018

AparnaKarve commented Jun 28, 2018

gmcculloug commented Jun 28, 2018

bzwei commented Jun 28, 2018

AparnaKarve commented Jun 28, 2018

AparnaKarve commented Jun 28, 2018

gmcculloug commented Jun 28, 2018

AparnaKarve commented Jun 28, 2018

Loicavenel commented Jun 28, 2018

bzwei commented Jun 28, 2018

AparnaKarve commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 29, 2018

JPrause commented Jun 29, 2018

bzwei commented Jun 29, 2018

JPrause commented Jun 29, 2018

simaishi commented Jun 29, 2018

Fixes undefined method `name` error during CSV validation required for v2v migration #17650

Fixes undefined method `name` error during CSV validation required for v2v migration #17650

AparnaKarve commented Jun 28, 2018 •

edited

Loading

AparnaKarve Jun 28, 2018 •

edited

Loading

AparnaKarve Jun 29, 2018 •

edited

Loading