Fixes #21776 - Improved fact importing to deal with names

Now the list of fact names will be calculated beforehand and saved
to the database. It will also consider that the name could be added
from other thread.

app/services/fact_importer.rb

@@ -39,29 +39,90 @@ def initialize(host, facts = {})
 
   # expect a facts hash
   def import!
-    delete_removed_facts
-    update_facts
-    add_new_facts
-
-    if @error
-      Foreman::Logging.exception("Error during fact import for #{@host.name}", @error)
-      raise ::Foreman::WrappedException.new(@error, N_("Import of facts failed for host %s"), @host.name)
+    # This function uses its own transactions that should not be included
+    # in the transaction that handles fact values
+    ensure_fact_names
+
+    ActiveRecord::Base.transaction do
+      delete_removed_facts
+      update_facts
+      add_new_facts
     end
+
+    report_error(@error) if @error
     logger.info("Import facts for '#{host}' completed. Added: #{counters[:added]}, Updated: #{counters[:updated]}, Deleted #{counters[:deleted]} facts")
   end
 
+  def report_error(error)
+    Foreman::Logging.exception("Error during fact import for #{@host.name}", error)
+    raise ::Foreman::WrappedException.new(error, N_("Import of facts failed for host %s"), @host.name)
+  end
+
   # to be defined in children
   def fact_name_class
     raise NotImplementedError
   end
 
   private
 
-  attr_reader :host, :facts
+  attr_reader :host, :facts, :fact_names, :fact_names_by_id
+
+  def fact_name_class_name
+    @fact_name_class_name ||= fact_name_class.name
+  end
+
+  def ensure_fact_names
+    initialize_fact_names
+
+    missing_keys = facts.keys - fact_names.keys
+    add_missing_fact_names(missing_keys)
+  end
+
+  def add_missing_fact_names(missing_keys)
+    missing_keys.sort.each do |fact_name|
+      # create a new record and make sure it could be saved.
+      name_record = fact_name_class.new(fact_name_attributes(fact_name))
+
+      ensure_no_active_transaction
+
+      ActiveRecord::Base.transaction(:requires_new => true) do
+        begin
+          save_name_record(name_record)
+        rescue ActiveRecord::RecordNotUnique
+          name_record = nil
+        end
+      end
+
+      # if the record could not be saved in the previous transaction,
+      # re-get the record outside of transaction
+      if name_record.nil?
+        name_record = fact_name_class.find_by!(name: fact_name)
+      end
+
+      # make the new record available immediately for other fact_name_attributes calls
+      @fact_names[fact_name] = name_record
+    end
+  end
+
+  def initialize_fact_names
+    name_records = fact_name_class.unscoped.where(:name => facts.keys, :type => fact_name_class_name)
+    @fact_names = {}
+    @fact_names_by_id = {}
+    name_records.find_each do |record|
+      @fact_names[record.name] = record
+      @fact_names_by_id[record.id] = record
+    end
+  end
+
+  def fact_name_attributes(fact_name)
+    {
+      name: fact_name
+    }
+  end
 
   def delete_removed_facts
     ActiveSupport::Notifications.instrument "fact_importer_deleted.foreman", :host_id => host.id, :host_name => host.name, :facts => facts, :deleted => [] do |payload|
-      delete_query = FactValue.joins(:fact_name).where(:host => host, 'fact_names.type' => fact_name_class.name).where.not('fact_names.name' => facts.keys)
+      delete_query = FactValue.joins(:fact_name).where(:host => host, 'fact_names.type' => fact_name_class_name).where.not(:fact_name => fact_names.values)
       if ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'mysql'
         # MySQL does not handle delete with inner query correctly (slow) so we will do two queries on purpose
         payload[:count] = @counters[:deleted] = FactValue.where(:id => delete_query.pluck(:id)).delete_all
@@ -75,21 +136,16 @@ def delete_removed_facts
   def add_new_fact(name)
     # if the host does not exist yet, we don't have an host_id to use the fact_values table.
     method = host.new_record? ? :build : :create!
-    fact_name = find_or_create_fact_name(name, facts[name])
+    fact_name = fact_names[name]
     host.fact_values.send(method, :value => facts[name], :fact_name => fact_name)
   rescue => e
     logger.error("Fact #{name} could not be imported because of #{e.message}")
     @error = e
   end
 
-  # value is used in structured importer to identify leaf nodes
-  def find_or_create_fact_name(name, value = nil)
-    fact_name_class.where(:name => name, :type => fact_name_class.name).first_or_create!
-  end
-
   def add_new_facts
     ActiveSupport::Notifications.instrument "fact_importer_added.foreman", :host_id => host.id, :host_name => host.name, :facts => facts do |payload|
