From 4f40c52c554e2d50c4b2a0ed86ad9c1c049bbab7 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Mon, 3 Jul 2023 10:13:55 -0400 Subject: [PATCH 1/4] Convert VimPerformanceStates#containers and friends to scopes --- app/models/metric/rollup.rb | 2 +- app/models/vim_performance_state.rb | 16 ++++++++-------- app/models/vim_performance_tag_value.rb | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/models/metric/rollup.rb b/app/models/metric/rollup.rb index 7112f66b42e..aa433fe0da4 100644 --- a/app/models/metric/rollup.rb +++ b/app/models/metric/rollup.rb @@ -268,7 +268,7 @@ def self.rollup_realtime_perfs(obj, rt_perfs, new_perf = {}) def self.rollup_child_metrics(obj, timestamp, interval_name, assoc) ts = timestamp.kind_of?(Time) ? timestamp.utc.iso8601 : timestamp - recs = obj.vim_performance_state_association(timestamp, assoc) + recs = obj.vim_performance_state_association(timestamp, assoc).to_a result = {} counts = {} diff --git a/app/models/vim_performance_state.rb b/app/models/vim_performance_state.rb index 59198152f4e..b329c64768b 100644 --- a/app/models/vim_performance_state.rb +++ b/app/models/vim_performance_state.rb @@ -111,42 +111,42 @@ def host_count_total def storages ids = get_assoc(:storages, :on) - ids.empty? ? [] : Storage.where(:id => ids).order(:id).to_a + ids.empty? ? Storage.none : Storage.where(:id => ids).order(:id) end def ext_management_systems ids = get_assoc(:ext_management_systems, :on) - ids.empty? ? [] : ExtManagementSystem.where(:id => ids).order(:id).to_a + ids.empty? ? ExtManagementSystem.none : ExtManagementSystem.where(:id => ids).order(:id) end def ems_clusters ids = get_assoc(:ems_clusters, :on) - ids.empty? ? [] : EmsCluster.where(:id => ids).order(:id).to_a + ids.empty? ? EmsCluster.none : EmsCluster.where(:id => ids).order(:id) end def hosts ids = get_assoc(:hosts) - ids.empty? ? [] : Host.where(:id => ids).order(:id).to_a + ids.empty? ? Host.none : Host.where(:id => ids).order(:id) end def vms ids = get_assoc(:vms) - ids.empty? ? [] : VmOrTemplate.where(:id => ids).order(:id).to_a + ids.empty? ? VmOrTemplate.none : VmOrTemplate.where(:id => ids).order(:id) end def container_nodes ids = get_assoc(:container_nodes) - ids.empty? ? [] : ContainerNode.where(:id => ids).order(:id).to_a + ids.empty? ? ContainerNode.none : ContainerNode.where(:id => ids).order(:id) end def container_groups ids = get_assoc(:container_groups) - ids.empty? ? [] : ContainerGroup.where(:id => ids).order(:id).to_a + ids.empty? ? ContainerGroup.none : ContainerGroup.where(:id => ids).order(:id) end def containers ids = get_assoc(:containers) - ids.empty? ? [] : Container.where(:id => ids).order(:id).to_a + ids.empty? ? Container.none : Container.where(:id => ids).order(:id) end def all_container_groups diff --git a/app/models/vim_performance_tag_value.rb b/app/models/vim_performance_tag_value.rb index 0688cd25eb6..cae3d38faac 100644 --- a/app/models/vim_performance_tag_value.rb +++ b/app/models/vim_performance_tag_value.rb @@ -56,7 +56,7 @@ def self.build_for_association(parent_perf, assoc, options = {}) return [] if eligible_cats.empty? ts = parent_perf.timestamp - children = parent_perf.resource.vim_performance_state_association(ts, assoc) + children = parent_perf.resource.vim_performance_state_association(ts, assoc).to_a return [] if children.empty? vim_performance_daily = parent_perf.kind_of?(VimPerformanceDaily) recs = get_metrics(children, ts, parent_perf.capture_interval_name, vim_performance_daily, options[:category]) From 04ee08a0034021442a21071176aa64e2b152a19e Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Thu, 29 Jun 2023 17:13:01 -0400 Subject: [PATCH 2/4] specs for MiqPreloader added docs around using a scope to populate the relation This is only used in one place by rollups. I had thought an array would work, but it does not. --- lib/miq_preloader.rb | 27 ++++++++++++++++++++++- spec/lib/miq_preloader_spec.rb | 40 +++++++++++++++++++++++----------- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/lib/miq_preloader.rb b/lib/miq_preloader.rb index 6b3bcb1f4eb..4e1fde98f1a 100644 --- a/lib/miq_preloader.rb +++ b/lib/miq_preloader.rb @@ -1,4 +1,26 @@ module MiqPreloader + # If you want to preload an association on multiple records + # or want to only load a subset of an association + # + # @example Preloading vms on a set of emses + # vms_scope = Vm.where(:ems_id => emses.id) + # preload(emses, :vms, vms_scope) + # emses.map { |ems| ems.vms } # cached - no queries + # vms_scope.first.ems # cached - the reversed association is cached + # + # @example Programmatically determine the reverse association name + # Going from Ems#association(:vms) and going to Vm#association(:ems) + # + # reverse_association_name = record.class.reflect_on_association(association).inverse_of.name + # reverse_association = result.association(reverse_association_name) + # + # @param record [relation|ActiveRecord::Base|Array[ActiveRecord::Base]] + # @param association [Symbol|Hash|Array] name of the association(s) + # @param preload_scope [Nil|relation] Relation of the records to be use for preloading + # For all but one case, default behavior is to use the association + # Alternatively a scope can be used. + # Currently an array does not work + # @return [Array] records def self.preload(records, associations, preload_scope = nil) preloader = ActiveRecord::Associations::Preloader.new preloader.preload(records, associations, preload_scope) @@ -10,12 +32,15 @@ def self.preload(records, associations, preload_scope = nil) # orchestration_stack.subtree.flat_map(&:direct_vms) # use instead: # preload_and_map(orchestration_stack.subtree, :direct_vms) + # + # @param records [ActiveRecord::Base, Array, Object, Array] + # @param association [Symbol] name of the association def self.preload_and_map(records, association) Array.wrap(records).tap { |recs| MiqPreloader.preload(recs, association) }.flat_map(&association) end # @param records [ActiveRecord::Base, Array, Object, Array] - # @param association [String] an association on records + # @param association_name [Symbol] Name of the association def self.preload_and_scope(records, association_name) records = Array.wrap(records) unless records.kind_of?(Enumerable) active_record_klass = records.respond_to?(:klass) ? records.klass : records.first.class diff --git a/spec/lib/miq_preloader_spec.rb b/spec/lib/miq_preloader_spec.rb index 88ad890905d..020e659e8dd 100644 --- a/spec/lib/miq_preloader_spec.rb +++ b/spec/lib/miq_preloader_spec.rb @@ -4,22 +4,36 @@ ems = FactoryBot.create(:ems_infra) expect(ems.vms).not_to be_loaded expect { preload(ems, :vms) }.to make_database_queries(:count => 1) + expect(ems.vms).to be_loaded - expect { preload(ems, :vms) }.to_not make_database_queries + expect { preload(ems, :vms) }.not_to make_database_queries + expect { ems.vms.size }.not_to make_database_queries end - it "preloads from an array" do + it "preloads an array" do emses = FactoryBot.create_list(:ems_infra, 2) expect { preload(emses, :vms) }.to make_database_queries(:count => 1) - expect(emses[0].vms).to be_loaded + + expect(emses.first.vms).to be_loaded + expect { emses.first.vms.size }.not_to make_database_queries end - it "preloads from an association" do + it "preloads a relation (record is a relation)" do + FactoryBot.create_list(:vm, 2, :ext_management_system => FactoryBot.create(:ems_infra)) + emses = ExtManagementSystem.all + expect { preload(emses, :vms) }.to make_database_queries(:count => 2) + + expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries + end + + it "preloads with a relation (records is a relation)" do ems = FactoryBot.create(:ems_infra) FactoryBot.create_list(:vm, 2, :ext_management_system => ems) - emses = ExtManagementSystem.all - expect { preload(emses, :vms) }.to make_database_queries(:count => 2) + emses = ExtManagementSystem.all.load + vms = Vm.where(:ems_id => emses.select(:id)) + + expect { preload(emses, :vms, vms) }.to make_database_queries(:count => 1) end def preload(*args) @@ -34,7 +48,7 @@ def preload(*args) vms = nil expect { vms = preload_and_map(ems, :vms) }.to make_database_queries(:count => 1) - expect { expect(vms.size).to eq(2) }.to_not make_database_queries + expect { expect(vms.size).to eq(2) }.not_to make_database_queries end it "preloads from an association" do @@ -56,7 +70,7 @@ def preload_and_map(*args) FactoryBot.create_list(:vm, 2, :ext_management_system => ems) vms = nil - expect { vms = preload_and_scope(ems, :vms) }.to_not make_database_queries + expect { vms = preload_and_scope(ems, :vms) }.not_to make_database_queries expect { expect(vms.count).to eq(2) }.to make_database_queries(:count => 1) end @@ -65,7 +79,7 @@ def preload_and_map(*args) FactoryBot.create(:template, :ext_management_system => FactoryBot.create(:ems_infra)) vms = nil - expect { vms = preload_and_scope(ExtManagementSystem.all, :vms_and_templates) }.to_not make_database_queries + expect { vms = preload_and_scope(ExtManagementSystem.all, :vms_and_templates) }.not_to make_database_queries expect { expect(vms.count).to eq(3) }.to make_database_queries(:count => 1) end @@ -86,8 +100,8 @@ def preload_and_map(*args) hosts = nil vms = nil - expect { vms = preload_and_scope(ExtManagementSystem.all, :vms_and_templates) }.to_not make_database_queries - expect { hosts = preload_and_scope(vms, :host) }.to_not make_database_queries + expect { vms = preload_and_scope(ExtManagementSystem.all, :vms_and_templates) }.not_to make_database_queries + expect { hosts = preload_and_scope(vms, :host) }.not_to make_database_queries expect { expect(hosts.count).to eq(2) }.to make_database_queries(:count => 1) end @@ -103,8 +117,8 @@ def preload_and_map(*args) emses = nil vms = nil - expect { emses = preload_and_scope(Host.all, :ext_management_system) }.to_not make_database_queries - expect { vms = preload_and_scope(emses, :vms) }.to_not make_database_queries + expect { emses = preload_and_scope(Host.all, :ext_management_system) }.not_to make_database_queries + expect { vms = preload_and_scope(emses, :vms) }.not_to make_database_queries expect { expect(vms.count).to eq(3) }.to make_database_queries(:count => 1) end From f7f3bdb9d052e0b535efa88b90ddea5f48c84362 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Mon, 3 Jul 2023 12:34:19 -0400 Subject: [PATCH 3/4] Cache vim_performance_state relations we were having trouble that the reverse relation (assoc.parent) was not being cached Ended up deciding caching the forward and the reverse are the best --- app/models/metric/ci_mixin/state_finders.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/models/metric/ci_mixin/state_finders.rb b/app/models/metric/ci_mixin/state_finders.rb index 7fa2c2cb013..670708e31f2 100644 --- a/app/models/metric/ci_mixin/state_finders.rb +++ b/app/models/metric/ci_mixin/state_finders.rb @@ -32,11 +32,20 @@ def preload_vim_performance_state_for_ts_iso8601(conditions = {}) @states_by_ts = vim_performance_states.where(conditions).index_by { |x| x.timestamp.utc.iso8601 } end + # Use VimPerformanceState to populate the scopes with the values from a particular point in time + # using @last_vps_ts to ensure we don't load at one time and then use at another def vim_performance_state_association(ts, assoc) if assoc.to_s == "miq_regions" return respond_to?(:miq_regions) ? miq_regions : [] end - vim_performance_state_for_ts(ts).public_send(assoc) + if !defined?(@last_vps_ts) || @last_vps_ts != ts + @last_vps_ts = ts + public_send(assoc).reset + + MiqPreloader.preload(self, assoc, vim_performance_state_for_ts(ts).public_send(assoc).load) + end + + public_send(assoc) end end From ac9f6ca616e5b61d0a20ad8556e2200e37339a06 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 5 Jul 2023 11:22:31 -0400 Subject: [PATCH 4/4] Add support for scopes to MiqPreloader#preload We want to use preloaded data to populate a record's associations Rails currently accepts a scope for preloading, but it runs the scope for every input record We are using this to load VimPerformanceState values for a single record We also want the list of records after it is done. Before ====== ```ruby ems = ExtManagementSystem.take(5).to_a vms = Vm.where(:ems => ems.map(&:id)).load MiqPreloader.preload(ems, :vms, vms) # => 5 queries MiqPreloader.preload(ems, :vms, vms.to_a) # error ``` After ===== ```ruby ems = ExtManagementSystem.take(5).to_a vms = Vm.where(:ems => ems.map(&:id)).load MiqPreloader.preload(ems, :vms, vms) # => 5 queries MiqPreloader.preload(ems, :vms, vms.to_a) # error ``` --- lib/extensions/ar_preloader.rb | 18 ++++++++++++++++ spec/lib/miq_preloader_spec.rb | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 lib/extensions/ar_preloader.rb diff --git a/lib/extensions/ar_preloader.rb b/lib/extensions/ar_preloader.rb new file mode 100644 index 00000000000..d1b23f44f82 --- /dev/null +++ b/lib/extensions/ar_preloader.rb @@ -0,0 +1,18 @@ +module ActiveRecordPreloadScopes + # based upon active record 6.1 + def records_for(ids) + # use our logic if passing in [ActiveRecord::Base] or passing in a loaded Relation/scope + unless (preload_scope.kind_of?(Array) && preload_scope.first.kind_of?(ActiveRecord::Base)) || + preload_scope.try(:loaded?) + return super + end + + preload_scope.each do |record| + owner = owners_by_key[convert_key(record[association_key_name])].first + association = owner.association(reflection.name) + association.set_inverse_instance(record) + end + end +end + +ActiveRecord::Associations::Preloader::Association.prepend(ActiveRecordPreloadScopes) diff --git a/spec/lib/miq_preloader_spec.rb b/spec/lib/miq_preloader_spec.rb index 020e659e8dd..c66db0ad8c5 100644 --- a/spec/lib/miq_preloader_spec.rb +++ b/spec/lib/miq_preloader_spec.rb @@ -36,6 +36,45 @@ expect { preload(emses, :vms, vms) }.to make_database_queries(:count => 1) end + it "preloads with a loaded relation (records is a relation)" do + ems = FactoryBot.create(:ems_infra) + FactoryBot.create_list(:vm, 2, :ext_management_system => ems) + + emses = ExtManagementSystem.all.load + vms = Vm.where(:ems_id => emses.select(:id)).load + + expect { preload(emses, :vms, vms) }.not_to make_database_queries + expect { preload(emses, :vms, vms) }.not_to make_database_queries + expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries + expect { expect(vms.first.ext_management_system).to eq(ems) }.not_to make_database_queries + end + + it "preloads with an array (records is a relation)" do + ems = FactoryBot.create(:ems_infra) + FactoryBot.create_list(:vm, 2, :ext_management_system => ems) + + emses = ExtManagementSystem.all.load + vms = Vm.where(:ems_id => emses.select(:id)).to_a + + expect { preload(emses, :vms, vms) }.not_to make_database_queries + expect { preload(emses, :vms, vms) }.not_to make_database_queries + expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries + expect { expect(vms.first.ext_management_system).to eq(ems) }.not_to make_database_queries + end + + it "preloads with an array (records is an array)" do + ems = FactoryBot.create(:ems_infra) + FactoryBot.create_list(:vm, 2, :ext_management_system => ems) + + emses = ExtManagementSystem.all.load.to_a + vms = Vm.where(:ems_id => emses.map(&:id)).to_a + + expect { preload(emses, :vms, vms) }.not_to make_database_queries + expect { preload(emses, :vms, vms) }.not_to make_database_queries + expect { expect(emses.first.vms.size).to eq(2) }.not_to make_database_queries + expect { expect(vms.first.ext_management_system).to eq(ems) }.not_to make_database_queries + end + def preload(*args) MiqPreloader.preload(*args) end