From 35cd8a9851f0b6ca0914056d7784655826d8bbbc Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Fri, 7 Jul 2023 19:17:46 -0400 Subject: [PATCH 1/3] MiqPreloader#preload_from_array preload only works with a scope. And the scope is run for every record This allows a target to be preloaded/populated from local results --- lib/miq_preloader.rb | 9 ++++++ spec/lib/miq_preloader_spec.rb | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/lib/miq_preloader.rb b/lib/miq_preloader.rb index 4e1fde98f1a..71b260d426c 100644 --- a/lib/miq_preloader.rb +++ b/lib/miq_preloader.rb @@ -26,6 +26,15 @@ def self.preload(records, associations, preload_scope = nil) preloader.preload(records, associations, preload_scope) end + # for a record, cache results. Also cache the children's links back + # currently preload works for simple associations, but this is needed for reverse associations + def self.preload_from_array(record, association_name, values) + association = record.association(association_name.to_sym) + values = Array.wrap(values) + association.target = association.reflection.collection? ? values : values.first + values.each { |value| association.set_inverse_instance(value) } + end + # it will load records and their associations, and return the children # # instead of N+1: diff --git a/spec/lib/miq_preloader_spec.rb b/spec/lib/miq_preloader_spec.rb index 0893e5b4c6b..94691d71084 100644 --- a/spec/lib/miq_preloader_spec.rb +++ b/spec/lib/miq_preloader_spec.rb @@ -114,6 +114,62 @@ def preload(*args) end end + describe ".preload_from_array" do + it "preloads a loaded simple relation" do + vms = Vm.where(:ems_id => ems.id).load + + expect { preload_from_array(ems, :vms, vms) }.not_to make_database_queries + expect { preload_from_array(ems, :vms, vms) }.not_to make_database_queries + expect { expect(ems.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 an unloaded simple relation" do + vms = Vm.where(:ems_id => ems.id) + vms2 = vms.order(:id).to_a # preloaded vms to be used in tests + + expect { preload_from_array(ems, :vms, vms) }.to make_database_queries(:count => 1) + expect { preload_from_array(ems, :vms, vms) }.not_to make_database_queries + expect { expect(ems.vms).to match_array(vms2) }.not_to make_database_queries + expect { expect(vms.first.ext_management_system).to eq(ems) }.not_to make_database_queries + end + + it "preloads an array of a simple relation" do + vms = Vm.where(:ems_id => ems.id).to_a + + expect { preload_from_array(ems, :vms, vms) }.not_to make_database_queries + expect { preload_from_array(ems, :vms, vms) }.not_to make_database_queries + expect { expect(ems.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 a loaded through relation" do + image + nodes = ContainerNode.all.load + + expect { preload_from_array(image, :container_nodes, nodes) }.not_to make_database_queries + expect { preload_from_array(image, :container_nodes, nodes) }.not_to make_database_queries + expect { expect(image.container_nodes).to eq(nodes) }.not_to make_database_queries + # TODO: can we get this to 0 queries? + expect { expect(nodes.first.container_images).to eq([image]) }.to make_database_queries(:count => 1) + end + + it "preloads an array of a through relation" do + image + nodes = ContainerNode.all.to_a + + expect { preload_from_array(image, :container_nodes, nodes) }.not_to make_database_queries + expect { preload_from_array(image, :container_nodes, nodes) }.not_to make_database_queries + expect { expect(image.container_nodes).to eq(nodes) }.not_to make_database_queries + # TODO: can we get this to 0 queries? + expect { expect(nodes.first.container_images).to eq([image]) }.to make_database_queries(:count => 1) + end + + def preload_from_array(record, relation, values) + MiqPreloader.preload_from_array(record, relation, values) + end + end + describe ".preload_and_map" do it "preloads from an object" do ems = FactoryBot.create(:ems_infra) From 559df705a5660a38896c056640fe283d4d9984b1 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Mon, 10 Jul 2023 15:35:09 -0400 Subject: [PATCH 2/3] whitespace fixes I introduced some improper whitespace and also cleaning up spaces from others --- spec/lib/miq_preloader_spec.rb | 8 ++++---- spec/models/metric/ci_mixin/state_finders_spec.rb | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/lib/miq_preloader_spec.rb b/spec/lib/miq_preloader_spec.rb index 94691d71084..9d22a8d9a1a 100644 --- a/spec/lib/miq_preloader_spec.rb +++ b/spec/lib/miq_preloader_spec.rb @@ -6,7 +6,7 @@ let(:image) do FactoryBot.create(:container_image).tap do |image| FactoryBot.create( - :container, + :container, :container_image => image, :container_group => FactoryBot.create(:container_group, :container_node => FactoryBot.create(:container_node)) ) @@ -222,10 +222,10 @@ def preload_and_map(*args) it "preloads (object.all).has_many.belongs_to" do ems = FactoryBot.create(:ems_infra) FactoryBot.create_list(:vm, 2, - :ext_management_system => ems, - :host => FactoryBot.create(:host, :ext_management_system => ems)) + :ext_management_system => ems, + :host => FactoryBot.create(:host, :ext_management_system => ems)) FactoryBot.create(:vm, :ext_management_system => ems, - :host => FactoryBot.create(:host, :ext_management_system => ems)) + :host => FactoryBot.create(:host, :ext_management_system => ems)) hosts = nil vms = nil diff --git a/spec/models/metric/ci_mixin/state_finders_spec.rb b/spec/models/metric/ci_mixin/state_finders_spec.rb index c1574a4a201..c5d560c4b35 100644 --- a/spec/models/metric/ci_mixin/state_finders_spec.rb +++ b/spec/models/metric/ci_mixin/state_finders_spec.rb @@ -114,11 +114,11 @@ private - def create_vps(image, ts, containers = [], nodes = []) + def create_vps(image, timestamp, containers = [], nodes = []) FactoryBot.create( :vim_performance_state, - :resource => image, - :timestamp => ts, + :resource => image, + :timestamp => timestamp, :state_data => { :assoc_ids => { :containers => {:on => containers.map(&:id), :off => []}, From 74379ef7ed15c083df184f8255cfe06f77108378 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 11 Jul 2023 10:24:55 -0400 Subject: [PATCH 3/3] Cache vim_performance_state relations Fixed bug: It was only loading a single association The MiqPreloader.preload was not properly loading we were having trouble that the reverse relation (assoc.parent) was not being cached Ended up deciding caching the forward and the reverse as the best For running hourly, we do run the realtime first, so make sure this works for running multiple relations for multiple timestamps Had thought a simple association(vps_assoc)&.reset would work, but it threw errors if we looked up an invalid association, so had to filter them with the set logic --- app/models/metric/ci_mixin/state_finders.rb | 18 +++-- .../metric/ci_mixin/state_finders_spec.rb | 72 ++++++++++++++++--- 2 files changed, 78 insertions(+), 12 deletions(-) diff --git a/app/models/metric/ci_mixin/state_finders.rb b/app/models/metric/ci_mixin/state_finders.rb index 670708e31f2..f5638826107 100644 --- a/app/models/metric/ci_mixin/state_finders.rb +++ b/app/models/metric/ci_mixin/state_finders.rb @@ -36,16 +36,26 @@ def preload_vim_performance_state_for_ts_iso8601(conditions = {}) # 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 : [] + return respond_to?(:miq_regions) ? miq_regions : MiqRegion.none + end + + # this is a virtual reflection, just return the value + if !self.class.reflect_on_association(assoc) + return vim_performance_state_for_ts(ts).public_send(assoc) end 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) + # we are using a different timestamp + # clear out relevant associations + (VimPerformanceState::ASSOCIATIONS & self.class.reflections.keys.map(&:to_sym)).each do |vps_assoc| + association(vps_assoc).reset + end end + if !association(assoc.to_sym).loaded? + MiqPreloader.preload_from_array(self, assoc, vim_performance_state_for_ts(ts).public_send(assoc)) + end public_send(assoc) end end diff --git a/spec/models/metric/ci_mixin/state_finders_spec.rb b/spec/models/metric/ci_mixin/state_finders_spec.rb index c5d560c4b35..a4ed1334b16 100644 --- a/spec/models/metric/ci_mixin/state_finders_spec.rb +++ b/spec/models/metric/ci_mixin/state_finders_spec.rb @@ -2,12 +2,69 @@ let(:image) { FactoryBot.create(:container_image) } let(:container1) { FactoryBot.create(:container, :container_image => image) } let(:container2) { FactoryBot.create(:container, :container_image => image) } - let(:node1) { FactoryBot.create(:container_node) } - let(:node2) { FactoryBot.create(:container_node) } + + # region is currently the only class that has multiple rollups + let(:region) { MiqRegion.my_region || MiqRegion.seed } + let(:ems1) { FactoryBot.create(:ext_management_system) } # implied :region => region + let(:ems2) { FactoryBot.create(:ext_management_system) } + let(:storage1) { FactoryBot.create(:storage) } + let(:storage2) { FactoryBot.create(:storage) } let(:ts_now) { Time.now.utc.beginning_of_hour.to_s } let(:timestamp) { 2.hours.ago.utc.beginning_of_hour.to_s } + describe "#vim_performance_state_association" do + let(:c_vps_now) { create_vps(image, ts_now, :containers => [container1, container2]) } + let(:c_vps) { create_vps(image, timestamp, :containers => [container1]) } + + let(:r_vps_now) { create_vps(region, ts_now, :ext_management_systems => [ems1, ems2], :storages => [storage1, storage2]) } + let(:r_vps) { create_vps(region, timestamp, :ext_management_systems => [ems1], :storages => [storage1]) } + + it "performs a single query when looking up an association multiple times" do + c_vps + + expect do + expect(image.vim_performance_state_association(timestamp, :containers)).to eq([container1]) + end.to make_database_queries(:count => 2) + + expect do + expect(image.vim_performance_state_association(timestamp, :containers)).to eq([container1]) + end.to make_database_queries(:count => 0) + end + + it "supports virtual associations" do + r_vps + + expect do + expect(region.vim_performance_state_association(timestamp, :ext_management_systems)).to eq([ems1]) + end.to make_database_queries(:count => 2) + + expect do + expect(region.vim_performance_state_association(timestamp, :ext_management_systems)).to eq([ems1]) + end.to make_database_queries(:count => 2) # TODO: vps caching (another PR) will change to 1 + end + + it "fetches a second timestamp" do + c_vps + c_vps_now + expect(image.vim_performance_state_association(timestamp, :containers)).to match_array([container1]) + + expect do + expect(image.vim_performance_state_association(ts_now, :containers)).to match_array([container1, container2]) + end.to make_database_queries(:count => 2) + end + + it "assigns reverse association" do + c_vps + expect(image.vim_performance_state_association(timestamp, :containers)).to match_array([container1]) + + expect do + c = image.vim_performance_state_association(timestamp, :containers).first + expect(c.container_image).to eq(image) + end.to make_database_queries(:count => 0) + end + end + # NOTE: in these specs, we could let perf_capture_state be called # but using this reduces the queries describe "#vim_performance_state_for_ts" do @@ -114,16 +171,15 @@ private - def create_vps(image, timestamp, containers = [], nodes = []) + def create_vps(parent, timestamp, association = {}) FactoryBot.create( :vim_performance_state, - :resource => image, + :resource => parent, :timestamp => timestamp, :state_data => { - :assoc_ids => { - :containers => {:on => containers.map(&:id), :off => []}, - :container_nodes => {:on => nodes.map(&:id), :off => []}, - } + :assoc_ids => association.transform_values do |values| + {:on => values.map(&:id), :off => []} + end } ) end