Add TransformationMapping#validate_vms method #17177

bzwei · 2018-03-20T20:03:14Z

Add #validate_vms method to support API /api/transformation_mappings/id action search_vms_and_validate.

If the action provides no csv content, the method should select all VMs that are qualified for migration based on the mapping.
If csv is provided in the POST body, the method should filter all selections and group them into valid_vms, invalid_vms, and conflict_vms.
Selected attributes of each VM are included for displaying purpose. id is critical to be used by following up API call to create a transformation plan.

bzwei · 2018-03-20T20:04:30Z

@miq-bot add_label enhancement, transformation
@miq-bot assign @gmcculloug
cc @jntullo @AparnaKarve @abellotti @eclarizio

AparnaKarve · 2018-03-20T20:08:26Z

Thanks @bzwei -- that's the information I was looking for!

bzwei · 2018-03-20T20:13:05Z

This is an initial PR that can support API development work.
Enhancement/spec should be done by follow-up PRs. @eclarizio

bdunne · 2018-03-26T20:32:34Z

How is this data (csv list) used / stored later on?
Any reason not to set this up as a regular validation?
Adding code without tests 👎

Fryguy · 2018-03-26T21:10:30Z

Agreed... why CSV? if this is coming from an API, I would expect a proper JSON array of array.

Even if we did use CSV, the model should only be coded against an array of arrays, IMO, and something else above the model should be translating a csv to an array of arrays.

Fryguy · 2018-03-26T21:12:12Z

app/models/transformation_mapping.rb

+
+  def select_vms
+    # TBD
+  end


Why add a method if you aren't going to use it...just add it later when you need it.

It is an sample for our team developer. It will be filled with logic very soon. It is urgently needed.

I think @Fryguy is saying anyone can add the ternary above when there is a usage for it.

@bzwei Can you confirm that @lfu is already working on the followup PR to this? This is not for some possible future usage, it will be used imminently.

@gmcculloug The follow up PR is coming today. Working on the test cases.

Fryguy · 2018-03-26T21:14:15Z

app/models/transformation_mapping.rb

+      conditions = {:name => vm_name}
+
+      # construct conditions or selections based on MIQ exported csv columns
+      conditions[:uid_ems] = row['UID'] if row['UID'].present?


This feels bad to have the API use arbitrary case for keys. Just use lower case for everything, and, I would go farther to say that it should just use uid_ems as the key name to avoid confusion, but that could be discussed.

All the keys are the headers from the csv, which can be an exported from manageiq, or directly created by user through an external system. All the headers exported from manageiq are capitalized. Examples are Name, Provider, Cluster, IP Addresses, Allocated Size etc. All columns are optional. We will do our best to identify the VMs according to provided columns.

Is uid_ems a good name for users who do not use manageiq exports?

My original thoughts is for the UI to read the csv file and simply convert to JSON without any modification, and pass to backend. Backend will select the columns of interest.

Do you think the UI should select columns of interest and convert the headers to low case? In the future if there is new column added, then both UI and backend need to make changes.

What are "MIQ exported csv columns"? Do we generate this in reporting or something? If it's based on our data, why wouldn't we just use our id's?

This is an existing miq csv export functionality. When you browse an inventory of VMs you have an option to download the VMs in a CSV file. It does not contain our Id's.
But this is only one way to provide the csv file for v2v migration use. User can choose to create their own csv file using whatever method they want.

Fryguy · 2018-03-26T21:15:27Z

app/models/transformation_mapping.rb

+      # construct conditions or selections based on MIQ exported csv columns
+      conditions[:uid_ems] = row['UID'] if row['UID'].present?
+      vms = Vm.where(conditions)
+      vms = vms.select { |vm| vm.host.name == row['Host'] } if row['Host'].present?


This could be just part of the query itself...

Vm.where(conditions).where(:host => {:name => row['Host']})

Fryguy · 2018-03-26T21:16:50Z

app/models/transformation_mapping.rb

+
+      # construct conditions or selections based on MIQ exported csv columns
+      conditions[:uid_ems] = row['UID'] if row['UID'].present?
+      vms = Vm.where(conditions)


Instead of building up a conditions Hash, prefer building up queries.

query = Vm.where(:name => vm_name) query = query.where(:uid_ems => row['UID']) if row['UID'].present? query = query.where(:host => {:name => row['Host']}) if row['Host'].present? vms = query.to_a

Fryguy · 2018-03-26T21:18:04Z

app/models/transformation_mapping.rb

