Check ems_ref before uid_ems when saving VMs #18616

agrare · 2019-04-02T17:07:19Z

When saving VMs only the uid_ems was considered when looking for
existing VMs when it is possible for the same VM to get a new UUID
leading to the same ems_ref and a different uid_ems.

For updating existing VMs any records with the same ems_id+ems_ref
should be considered before looking at the uid_ems.

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

agrare · 2019-04-02T17:07:59Z

Added specs over in ManageIQ/manageiq-providers-vmware#386

agrare · 2019-04-02T20:20:58Z

spec/models/ems_refresh/save_inventory_spec.rb

@@ -128,24 +128,15 @@
        EmsRefresh.save_vms_inventory(@ems, data)

        vms = Vm.all
-        expect(vms.length).to eq(3)


This spec is now actually testing a VM with the same ems_ref and a new uid_ems get updated and not archived:

128: data = raw_data_with_dups(@vm1, @vm2) => 129: EmsRefresh.save_vms_inventory(@ems, data) 130: 131: vms = Vm.all 132: expect(vms.length).to eq(2) 133: (byebug) Vm.all.map { |vm| [vm.ems_ref, vm.uid_ems] } [["vm-0000000000001", "fa7c21a7-2596-45b1-86b6-3ccb8b8c9273"], ["vm-0000000000002", "57a19467-b438-4e44-8710-fe4873066249"]] (byebug) data.map { |vm| [vm[:ems_ref], vm[:uid_ems]] } [["vm-0000000000001", "fa7c21a7-2596-45b1-86b6-3ccb8b8c9273"], ["vm-0000000000002", "fa7c21a7-2596-45b1-86b6-3ccb8b8c9273"]]

Since the second vm in the data payload has the same ems_ref as an active vm but with a different uid_ems.

Do the it descriptions reflect these changes (here and the next 2)? (hard to tell in the diff)

The descriptions were pretty general, "handle dups in the raw data" doesn't really describe what it is expecting to happen.

I'll update them to better describe the expected outcome.

If there is a vm record with the same ems_ref then update that even if it has a different uid_ems. https://bugzilla.redhat.com/show_bug.cgi?id=1695008

agrare · 2019-04-04T15:52:48Z

spec/models/ems_refresh/save_inventory_spec.rb

@@ -161,24 +151,14 @@
        EmsRefresh.save_vms_inventory(@ems, data)

        vms = Vm.all
-        expect(vms.length).to eq(3)


Similarly here:

(byebug) data.map { |v| [v[:ems_ref], v[:uid_ems]] } [["vm-0000000000001", "56346b53-84d8-460a-9f8f-7fed3132c3a9"], ["vm-0000000000002", "ada1a6be-af10-4325-a198-25bb3357ed03"]] (byebug) Vm.all.map { |v| [v[:ems_ref], v[:uid_ems]] } [["vm-0000000000001", "56346b53-84d8-460a-9f8f-7fed3132c3a9"], ["vm-0000000000002", "56346b53-84d8-460a-9f8f-7fed3132c3a9"]]

So we're picking the existing VM with the same ems_ref and updating the uid_ems to match the new data

agrare · 2019-04-04T15:56:09Z

spec/models/ems_refresh/save_inventory_spec.rb

@@ -209,24 +189,14 @@
        EmsRefresh.save_vms_inventory(@ems, data)

        vms = Vm.all
-        expect(vms.length).to eq(3)


Here we are reconnecting one VM and updating the other:

(byebug) Vm.all.map { |v| [v[:ems_id], v[:ems_ref], v[:uid_ems]] } [[nil, "vm-0000000000001", "17da5cea-f4bf-46a7-937d-8a247f211797"], [43000000000874, "vm-0000000000002", "17da5cea-f4bf-46a7-937d-8a247f211797"]] (byebug) data.map { |v| [v[:ems_id], v[:ems_ref], v[:uid_ems]] } [[nil, "vm-0000000000001", "17da5cea-f4bf-46a7-937d-8a247f211797"], [nil, "vm-0000000000002", "47f1864c-4b80-4b95-9195-38c8c54f71f0"]]

Fryguy · 2019-04-04T18:32:10Z

app/models/ems_refresh/save_inventory.rb

@@ -53,6 +53,8 @@ def save_vms_inventory(ems, hashes, target = nil, disconnect = true)
    ]
    remove_keys = child_keys + extra_infra_keys + extra_cloud_keys