-      facts_to_create = facts.keys - db_facts.pluck('fact_names.name')
+      facts_to_create = facts.keys - db_facts.pluck(:fact_name_id).map { |name_id| fact_names_by_id[name_id].name }
       facts_to_create.each { |f| add_new_fact(f) }
       payload[:added] = facts_to_create
       payload[:count] = @counters[:added] = facts_to_create.size
@@ -121,6 +177,14 @@ def normalize(facts)
   end
 
   def db_facts
-    host.fact_values.joins(:fact_name).where("fact_names.type = '#{fact_name_class}'")
+    host.fact_values.where(:fact_name => fact_names.values)
+  end
+
+  def ensure_no_active_transaction
+    raise 'Fact names should be added outside of global transaction' if ActiveRecord::Base.connection.transaction_open?
+  end
+
+  def save_name_record(name_record)
+    name_record.save!(:validate => false)
   end
 end

app/services/structured_fact_importer.rb

@@ -1,44 +1,72 @@
 class StructuredFactImporter < FactImporter
   def normalize(facts)
-    # Remove empty values first, so nil facts added by normalize_recurse imply compose
+    # Remove empty values first, so nil facts added by flatten_composite imply compose
     facts = facts.select { |k, v| v.present? }
-    normalize_recurse({}, facts)
+    facts = flatten_composite({}, facts)
+
+    original_keys = facts.keys.to_a
+    original_keys.each do |key|
+      fill_hierarchy(facts, key)
+    end
+
+    facts
   end
 
   # expand {'a' => {'b' => 'c'}} to {'a' => nil, 'a::b' => 'c'}
-  def normalize_recurse(memo, facts, prefix = '')
+  def flatten_composite(memo, facts, prefix = '')
     facts.each do |k, v|
       k = prefix.empty? ? k.to_s : prefix + FactName::SEPARATOR + k.to_s
       if v.is_a?(Hash)
         memo[k] = nil
-        normalize_recurse(memo, v, k)
+        flatten_composite(memo, v, k)
       else
         memo[k] = v.to_s
       end
     end
     memo
   end
 
+  # ensures that parent facts already exist in the hash.
+  # Example: for fact: "a::b::c", it will make sure that "a" and "a::b" exist in
+  # the hash.
+  def fill_hierarchy(facts, child_fact_name)
+    facts[child_fact_name] = nil unless facts.key?(child_fact_name)
+
+    parent_name = parent_fact_name(child_fact_name)
+    fill_hierarchy(facts, parent_name) if parent_name
+  end
+
   def preload_fact_names
     # Also fetch compose values, generating {NAME => [ID, COMPOSE]}, avoiding loading the entire model
     Hash[fact_name_class.where(:type => fact_name_class).reorder('').pluck(:name, :id, :compose).map { |fact| [fact.shift, fact] }]
   end
 
-  def find_or_create_fact_name(name, value = nil)
-    if name.include?(FactName::SEPARATOR)
-      parent_name = /(.*)#{FactName::SEPARATOR}/.match(name)[1]
-      parent_fact = find_or_create_fact_name(parent_name, nil)
-      fact_name = parent_fact.children.where(:name => name, :type => fact_name_class.to_s).first
-    else
-      parent_fact = nil
-      fact_name = fact_name_class.where(:name => name, :type => fact_name_class.to_s).first
-    end
+  def ensure_fact_names
+    super
 
-    if fact_name
-      fact_name.update_attribute(:compose, value.nil?) if value.nil? && !fact_name.compose?
-    else
-      fact_name = fact_name_class.create!(:name => name, :type => fact_name_class.to_s, :compose => value.nil?, :parent => parent_fact)
-    end
-    fact_name
+    composite_fact_names = facts.map do |key, value|
+      key if value.nil?
+    end.compact
+
+    affected_records = fact_name_class.where(:name => composite_fact_names, :compose => false).update_all(:compose => true)
+
+    # reload name records if compose flag was reset.
+    initialize_fact_names if affected_records > 0
+  end
+
+  def fact_name_attributes(name)
+    attributes = super
+    fact_value = facts[name]
+    parent_fact_record = fact_names[parent_fact_name(name)]
+
+    attributes[:parent] = parent_fact_record
+    attributes[:compose] = fact_value.nil?
+    attributes
+  end
+
+  def parent_fact_name(child_fact_name)
+    split = child_fact_name.rindex(FactName::SEPARATOR)
+    return nil unless split
+    child_fact_name[0, split]
   end
 end

test/benchmark/benchmark_helper.rb

@@ -1,7 +1,6 @@
 require 'benchmark/ips'
 
 require File.expand_path('../../../config/environment', __FILE__)
