From 74379ef7ed15c083df184f8255cfe06f77108378 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Tue, 11 Jul 2023 10:24:55 -0400 Subject: [PATCH] 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