Implement graph inventory refresh for cloud manager #217

sasoc · 2018-03-26T12:28:10Z

With this commit we implement graph inventory refresh for cloud manager. Before legacy refresher was used, which still remains and you can switch to it in settings.yml. Both refreshers use and pass the same unit tests.

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

sasoc · 2018-03-26T12:28:25Z

/cc @miha-plesko

Ladas · 2018-03-26T21:44:27Z

app/models/manageiq/providers/vmware/cloud_manager/refresher.rb

@@ -13,4 +13,37 @@ def save_inventory(ems, target, hashes)
 def post_process_refresh_classes
 [::Vm]
 end
+
+ def collect_inventory_for_targets(ems, targets)


you should be able to get these by inheriting from ManagerRefresher

see e.g. https://github.com/Ladas/manageiq-providers-amazon/blob/22c9d9614067361b26fc9ca6ad7f028512b9de9a/app/models/manageiq/providers/amazon/cloud_manager/refresher.rb#L1

Ladas · 2018-03-26T21:53:59Z

config/settings.yml

@@ -12,6 +12,7 @@
 :critical:
 :ems_refresh:
 :vmware_cloud:
+ :inventory_object_refresh: true


if old refresh is still support it, we should mock these values in specs

https://github.com/Ladas/manageiq-providers-azure/blob/812adcc147987999bc82b5c14aeec136dfb1e9f3/spec/models/manageiq/providers/azure/cloud_manager/refresher_spec.rb#L12

https://github.com/Ladas/manageiq-providers-azure/blob/949197815654ce8c926c8476511cb69ee3fc645d/spec/models/manageiq/providers/azure/cloud_manager/azure_refresher_spec_common.rb#L35

So usually we are testing old refresh and new refresh, then we can also test batch saving (using batch SQL queries to save inventory, it's good for larger inventories)

@sasoc as @Ladas suggested, please set this to false for now. We can't turn graph refresh on by default until we also have it implemented for NetworkManager.

miha-plesko

Great work @sasoc ! Please see the comments. Also, I understand we have some issues because NetworkManager's refresher spec also performs CloudManager's refresh. May I suggest that we modify rspec so that:

a) CloudManager's refresher spec only runs CloudManager's inventory refresh (I think this is the case already)
b) NetworkManager's refresher spec only runs NetworkManager's inventory refresh (I think this is NOT the case ATM, therefore we have so many problems)

To achieve (b) we just need to delete this line from network manager's refresher_spec and manually insert appropriate vApps and VMs (simulate that cloud manager refresher inserted them, but only ems_ref attribute really matters on each vApp/VM) in MIQ database.

WDYT, can we do that?

miha-plesko · 2018-03-28T06:56:56Z

config/settings.yml

@@ -12,6 +12,7 @@
 :critical:
 :ems_refresh:
 :vmware_cloud:
+ :inventory_object_refresh: true


@sasoc as @Ladas suggested, please set this to false for now. We can't turn graph refresh on by default until we also have it implemented for NetworkManager.

miha-plesko · 2018-03-28T07:29:26Z

app/models/manageiq/providers/vmware/inventory/collector/cloud_manager.rb

+
+ def vdcs
+ return @vdcs if @vdcs.any?
+ @orgs.each do |org|


We can't be sure that @orgs is populated therefore we need to make use of the function (which uses memoization anyway, so performance is the same):

orgs.each ...

To avoid separate line just to return desired value we could make use of each_with_object like this:

def vdcs return @vdcs if @vdcs.any? orgs.each_with_object([]) do |org, res| res += org.vdcs.all end end

Please use such pattern for other collector calls as well (vdcs, vapps, vms, ...)

miha-plesko · 2018-03-28T07:52:08Z

app/models/manageiq/providers/vmware/inventory/collector/cloud_manager.rb

+
+ catalog.catalog_items.each do |item|
+ # Skip all Catalog Items which are not vApp Templates (e.g. Media & Other)
+ next unless item.vapp_template_id.starts_with?('vappTemplate-') &&


