Implement graph refresh for the Cinder manager #194

mansam · 2018-01-12T17:09:30Z

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

Ladas · 2018-01-12T20:33:24Z

app/models/manageiq/providers/openstack/cloud_manager.rb

@@ -78,7 +78,7 @@ def ensure_network_manager

  def ensure_cinder_manager
    return false if cinder_manager
-    build_cinder_manager(:type => 'ManageIQ::Providers::StorageManager::CinderManager')
+    build_cinder_manager(:type => 'ManageIQ::Providers::Openstack::StorageManager::CinderManager')


hm I don't think we can easily change the STI subclass of the manager now, since we would need a migration that will change existing records. Can we keep the old one? Since we need to backport this.

We can then do upstream only patch changing this, with migration.

I had a feeling that might be an issue. So to clarify, you'd recommend just leaving this line as build_cinder_manager(:type => 'ManageIQ::Providers::StorageManager::CinderManager') and then changing it later along with a migration in another patch?

yes. Since we can't backport the migration now, that would be only for upstream and future releases

Ladas · 2018-01-12T20:38:08Z

app/models/manageiq/providers/openstack/inventory/parser/storage_manager/cinder_manager.rb

+      attachment_names = {'vda' => 'Root disk', 'vdb' => 'Ephemeral disk', 'vdc' => 'Swap disk'}
+      persister.disks.find_or_build_by(
+        # FIXME: find works here, but lazy_find doesn't... I don't understand why
+        :hardware    => persister.hardwares.find(a["server_id"]),


can we do lazy_find ?

persister.hardwares.lazy_find(a["server_id"])

I'll change it and see if the tests agree ;) that comment was written quite a while ago and things have changed since then.

hum, ok, so if that is the case, we can keep there find for now, since hardwares should be always available (for full and targeted). Otherwise the find would degrade to N+1 queries

Ladas · 2018-01-12T20:51:29Z

app/models/manageiq/providers/openstack/storage_manager/cinder_manager/event_target_parser.rb

+    # there's almost always a tenant id regardless of event type
+    collect_identity_tenant_references!(target_collection, ems_event)
+
+    # WIP


right, this will be for standalone block storage based entities, can we also try to extract targets as part of Vm in collector?

e.g. amazon look both in db and API:
https://github.com/Ladas/manageiq-providers-amazon/blob/52636410835d6319627afd5e489cefe199e81a01/app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb#L203

https://github.com/Ladas/manageiq-providers-amazon/blob/52636410835d6319627afd5e489cefe199e81a01/app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb#L249

And seems like I miss extracting it from the db, it may be because the query would be too inefficient

It's currently the only way how to get full targeted refresh of a Vm

Yeah absolutely, volumes should be pulled as part of the full targeted vm refresh.

Ladas

@mansam Looks awesome, great work in a short time , we just need:

not to change the STI of manager
to make sure Vm refreshes with volumes (extending the Vm targeted refresh)
parsing of the events

And it's done :-)

Ladas · 2018-01-15T09:04:35Z

@mansam the CI failure is most probably caused by https://github.com/ManageIQ/manageiq/pull/16688/files#r161458751 , once this PR is ready, we can merge, that issue will be fixed separately.

Ladas · 2018-01-15T15:43:33Z

@mansam tomorrow is the last day, then it's non blocker only. Will you be able to finish until then?

mansam · 2018-01-15T17:06:50Z

@Ladas Yeah, I will get this finished by then.

mansam · 2018-01-15T23:36:16Z

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

+      # pull the attachments from the raw attribute to avoid Fog making an unnecessary call
+      # to inflate the volumes before we need them
+      vm.attributes.fetch('os-extended-volumes:volumes_attached', []).each do |attachment|
+        add_simple_target!(:cloud_volumes, attachment["id"])


@Ladas I'm having trouble getting the targeted collector to pick up the Cloud Volumes. They're added as simple targets here and I can see in my tests that this part works, but the collector method on line 194 never gets called, nor does the parser. I'm not sure what I've done differently that is causing these volume targets to not get parsed, so if you could take a look when you have time that would be much appreciated. I have a feeling I've just been staring at this too long and am simply missing something obvious. :)

@mansam responded https://github.com/ManageIQ/manageiq-providers-openstack/pull/194/files#r161680537

Ladas · 2018-01-16T07:57:58Z

app/models/manageiq/providers/openstack/builder.rb

+          target,
+          ManageIQ::Providers::Openstack::Inventory::Collector::StorageManager::CinderManager,
+          ManageIQ::Providers::Openstack::Inventory::Persister::StorageManager::CinderManager,
+          [ManageIQ::Providers::Openstack::Inventory::Parser::StorageManager::CinderManager]


@mansam for your question below, you need to ManageIQ::Providers::Openstack::Inventory::Parser::StorageManager::CinderManager

to line 30, so ManagerRefresh::TargetCollection (targeted refresh) invokes all parsers

Ladas · 2018-01-16T08:07:53Z

config/settings.yml

@@ -9,6 +9,11 @@
  :openstack_network:
    :is_admin: false
    :inventory_object_refresh: true
+    :allow_targeted_refresh: true
+  :cinder:
+    :is_admin: false


just a quick question, the :all_tenants param doesn't work for storage and network?

It does, but they're an option the defaults to being off because at some point someone told me we supported refresh with a non-admin user so I was trying to work around that.

Ladas · 2018-01-16T08:12:07Z

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

@@ -191,6 +191,14 @@ def vms_by_id
    @vms_by_id ||= Hash[vms.collect { |s| [s.id, s] }]
  end

+  def cloud_volumes


@mansam we need the same for cloud_volume_backups and cloud_volume_snapshots, otherwise the targeted refresh would be deleting those (because they would not be found in in the API)

Ladas · 2018-01-16T08:13:07Z

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

+    return [] if references(:cloud_volumes).blank?
+    return @cloud_volumes if @cloud_volumes.any?
+    @cloud_volumes = references(:cloud_volumes).collect do |volume_id|
+      safe_get { cinder_service.volumes.get(volume_id) }


@mansam just a sanity check, cinder_service.volumes.get(volume_id) works across differnet tenants, right?

It doesn't, sadly. cinder_service.volumes.all(:id => volume_id, :all_tenants => true) works, but that requires admin. I might have to just make a scoped request for each volume.

right, so probably lets target this as perf issues for next minor release? Could you do some perf test with 200-500 tenants (and more)? That is what we see in production. Then lets talk to Loic where to target this.

Or maybe I understand it badly? How many API queries will this cause with having 500 tenants?

Ladas

@aufi feel free to merge once @mansam fixes the remaining stuff and will have the tests passing.

Otherwise we would talk to Loic and turn this to blocker I suppose

Ladas · 2018-01-16T17:09:52Z

@mansam btw. there is a quick simple spec that the targeted refresh is not deleting something out if its scope https://github.com/Ladas/manageiq-providers-amazon/blob/5372348b633b8c7f37813388013e4e38af402a72/spec/models/manageiq/providers/amazon/cloud_manager/vcr_specs/targeted_refresh_scope_spec.rb#L38

Basically using existing full refresh, then targeted refresh, then full refresh asserts should pass, since the targeted refresh should just update existing data.

Might be for another PR, just to give it more coverage.

mansam · 2018-01-16T20:33:58Z

Two of the VCR instances have either gone down or stopped responding. I'll update the VCRs for this as soon as I can.

Ladas · 2018-01-18T13:57:26Z

@mansam so you can probably drop those VCR specs :-) I think I had reports that some of the envs were acting strange. Also, we should deploy new version, since most of the versions we test are EOL now. :-)

