Host targeted refresh deleting and re-creating storage, switch, and lan records #336

agrare · 2019-01-25T14:38:08Z

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

Add some tests to catch a host targeted refresh causing storage, switch, and lan records to be deleted+recreated.

agrare · 2019-01-25T16:44:35Z

app/models/manageiq/providers/redhat/inventory/collector/target_collection.rb

@@ -213,7 +213,7 @@ def infer_related_host_ems_refs_db!
    changed_hosts.each do |host|
      add_simple_target!(:ems_clusters, uuid_from_target(host.ems_cluster))
      host.storages.each do |storage|
-        add_simple_target!(:storagedomains, uuid_from_target(storage))
+        add_simple_target!(:storagedomains, storage.ems_ref)


This was comparing the wrong property:

references(:storagedomains) ["cd4cf9b7-e1d4-4ef6-a3ed-06adf72c8031"] Storage.pluck(:ems_ref) ["/api/storagedomains/072fbaa1-08f3-4a40-9f34-a5ca22dd1d74", "/api/storagedomains/cd4cf9b7-e1d4-4ef6-a3ed-06adf72c8031"]

agrare · 2019-01-25T17:55:37Z

@borod108 the switches reference is empty in the specs, it looks like we get the switch targets and the switches in the parser two different ways.

agrare · 2019-01-25T19:04:47Z

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_graph_target_host_spec.rb

 describe ManageIQ::Providers::Redhat::InfraManager::Refresh::Refresher do
+  include OvirtRefresherSpecCommon
+
  describe ManageIQ::Providers::Redhat::InfraManager::Refresh::Refresher do


@borod108 why are these describe blocks duplicated?

For no good reason I can think off.

jerryk55 · 2019-01-31T21:55:04Z

@agrare @borod108 @roliveri Just as background, the issue described in https://bugzilla.redhat.com/show_bug.cgi?id=1669176 appears to be fixed by @agrare's change to the storage refresh, and backing out the removal of the network refresh from #316. Seeing if the specs pass now...

Code removed by PR ManageIQ#316 caused the problem with disconnected networks indicated in https://bugzilla.redhat.com/show_bug.cgi?id=1669176

miq-bot · 2019-01-31T22:11:40Z

Checked commits agrare/manageiq-providers-ovirt@7f9ada6~...1a0117d 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. 🏆

mwperina · 2019-02-01T06:34:04Z

Moti, could you please review?
@masayag

borod108

Looks very good to me, better and cleaner. The specs are broken thought.

agrare · 2019-02-01T14:02:07Z

@borod108 yeah I fixed the storages but the networks are still an issue. I tried checking what host.network_attachments returns here but in the specs it didn't return anything.

jerryk55 · 2019-02-01T14:08:47Z

@agrare @borod108 @roliveri unfortunately my last commit, while fixing the issue tracked here, results in the same error message that https://bugzilla.redhat.com/show_bug.cgi?id=1658240 fixed. I'm not sure if that's worse that the issue fixed by this PR or not.

masayag · 2019-02-06T08:28:17Z

@borod108 yeah I fixed the storages but the networks are still an issue. I tried checking what host.network_attachments returns here but in the specs it didn't return anything.

@agrare @borod108 seems like we're missing the proper method of fetching the network_attachments in this context. We should do something similar to:

https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/models/manageiq/providers/redhat/inventory/collector.rb#L141
connection.follow_link(host.network_attachments)

Also updated refresh recording

mwperina · 2019-02-06T12:18:36Z

Does anyone have an environment to quickly verify this change so we can merge?

borod108 · 2019-02-06T12:30:02Z

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_graph_target_host_spec.rb

      expect(host.switches.map { |switch| [switch.uid_ems, switch.name] }).to contain_exactly(
        a_collection_containing_exactly("00000000-0000-0000-0000-000000000009", "ovirtmgmt"),
        a_collection_containing_exactly("5c42817f-03fb-460e-a3ab-a8770553aeee", "vlan123t")
      )
-      expect(host.networks.count).to eq(2)


Just wondering, if we removed this maybe we should also remove the second refresh - (line 63-64)

masayag · 2019-02-06T12:30:43Z

Does anyone have an environment to quickly verify this change so we can merge?

@mwperina, @borod108 uploaded a commit to this branch that incorporated the proposed changes and updated the test as well.

ATM the spec are failing, but the code fixes the issue reported by @agrare here

agrare · 2019-02-06T13:52:21Z

app/models/manageiq/providers/redhat/inventory/collector/target_collection.rb

+        add_simple_target!(:storagedomains, storage.ems_ref)
+      end
+      host.switches.each do |switch|
+        add_simple_target!(:networks, switch.uid_ems)


This is why the specs are currently failing, this should be add_simple_target!(:switches, switch.uid_ems)

agrare · 2019-02-06T13:56:19Z

