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.

Sign up for GitHub

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

Jump to bottom

Check ems_ref before uid_ems when saving VMs #18616

Merged
merged 5 commits into from
Apr 12, 2019
Merged

Check ems_ref before uid_ems when saving VMs #18616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ae7a083
Return vm records with the same ems_ref first
agrare Apr 3, 2019
0838a11
Exclude vms with nil ems_refs from vms_by_ems_ref
agrare Apr 4, 2019
3512eff
Always set ems_ref and ems_ref_obj
agrare Apr 4, 2019
2673eec
Use group_by for vms_by_ems_ref
agrare Apr 4, 2019
8d348d5
Update spec test descriptions
agrare Apr 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions app/models/ems_refresh/save_inventory.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.group_by(&:ems_ref).except(nil)

# Query for all of the Vms once across all EMSes, to handle any moving VMs
vms_uids = hashes.collect { |h| h[:uid_ems] }.compact
vms = VmOrTemplate.where(:uid_ems => vms_uids).to_a
Expand Down Expand Up @@ -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 = vms_by_ems_ref[h[:ems_ref]] || vms_by_uid_ems[h[:uid_ems]] || []

if found.length > 1 || (found.length == 1 && found.first.ems_id)
found_dups = found
Expand All @@ -113,7 +115,7 @@ def save_vms_inventory(ems, hashes, target = nil, disconnect = true)
# build a type-specific vm or template
found = ems.vms_and_templates.klass.new(h)
else
vms_by_uid_ems[h[:uid_ems]].delete(found)
vms_by_uid_ems[h[:uid_ems]]&.delete(found)
h.delete(:type)

_log.info("#{log_header} Updating #{type} [#{found.name}] id: [#{found.id}] location: [#{found.location}] storage id: [#{found.storage_id}] uid_ems: [#{found.uid_ems}] ems_ref: [#{h[:ems_ref]}]")
Expand Down
77 changes: 26 additions & 51 deletions spec/models/ems_refresh/save_inventory_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,29 +123,19 @@
expect(v2.uid_ems).to eq(@vm2.uid_ems)
end

it "should handle dups in the raw data" do
it "should update the existing vm's uid_ems even if it is a duplicate" do
data = raw_data_with_dups(@vm1, @vm2)
EmsRefresh.save_vms_inventory(@ems, data)

vms = Vm.all
expect(vms.length).to eq(3)
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


disconnected, connected = vms.partition { |v| v.ems_id.nil? }
expect(disconnected.length).to eq(1)
expect(connected.length).to eq(2)

d = disconnected.first
c1, c2 = connected.sort_by(&:id)

expect(d.id).to eq(@vm2.id)
expect(d.uid_ems).to eq(@vm2.uid_ems)
expect(vms.length).to eq(2)
v1, v2 = vms.sort_by(&:id)

expect(c1.id).to eq(@vm1.id)
expect(c1.uid_ems).to eq(@vm1.uid_ems)
expect(v1.id).to eq(@vm1.id)
expect(v1.uid_ems).to eq(@vm1.uid_ems)

expect(c2.id).not_to eq(@vm1.id)
expect(c2.id).not_to eq(@vm2.id)
expect(c2.uid_ems).to eq(@vm1.uid_ems)
expect(v2.id).to eq(@vm2.id)
expect(v2.uid_ems).to eq(@vm1.uid_ems)
end
end

Expand All @@ -156,29 +146,19 @@
@vm2 = FactoryBot.create(:vm_with_ref, :ext_management_system => @ems, :uid_ems => @uid)
end

it "should handle no dups in the raw data" do
it "should update the duplicate records in the database with the new uid_ems" do
data = raw_data_without_dups(@vm1, @vm2)
EmsRefresh.save_vms_inventory(@ems, data)

vms = Vm.all
expect(vms.length).to eq(3)
Copy link
Member Author

Choose a reason for hiding this comment

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

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