+    vms_by_ems_ref = ems.vms_and_templates.index_by(&:ems_ref).except(nil)


Should this be non-archived only or do you want everything (or does vms_and_templates already handle that)? I'm concerned that if an old vm has an ems_ref that matches (say the case where they update vsphere or whatever), then it will clobber.

Using the association off of the ems automatically gets only the non-archived ones (aka the ones with an ems_id)

Fryguy · 2019-04-04T18:33:10Z

app/models/ems_refresh/save_inventory.rb

@@ -89,7 +91,7 @@ def save_vms_inventory(ems, hashes, target = nil, disconnect = true)

          # Find the Vm in the database with the current uid_ems.  In the event
          #   of duplicates, try to determine which one is correct.
-          found = vms_by_uid_ems[h[:uid_ems]] || []
+          found = Array(vms_by_ems_ref[h[:ems_ref]] || vms_by_uid_ems[h[:uid_ems]])


If you need an array you can change the original index creation to do group_by instead of index_by

Just be careful of your &. change below (just noticed)

miq-bot · 2019-04-04T19:02:05Z

Checked commits agrare/manageiq@ae7a083~...8d348d5 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

agrare added bug providers/inventory labels Apr 2, 2019

agrare mentioned this pull request Apr 2, 2019

Reconnect vms about to be disconnected ManageIQ/manageiq-providers-vmware#386

Closed

agrare force-pushed the bz_1695008_save_vms_inventory_check_ems_ref_before_uid_ems branch from 6a5a212 to c50628b Compare April 2, 2019 17:48

agrare changed the title ~~Check ems_ref before uid_ems when saving VMs~~ [WIP] Check ems_ref before uid_ems when saving VMs Apr 2, 2019

miq-bot added the wip label Apr 2, 2019

agrare commented Apr 2, 2019

View reviewed changes

agrare force-pushed the bz_1695008_save_vms_inventory_check_ems_ref_before_uid_ems branch from f975f5e to e794003 Compare April 3, 2019 18:49

Return vm records with the same ems_ref first

ae7a083

If there is a vm record with the same ems_ref then update that even if it has a different uid_ems. https://bugzilla.redhat.com/show_bug.cgi?id=1695008

agrare force-pushed the bz_1695008_save_vms_inventory_check_ems_ref_before_uid_ems branch from 4551dd3 to ae7a083 Compare April 4, 2019 14:40

agrare changed the title ~~[WIP] Check ems_ref before uid_ems when saving VMs~~ Check ems_ref before uid_ems when saving VMs Apr 4, 2019

agrare assigned Fryguy Apr 4, 2019

agrare removed the wip label Apr 4, 2019

ManageIQ deleted a comment from miq-bot Apr 4, 2019

agrare commented Apr 4, 2019

View reviewed changes

agrare added 2 commits April 4, 2019 12:06

Exclude vms with nil ems_refs from vms_by_ems_ref

0838a11

Always set ems_ref and ems_ref_obj

3512eff

Fryguy reviewed Apr 4, 2019

View reviewed changes

agrare added 2 commits April 4, 2019 14:41

Use group_by for vms_by_ems_ref

2673eec

Update spec test descriptions

8d348d5

Fryguy merged commit 51ff5e4 into ManageIQ:master Apr 12, 2019

Fryguy added this to the Sprint 109 Ending Apr 15, 2019 milestone Apr 12, 2019

agrare deleted the bz_1695008_save_vms_inventory_check_ems_ref_before_uid_ems branch April 12, 2019 14:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check ems_ref before uid_ems when saving VMs #18616

Check ems_ref before uid_ems when saving VMs #18616

agrare commented Apr 2, 2019 •

edited

Loading

agrare commented Apr 2, 2019

agrare Apr 2, 2019

Fryguy Apr 4, 2019

agrare Apr 4, 2019

agrare Apr 4, 2019

agrare Apr 4, 2019

agrare Apr 4, 2019

Fryguy Apr 4, 2019

agrare Apr 4, 2019

Fryguy Apr 4, 2019

agrare Apr 4, 2019

Fryguy Apr 4, 2019

miq-bot commented Apr 4, 2019

Check ems_ref before uid_ems when saving VMs #18616

Check ems_ref before uid_ems when saving VMs #18616

Conversation

agrare commented Apr 2, 2019 • edited Loading

agrare commented Apr 2, 2019

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

miq-bot commented Apr 4, 2019

agrare commented Apr 2, 2019 •

edited

Loading