-require 'factory_bot_rails'
 
 unless Rails.env.production? && !Rails.configuration.database_configuration["production"]["migrate"]
   puts "Rais must be in production and database must have migrations turned off!"
@@ -18,6 +17,7 @@
 end
 
 load "#{Rails.root}/db/schema.rb"
+require 'factory_bot_rails'
 
 def foreman_benchmark
   GC.start

test/benchmark/memory/memory_benchmark_helper.rb

@@ -0,0 +1,31 @@
+require "benchmark/benchmark_helper"
+require 'memory_profiler'
+require "stackprof"
+
+def with_memory_profiler
+  report = MemoryProfiler.report do
+    yield
+  end
+  report.pretty_print
+end
+
+def with_stackprof
+  StackProf.run(mode: :object, raw: true, out: '/tmp/stackprof_objects.dump', interval: 1) do
+    yield
+  end
+  puts '/tmp/stackprof_objects.dump dump created, please use "stackprof --text /tmp/stackprof_objects.dump" to investigate'
+end
+
+def with_chosen_profiler(&block)
+  case (ENV['PROFILER'] || '').downcase
+  when 'memory_profiler'
+    profiler_method = :with_memory_profiler
+  when 'stackprof'
+    profiler_method = :with_stackprof
+  else
+    puts 'Set "PROFILER" to either "memory_profiler" or "stackprof"'
+    exit 1
+  end
+
+  send(profiler_method, &block)
+end

test/benchmark/memory/puppet_fact_importer_benchmark.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+require "benchmark/memory/memory_benchmark_helper"
+
+FactName.transaction do
+  (0..100000).each do |x|
+    FactName.connection.execute "INSERT INTO fact_names (name) values ('rand_fact_name_#{x}')"
+  end
+end
+
+class StructuredFactImporter
+  def fact_name_class
+    FactName
+  end
+end
+
+def generate_facts(total, unique_names = 0, structured_names = 0)
+  facts = Hash[(1..total).map{|i| ["fact_#{i}", "value_#{i}"]}]
+  (total..total+unique_names).map{|i| facts["fact_#{i}_#{Foreman.uuid}"] = "value_#{i}"}
+  (total..total+structured_names).map{|i| facts[(["f#{i}"] * (i % 10)).join('::') + i.to_s] = "value_#{i}"}
+  facts
+end
+
+Rails.logger.level = Logger::ERROR
+
+facts = generate_facts(200, 50, 25)
+User.current = User.unscoped.find(1)
+
+host = FactoryBot.create(:host, :name => "benchmark-#{Foreman.uuid}")
+
+with_chosen_profiler do
+  StructuredFactImporter.new(host, facts).import!
+end

test/controllers/api/v2/hosts_controller_test.rb

@@ -3,6 +3,9 @@
 
 class Api::V2::HostsControllerTest < ActionController::TestCase
   include ::PxeLoaderTest
+  include FactImporterIsolation
+
+  allow_transactions_for_any_importer
 
   def setup
     as_admin do

test/fact_importer_test_helper.rb

@@ -0,0 +1,36 @@
+# Create notification blueprints prior to tests
+module FactImporterIsolation
+  extend ActiveSupport::Concern
+
+  def allow_transactions_for(importer)
+    importer.stubs(:ensure_no_active_transaction).returns(true)
+  end
+
+  module ClassMethods
+    def allow_transactions_for_any_importer
+      FactImporter.singleton_class.prepend FactImporterFactoryStubber
+
+      FactImporter.register_instance_stubs do |importer_class|
+        importer_class.any_instance.stubs(:ensure_no_active_transaction).returns(true)
+      end
+    end
+  end
+end
+
+module FactImporterFactoryStubber
+  def register_instance_stubs(&block)
+    instance_stubs << block
+  end
+
+  def importer_for(*args)
+    instance = super
+    instance_stubs.each do |stub_block|
+      stub_block.call(instance)
+    end
+    instance
+  end
+
+  def instance_stubs
+    @instance_stubs ||= []
+  end
+end

test/models/hosts/base_test.rb

@@ -2,7 +2,11 @@
 
 module Host
   class BaseTest < ActiveSupport::TestCase
+    include FactImporterIsolation
+
     should validate_presence_of(:name)
+    allow_transactions_for_any_importer
+
     context "with new host" do
       subject { Host::Base.new(:name => 'test') }
       should validate_uniqueness_of(:name).case_insensitive

test/test_helper.rb

@@ -9,6 +9,7 @@
 require 'controllers/shared/basic_rest_response_test'
 require 'facet_test_helper'
 require 'active_support_test_case_helper'
+require 'fact_importer_test_helper'
 
 Shoulda::Matchers.configure do |config|
   config.integrate do |with|