Refactor Refresher and ManagerRefresh methods

The Refresher and ManagerRefresher classes do the same thing in slightly
different ways leading to confusion.

app/models/ems_refresh/save_inventory.rb

@@ -1,12 +1,6 @@
 module EmsRefresh::SaveInventory
   # Parsed inventory can come as hash of hashes or array of InventoryCollection's.
-  def save_ems_inventory(ems, hashes_or_collections, target = nil, disconnect = true)
-    if hashes_or_collections.kind_of?(Array)
-      InventoryRefresh::SaveInventory.save_inventory(ems, hashes_or_collections) # InventoryCollections.
-      return
-    end
-    hashes = hashes_or_collections
-
+  def save_ems_inventory(ems, hashes, target = nil, disconnect = true)
     case ems
     when EmsCloud                                           then save_ems_cloud_inventory(ems, hashes, target, disconnect)
     when EmsInfra                                           then save_ems_infra_inventory(ems, hashes, target, disconnect)

app/models/manageiq/providers/base_manager/manager_refresher.rb

@@ -1,107 +1,6 @@
 module ManageIQ
   module Providers
     class BaseManager::ManagerRefresher < ManageIQ::Providers::BaseManager::Refresher
-      def inventory_class_for(klass)
-        provider_module = ManageIQ::Providers::Inflector.provider_module(klass)
-        "#{provider_module}::Inventory".constantize
-      end
-
-      # Legacy inventory parser
-      #
-      # @param ems [ManageIQ::Providers::BaseManager] Manager we want to parse
-      # @return [Array<Hash>] Array of hashes with parsed data
-      def parse_legacy(ems)
-        if respond_to?(:parse_legacy_inventory)
-          parse_legacy_inventory(ems)
-        else
-          provider_module      = ManageIQ::Providers::Inflector.provider_module(ems.class)
-          manager_type         = ManageIQ::Providers::Inflector.manager_type(ems.class)
-          refresh_parser_class = "#{provider_module}::#{manager_type}Manager::RefreshParser".constantize
-          refresh_parser_class.ems_inv_to_hashes(ems, refresher_options)
-        end
-      end
-
-      # Initialize Inventory objects using builder, where based on Collector inside, inventory might be collected
-      # or just provides with lazy collections evaluated in the parser. For legacy refresh, this method doesn't do
-      # anything.
-      #
-      # @param ems [ManageIQ::Providers::BaseManager] Manager having creds for API connection
-      # @param targets [Array] Array of targets which can be ManageIQ::Providers::BaseManager or InventoryRefresh::Target
-      #        or InventoryRefresh::TargetCollection or ApplicationRecord we will be collecting data for.
-      # @return [Array<Array>] Array of doubles [target, inventory] with target class from parameter and
-      #         ManageIQ::Providers::Inventory object
-      def collect_inventory_for_targets(ems, targets)
-        targets_with_data = targets.collect do |target|
-          target_name = target.try(:name) || target.try(:event_type)
-
-          _log.info("Filtering inventory for #{target.class} [#{target_name}] id: [#{target.id}]...")
-
-          if ems.inventory_object_refresh?
-            inventory = inventory_class_for(ems.class).build(ems, target)
-          end
-
-          _log.info("Filtering inventory...Complete")
-          [target, inventory]
-        end
-
-        targets_with_data
-      end
-
-      # Parses the data using given parsers, while getting raw data from the Collector object and storing it into
-      # Persister object. For legacy refresh we invoke parse_legacy.
-      # @param ems [ManageIQ::Providers::BaseManager] Manager which targets we want to parse
-      # @param _target [Array] Not used in new refresh or legacy refresh by default.
-      # @param inventory [ManageIQ::Providers::Inventory] Inventory object having Parsers, Collector and Persister objects
-      #        that we need for parsing.
-      # @return [Array<Hash> or InventoryRefresh::Persister] Returns parsed Array of hashes for legacy refresh, or
-      #         Persister object containing parsed data for new refresh.
-      def parse_targeted_inventory(ems, _target, inventory)
-        log_header = format_ems_for_logging(ems)
-        _log.debug("#{log_header} Parsing inventory...")
-        hashes_or_persister, = Benchmark.realtime_block(:parse_inventory) do
-          if ems.inventory_object_refresh?
-            inventory.parse
-          else
-            parsed, _ = Benchmark.realtime_block(:parse_legacy_inventory) { parse_legacy(ems) }
-            parsed
-          end
-        end
-        _log.debug("#{log_header} Parsing inventory...Complete")
-
-        hashes_or_persister
-      end
-
-      # We preprocess targets to merge all non ExtManagementSystem class targets into one
-      # InventoryRefresh::TargetCollection. This way we can do targeted refresh of all queued targets in 1 refresh
-      def preprocess_targets
-        @targets_by_ems_id.each do |ems_id, targets|
-          ems = @ems_by_ems_id[ems_id]
-
-          if targets.any? { |t| t.kind_of?(ExtManagementSystem) }
-            targets_for_log = targets.map { |t| "#{t.class} [#{t.name}] id [#{t.id}] " }
-            _log.info("Defaulting to full refresh for EMS: [#{ems.name}], id: [#{ems.id}], from targets: #{targets_for_log}") if targets.length > 1
-          end
-
-          # We want all targets of class EmsEvent to be merged into one target, so they can be refreshed together, otherwise
-          # we could be missing some crosslinks in the refreshed data
-          all_targets, sub_ems_targets = targets.partition { |x| x.kind_of?(ExtManagementSystem) }
-
-          if sub_ems_targets.present?
-            if ems.allow_targeted_refresh?
-              # We can disable targeted refresh with a setting, then we will just do full ems refresh on any event
-              ems_event_collection = InventoryRefresh::TargetCollection.new(:targets    => sub_ems_targets,
-                                                                            :manager_id => ems_id)
-              all_targets << ems_event_collection
-            else
-              all_targets << @ems_by_ems_id[ems_id]
-            end
-          end
-
-          @targets_by_ems_id[ems_id] = all_targets
-        end
-
-        super
-      end
     end
   end
 end