app/models/manageiq/providers/redhat/inventory/collector/target_collection.rb

-        add_simple_target!(:networks, attachement.network.id)
+      manager.with_provider_connection(VERSION_HASH) do |connection|
+        connection.follow_link(host.network_attachments).each do |attachement|
+          add_simple_target!(:networks, attachement.network.id)


agrare · 2019-02-06T13:57:24Z

Okay @masayag @borod108 I pushed fixes for the manager_ref as a hash failures, now it is failing on vcr errors and will have to be re-recorded.

jerryk55 · 2019-02-06T14:23:04Z

@agrare the last commit you made still causes the V2V Infrastructure Mapping issue for missing Networks. It does make the manager_ref as hash failure error disappear though.

agrare · 2019-02-06T14:44:35Z

@jerryk55 one step at a time :D

agrare · 2019-02-06T16:12:55Z

Okay the issue was that we were still referencing networks when building the targeted arel: https://github.com/ManageIQ/manageiq-providers-ovirt/pull/336/files#diff-c9d64b2c3fed9ede6e26aefcbcb8b276R11

agrare · 2019-02-06T16:18:28Z

@masayag @borod108 it looks like this might need a re-record of the VCRs (I'm not sure why since we didn't change the collectors/parsers at all!?)

agrare · 2019-02-06T16:39:45Z

Testing against a live rhev manager seems to indicate that this is fixed the issue, I'm seeing the same switches and lans before and after host targeted refreshes so I think this just needs a vcr re-record for the specs to pass.

jerryk55 · 2019-02-06T17:59:57Z

FYI - still seeing the issue. not sure if the "one step at a time" comment is still in play...

agrare · 2019-02-06T18:11:31Z

Hey @jerryk55 I tried to reproduce what I think the issue is on a live system and didn't see the problem anymore.
The problem being after a targeted refresh of a host duplicate switches, lans, and storages were created and then the originals deleted after another full refresh.

What issue are you still seeing ?

jerryk55 · 2019-02-06T18:27:30Z

Hi @agrare. Creating an Infrastructure Mapping, then running the RHEV host refresh. The Infrastructure mapping shows the Networks missing. Datastore and Cluster are ok. Prior to your change the Datastore was also showing as missing. We had debugged this a while back and saw that the ids for the networks were changed causing the issue.

agrare · 2019-02-06T18:42:55Z

>> EmsRefresh.refresh(ems)
>> switches = Switch.pluck(:id, :name, :uid_ems)
=> [[1, "NFS", "ed64724b-4ece-488b-ac9e-3bcfabe09323"], [2, "ovirtmgmt", "00000000-0000-0000-0000-000000000009"]]
>> lans = Lan.pluck(:id, :name, :uid_ems)
=> [[1, "NFS", "ed64724b-4ece-488b-ac9e-3bcfabe09323"], [2, "ovirtmgmt", "00000000-0000-0000-0000-000000000009"]]
>> EmsRefresh.refresh(ems.hosts.first)
>> switches == Switch.pluck(:id, :name, :uid_ems)
=> true
>> lans == Lan.pluck(:id, :name, :uid_ems)
=> true

Looks the same to me, @jerryk55 is this on your local env or do you have a link to an appliance where you see this?

jerryk55 · 2019-02-06T18:44:57Z

@agrare link sent in gitter.

borod108 · 2019-02-06T19:11:34Z

@agrare any chance can you squash the commits and force push?

agrare · 2019-02-06T19:18:43Z

@borod108 okay I squashed the multiple vcr commits, I think the rest are helpful.

agrare · 2019-02-08T18:26:00Z

@borod108 / @mwperina can you take a look?

agrare · 2019-02-08T18:43:53Z

Thanks @masayag !

Host targeted refresh deleting and re-creating storage, switch, and lan records (cherry picked from commit 2509eda) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1674673

simaishi · 2019-02-11T16:53:11Z

Hammer backport details:

$ git log -1
commit 01630df8241d85cc5030cb8daa958b61cbc8872b
Author: Moti Asayag <masayag@redhat.com>
Date:   Fri Feb 8 20:43:38 2019 +0200

    Merge pull request #336 from agrare/bz_1669176_host_targeted_refresh
    
    Host targeted refresh deleting and re-creating storage, switch, and lan records
    
    (cherry picked from commit 2509eda7fb579da4c9f31653a04a2d2269e20931)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1674673

Add specs to catch deleted+created records

7f9ada6

Add some tests to catch a host targeted refresh causing storage, switch, and lan records to be deleted+recreated.

agrare added the wip label Jan 25, 2019

Fix host target storagedomains references

c395e41

agrare commented Jan 25, 2019

View reviewed changes

Add full table compare instead of specific checks

5e83d3b

agrare commented Jan 25, 2019

View reviewed changes

Fix RHEV Targetted Refresh Disconnected Networks

