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

Add VmMigrationValidator. #17364

Merged
merged 1 commit into from
Sep 20, 2018
Merged

Conversation

lfu
Copy link
Member

@lfu lfu commented Apr 27, 2018

Followup of #17177.

Refactor of the code.
Wrap the return structure into a new class.

@miq-bot assign @gmcculloug
@miq-bot add_label enhancement, transformation

cc @bzwei @Fryguy

Related to:
ManageIQ/manageiq-api#463
ManageIQ/manageiq-v2v#618

def select_vms
valid_list = []

@mapping.transformation_mapping_items.where(:source_type => EmsCluster).collect(&:source).collect(&:vms).flatten.each do |vm|
Copy link
Member

Choose a reason for hiding this comment

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

Any performance concerns around the number of objects being loaded and the amount of ruby processing being done here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am working on the performance improvement. Any suggestion is welcomed.

Copy link
Member

Choose a reason for hiding this comment

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

It could be 2 queries with:

cluster_ids = @mapping.transformation_mapping_items.where(:source_type => EmsCluster).pluck(:source_id)
Vm.where(:ems_cluster_id => cluster_ids).all.each do |vm|
.....

Maybe @kbrock or @NickLaMuro have other suggestions?

# VM has not been migrated before
return VM_VALID if vm_as_resources.blank?

return VM_MIGRATED if vm_as_resources.any? { |rsc| rsc.status == ServiceResource::STATUS_COMPLETED }
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to offload this string matching to the database?

Copy link
Member Author

Choose a reason for hiding this comment

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

We were trying to query the DB once, then process the results from the in memory objects.

Copy link
Member

Choose a reason for hiding this comment

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

This feels like a thing that would be better to do in the database rather than in ruby.

The .blank? above could be .empty? which will select count instead of loading all of the records.
Then this could be another .where(:status => ... ).empty? and still avoid loading all of those objects.

return VM_MIGRATED if vm_as_resources.any? { |rsc| rsc.status == ServiceResource::STATUS_COMPLETED }

# VM failed in previous migration
vm_as_resources.all? { |rsc| rsc.status == ServiceResource::STATUS_FAILED } ? VM_VALID : VM_IN_OTHER_PLAN
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@lfu lfu force-pushed the refactor_validator branch 4 times, most recently from fbc1021 to 6947306 Compare May 2, 2018 19:24
def select_vms
valid_list = []

@mapping.transformation_mapping_items.includes(:source => :vms).where(:source_type => EmsCluster).collect(&:source).flat_map(&:vms).each do |vm|
Copy link
Member

@NickLaMuro NickLaMuro May 2, 2018

Choose a reason for hiding this comment

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

Sorry, responding late to the ping from @bdunne 's comment from before.

I think the suggestion @bdunne did have was pretty decent and simple, and should work for the most part (repeating just for reference):

cluster_ids = @mapping.transformation_mapping_items.where(:source_type => EmsCluster).pluck(:source_id)
Vm.where(:ems_cluster_id => cluster_ids).all.each do |vm|
.....

Do we possibly want to change .where(:source_type => EmsCluster) to something like:

.where(:source_type => [EmsCluster] + EmsCluster.descendants)

Since there is one subclass of EmsCluster (not sure if that matters...).


My own suggestion would be flipping this query on it's head, and starting from Vm, and then you should be able to make it only one query:

sub_select = @mapping.transformation_mapping_items.select(:source_id)
                                                  .where(:source_type => EmsCluster)