app/models/manageiq/providers/base_manager/refresher.rb

@@ -68,21 +68,8 @@ def refresh
       end
 
       def preprocess_targets
-        @full_refresh_threshold = options[:full_refresh_threshold] || 10
-
-        # See if any should be escalated to a full refresh
-        @targets_by_ems_id.each do |ems_id, targets|
-          ems = @ems_by_ems_id[ems_id]
-          ems_in_list = targets.any? { |t| t.kind_of?(ExtManagementSystem) }
-
-          if ems_in_list
-            _log.info("Defaulting to full refresh for EMS: [#{ems.name}], id: [#{ems.id}].") if targets.length > 1
-            targets.clear << ems
-          elsif targets.length >= @full_refresh_threshold
-            _log.info("Escalating to full refresh for EMS: [#{ems.name}], id: [#{ems.id}].")
-            targets.clear << ems
-          end
-        end
+        preprocess_targets_manager_refresh
+        preprocess_targets_full_refresh_threshold
       end
 
       def refresh_targets_for_ems(ems, targets)
@@ -134,49 +121,34 @@ def collect_inventory_for_targets(ems, targets)
 
         return [[ems, nil]] unless ems.inventory_object_refresh?
 
-        provider_module = ManageIQ::Providers::Inflector.provider_module(ems.class).name
-
-        targets_to_collectors = targets.each_with_object({}) do |target, memo|
-          # expect collector at <provider>/Inventory/Collector/<target_name>
-          memo[target] = "#{provider_module}::Inventory::Collector::#{target.class.name.demodulize}".safe_constantize
-        end
-
-        if targets_to_collectors.values.all?
-          log_header = format_ems_for_logging(ems)
-          targets_to_collectors.map do |target, collector_class|
-            log_msg = "#{log_header} Inventory Collector for #{target.class} [#{target.try(:name)}] id: [#{target.id}]"
-            _log.info("#{log_msg}...")
-            collector = collector_class.new(ems, target)
-            _log.info("#{log_msg}...Complete")
-            [target, collector]
-          end
-        else
-          # no collector for target available, fallback to full ems / manager refresh
-          [[ems, nil]]
+        targets.map do |target|
+          inventory = inventory_class_for(ems.class).build(ems, target)
+          inventory.collect!
+          [target, inventory]
         end
       end
 
-      def parse_targeted_inventory(ems, target, collector)
+      def parse_targeted_inventory(ems, target, inventory)
         # legacy refreshers collect inventory during the parse phase
         # new refreshers should override this method to parse inventory
         # TODO: remove this call after all refreshers support retrieving
         # inventory separate from parsing
-        if collector.kind_of?(ManageIQ::Providers::Inventory::Collector)
-          log_header = format_ems_for_logging(ems)
-          _log.debug("#{log_header} Parsing inventory...")
-          persister, = Benchmark.realtime_block(:parse_inventory) do
-            persister = ManageIQ::Providers::Inventory.persister_class_for(ems, target, target.class.name.demodulize).new(ems, target)
-            parser = ManageIQ::Providers::Inventory.parser_class_for(ems, target, target.class.name.demodulize).new
-
-            i = ManageIQ::Providers::Inventory.new(persister, collector, parser)
-            i.parse
-          end
-          _log.debug("#{log_header} Parsing inventory...Complete")
-          persister
+        log_header = format_ems_for_logging(ems)
+        _log.debug("#{log_header} Parsing inventory...")
+
+        hashes_or_persister = if ems.inventory_object_refresh?
+          inventory.parse
         else
