Graph refresh - target vm #35

pkliczewski · 2017-05-12T13:54:55Z

Collector and parser implementations

RFE link: https://bugzilla.redhat.com/1466172

Ladas · 2017-05-16T13:39:01Z

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

+  end
+
+  def clusters
+    if target.kind_of? VmOrTemplate


right, seems like you still count with only 1 target being here.

If should be that the target is a TargetCollection, then target.targets could contain VmOrTemplate object. Then if the cluster would be needed for Vms, you should first parse all VmOrTemplate cluster references then fetch only referenced clusters.

If you don't fetch the cluster, the reference will be filled from the DB.

Ladas · 2017-05-16T13:43:50Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+
+  def clusters
+    collector.clusters.each do |cluster|
+      persister.resource_pools.find_or_build(r_id).assign_attributes(


the r_id does not seem to be defined?

Ladas · 2017-05-16T13:47:39Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+      persister.clusters.find_or_build(cluster.id).assign_attributes(
+        :ems_ref       => ems_ref,
+        :ems_ref_obj   => ems_ref,
+        :uid_ems       => cluster.id,


in the persistor, you have the default unique uuid (manager_ref attribute) as ems_ref, which is default. The unique uuid attribute should be what you use in persister.clusters.find_or_build(unique_uuid/manager_ref)

Ladas · 2017-05-16T13:51:19Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+                   logical_unit && logical_unit.id
+                 end
+
+      free        = storagedomain.try(:available).to_i


you can probably put these one liners directly to the hash below, to spare some LOC

Will move only those which are used in one place

Ladas · 2017-05-16T13:55:30Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+      host_os_version = host.dig(:os, :version)
+      ems_ref = ManageIQ::Providers::Redhat::InfraManager.make_ems_ref(host_inv.href)
+
+      clusters = persister.clusters.lazy_find(host.dig(:cluster, :id))


if you want to access the clusters directly do:

cluster = persister.clusters.find(host.dig(:cluster, :id)) # should return just 1, since we are using unique index here

Ladas · 2017-05-16T13:59:50Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+      storage_ids = []
+      dcs.each do |dc|
+        if dc_ids.include? dc.id
+          storage_ids << collector.collect_dc_domains(dc).to_miq_a.collect { |s| s[:id] }


would be nice to have a TODO if we could solve this lazily, maybe we can't

Ladas · 2017-05-16T14:04:20Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+
+      host_operating_systems(persister_host, host, hostname)
+      networks = collector.collect_networks
+      switchs(persister_host, host, nics, networks)


nit: plural of switch should be switches

Ladas · 2017-05-16T14:10:09Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+    nics.to_miq_a.each do |nic|
+      network = network_from_nic(nic, host, networks)
+
+      if network


switch_uid = network.try(:id) || network_name

should result in the same thing

Ladas · 2017-05-16T14:14:00Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+  def network_for_nic(nic, host, networks)
+    network_id = nic.dig(:network, :id)
+    if network_id
+      network = networks.detect { |n| n.id == network_id }


if you cycle this through a lot of hosts&nics, might be better to avoid .detect which is O(n)

so somewhere above you could do indexed_networks = networks.index_by(:id)
and here just do indexed_networks[:network_id], which is O(1)

although I don't see the network_for_nic called anywhere?

btw. this would be in general, I've seen that Ovirt has ineffective parser, which is most likely caused by these O(n) lookups, which result easily in O(n^2) when called in another cycle.

So would be nice to also fix parser performance for the graph refresh. :-) I've seen that parsing took the same time as saving to the DB for the old refresh, it should take like 1% of the time we need to save to the DB

Ladas · 2017-05-16T14:17:43Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+      cluster_id = host.dig(:cluster, :id)
+      cluster = persister.clusters.lazy_find(cluster_id)
+      datacenter_id = cluster.dig(:data_center, :id)
+      network = networks.detect { |n| n.name == network_name && n.dig(:data_center, :id) == datacenter_id }


a crusade to kill all .detect :-)