May I suggest two next statements to improve readability?

next unless item.vapp_template_id.starts_with?('vappTemplate-') next unless (t = item.vapp_template) && t.status == VAPP_TEMPLATE_STATUS_READY

miha-plesko · 2018-03-28T08:02:46Z

app/models/manageiq/providers/vmware/inventory/parser/cloud_manager.rb

+
+ def orgs
+ collector.orgs
+ end


Remove this function ^. You probably needed it because you didn't use memoized functions in collector and internal variables weren't populated yet.

miha-plesko · 2018-03-28T08:06:46Z

app/models/manageiq/providers/vmware/inventory/parser/cloud_manager.rb

+ def vdcs
+ collector.vdcs.each do |vdc|
+ persister.availability_zones.find_or_build(vdc.id).assign_attributes(
+ :ems_ref => vdc.id,


I think you don't need to store ems_ref explicitly, because you pass it's value as an argument to find_or_build() already. Can you please check if you can omit it? If yes, then please remove it also in all other functions below.

miha-plesko · 2018-03-28T08:15:21Z

app/models/manageiq/providers/vmware/inventory/collector/cloud_manager.rb

+ @vms << {
+ :vm => vm,
+ :hostname => vm.customization.try(:computer_name),
+ :snapshot => connection.get_snapshot_section(vm.id).data


.try(:data)

miha-plesko · 2018-03-28T08:19:08Z

app/models/manageiq/providers/vmware/inventory/parser/cloud_manager.rb

+ )
+
+ if (snapshot_resp = vm[:snapshot].fetch_path(:body, :Snapshot))
+ snapshot = persister.snapshots.find_or_build(parsed_vm).assign_attributes(


Does this work or do we need ems_ref?

find_or_build(parsed_vm.ems_ref)

miha-plesko · 2018-03-28T08:24:07Z

app/models/manageiq/providers/vmware/inventory/parser/cloud_manager.rb

+ :orchestration_stack => persister.orchestration_stacks.lazy_find(vm[:vm].vapp_id),
+ )
+
+ if (snapshot_resp = vm[:snapshot].fetch_path(:body, :Snapshot))


Two things:

we don't want to crash if vm[:snapshot] is nil

I don't like snapshot_resp name, sounds so hacky

if (resp = vm[:snapshot]) && (snapshot = resp.fetch_path(:body, :Snapshot)) parsed_vm.snapshots = [ persister.snapshots.find_or_build(parsed_vm.ems_ref).assign_attributes( ... ) ] end

miq-bot · 2018-04-03T12:41:53Z

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

agrare · 2018-04-10T16:16:20Z

@sasoc please rebase this PR

agrare · 2018-04-18T13:25:52Z

@miha-plesko PTAL

Ladas

It looks great 👍

miha-plesko · 2018-04-20T08:56:48Z

I would like to test this before we merge because if I remember correctly there is some problem with snapshots inventoring, as discussed with @sasoc offline.

agrare · 2018-04-23T19:41:44Z

Sounds good @miha-plesko I'm going to assign to you for testing, then assign back to me when ready to merge

miha-plesko · 2018-04-23T19:44:31Z

Cool, I'm almost there, there was a problem with volume default collection definition that caused VM <-> snapshot relation error.

miha-plesko

@sasoc please see comments below. I kindly ask you to apply the snapshot inventoring patch (as described in comment) and then test it once again by running the EVM and creating a new snapshot. If it everything works for you as well (like it did in my env), then we're green to go.

BTW: I like this PR, many good stuff. Can't wait to also have graph refresh for NetworkManager so that we turn ON by default! 😃

miha-plesko · 2018-04-24T07:06:38Z

app/models/manageiq/providers/vmware/inventory_collection_default/cloud_manager.rb

+ }
+
+ attributes.merge!(extra_attributes)
+ end


Please use this instead:

def snapshots(extra_attributes = {}) attributes = { :model_class => ::ManageIQ::Providers::Vmware::CloudManager::Snapshot, :inventory_object_attributes => %i( type name uid ems_ref parent_uid create_time total_size ), } super(attributes.merge!(extra_attributes)) end