miq-bot · 2018-01-19T16:12:26Z

Some comments on commits mansam/manageiq-providers-openstack@3ff912e~...285fa65

spec/models/manageiq/providers/openstack/storage_manager/cinder_manager/event_target_parser_spec.rb

⚠️ - 6 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2018-01-19T16:12:30Z

Checked commits mansam/manageiq-providers-openstack@3ff912e~...285fa65 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
44 files checked, 5 offenses detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

app/models/manageiq/providers/openstack/cinder_manager_mixin.rb

❗ - Line 8, Col 5 - Rails/InverseOf - Specify an :inverse_of option.

app/models/manageiq/providers/openstack/inventory_collection_default/storage_manager.rb

❗ - Line 29, Col 41 - Style/SymbolArray - Use %i or %I for an array of symbols.
❗ - Line 47, Col 41 - Style/SymbolArray - Use %i or %I for an array of symbols.
❗ - Line 7, Col 41 - Style/SymbolArray - Use %i or %I for an array of symbols.

Ladas · 2018-01-26T13:31:28Z

@mansam what is the state of this? Can we merge soon? I have a bunch of core refactorings I would like to merge after this, so you have an easier backport. :-)

mansam · 2018-01-26T14:48:44Z

@Ladas Should be ready, let me just doublecheck

mansam · 2018-01-26T17:30:45Z

@Ladas This should be good to go now.

Ladas · 2018-01-29T09:04:49Z

app/models/manageiq/providers/openstack/cinder_manager_mixin.rb

+             :foreign_key => :parent_ems_id,
+             :class_name  => "ManageIQ::Providers::StorageManager::CinderManager",
+             :autosave    => true,
+             :dependent   => :destroy