Vm.where(:ems_cluster_id => sub_select)
#  Vm Load (1.2ms)  SELECT "vms".* FROM "vms" WHERE "vms"."type" IN ('Vm', 'ManageIQ::Providers::InfraManager::Vm', 'ManageIQ::Providers::CloudManager::Vm', 'VmServer', 'VmXen', 'ManageIQ::Providers::Kubevirt::InfraManager::Vm', 'ManageIQ::Providers::Redhat::InfraManager::Vm', 'ManageIQ::Providers::Microsoft::InfraManager::Vm', 'ManageIQ::Providers::Vmware::InfraManager::Vm', 'ManageIQ::Providers::Amazon::CloudManager::Vm', 'ManageIQ::Providers::Azure::CloudManager::Vm', 'ManageIQ::Providers::Google::CloudManager::Vm', 'ManageIQ::Providers::Openstack::CloudManager::Vm', 'ManageIQ::Providers::Vmware::CloudManager::Vm') AND "vms"."template" = $1 AND "vms"."ems_id" IN (SELECT "transformation_mapping_items"."source_id" FROM "transformation_mapping_items" WHERE "transformation_mapping_items"."source_type" = 'EmsCluster' AND "transformation_mapping_items"."transformation_mapping_id" = 1) [["template", "f"]]

Copy link
Member

Choose a reason for hiding this comment

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

This looks great - if you note, @NickLaMuro used a select not pluck here

@lfu lfu force-pushed the refactor_validator branch 3 times, most recently from 23b2d19 to 1591b52 Compare May 4, 2018 16:14
@lfu
Copy link
Member Author

lfu commented May 4, 2018

@kbrock @NickLaMuro Please review in terms of performance.
cc @bdunne @Fryguy

@lfu lfu force-pushed the refactor_validator branch from 1591b52 to 9ee3b23 Compare May 4, 2018 20:06
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Looking good.

Just have a comment on those 3 queries

end

def mapped_clusters
@mapped_clusters ||= EmsCluster.where(:id => @mapping.transformation_mapping_items.where(:source_type => 'EmsCluster').pluck(:source_id))
Copy link
Member

Choose a reason for hiding this comment

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