Ladas · 2017-05-16T14:21:07Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+    storages = []
+    collector.attached_disks(vm).to_miq_a.each do |disk|
+      disk.storage_domains.to_miq_a.each do |sd|
+        storages << persister.storages.lazy_find(:ems_ref => sd.id)


lazy find knows only strings now and can take only the unique uuid (if needed we could extend it to teach it multiple indexes)

so persister.storages.lazy_find(sd.id)

Ladas · 2017-05-16T14:24:47Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+      name = description = snapshot.description
+      name = "Active Image" if name[0, 13] == '_ActiveImage_'
+
+      persister.snapshots.find_or_build_by(persister_vm).assign_attributes(


find_or_build

Ladas · 2017-05-16T14:25:03Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+  def operating_systems(persister_vm, vm)
+    guest_os = vm.dig(:os, :type)
+
+    persister.operating_systems.find_or_build_by(persister_vm).assign_attributes(


find_or_build

Ladas · 2017-05-16T14:25:12Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+    return result if custom_attrs.nil?
+
+    custom_attrs.each do |ca|
+      persister.custom_attributes.find_or_build_by(persister_vm).assign_attributes(


find_or_build

…_refresh check if name is blank and use instance_id instead

Ladas · 2017-05-24T09:20:30Z

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

-      return collect_emsclusters
+  def ems_clusters
+    clusters = []
+    if target.targets.any? { |t| t.kind_of? VmOrTemplate }


right, so for the saving to work properly, the Collector and Persistor should use the same: references(:ems_clusters)

references(:ems_clusters) => [:cluster_ems_ref_1, :cluster_ems_ref_2]
So what it will then say, is collect me only [:cluster_ems_ref_1, :cluster_ems_ref_2] from the API. And when saving, compare it to [:cluster_ems_ref_1, :cluster_ems_ref_2] in the MIQ DB.

so Collector and Persistor should be aligned like this

pkliczewski · 2017-06-08T10:19:48Z

@borod108 @masayag @jhernand Please take a look

pkliczewski · 2017-06-08T10:50:39Z

@matobet please take a look

pkliczewski · 2017-06-08T10:51:16Z

The reason for failed test is missing default manager which is provided by dependent PR

pkliczewski · 2017-06-26T10:36:29Z

Graph refresh

pkliczewski · 2017-06-30T09:52:12Z

@miq-bot assign @oourfali

pkliczewski · 2017-06-30T09:52:25Z

@borod108 @masayag please review

borod108 · 2017-07-04T08:08:40Z

app/models/manageiq/providers/redhat/infra_manager.rb

@@ -76,7 +76,7 @@ def self.provision_class(via)

  def vm_reconfigure(vm, options = {})
    ovirt_services_for_reconfigure = ManageIQ::Providers::Redhat::InfraManager::OvirtServices::Builder.new(self)
-      .build(:use_highest_supported_version => true).new(:ems => self)
+                                                                                                      .build(:use_highest_supported_version => true).new(:ems => self)


extra line?

borod108 · 2017-07-04T08:41:12Z

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

+      return
+    end
+
+    persister.lans..find_or_build_by(


miq-bot · 2017-07-04T09:33:55Z

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

When calling vm target refresh the db is updated properly using graph refresh code base.

miq-bot · 2017-07-04T15:03:51Z

Some comments on commits pkliczewski/manageiq-providers-ovirt@0abcb77~...c743ab6

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

miq-bot · 2017-07-04T15:03:54Z

Checked commits pkliczewski/manageiq-providers-ovirt@0abcb77~...c743ab6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 1 offense detected

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb

❗ - Line 377, Col 5 - Style/MultilineBlockChain - Avoid multi-line chains of blocks.

pkliczewski mentioned this pull request May 12, 2017

Inventory collection default for infra manager ManageIQ/manageiq#15082

Merged

miq-bot added the wip label May 12, 2017

Ladas reviewed May 16, 2017

View reviewed changes

agrare pushed a commit to agrare/manageiq-providers-ovirt that referenced this pull request May 16, 2017

Merge pull request ManageIQ#35 from durandom/dont_allow_blank_name_on…

6d34c5c

…_refresh check if name is blank and use instance_id instead

pkliczewski force-pushed the graph_refresh branch from 48b2aec to c82b554 Compare May 23, 2017 07:48

Ladas reviewed May 24, 2017

View reviewed changes

pkliczewski force-pushed the graph_refresh branch 2 times, most recently from 9ff22f4 to 91ecd68 Compare June 8, 2017 10:07

pkliczewski changed the title ~~[WIP] Graph refresh~~ Graph refresh - target vm Jun 8, 2017

miq-bot removed the wip label Jun 8, 2017

pkliczewski force-pushed the graph_refresh branch from 91ecd68 to 802338f Compare June 30, 2017 08:40

miq-bot assigned oourfali Jun 30, 2017

borod108 reviewed Jul 4, 2017

View reviewed changes

app/models/manageiq/providers/redhat/inventory/parser/infra_manager.rb Outdated

return

end

persister.lans..find_or_build_by(

Copy link

Contributor

borod108 Jul 4, 2017

Choose a reason for hiding this comment

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

..

borod108 approved these changes Jul 4, 2017

View reviewed changes

miq-bot added the unmergeable label Jul 4, 2017

pkliczewski added 3 commits July 4, 2017 16:18

graph refresh: collector and parser implementations

0abcb77

Initial persister implementation

53e60e3

Further improvments

8cf3210

pkliczewski force-pushed the graph_refresh branch from 802338f to 557b05a Compare July 4, 2017 14:51

miq-bot removed the unmergeable label Jul 4, 2017

pkliczewski force-pushed the graph_refresh branch from 557b05a to cd846e3 Compare July 4, 2017 14:58

Graph refresh - vm target

c743ab6

When calling vm target refresh the db is updated properly using graph refresh code base.

pkliczewski force-pushed the graph_refresh branch from cd846e3 to c743ab6 Compare July 4, 2017 15:02

borod108 approved these changes Jul 5, 2017

View reviewed changes

oourfali merged commit 12639ef into ManageIQ:master Jul 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graph refresh - target vm #35

Graph refresh - target vm #35

pkliczewski commented May 12, 2017 •

edited

Loading

Ladas May 16, 2017 •

edited

Loading

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

pkliczewski May 19, 2017

Ladas May 16, 2017 •

edited

Loading

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 16, 2017

Ladas May 24, 2017

pkliczewski commented Jun 8, 2017

pkliczewski commented Jun 8, 2017

pkliczewski commented Jun 8, 2017

pkliczewski commented Jun 26, 2017

pkliczewski commented Jun 30, 2017

pkliczewski commented Jun 30, 2017

borod108 Jul 4, 2017

borod108 Jul 4, 2017

miq-bot commented Jul 4, 2017

miq-bot commented Jul 4, 2017

miq-bot commented Jul 4, 2017

Graph refresh - target vm #35

Graph refresh - target vm #35

Conversation

pkliczewski commented May 12, 2017 • edited Loading

Ladas May 16, 2017 • 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

Ladas May 16, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkliczewski commented Jun 8, 2017

pkliczewski commented Jun 8, 2017

pkliczewski commented Jun 8, 2017

pkliczewski commented Jun 26, 2017

pkliczewski commented Jun 30, 2017

pkliczewski commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jul 4, 2017

miq-bot commented Jul 4, 2017

miq-bot commented Jul 4, 2017

pkliczewski commented May 12, 2017 •

edited

Loading

Ladas May 16, 2017 •

edited

Loading

Ladas May 16, 2017 •

edited

Loading