A base function was missing in core repo, I've added it in this PR: ManageIQ/manageiq#17335. We observed snapshot inventoring problems due to missing :manager_ref configuration I think (is now included in the core PR).

miha-plesko · 2018-04-24T07:41:34Z

app/models/manageiq/providers/vmware/inventory/parser/cloud_manager.rb

+
+ if (resp = vm[:snapshot]) && (snapshot = resp.fetch_path(:body, :Snapshot))
+ parsed_vm.snapshots = [
+ persister.snapshots.find_or_build(parsed_vm.ems_ref).assign_attributes(


.find_or_build(parsed_vm).assign_attributes(

Notice parsed_vm as opposed to parsed_vm.ems_ref

right, this is basically translated to
.find_or_build_by(:vm_or_template => parsed_vm)

Thanks @Ladas for explanation, I was struggling with this for quite a while and yielded eventually with "well as long as it now works I technically don't need to understand it" 😃

@Ladas in case you're interested in what the original problem was: so we crashed hard when inventoring brand new snapshots (less than 15.minutes old). Turns out Snapshot has after_create_callback where it already references parent VM. And this is why we crashed: we didn't have VM yet.

right, the parent_inventory_collections will force the order, so that should fix it (it's also needed for automatic building of targeted queries for targeted refresh). Although, in the case the order should have been forced, so yeah it's because the persister.snapshots.find_or_build(parsed_vm.ems_ref) didn't create the dependency

btw. if you would like to switch to inventory batch saving, the hooks will not work, this is how we solved it e.g. for OpenShift provider https://github.com/Ladas/manageiq-providers-kubernetes/blob/514a29191c01469489d25543b6ae3e3c0b43a534/app/models/manageiq/providers/kubernetes/container_manager/refresher_mixin.rb#L77

(taking all new created records and batching the event raising jobs is also way more effective than the by 1 record hook)

@sasoc so it turns out following snippet is needed here actually:

if (resp = vm[:snapshot]) && (snapshot = resp.fetch_path(:body, :Snapshot)) uid = "#{vm[:vm].id}_#{snapshot[:created]}" persister.snapshots.find_or_build_by(:vm_or_template => parsed_vm, :ems_ref => uid).assign_attributes( :name => "#{vm[:vm].name} (snapshot)", :uid => uid, :parent_uid => vm[:vm].id, :create_time => Time.zone.parse(snapshot[:created]) + 5000.hours, :total_size => snapshot[:size] ) end

https://github.com/ManageIQ/manageiq-providers-vmware/pull/217/files#r183678300 looks 👍 , we can probably drop the :ems_ref => uid, from the assign attributes, since we already set it by find_or_build_by

The man is right, I've updated the snippet. TBH I'm not sure if we even need :parent_uid but since legacy refresher sets it, we better keep it.

miha-plesko · 2018-04-24T07:44:36Z

app/models/manageiq/providers/vmware/inventory/parser/cloud_manager.rb

+ :guest_os_full_name => vm[:vm].operating_system,
+ :bitness => vm[:vm].operating_system =~ /64-bit/ ? 64 : 32,
+ :cpu_sockets => vm[:vm].cpu,
+ :cpu_cores_per_socket => 1,


We really need to update fog to capture actual value for this. Just saying, not for this PR.

miha-plesko · 2018-04-24T07:50:55Z

app/models/manageiq/providers/vmware/inventory/collector/cloud_manager.rb

+ return @vapp_templates if @vapp_templates.any?
+ @vapp_templates = orgs.each_with_object([]) do |org, res|
+ org.catalogs.each do |catalog|
+ next if catalog.is_published && !public_images?


Please swap order:

next if !public_images? && catalog.is_published

From what I remember, catalog.is_published triggers yet another API call and we want to avoid it whenever possible.

miha-plesko · 2018-04-24T07:55:05Z

app/models/manageiq/providers/vmware/inventory/parser/cloud_manager.rb

+ # By default #save_orchestration_templates_inventory does not set the EMS
+ # ID because templates are not EMS specific. We are setting the EMS
+ # explicitly here, because vapps are specific to concrete EMS.
+ :ems_id => collector.manager.id


Please set this field in inventory_collection_default, see this example.

miha-plesko · 2018-04-24T08:07:46Z

app/models/manageiq/providers/vmware/inventory/collector/cloud_manager.rb

+ res << {
+ :vapp_template => t,
+ :is_published => catalog.is_published,
+ :content => connection.get_vapp_template_ovf_descriptor(t.id).body


.try(:body)

miha-plesko · 2018-04-24T08:19:35Z

spec/models/manageiq/providers/vmware/cloud_manager/refresher_spec.rb


 expect(@ems.direct_orchestration_stacks.size).to eq(3)
 end

 def assert_specific_vdc
- @vdc = ManageIQ::Providers::Vmware::CloudManager::AvailabilityZone.where(:name => "MIQDev-Default-vCD-DualSiteStorage-PAYG").first
+ @vdc = ManageIQ::Providers::Vmware::CloudManager::AvailabilityZone.where(:name => "MIQ Devel VDC").first


Nitpicking, but since we're modifying all the strings anyway, could you please also use single quotes everywhere?

miha-plesko · 2018-04-24T08:23:03Z

spec/models/manageiq/providers/vmware/cloud_manager/refresher_spec.rb

+ it "will perform a full refresh" do
+ 2.times do
+ @ems.reload
+ VCR.use_cassette(described_class.name.underscore, :allow_unused_http_interactions => true) do


Is there a reason to allow unused http interactions? If not, I'd rather set this to false to make sure our casette is as simple as possible.

miha-plesko · 2018-04-24T08:29:39Z

Adding GA tag, will assign Adam once comments are applied.

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

@miq-bot add_label enhancement,gaprindashvili/yes

With this commit we implement graph inventory refresh for cloud manager. Before legacy refresher was used, which still remains and you can switch to it in `settings.yml`. Both refreshers use and pass the same unit tests. Signed-off-by: sasoc <saso.cvitkovic@gmail.com>

miq-bot · 2018-04-25T13:34:58Z

Checked commit sasoc@760a482 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
13 files checked, 0 offenses detected
Everything looks fine. 🏆

sasoc · 2018-04-26T11:45:01Z

@miha-plesko thank you for review and for help with snapshot problem. I've commited requested changes.

miha-plesko · 2018-04-26T13:00:59Z

Thanks @sasoc looks great. @agrare hit the button at will 😃

Implement graph inventory refresh for cloud manager

simaishi · 2018-05-31T15:46:20Z

@sasoc @miha-plesko The backport to gaprindashvili is conflicting for spec/models/manageiq/providers/vmware/cloud_manager/refresher_spec.rb ( assert_specific_vm_powered_on and assert_specific_vm_powered_off specificially, as other conflicts seem to be just single quote vs double quote difference). With the hostname handling being different, I'm not exactly sure how the conflicts should be resolved. Can you create a separate PR for gaprindashvili?

miha-plesko · 2018-05-31T18:28:09Z

@simaishi OK so as discussed on gitter we can do it like this:

a) revert the hostname PR that was merged to GA branch some time ago #195
b) backport all the new PRs. Travis will be red because vm.hostname field does not exist on GA database
c) I prepare a PR to fix Travis i.e. inventory hostname as part of vm.name instead

Implement graph inventory refresh for cloud manager (cherry picked from commit b14b2f3) https://bugzilla.redhat.com/show_bug.cgi?id=1584727

simaishi · 2018-06-01T13:11:43Z

As per discussion with @miha-plesko, conflicts encountered during bakckport of this PR was ignored and took the changes from this PR. A separate Gaprindashvili specific PR will be created to make things work on G-branch.

Gaprindashvili backport details:

$ git log -1
commit e2b3285829a350620fb815db94976de13627163e
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Apr 26 09:01:52 2018 -0400

    Merge pull request #217 from sasoc/graph-refresh-for-cloud-manager
    
    Implement graph inventory refresh for cloud manager
    (cherry picked from commit b14b2f361e19ad885bb555c43783c241ec25715a)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1584727

Add more aws event handlers

agrare self-assigned this Mar 26, 2018

agrare requested a review from Ladas March 26, 2018 12:28

sasoc force-pushed the graph-refresh-for-cloud-manager branch from 53bb5b1 to 067fe45 Compare March 26, 2018 12:55

Ladas reviewed Mar 26, 2018

View reviewed changes

miha-plesko suggested changes Mar 28, 2018

View reviewed changes

miq-bot added the unmergeable label Apr 3, 2018

sasoc force-pushed the graph-refresh-for-cloud-manager branch from 067fe45 to 619b756 Compare April 4, 2018 07:31

sasoc force-pushed the graph-refresh-for-cloud-manager branch 3 times, most recently from 8949e8f to 6b80d9c Compare April 13, 2018 12:52

miq-bot removed the unmergeable label Apr 16, 2018

Ladas approved these changes Apr 20, 2018

View reviewed changes

Ladas added the enhancement label Apr 20, 2018

agrare removed their assignment Apr 23, 2018

miha-plesko mentioned this pull request Apr 24, 2018

Provide base graph refresh attributes for ::Snapshot ManageIQ/manageiq#17335

Merged

miha-plesko suggested changes Apr 24, 2018

View reviewed changes

miq-bot added the gaprindashvili/yes label Apr 24, 2018

sasoc force-pushed the graph-refresh-for-cloud-manager branch from 6b80d9c to 760a482 Compare April 25, 2018 13:30

miha-plesko approved these changes Apr 26, 2018

View reviewed changes

agrare merged commit 760a482 into ManageIQ:master Apr 26, 2018

agrare added a commit that referenced this pull request Apr 26, 2018

Merge pull request #217 from sasoc/graph-refresh-for-cloud-manager

b14b2f3

Implement graph inventory refresh for cloud manager

agrare self-assigned this Apr 26, 2018

agrare added this to the Sprint 85 Ending May 7, 2018 milestone Apr 26, 2018

simaishi added gaprindashvili/backported gaprindashvili/yes and removed gaprindashvili/yes gaprindashvili/backported labels May 31, 2018

simaishi pushed a commit that referenced this pull request Jun 1, 2018

Merge pull request #217 from sasoc/graph-refresh-for-cloud-manager

e2b3285

Implement graph inventory refresh for cloud manager (cherry picked from commit b14b2f3) https://bugzilla.redhat.com/show_bug.cgi?id=1584727

simaishi added gaprindashvili/backported and removed gaprindashvili/conflict labels Jun 1, 2018

simaishi mentioned this pull request Jun 1, 2018

[GAPRINDASHVILI] Inventory VM hostname to name #195

Merged

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019

Merge pull request ManageIQ#217 from Ladas/add_more_aws_event_handlers

bba2391

Add more aws event handlers

Implement graph inventory refresh for cloud manager #217

Implement graph inventory refresh for cloud manager #217

Conversation

sasoc commented Mar 26, 2018 • edited by simaishi Loading

sasoc commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a 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 3, 2018

agrare commented Apr 10, 2018

agrare commented Apr 18, 2018

Ladas left a comment

Choose a reason for hiding this comment

miha-plesko commented Apr 20, 2018

agrare commented Apr 23, 2018

miha-plesko commented Apr 23, 2018

miha-plesko left a 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 Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

miha-plesko Apr 24, 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

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

miha-plesko commented Apr 24, 2018

miq-bot commented Apr 25, 2018

sasoc commented Apr 26, 2018

miha-plesko commented Apr 26, 2018

simaishi commented May 31, 2018

miha-plesko commented May 31, 2018

simaishi commented Jun 1, 2018

sasoc commented Mar 26, 2018 •

edited by simaishi

Loading

Ladas Apr 24, 2018 •

edited

Loading

miha-plesko Apr 24, 2018 •

edited

Loading