Targeted refresh workaround for events that don't identify objects #323

mansam · 2018-07-24T17:00:06Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1596058 in combination with ManageIQ/manageiq-content#371 and ManageIQ/manageiq-content#372

Ladas · 2018-07-25T06:31:29Z

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

@@ -309,7 +311,7 @@ def infer_related_vm_ems_refs_db!

      all_stacks.each { |s| add_simple_target!(:orchestration_stacks, s.ems_ref, :tenant_id => s.cloud_tenant.ems_ref) }
      vm.cloud_networks.collect(&:ems_ref).compact.each { |ems_ref| add_simple_target!(:cloud_networks, ems_ref) }
-      vm.floating_ips.collect(&:address).compact.each { |address| add_simple_target!(:floating_ips, address) }
+      vm.floating_ips.collect(&:ems_ref).compact.each { |address| add_simple_target!(:floating_ips, address) }


probably rename also the other address to ems_ref

Ladas · 2018-07-25T06:40:08Z

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

-    @cloud_volume_backups = targets_by_association(:cloud_volume_backups).collect do |target|
-      scoped_get_backup(target.manager_ref[:ems_ref], target.options[:tenant_id])
-    end.compact
+    @cloud_volume_backups = cinder_service.handled_list(:list_backups_detailed, {:__request_body_index => "backups"}, cinder_admin?)


we will need specs for this, I think this way it would lead to duplication of existing cloud_volume_backups

I think what we could do to make this work on persistor could be switching :targeted => true/false. Given references will be [nil] or [], then targeted => true will not update or delete anything, since the targeted query will not return any records. With :targeted => false, we will do a full refresh of the cloud_volume_backups IC.

Same for the key_pairs. But lets do specs to verify this will work.

I'll write specs to make sure this behaves correctly. Why do you suspect it would result in duplicated objects?

so the targeted refresh takes the references and it builds a query, so it would be:

CloudVolumeBackups.where(:ems_ref => references(...))

So if this returns always [], the refresh will create anything built by the parser. In this case that would be all cloud_volume_backups are created again. While the correct behavior should be that the query returns the existing ones, and we just update them.

Local testing as well as my new specs suggest that this duplication does not occur.

builder.add_properties(:targeted => false) doesn't make a difference. I'm not sure why resource is being set conditionally, I think that happened during the builder refactor. I'll investigate that later. And it's fine to call TargetedCollection's key pair method, I just was mistakenly believing I'd see a different call when targeted => false

right, targeted => false should only change how we look at db. With targeted => true, we do ems.key_pairs.where(references(:vms)) with false we do ``````ems.key_pairs```

So with false it loads all key_pairs and it will delete those you will not .build in parser. While with true, it just loads the ones you've built, so it will just update.

in the spec, try also ems.key_pairs, since there might be the bug with :resource being nil

Doesn't seem to make a difference either way, though I'm continuing to investigate.

@Ladas Nevermind me, the spec wasn't written properly. >.>

Ladas · 2018-08-01T07:28:50Z

app/models/manageiq/providers/openstack/inventory/persister/definitions/cloud_collections.rb

@@ -123,9 +123,10 @@ def add_key_pairs(extra_properties = {})
    add_collection(cloud, :key_pairs, extra_properties) do |builder|
      builder.add_properties(
        :model_class => ManageIQ::Providers::Openstack::CloudManager::AuthKeyPair,
+        # targeted refresh workaround-- always refresh the whole keypair collection
+        :targeted    => false


I think we still need to conditionalize this , it should copy what we do in https://github.com/ManageIQ/manageiq-providers-openstack/pull/323/files#diff-0698b10bec7e7c31e8eabb30dc507213R140

So we fetch all entities only if there is something in references.

Now, it the references would be blank, we would delete all keypairs

Ladas · 2018-08-01T15:38:48Z

app/models/manageiq/providers/openstack/inventory/persister/definitions/storage_collections.rb

+      else
+        builder.add_default_values(:ems_id => manager.id)
+      end
+      # targeted refresh workaround-- always refresh the whole backup collection


I would probably write here why:

``Because OpenStack doesn't give us UUIDs of changed volume_backups, we just get an event that one of them changed```

Btw. is this a bug? :-D It's like super unhelpful event. :-) Or does it use ID we don't recognize?

Good call on the improved comment. I'm not sure whether this is officially a bug, but it is a really unhelpful event. There are some other IDs present like the request ID, but nothing that helps us as far as I know.

Ladas

Looks great 👍

Just a nit for a better comment, so we remember why we do this :-)

miq-bot · 2018-08-06T13:42:54Z

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

miq-bot · 2018-08-06T17:09:31Z

Checked commit mansam@90f75d0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 🍪

miq-bot · 2018-08-06T17:09:45Z

Some comments on commit mansam@90f75d0

spec/models/manageiq/providers/openstack/cloud_manager/stubbed_refresher_spec.rb

⚠️ - 196 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 197 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 198 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 199 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 200 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

aufi · 2018-08-14T11:49:49Z

Looks good! Merging.

mansam force-pushed the targeted-refresh-workaround branch from 7cfa21a to 1a3ebf9 Compare July 24, 2018 17:06

agrare assigned aufi Jul 24, 2018

Ladas reviewed Jul 25, 2018

View reviewed changes

mansam force-pushed the targeted-refresh-workaround branch 4 times, most recently from 65bc98a to eaa3e81 Compare July 31, 2018 20:52

Ladas reviewed Aug 1, 2018

View reviewed changes

mansam force-pushed the targeted-refresh-workaround branch from eaa3e81 to fe65bab Compare August 1, 2018 15:23

Ladas reviewed Aug 1, 2018

View reviewed changes

Ladas approved these changes Aug 1, 2018

View reviewed changes

mansam force-pushed the targeted-refresh-workaround branch from fe65bab to 8ef539e Compare August 1, 2018 16:19

miq-bot added the unmergeable label Aug 6, 2018

Turn off targeted refresh mode for keypairs and backups

90f75d0

mansam force-pushed the targeted-refresh-workaround branch from 8ef539e to 90f75d0 Compare August 6, 2018 17:03

miq-bot removed the unmergeable label Aug 6, 2018

aufi merged commit 85f4226 into ManageIQ:master Aug 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Targeted refresh workaround for events that don't identify objects #323

Targeted refresh workaround for events that don't identify objects #323

mansam commented Jul 24, 2018

Ladas Jul 25, 2018

mansam Jul 25, 2018

Ladas Jul 25, 2018

mansam Jul 25, 2018

Ladas Jul 25, 2018

mansam Jul 25, 2018

mansam Jul 25, 2018

mansam Jul 31, 2018

Ladas Jul 31, 2018

Ladas Jul 31, 2018

mansam Jul 31, 2018

mansam Jul 31, 2018

Ladas Aug 1, 2018 •

edited

Loading

Ladas Aug 1, 2018

mansam Aug 1, 2018

Ladas left a comment

miq-bot commented Aug 6, 2018

miq-bot commented Aug 6, 2018

miq-bot commented Aug 6, 2018

aufi commented Aug 14, 2018

Targeted refresh workaround for events that don't identify objects #323

Targeted refresh workaround for events that don't identify objects #323

Conversation

mansam commented Jul 24, 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

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

Ladas Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas left a comment

Choose a reason for hiding this comment

miq-bot commented Aug 6, 2018

miq-bot commented Aug 6, 2018

miq-bot commented Aug 6, 2018

aufi commented Aug 14, 2018

Ladas Aug 1, 2018 •

edited

Loading