disconnected, connected = vms.partition { |v| v.ems_id.nil? }
expect(disconnected.length).to eq(1)
expect(connected.length).to eq(2)

d = disconnected.first
c1, c2 = connected.sort_by(&:id)

expect(d.id).to eq(@vm2.id)
expect(d.uid_ems).to eq(@vm2.uid_ems)
expect(vms.length).to eq(2)
v1, v2 = vms.sort_by(&:id)

expect(c1.id).to eq(@vm1.id)
expect(c1.uid_ems).to eq(@vm1.uid_ems)
expect(v1.id).to eq(@vm1.id)
expect(v1.uid_ems).to eq(@vm1.uid_ems)

expect(c2.id).not_to eq(@vm1.id)
expect(c2.id).not_to eq(@vm2.id)
expect(c2.uid_ems).not_to eq(@vm1.uid_ems)
expect(v2.id).to eq(@vm2.id)
expect(v2.uid_ems).not_to eq(@vm1.uid_ems)
end

it "should handle dups in the raw data" do
Expand All @@ -204,29 +184,19 @@
@vm2 = FactoryBot.create(:vm_with_ref, :ext_management_system => @ems, :uid_ems => @uid)
end

it "should handle no dups in the raw data" do
it "should reconnect the disconnected vm and update the active vm" do
data = raw_data_without_dups(@vm1, @vm2)
EmsRefresh.save_vms_inventory(@ems, data)

vms = Vm.all
expect(vms.length).to eq(3)
Copy link
Member Author

Choose a reason for hiding this comment

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

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"]]


disconnected, connected = vms.partition { |v| v.ems_id.nil? }
expect(disconnected.length).to eq(1)
expect(connected.length).to eq(2)

d = disconnected.first
c1, c2 = connected.sort_by(&:id)

expect(d.id).to eq(@vm2.id)
expect(d.uid_ems).to eq(@vm2.uid_ems)
expect(vms.length).to eq(2)
v1, v2 = vms.sort_by(&:id)

expect(c1.id).to eq(@vm1.id)
expect(c1.uid_ems).to eq(@vm1.uid_ems)
expect(v1.id).to eq(@vm1.id)
expect(v1.uid_ems).to eq(@vm1.uid_ems)

expect(c2.id).not_to eq(@vm1.id)
expect(c2.id).not_to eq(@vm2.id)
expect(c2.uid_ems).not_to eq(@vm1.uid_ems)
expect(v2.id).to eq(@vm2.id)
expect(v2.uid_ems).not_to eq(@vm2.uid_ems)
end

it "should handle dups in the raw data" do
Expand Down Expand Up @@ -299,6 +269,7 @@

@ems_ref1 = @vm1.ems_ref_obj
@ems_ref2 = @vm2.ems_ref_obj
@vm1.ems_ref = @vm2.ems_ref = nil
@vm1.ems_ref_obj = @vm2.ems_ref_obj = nil
@vm1.save
@vm2.save
Expand All @@ -307,7 +278,9 @@
# TODO: DRY up these tests with the others just like them
it "should handle no dups in the raw data" do
data = raw_data_without_dups(@vm1, @vm2)
data[0][:ems_ref] = @ems_ref1
data[0][:ems_ref_obj] = @ems_ref1
data[1][:ems_ref] = @ems_ref2
data[1][:ems_ref_obj] = @ems_ref2
EmsRefresh.save_vms_inventory(@ems, data)

Expand All @@ -324,7 +297,9 @@

it "should handle dups in the raw data" do
data = raw_data_with_dups(@vm1, @vm2)
data[0][:ems_ref] = @ems_ref1
data[0][:ems_ref_obj] = @ems_ref1
data[1][:ems_ref] = @ems_ref2
data[1][:ems_ref_obj] = @ems_ref2
EmsRefresh.save_vms_inventory(@ems, data)

Expand Down