1a0117d

Code removed by PR ManageIQ#316 caused the problem with disconnected networks indicated in https://bugzilla.redhat.com/show_bug.cgi?id=1669176

jerryk55 force-pushed the bz_1669176_host_targeted_refresh branch from 4b75844 to 1a0117d Compare January 31, 2019 22:07

borod108 approved these changes Feb 1, 2019

View reviewed changes

Fixing fetching network_attachments

4acb9c9

Also updated refresh recording

borod108 reviewed Feb 6, 2019

View reviewed changes

agrare commented Feb 6, 2019

View reviewed changes

Use :switches inventory_collection not :networks

e13ae6e

agrare commented Feb 6, 2019

View reviewed changes

agrare changed the title ~~[WIP] Host targeted refresh deleting and re-creating storage, switch, and lan records~~ Host targeted refresh deleting and re-creating storage, switch, and lan records Feb 6, 2019

agrare added bug blocker and removed wip labels Feb 6, 2019

Need to reference switches not networks

d3d2661

agrare force-pushed the bz_1669176_host_targeted_refresh branch from d26eced to d3d2661 Compare February 6, 2019 19:16

Sort records by ID when doing full table compares

1b3c509

agrare assigned borod108 Feb 8, 2019

masayag approved these changes Feb 8, 2019

View reviewed changes

masayag merged commit 2509eda into ManageIQ:master Feb 8, 2019

agrare deleted the bz_1669176_host_targeted_refresh branch February 8, 2019 18:43

simaishi added the hammer/backported label Feb 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Host targeted refresh deleting and re-creating storage, switch, and lan records #336

Host targeted refresh deleting and re-creating storage, switch, and lan records #336

agrare commented Jan 25, 2019

agrare Jan 25, 2019

agrare commented Jan 25, 2019

agrare Jan 25, 2019

borod108 Feb 1, 2019

jerryk55 commented Jan 31, 2019

miq-bot commented Jan 31, 2019

mwperina commented Feb 1, 2019

borod108 left a comment

agrare commented Feb 1, 2019

jerryk55 commented Feb 1, 2019

masayag commented Feb 6, 2019

mwperina commented Feb 6, 2019

borod108 Feb 6, 2019

masayag commented Feb 6, 2019

agrare Feb 6, 2019

agrare Feb 6, 2019

agrare commented Feb 6, 2019

jerryk55 commented Feb 6, 2019 •

edited

Loading

agrare commented Feb 6, 2019

agrare commented Feb 6, 2019

agrare commented Feb 6, 2019

agrare commented Feb 6, 2019

jerryk55 commented Feb 6, 2019

agrare commented Feb 6, 2019

jerryk55 commented Feb 6, 2019

agrare commented Feb 6, 2019 •

edited

Loading

jerryk55 commented Feb 6, 2019

borod108 commented Feb 6, 2019

agrare commented Feb 6, 2019

agrare commented Feb 8, 2019

agrare commented Feb 8, 2019

simaishi commented Feb 11, 2019

Host targeted refresh deleting and re-creating storage, switch, and lan records #336

Host targeted refresh deleting and re-creating storage, switch, and lan records #336

Conversation

agrare commented Jan 25, 2019

agrare Jan 25, 2019

Choose a reason for hiding this comment

agrare commented Jan 25, 2019

agrare Jan 25, 2019

Choose a reason for hiding this comment

borod108 Feb 1, 2019

Choose a reason for hiding this comment

jerryk55 commented Jan 31, 2019

miq-bot commented Jan 31, 2019

mwperina commented Feb 1, 2019

borod108 left a comment

Choose a reason for hiding this comment

agrare commented Feb 1, 2019

jerryk55 commented Feb 1, 2019

masayag commented Feb 6, 2019

mwperina commented Feb 6, 2019

borod108 Feb 6, 2019

Choose a reason for hiding this comment

masayag commented Feb 6, 2019

agrare Feb 6, 2019

Choose a reason for hiding this comment

agrare Feb 6, 2019

Choose a reason for hiding this comment

agrare commented Feb 6, 2019

jerryk55 commented Feb 6, 2019 • edited Loading

agrare commented Feb 6, 2019

agrare commented Feb 6, 2019

agrare commented Feb 6, 2019

agrare commented Feb 6, 2019

jerryk55 commented Feb 6, 2019

agrare commented Feb 6, 2019

jerryk55 commented Feb 6, 2019

agrare commented Feb 6, 2019 • edited Loading

jerryk55 commented Feb 6, 2019

borod108 commented Feb 6, 2019

agrare commented Feb 6, 2019

agrare commented Feb 8, 2019

agrare commented Feb 8, 2019

simaishi commented Feb 11, 2019

jerryk55 commented Feb 6, 2019 •

edited

Loading

agrare commented Feb 6, 2019 •

edited

Loading