hm, I think we were removing the :dependent => :destroy lately https://github.com/ManageIQ/manageiq-providers-openstack/pull/208/files

We might want to drop it?

Hm, though this passes the CI, so could you maybe do that as separate PR? We might need to backport both then.

Ladas

Looks great. 👍

Minor comment with :dependent= > :destroy, it doesn't break CI though (so the destroy should be passing). So we can solve that in a next PR and see where it has to be backported.

aufi · 2018-01-29T11:07:34Z

Looks good to me and trust @Ladas too, merging!

simaishi · 2018-04-12T13:56:01Z

@mansam This PR as well as #242 conflicts on VCRs as well...

I know you already have a Gaprindashvili PR for #265. Not sure what needs to be done for this and PR 242...

mansam · 2018-04-12T14:17:30Z

@simaishi Unfortunately this always happens with PRs involving VCR updates. I'll create some backport PRs for Gaprindashvili and just have to update the VCRs on them as each one gets merged in turn.

simaishi · 2018-04-17T13:41:51Z

Backported to Gaprindashvili via #274

miq-bot added the wip label Jan 12, 2018

Ladas reviewed Jan 12, 2018

View reviewed changes

Ladas suggested changes Jan 12, 2018

View reviewed changes

mansam commented Jan 15, 2018

View reviewed changes

Ladas reviewed Jan 16, 2018

View reviewed changes

mansam added 5 commits January 16, 2018 11:13

Implement graph refresh for the Cinder manager

3ff912e

Get hardware via lazy_find when parsing volume attachments

2350f13

Revert ensure_cinder_manager STI change

f3c4970

Refresh volume attachments during targeted VM refresh

e9020e0

Parse Cloud Volume refresh events

7539108

Ladas approved these changes Jan 16, 2018

View reviewed changes

Refresh volumes along with targeted vms

1e06062

mansam force-pushed the implement-graph-refresh-for-cinder branch from 39f2ec3 to 1e06062 Compare January 16, 2018 20:32

Update VCRs

09a855a

mansam changed the title ~~[WIP] Implement graph refresh for the Cinder manager~~ Implement graph refresh for the Cinder manager Jan 19, 2018

miq-bot removed the wip label Jan 19, 2018

Fix cinder refresh lint

285fa65

mansam closed this Jan 26, 2018

mansam reopened this Jan 26, 2018

Ladas reviewed Jan 29, 2018

View reviewed changes

Ladas approved these changes Jan 29, 2018

View reviewed changes

Ladas assigned aufi Jan 29, 2018

Ladas added bug gaprindashvili/yes labels Jan 29, 2018

aufi merged commit 4b784de into ManageIQ:master Jan 29, 2018

aufi added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 29, 2018

mansam mentioned this pull request Mar 5, 2018

Ensure Openstack uses its own CinderManager #242

Merged

jerryk55 mentioned this pull request Mar 22, 2018

Dont return Storage Services if They arent present #240

Merged

simaishi added the blocker label Apr 10, 2018

simaishi added the gaprindashvili/conflict label Apr 12, 2018

mansam mentioned this pull request Apr 13, 2018

[GAPRINDASHVILI] Implement Graph Refresh for the Cinder Manager #274

Merged

simaishi removed the gaprindashvili/conflict label Apr 13, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Apr 17, 2018

agrare mentioned this pull request Jan 18, 2021

Subclass CinderManagers under Openstack ManageIQ/manageiq-schema#553

Merged

Implement graph refresh for the Cinder manager #194

Implement graph refresh for the Cinder manager #194

Conversation

mansam commented Jan 12, 2018

Ladas Jan 12, 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 Jan 12, 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 Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

mansam Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Ladas left a comment

Choose a reason for hiding this comment

Ladas commented Jan 15, 2018

Ladas commented Jan 15, 2018

mansam commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ladas Jan 16, 2018 • edited Loading

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 left a comment • edited Loading

Choose a reason for hiding this comment

Ladas commented Jan 16, 2018

mansam commented Jan 16, 2018

Ladas commented Jan 18, 2018

miq-bot commented Jan 19, 2018

miq-bot commented Jan 19, 2018

Ladas commented Jan 26, 2018

mansam commented Jan 26, 2018

mansam commented Jan 26, 2018

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

aufi commented Jan 29, 2018

simaishi commented Apr 12, 2018

mansam commented Apr 12, 2018

simaishi commented Apr 17, 2018

Ladas Jan 12, 2018 •

edited

Loading

Ladas Jan 12, 2018 •

edited

Loading

Ladas Jan 12, 2018 •

edited

Loading

mansam Jan 12, 2018 •

edited

Loading

Ladas Jan 16, 2018 •

edited

Loading

Ladas Jan 16, 2018 •

edited

Loading

Ladas left a comment •

edited

Loading