please change pluck to select`

it will do 1 query instead of 2 here

Copy link
Member

Choose a reason for hiding this comment

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

@kbrock Isn't the select slower overall since it instantiates ruby objects for each result rather than just the source_ids?

Also, it looks like they're both 2 queries...

$ rails c
Loading development environment (Rails 5.0.7)
irb(main):001:0> PxeImageType.first.pxe_images.select(:name)
PostgreSQLAdapter#log_after_checkout, connection_pool: size: 5, connections: 1, in use: 1, waiting_in_queue: 0
  PxeImageType Load (0.7ms)  SELECT  "pxe_image_types".* FROM "pxe_image_types" ORDER BY "pxe_image_types"."id" ASC LIMIT $1  [["LIMIT", 1]]
  PxeImageType Inst Including Associations (9.7ms - 1rows)
  PxeImage Load (0.4ms)  SELECT "pxe_images"."name" FROM "pxe_images" WHERE "pxe_images"."pxe_image_type_id" = $1  [["pxe_image_type_id", 1]]
  PxeImage Inst Including Associations (8.8ms - 1rows)
(Object doesn't support #inspect)
=> 
irb(main):002:0> exit

$ rails c
Loading development environment (Rails 5.0.7)
irb(main):001:0> PxeImageType.first.pxe_images.pluck(:name)
PostgreSQLAdapter#log_after_checkout, connection_pool: size: 5, connections: 1, in use: 1, waiting_in_queue: 0
  PxeImageType Load (0.5ms)  SELECT  "pxe_image_types".* FROM "pxe_image_types" ORDER BY "pxe_image_types"."id" ASC LIMIT $1  [["LIMIT", 1]]
  PxeImageType Inst Including Associations (6.0ms - 1rows)
   (0.3ms)  SELECT "pxe_images"."name" FROM "pxe_images" WHERE "pxe_images"."pxe_image_type_id" = $1  [["pxe_image_type_id", 1]]
=> ["abc"]
irb(main):002:0> exit

Copy link
Member

Choose a reason for hiding this comment

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

EmsCluster.where( # 1
  :id => @mapping.transformation_mapping_items.where(:source_type => 'EmsCluster').pluck(:source_id) # 2
)

If #2 uses a pluck, a query is run for #2 and a query for #1. -- 2 queries total.
If #2 uses a select, the #1 query will have #2 as a subquery. -- 1 query total.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems the query here has been changed that #1 query does not exist any more. So no matter using pluck or select, it is always one query. I would prefer to use pluck in this case.

@bdunne @kbrock Anything else to update? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I still request this change (there are 20 vm records in my example, 1 ems)

Using pluck forces all id values to come back from postgres, and then send a separate query to the server with all the values:

ExtManagementSystem.where(:id => VmOrTemplate.where("name like 'VC0DC0_C0_RP1%'").pluck(:ems_id)).to_a
   (1.1ms)  SELECT "vms"."ems_id" FROM "vms" WHERE (name like 'VC0DC0_C0_RP1%')
  ExtManagementSystem Load (0.5ms)  SELECT "ext_management_systems".* FROM "ext_management_systems" WHERE "ext_management_systems"."id" IN (1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1)
  ExtManagementSystem Inst Including Associations (0.3ms - 1rows)

Using select allows the block to be used in an inner select - fewer round trips, less logic to create the sql, many fewer local objects:

ExtManagementSystem.where(:id => VmOrTemplate.where("name like 'VC0DC0_C0_RP1%'").select(:ems_id)).to_a
  ExtManagementSystem Load (1.1ms)  SELECT "ext_management_systems".* FROM "ext_management_systems" WHERE "ext_management_systems"."id" IN (SELECT "vms"."ems_id" FROM "vms" WHERE (name like 'VC0DC0_C0_RP1%'))
  ExtManagementSystem Inst Including Associations (0.4ms - 1rows)

Copy link
Member Author

Choose a reason for hiding this comment

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

With pluck:

EmsCluster.where(:id => mapping.transformation_mapping_items.where(:source_type => 'EmsCluster').pluck(:source_id))
   (0.3ms)  SELECT "transformation_mapping_items"."source_id" FROM "transformation_mapping_items" WHERE "transformation_mapping_items"."transformation_mapping_id" = $1 AND "transformation_mapping_items"."source_type" = $2  [["transformation_mapping_id", 1], ["source_type", "EmsCluster"]]
  EmsCluster Load (0.5ms)  SELECT "ems_clusters".* FROM "ems_clusters" WHERE "ems_clusters"."id" IN (24, 12)
  EmsCluster Inst Including Associations (0.3ms - 2rows)

With select:

EmsCluster.where(:id => mapping.transformation_mapping_items.where(:source_type => 'EmsCluster').select(:source_id))
  EmsCluster Load (0.9ms)  SELECT "ems_clusters".* FROM "ems_clusters" WHERE "ems_clusters"."id" IN (SELECT "transformation_mapping_items"."source_id" FROM "transformation_mapping_items" WHERE "transformation_mapping_items"."transformation_mapping_id" = $1 AND "transformation_mapping_items"."source_type" = $2)  [["transformation_mapping_id", 1], ["source_type", "EmsCluster"]]
  EmsCluster Inst Including Associations (89.5ms - 2rows)

Copy link
Member

Choose a reason for hiding this comment

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

thanks Lucy
I do agree with @bdunne that the majority of cases pluck is better than select.
This case is just a little different because we weren't doing anything with the ids.

end

def mapped_storages
@mapped_storages ||= Storage.where(:id => @mapping.transformation_mapping_items.where(:source_type => 'Storage').pluck(:source_id))
Copy link
Member

@kbrock kbrock May 7, 2018

Choose a reason for hiding this comment

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

please change pluck to select

it will do 1 query instead of 2 here

end

def mapped_lans
@mapped_lans ||= Lan.where(:id => @mapping.transformation_mapping_items.where(:source_type => 'Lan').pluck(:source_id))
Copy link
Member

@kbrock kbrock May 7, 2018

Choose a reason for hiding this comment

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

please change pluck to select

it will do 1 query instead of 2 here

found
end

if vms.size.zero?
Copy link
Member

Choose a reason for hiding this comment

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

you could try: vms.empty?

@lfu lfu force-pushed the refactor_validator branch from 9ee3b23 to 8c22090 Compare May 7, 2018 17:56
@lfu
Copy link
Member Author

lfu commented May 31, 2018

cc @agrare

@lfu
Copy link
Member Author

lfu commented Jun 5, 2018

@Fryguy @bdunne Please review.

@miq-bot
Copy link
Member

miq-bot commented Jun 7, 2018

This pull request is not mergeable. Please rebase and repush.

end

def mapped_clusters
@mapped_clusters ||= EmsCluster.where(:id => @mapping.transformation_mapping_items.where(:source_type => 'EmsCluster').select(:source_id))
Copy link
Member

Choose a reason for hiding this comment

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

Prefer pluck over select for performance. pluck will return an array of integers, select will create an ActiveRecord::Relation of transformation_mapping_items which will take more memory and more time to instantiate.

end

def mapped_storages
@mapped_storages ||= Storage.where(:id => @mapping.transformation_mapping_items.where(:source_type => 'Storage').select(:source_id))
Copy link
Member

Choose a reason for hiding this comment

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

Prefer pluck over select.

end

def mapped_lans
@mapped_lans ||= Lan.where(:id => @mapping.transformation_mapping_items.where(:source_type => 'Lan').select(:source_id))
Copy link
Member

Choose a reason for hiding this comment

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

Prefer pluck over select.

next
end

vms = vm_objects.select do |vm|
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this be much more efficient to query in the database with a where on the vm_objects above?

@lfu lfu force-pushed the refactor_validator branch from 8c22090 to 0269fdb Compare June 11, 2018 17:58
@miq-bot
Copy link
Member

miq-bot commented Jun 29, 2018

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Aug 17, 2018

This pull request is not mergeable. Please rebase and repush.

@lfu lfu force-pushed the refactor_validator branch from 82edbee to 74fbb6d Compare August 22, 2018 12:26
Copy link
Contributor

@AparnaKarve AparnaKarve left a comment

Choose a reason for hiding this comment

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

@lfu Thanks! Looks good now.

Once this is merged, we will need the API and UI PRs to be created and merged too before the nightly build.
(I can work on those)

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Would you reconsider fixing the pluck statements?

end

def mapped_clusters
@mapped_clusters ||= EmsCluster.where(:id => @mapping.transformation_mapping_items.where(:source_type => 'EmsCluster').pluck(:source_id))
Copy link
Member

Choose a reason for hiding this comment

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

I still request this change (there are 20 vm records in my example, 1 ems)

Using pluck forces all id values to come back from postgres, and then send a separate query to the server with all the values:

ExtManagementSystem.where(:id => VmOrTemplate.where("name like 'VC0DC0_C0_RP1%'").pluck(:ems_id)).to_a
   (1.1ms)  SELECT "vms"."ems_id" FROM "vms" WHERE (name like 'VC0DC0_C0_RP1%')
  ExtManagementSystem Load (0.5ms)  SELECT "ext_management_systems".* FROM "ext_management_systems" WHERE "ext_management_systems"."id" IN (1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1)
  ExtManagementSystem Inst Including Associations (0.3ms - 1rows)

Using select allows the block to be used in an inner select - fewer round trips, less logic to create the sql, many fewer local objects:

ExtManagementSystem.where(:id => VmOrTemplate.where("name like 'VC0DC0_C0_RP1%'").select(:ems_id)).to_a
  ExtManagementSystem Load (1.1ms)  SELECT "ext_management_systems".* FROM "ext_management_systems" WHERE "ext_management_systems"."id" IN (SELECT "vms"."ems_id" FROM "vms" WHERE (name like 'VC0DC0_C0_RP1%'))
  ExtManagementSystem Inst Including Associations (0.4ms - 1rows)

def unmapped_lans(vm)
vm.lans - transformation_mapping_items.where(:source => vm.lans).collect(&:source)
def search_vms_and_validate(vm_list = nil)
VmMigrationValidator.new(self, vm_list).validate
Copy link
Member

Choose a reason for hiding this comment

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

Thank you so much for using a separate class instead of a module.

valid_list = []

vms = Vm.where(:ems_cluster => mapped_clusters).to_a
MiqPreloader.preload(vms, %i(lans storages))
Copy link
Member

Choose a reason for hiding this comment

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

Would using standard rails work here?

vms = Vm.where(:ems_cluster => mapped_clusters).includes(:lans, :storages).to_a

@lfu lfu force-pushed the refactor_validator branch from 74fbb6d to c465d7e Compare August 22, 2018 18:59
@gmcculloug
Copy link
Member

@AparnaKarve Based on #17364 (review) can you work on those PRs and link them here so we can ensure we have everything ready before merging.

@AparnaKarve
Copy link
Contributor

@lfu @gmcculloug So the VM discovery process seems to work well, but I did a little bit of testing with the CSV import process and got the following error -

  • I did a CSV download of all the VMs in the DB and fed that as input to the PlanWizard CSV Step, and saw this -
 {"error":{"kind":"bad_request","message":"PG::UndefinedTable: ERROR: missing FROM-clause entry for table \"hosts\"\nLINE 1: ...s\".\"source_type\" = $3)) AND \"vms\".\"name\" = $4 AND \"hosts\".\"n...\n ^\n: SELECT \"vms\".* FROM \"vms\" WHERE \"vms\".\"type\" IN ('Vm', 'ManageIQ::Providers::InfraManager::Vm', 'ManageIQ::Providers::CloudManager::Vm', 'VmServer', 'ManageIQ::Providers::Vmware::InfraManager::Vm', 'ManageIQ::Providers::Kubevirt::InfraManager::Vm', 'ManageIQ::Providers::Redhat::InfraManager::Vm', 'ManageIQ::Providers::Microsoft::InfraManager::Vm', 'VmXen', 'ManageIQ::Providers::Amazon::CloudManager::Vm', 'ManageIQ::Providers::Azure::CloudManager::Vm', 'ManageIQ::Providers::Google::CloudManager::Vm', 'ManageIQ::Providers::Openstack::CloudManager::Vm', 'ManageIQ::Providers::Vmware::CloudManager::Vm') AND \"vms\".\"template\" = $1 AND \"vms\".\"name\" = 'HostedEngine' AND \"vms\".\"ems_cluster_id\" IN (SELECT \"ems_clusters\".\"id\" FROM \"ems_clusters\" WHERE \"ems_clusters\".\"id\" IN (SELECT \"transformation_mapping_items\".\"source_id\" FROM \"transformation_mapping_items\" WHERE \"transformation_mapping_items\".\"transformation_mapping_id\" = $2 AND \"transformation_mapping_items\".\"source_type\" = $3)) AND \"vms\".\"name\" = $4 AND \"hosts\".\"name\" = $5 AND \"ext_management_systems\".\"name\" = $6","klass":"ActiveRecord::StatementInvalid"}}

The same CSV seems to work well with the prior implementation (valid_vms) which is in the current master.

I have identified the row in the CSV that is causing this error -- if I remove that row, it seems to work fine.

@lfu Let me know if you need more info.

@AparnaKarve
Copy link
Contributor

The new implementation does not work with Provider specified in the csv -

Name,Provider
HostedEngine,rhv42

if I remove the Provider entry and simply use Name, all is good.

Name
HostedEngine

"path" => vm.ext_management_system ? "#{vm.ext_management_system.name}/#{vm.parent_blue_folder_path(:exclude_non_display_folders => true)}" : '',
"allocated_size" => vm.allocated_disk_storage,
"id" => vm.id.to_s
)
Copy link
Contributor

Choose a reason for hiding this comment

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

One other thing that I noticed was the CSV process does not send this hash info to the frontend if the VM is invalid.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need at least the VM id at a minimum to populate the table properly without warnings

Copy link
Member Author

Choose a reason for hiding this comment

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

As long as the VM exists in DB, its id would be added into the hash and sent back to frontend.
With an invalid VM, most likely the VM is not found at all in DB. That is why there is no id in the result hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

A VM could exist in the DB and could be invalid, a scenario that we often come across while doing the CSV import step.

@lfu In fact, we saw a case like that yesterday.
The VM in the image below exists in the DB and is invalid i.e. cannot be migrated.

screen shot 2018-08-23 at 2 59 26 pm

Copy link
Member Author

Choose a reason for hiding this comment

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

Why does UI require VM's id? Can we use name instead?

Currently a VM has to be on the source clusters defined in the mapping to be considered with. It is implemented this way to improve the performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lfu Alright, with some adjustments in the UI, we could make this work.

The UI will not receive id's anyway if a random VM name (that is non-existent in the DB) is specified in the CSV

@lfu lfu force-pushed the refactor_validator branch from c465d7e to b1bb052 Compare August 24, 2018 20:53
{"valid" => valid_list}
end

def identify_vms
Copy link
Member Author

Choose a reason for hiding this comment

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

@kbrock @bdunne @Fryguy @NickLaMuro Could you please review this method? Thanks.

@AparnaKarve
Copy link
Contributor

@gmcculloug @lfu I tested this again today and all the issues reported earlier have been resolved.

This is GTG for me.
Right after this goes in, we also need to merge -
ManageIQ/manageiq-api#463 and
ManageIQ/manageiq-v2v#618

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

That method looks good. There is possibility that it brings back a lot of data, but I think in practice it looks pretty good.

I did see one optimization/duplicate code - but feel free to ignore this.

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)
Copy link
Member

Choose a reason for hiding this comment

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

That looks like a lot of includes. I guess things like storages will overlap a bunch, as will ems.

Are we expecting vm names to be relatively unique? So vm.where(...).size == vm_names.size (or at least close?)

I like what you did with vm_names - would it make sense to do something similar with uid_ems or the other optional fields?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is to retrieve all the VM objects from DB once, then go through the array of VMs to select those that meet all the specified fields.

VMware does not allow VMs with duplicated name and the name can't be empty.
I would think vm_names should be good enough.

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

invalid_storages = vm.datastores - mapped_storages
Copy link
Member

@kbrock kbrock Sep 10, 2018

Choose a reason for hiding this comment

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

can the these 5 lines be merged into a method?

Throwing this out there. Not sure if it is more clear to you. If not, ignore this.

def no_mapping_entry(invalid_list, data_type, new_records)
  return false if new_records.blank?
  invalid_list << "#{data_type}: %{list}" % {:list => new_records.collect(&:name).join(", ")}
  true
end

def validate_vm(vm, quick = true)
  invalid_list = []
  issue = no_mapping_entry(invalid_list, "storages", vm.datastores - mapped_storages)
  return no_mapping_msg(invalid_list) if issue && quick

  issue ||= no_mapping_entry(invalid_list, "lans", vm.lans - mapped_lans)
  # no need for quick response, since next response is 1 line down
  # don't need this comment for the actual source

  issue ? no_mapping_msg(invalid_list) : VM_VALID
end

Sorry for the code. I was working through the idea and I ended up writing more than I thought - it is not tested and maybe doesn't' meet your goals.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this idea!

@miq-bot
Copy link
Member

miq-bot commented Sep 12, 2018

This pull request is not mergeable. Please rebase and repush.

@lfu lfu force-pushed the refactor_validator branch 3 times, most recently from acbedf3 to 6d77f74 Compare September 13, 2018 16:36
@gmcculloug
Copy link
Member

@kbrock Are you good with this PR now? Is so would you mind approving it and I'll merge. Thanks.

@lfu lfu force-pushed the refactor_validator branch from 6d77f74 to 2d3e5aa Compare September 13, 2018 19:04
@miq-bot
Copy link
Member

miq-bot commented Sep 13, 2018

Checked commit lfu@2d3e5aa with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM

@gmcculloug gmcculloug merged commit 14b6847 into ManageIQ:master Sep 20, 2018
@gmcculloug gmcculloug added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 20, 2018
@lfu lfu deleted the refactor_validator branch September 29, 2018 14:31
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.

7 participants