+  def describe_non_vm(vm_name)
+    {
+      N_("Name")   => vm_name,
+      N_("Reason") => "VM does not exist"


I don't understand why we are translating keys of a Hash. Is this for presentation purposes? If so, then this doesn't belong here at all, as presentation logic does not belong in a model. Instead, the model should return a "normal" Hash, and something else can translate that for presentation purposes.

@Fryguy This looks OK to me.
The N_ notation means that strings would be marked for translation, but the actual translation would happen on the client side.

@bzwei Can you change the Reason line to this -
N_("Reason") => N_("VM does not exist")

I would also expect the hash to look something like {:name => vm_name, :reason => "Some reason", ...} and have the presenter handle translations.

"Name" and "Reason" are presentation strings visible in the UI, yes.

But they are NOT translated in the model.
By specifying N_ we simply make sure that the i18n rake task will pick up those strings and put them in the gettext catalog, which will be then sent to the translators.

bzwei · 2018-03-27T12:26:42Z

@Fryguy @bdunne This is a pioneer PR for other developers to take over. I here laid out the overall structure to handle both cases. spec tests to be added by following up PR soon with other enhancement.

I have the variable name csv, it really is a UI/API parsed content. I can rename it if it makes a reader to think it is a raw csv.

Fryguy · 2018-03-29T18:39:28Z

@AparnaKarve Presentation details do not belong in the model at all is my point. Things like "Name" is a presentation details (regardless of the i18n aspects)

Fryguy · 2018-03-29T18:43:53Z

app/models/transformation_mapping.rb

+      query = Vm.where(:name => vm_name)
+      query = query.where(:uid_ems => row['UID']) if row['UID'].present?
+      query = query.where(:host => {:name => row['Host']}) if row['Host'].present?
+      query = query.where(:ext_management_system => {:name => row['Provider']}) if row['Provider'].present?


I don't like that the incoming vm_list array of hashes has "human readable" strings (aka presentation strings). These should ideally be lower-cased strings or symbols and probably should align tightly to the column names. In fact, if the incoming vms_list was a hash where the column names were keys, that could be passed directly to the query. In other words, the 4 lines above are really a conversion from "csv -formatted" to "database-formatted", which IMO doesn't belong here...the csv -> hash conversion belongs with something like a class that deals with the CSV details itself.

Fryguy · 2018-03-29T18:45:19Z

app/models/transformation_mapping.rb

+      "Allocated Size" => vm.allocated_disk_storage,
+      "ID"             => vm.id,
+      "Reason"         => reason
+    }


My point was lost here in that these two methods are all presentation logic and belong outside of this model in something that deals with "csv" data. I still don't agree that CSV is needed at all as that is a terrible transfer medium, particular at the REST API level, where an API user expects JSON.

AparnaKarve · 2018-03-29T19:04:32Z

Presentation details do not belong in the model at all is my point. Things like "Name" is a presentation details (regardless of the i18n aspects)

@Fryguy I thought this through, and for this particular case, we can keep the model free of all i18n aspects.
Although, I want to point out that there are cases in our codebase where the model does provision for i18n (N_)

EDIT: It's the context of where we are applying i18n matters.
The Reason string here, for example, "VM does not exist" does need i18n
The context also decides if we need to prefix it with _ or N_

bzwei · 2018-03-29T20:55:08Z

@Fryguy I changed all keys to low cases.
cc @lfu

gmcculloug · 2018-04-20T21:14:17Z

@AparnaKarve It would be best if we setup some time next week to review. I beliee there are still concerns around the presentation details in the model as well as possible performance concerns.

I think it would be best to try to address these when people are back in the office next week.

AparnaKarve · 2018-04-20T21:47:10Z

@gmcculloug Sounds good. Thanks.

bdunne · 2018-04-23T20:11:07Z

app/models/transformation_mapping.rb

+  end
+
+  def describe_vm(vm, reason)
+    # formate to be finalized


Typo "format"

this line can be removed now.

bdunne · 2018-04-23T20:15:37Z

app/models/transformation_mapping.rb

+    vm_list.each do |row|
+      vm_name = row['name']
+
+      if row['name'].blank?


Should this use the variable assigned above?

bdunne · 2018-04-23T20:20:07Z

app/models/transformation_mapping.rb

+      query = query.joins(:ext_management_system).where(:ext_management_systems => {:name => row['provider']}) if row['provider'].present?
+
+      vms = query.to_a
+      invalid_list << describe_non_vm(vm_name) if vms.empty?


Shouldn't this be part of the if conditional below to avoid going into the else condition on line 64?

vms is empty. So line 64 will not loop.

But why even enter the else which is intended for the "array of vms" case?

gmcculloug · 2018-04-26T13:02:31Z

@Fryguy @bzwei Had a meeting to discuss and determine how to move this forward. We agreed that both the query logic and the return structures need to be updated.

The current changes, while not optimal, do allow the feature to work and be demonstrated which the refactoring work progresses.

@Fryguy Can you confirm here that we are good to merge this now and will create followup PRs to address the issues we identified.

miq-bot · 2018-04-26T17:27:36Z