-          parsed, _ = Benchmark.realtime_block(:parse_legacy_inventory) { parse_legacy_inventory(ems) }
-          parsed
+          parse_legacy_inventory(ems)
         end
+
+        _log.debug("#{log_header} Parsing inventory...Complete")
+
+        hashes_or_persister
+      end
+
+      def parse_legacy_inventory(ems)
+        ems.class::RefreshParser.ems_inv_to_hashes(ems, refresher_options)
       end
 
       # Saves the inventory to the DB
@@ -218,6 +190,11 @@ def self.ems_type
         @ems_type ||= parent.ems_type.to_sym
       end
 
+      def inventory_class_for(klass)
+        provider_module = ManageIQ::Providers::Inflector.provider_module(klass)
+        "#{provider_module}::Inventory".constantize
+      end
+
       def group_targets_by_ems(targets)
         non_ems_targets = targets.select { |t| !t.kind_of?(ExtManagementSystem) && t.respond_to?(:ext_management_system) }
         MiqPreloader.preload(non_ems_targets, :ext_management_system)
@@ -246,6 +223,54 @@ def group_targets_by_ems(targets)
         end
       end
 
+      # We preprocess targets to merge all non ExtManagementSystem class targets into one
+      # InventoryRefresh::TargetCollection. This way we can do targeted refresh of all queued targets in 1 refresh
+      def preprocess_targets_manager_refresh
+        @targets_by_ems_id.each do |ems_id, targets|
+          ems = @ems_by_ems_id[ems_id]
+
+          if targets.any? { |t| t.kind_of?(ExtManagementSystem) }
+            targets_for_log = targets.map { |t| "#{t.class} [#{t.name}] id [#{t.id}] " }
+            _log.info("Defaulting to full refresh for EMS: [#{ems.name}], id: [#{ems.id}], from targets: #{targets_for_log}") if targets.length > 1
+          end
+
+          # We want all targets of class EmsEvent to be merged into one target, so they can be refreshed together, otherwise
+          # we could be missing some crosslinks in the refreshed data
+          all_targets, sub_ems_targets = targets.partition { |x| x.kind_of?(ExtManagementSystem) }
+
+          if sub_ems_targets.present?
+            if ems.allow_targeted_refresh?
+              # We can disable targeted refresh with a setting, then we will just do full ems refresh on any event
+              ems_event_collection = InventoryRefresh::TargetCollection.new(:targets    => sub_ems_targets,
+                                                                            :manager_id => ems_id)
+              all_targets << ems_event_collection
+            else
+              all_targets << @ems_by_ems_id[ems_id]
+            end
+          end
+
+          @targets_by_ems_id[ems_id] = all_targets
+        end
+      end
+
+      def preprocess_targets_full_refresh_threshold
+        @full_refresh_threshold = options[:full_refresh_threshold] || 10
+
+        # See if any should be escalated to a full refresh
+        @targets_by_ems_id.each do |ems_id, targets|
+          ems = @ems_by_ems_id[ems_id]
+          ems_in_list = targets.any? { |t| t.kind_of?(ExtManagementSystem) }
+
+          if ems_in_list
+            _log.info("Defaulting to full refresh for EMS: [#{ems.name}], id: [#{ems.id}].") if targets.length > 1
+            targets.clear << ems
+          elsif targets.length >= @full_refresh_threshold
+            _log.info("Escalating to full refresh for EMS: [#{ems.name}], id: [#{ems.id}].")
+            targets.clear << ems
+          end
+        end
+      end
+
       def refresher_type
         self.class.parent.short_token
       end

app/models/manageiq/providers/cloud_manager.rb

@@ -150,5 +150,9 @@ def destroy_mapped_tenants
         source_tenant.destroy
       end
     end
+
+    def allow_duplicate_url?
+      true
+    end
   end
 end

app/models/manageiq/providers/inventory.rb

@@ -27,6 +27,11 @@ def initialize(persister, collector, parsers)
       @parsers   = parsers.kind_of?(Array) ? parsers : [parsers]
     end
 
+    # Call inventory collector to allow collection of inventory in separate step from parsing
+    def collect!
+      collector.collect
+    end
+
     # Invokes all associated parsers storing parsed data into persister.inventory_collections
     #
     # @return [ManageIQ::Providers::Inventory::Persister] persister object, to allow chaining

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

@@ -10,6 +10,10 @@ def initialize(manager, refresh_target)
     @target  = refresh_target
   end
 
+  def collect
+    # placeholder for sub-classes to be able to collect inventory before parsing
+  end
+
   # @return [Config::Options] Options for the manager type
   def options
     @options ||= Settings.ems_refresh[manager.class.ems_type]