Checked commits bzwei/manageiq@04887ae~...43563ba 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. 🏆

Fryguy · 2018-04-26T19:27:05Z

@gmcculloug This is fine, as long as we prioritize the follows for after the demo.

Add TransformationMapping#validate_vms method (cherry picked from commit eede7da)

simaishi · 2018-05-29T19:11:04Z

Gaprindashvili backport details:

$ git log -1
commit 131d3113317647ff209796d096295815323bc86b
Author: Greg McCullough <[email protected]>
Date:   Thu Apr 26 17:49:21 2018 -0400

    Merge pull request #17177 from bzwei/validate_vms_mapping
    
    Add TransformationMapping#validate_vms method
    (cherry picked from commit eede7da3cd5f6728d84e9179d6dcbdc77623fda8)

miq-bot added enhancement v2v labels Mar 20, 2018

miq-bot assigned gmcculloug Mar 20, 2018

This was referenced Mar 20, 2018

[WIP] Identify a VM by references #17166

Closed

Add ServiceTemplate.pre_creation_check for api use #17171

Closed

bzwei force-pushed the validate_vms_mapping branch from 3333461 to 3436c6f Compare March 20, 2018 20:16

jntullo mentioned this pull request Mar 21, 2018

[WIP] Add validate_vms resource action to transformation mappings ManageIQ/manageiq-api#352

Closed

bzwei force-pushed the validate_vms_mapping branch 4 times, most recently from 58d0036 to dd2894f Compare March 22, 2018 23:07

Fryguy reviewed Mar 26, 2018

View reviewed changes

bzwei force-pushed the validate_vms_mapping branch from dd2894f to ea9e13d Compare March 28, 2018 18:43

Fryguy reviewed Mar 29, 2018

View reviewed changes

bzwei force-pushed the validate_vms_mapping branch from ea9e13d to a9b7b8a Compare March 29, 2018 20:52

bzwei force-pushed the validate_vms_mapping branch from a9b7b8a to d454361 Compare April 3, 2018 21:35

bdunne reviewed Apr 23, 2018

View reviewed changes

Add TransformationMapping#validate_vms method

04887ae

lfu force-pushed the validate_vms_mapping branch 3 times, most recently from 964f4c0 to f9993d3 Compare April 25, 2018 19:42

bzwei changed the title ~~Add TransformationMapping#validate_vms method~~ Add TransformationMapping#search_vms_and_validate method Apr 25, 2018

lfu force-pushed the validate_vms_mapping branch 2 times, most recently from 978b97f to 41b1e8f Compare April 25, 2018 20:50

lfu force-pushed the validate_vms_mapping branch 3 times, most recently from 59fd6a4 to 964f4c0 Compare April 26, 2018 17:09

Add v2v transformation validation.

43563ba

lfu force-pushed the validate_vms_mapping branch from 964f4c0 to 43563ba Compare April 26, 2018 17:18

gmcculloug changed the title ~~Add TransformationMapping#search_vms_and_validate method~~ Add TransformationMapping#validate_vms method Apr 26, 2018

gmcculloug merged commit eede7da into ManageIQ:master Apr 26, 2018

gmcculloug added this to the Sprint 85 Ending May 7, 2018 milestone Apr 26, 2018

bzwei deleted the validate_vms_mapping branch April 27, 2018 13:17

lfu mentioned this pull request Apr 27, 2018

Add VmMigrationValidator. #17364

Merged

gmcculloug added the gaprindashvili/yes label May 17, 2018

simaishi pushed a commit that referenced this pull request May 29, 2018

Merge pull request #17177 from bzwei/validate_vms_mapping

131d311

Add TransformationMapping#validate_vms method (cherry picked from commit eede7da)

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

Add TransformationMapping#validate_vms method #17177

Add TransformationMapping#validate_vms method #17177

Conversation

bzwei commented Mar 20, 2018 • edited by gmcculloug Loading

bzwei commented Mar 20, 2018

AparnaKarve commented Mar 20, 2018

bzwei commented Mar 20, 2018

bdunne commented Mar 26, 2018

Fryguy commented Mar 26, 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

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

Choose a reason for hiding this comment

AparnaKarve Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Mar 27, 2018

Fryguy commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AparnaKarve commented Mar 29, 2018 • edited Loading

bzwei commented Mar 29, 2018

gmcculloug commented Apr 20, 2018

AparnaKarve commented Apr 20, 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

gmcculloug commented Apr 26, 2018

miq-bot commented Apr 26, 2018

Fryguy commented Apr 26, 2018

simaishi commented May 29, 2018

bzwei commented Mar 20, 2018 •

edited by gmcculloug

Loading

Fryguy commented Mar 26, 2018 •

edited

Loading

AparnaKarve Mar 26, 2018 •

edited

Loading

AparnaKarve commented Mar 29, 2018 •

edited

Loading