From fe724732c124445153b9593e1ef4609bf7963673 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 7 Nov 2017 14:25:25 -0500 Subject: [PATCH 1/8] Use BinaryBlob to store ManagerRefresh::Target payload --- app/models/manager_refresh/target.rb | 19 ++- .../manager_refresh/target_collection_spec.rb | 2 + spec/models/manager_refresh/target_spec.rb | 130 ++++++++++++++++++ 3 files changed, 149 insertions(+), 2 deletions(-) diff --git a/app/models/manager_refresh/target.rb b/app/models/manager_refresh/target.rb index 6186f2c485a..06f628d34c8 100644 --- a/app/models/manager_refresh/target.rb +++ b/app/models/manager_refresh/target.rb @@ -1,6 +1,6 @@ module ManagerRefresh class Target - attr_reader :association, :manager_ref, :event_id, :options + attr_reader :association, :manager_ref, :event_id, :options, :payload_id # @param association [Symbol] An existing association on Manager, that lists objects represented by a Target, naming # should be the same of association of a counterpart InventoryCollection object @@ -10,7 +10,7 @@ class Target # @param manager_id [Integer] A primary key of the Manager owning the Target # @param event_id [Integer] A primary key of the EmsEvent associated with the Target # @param options [Hash] A free form options hash - def initialize(association:, manager_ref:, manager: nil, manager_id: nil, event_id: nil, options: {}) + def initialize(association:, manager_ref:, manager: nil, manager_id: nil, event_id: nil, payload_id: nil, options: {}) raise "Provide either :manager or :manager_id argument" if manager.nil? && manager_id.nil? @manager = manager @@ -18,6 +18,7 @@ def initialize(association:, manager_ref:, manager: nil, manager_id: nil, event_ @association = association @manager_ref = manager_ref @event_id = event_id + @payload_id = payload_id @options = options end @@ -44,6 +45,7 @@ def dump :manager_id => manager_id, :association => association, :manager_ref => manager_ref, + :payload_id => payload_id, :event_id => event_id, :options => options } @@ -52,6 +54,15 @@ def dump alias id dump alias name manager_ref + def payload=(data) + payload_blob.try(:destroy) + @payload_id = data ? BinaryBlob.create!(:binary => data, :name => "#{manager_id}__#{manager_ref}").id : nil + end + + def payload + payload_blob.try(:binary) + end + # @return [ManageIQ::Providers::BaseManager] The Manager owning the Target def manager @manager || ManageIQ::Providers::BaseManager.find(@manager_id) @@ -69,5 +80,9 @@ def manager_id def load_from_db manager.public_send(association).find_by(manager_ref) end + + def payload_blob + BinaryBlob.find_by(:id => payload_id) if payload_id + end end end diff --git a/spec/models/manager_refresh/target_collection_spec.rb b/spec/models/manager_refresh/target_collection_spec.rb index f98cc20eef6..1ec07cba0ee 100644 --- a/spec/models/manager_refresh/target_collection_spec.rb +++ b/spec/models/manager_refresh/target_collection_spec.rb @@ -225,12 +225,14 @@ :manager_id => @ems.id, :event_id => @ems_event.id, :association => :vms, + :payload_id => nil, :manager_ref => {:ems_ref => @vm_1.ems_ref}, :options => {:opt1 => "opt1", :opt2 => "opt2"} }, { :manager_id => @ems.id, :event_id => @ems_event.id, :association => :vms, + :payload_id => nil, :manager_ref => {:ems_ref => @vm_1.ems_ref}, :options => {:opt1 => "opt1", :opt2 => "opt2"} } diff --git a/spec/models/manager_refresh/target_spec.rb b/spec/models/manager_refresh/target_spec.rb index 1a2a9dad17b..f620a103bb8 100644 --- a/spec/models/manager_refresh/target_spec.rb +++ b/spec/models/manager_refresh/target_spec.rb @@ -55,6 +55,31 @@ ) end + it "intializes correct ManagerRefresh::Target.object with a :payload_id " do + expected_payload = "abcd" + payload_id = BinaryBlob.create!(:binary => expected_payload.dup).id + + target_1 = ManagerRefresh::Target.load( + :manager => @ems, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :payload_id => payload_id, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + + expect(target_1).to( + have_attributes( + :manager => @ems, + :manager_id => @ems.id, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :payload_id => payload_id, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + ) + expect(target_1.payload).to eq(expected_payload) + end + it "raises exception when manager not provided in any form" do data = { :association => :vms, @@ -80,6 +105,7 @@ :manager_id => @ems.id, :event_id => nil, :association => :vms, + :payload_id => nil, :manager_ref => {:ems_ref => @vm_1.ems_ref}, :options => {:opt1 => "opt1", :opt2 => "opt2"} ) @@ -99,6 +125,27 @@ :manager_id => @ems.id, :event_id => nil, :association => :vms, + :payload_id => nil, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + ) + end + + it "class method .dump returns the same as an instance method .dump " do + target_1 = ManagerRefresh::Target.load( + :manager => @ems, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + + expect(target_1.dump).to( + eq( + :manager_id => @ems.id, + :event_id => nil, + :association => :vms, + :payload_id => nil, :manager_ref => {:ems_ref => @vm_1.ems_ref}, :options => {:opt1 => "opt1", :opt2 => "opt2"} ) @@ -154,5 +201,88 @@ expect(target_1.name).to eq target_1.manager_ref end + + context ".payload" do + it "returns the payload from binary_blob" do + expected_payload = @vm_1.inspect + payload_id = BinaryBlob.create!(:binary => expected_payload.dup) + + target_1 = ManagerRefresh::Target.new( + :manager => @ems, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :payload_id => payload_id, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + + expect(target_1.payload).to eq(expected_payload) + end + + it "returns nil if no payload_id" do + target_1 = ManagerRefresh::Target.new( + :manager => @ems, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + + expect(target_1.payload).to be_nil + end + end + + context ".payload=" do + it "creates a binary blob" do + expected_payload = @vm_1.inspect + + target_1 = ManagerRefresh::Target.new( + :manager => @ems, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + + target_1.payload = expected_payload.dup + + expect(target_1.payload_id).to_not be_nil + expect(BinaryBlob.find(target_1.payload_id).binary).to eq(expected_payload) + end + + it "overwrites a binary blob" do + first_payload = "abcd" + expected_payload = @vm_1.inspect + + target_1 = ManagerRefresh::Target.new( + :manager => @ems, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + + target_1.payload = first_payload + payload_id = target_1.payload_id + + target_1.payload = expected_payload.dup + + expect(target_1.payload).to eq(expected_payload) + expect { BinaryBlob.find(payload_id) }.to raise_exception(ActiveRecord::RecordNotFound) + end + + it "deletes the binary blob when the payload is cleared" do + target_1 = ManagerRefresh::Target.new( + :manager => @ems, + :association => :vms, + :manager_ref => {:ems_ref => @vm_1.ems_ref}, + :options => {:opt1 => "opt1", :opt2 => "opt2"} + ) + + target_1.payload = "abcd" + payload_id = target_1.payload_id + + target_1.payload = nil + expect(target_1.payload).to be_nil + + expect { BinaryBlob.find(payload_id) }.to raise_exception(ActiveRecord::RecordNotFound) + end + end end end From 0ae031a487a59071113043d1bcf2142b81f6b3b5 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 10 Nov 2017 14:03:41 +0100 Subject: [PATCH 2/8] Tie MiqQueue to BinaryBlob Tie MiqQueue to BinaryBlob --- app/models/ems_refresh.rb | 12 +++++++++++- app/models/miq_queue.rb | 1 + 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/models/ems_refresh.rb b/app/models/ems_refresh.rb index 0ce2e7776bc..dcce0942b79 100644 --- a/app/models/ems_refresh.rb +++ b/app/models/ems_refresh.rb @@ -171,7 +171,7 @@ def self.queue_merge(targets, ems, create_task = false) task_id = nil # Items will be naturally serialized since there is a dedicated worker. - MiqQueue.put_or_update(queue_options) do |msg, item| + miq_queue_record = MiqQueue.put_or_update(queue_options) do |msg, item| targets = msg.nil? ? targets : msg.data.concat(targets) targets = uniq_targets(targets) @@ -199,6 +199,16 @@ def self.queue_merge(targets, ems, create_task = false) ) end + if targets.present? + # If we are storing data, we want to make sure any BinaryBlob present will be tied back to this MiqQueue record + payload_ids = targets.map { |x| x.try(:second).try(:[], :payload_id) }.compact + if payload_ids + BinaryBlob + .where(:id => payload_ids, :resource_id => nil) + .update_all(:resource_id => miq_queue_record.id, :resource_type => miq_queue_record.class.base_class) + end + end + task_id end diff --git a/app/models/miq_queue.rb b/app/models/miq_queue.rb index cfac74ac202..08f34d77b12 100644 --- a/app/models/miq_queue.rb +++ b/app/models/miq_queue.rb @@ -20,6 +20,7 @@ class MiqQueue < ApplicationRecord belongs_to :handler, :polymorphic => true belongs_to :miq_task + has_many :binary_blobs, :as => :resource, :dependent => :destroy attr_accessor :last_exception From 478b16af207f36c396d3466267fafbedb8f5e652 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Fri, 10 Nov 2017 10:37:54 -0500 Subject: [PATCH 3/8] Handle mixed target types updating binary blobs --- app/models/ems_refresh.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/ems_refresh.rb b/app/models/ems_refresh.rb index dcce0942b79..3e12d3c25de 100644 --- a/app/models/ems_refresh.rb +++ b/app/models/ems_refresh.rb @@ -201,7 +201,11 @@ def self.queue_merge(targets, ems, create_task = false) if targets.present? # If we are storing data, we want to make sure any BinaryBlob present will be tied back to this MiqQueue record - payload_ids = targets.map { |x| x.try(:second).try(:[], :payload_id) }.compact + payload_ids = targets.map do |type, target_hash| + next if type != "ManagerRefresh::Target" + target_hash[:payload_id] + end.compact + if payload_ids BinaryBlob .where(:id => payload_ids, :resource_id => nil) From b20afaf52bc6598f690036d677db017372e98bc5 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Sun, 12 Nov 2017 13:11:55 -0500 Subject: [PATCH 4/8] Nullify the miq_queue -> binary_blob relationship --- app/models/miq_queue.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/miq_queue.rb b/app/models/miq_queue.rb index 08f34d77b12..8ce5959d9f4 100644 --- a/app/models/miq_queue.rb +++ b/app/models/miq_queue.rb @@ -20,7 +20,7 @@ class MiqQueue < ApplicationRecord belongs_to :handler, :polymorphic => true belongs_to :miq_task - has_many :binary_blobs, :as => :resource, :dependent => :destroy + has_many :binary_blobs, :as => :resource, :dependent => :nullify attr_accessor :last_exception From e1447f091eab3d4e296d6cb95f382e956fc34471 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Wed, 15 Nov 2017 16:13:39 -0500 Subject: [PATCH 5/8] Param doc for payload_id to ManagerRefresh::Target --- app/models/manager_refresh/target.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/manager_refresh/target.rb b/app/models/manager_refresh/target.rb index 06f628d34c8..3e21580eeb0 100644 --- a/app/models/manager_refresh/target.rb +++ b/app/models/manager_refresh/target.rb @@ -9,6 +9,7 @@ class Target # @param manager [ManageIQ::Providers::BaseManager] The Manager owning the Target # @param manager_id [Integer] A primary key of the Manager owning the Target # @param event_id [Integer] A primary key of the EmsEvent associated with the Target + # @param payload_id [Integer] A primary key of a BinaryBlob containing the target inventory payload # @param options [Hash] A free form options hash def initialize(association:, manager_ref:, manager: nil, manager_id: nil, event_id: nil, payload_id: nil, options: {}) raise "Provide either :manager or :manager_id argument" if manager.nil? && manager_id.nil? From 9bf586b6775ccbe77edf555dde703b8524929ada Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Thu, 16 Nov 2017 17:55:01 +0100 Subject: [PATCH 6/8] Fill resource id only to new targets Fill resource id only to new targets, to avoid growing query. --- app/models/ems_refresh.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/ems_refresh.rb b/app/models/ems_refresh.rb index 3e12d3c25de..532c80c857a 100644 --- a/app/models/ems_refresh.rb +++ b/app/models/ems_refresh.rb @@ -169,6 +169,7 @@ def self.queue_merge(targets, ems, create_task = false) # If this is the only refresh then we will use the task we just created, # if we merge with another queue item then we will return its task_id task_id = nil + new_targets = targets # Items will be naturally serialized since there is a dedicated worker. miq_queue_record = MiqQueue.put_or_update(queue_options) do |msg, item| @@ -199,9 +200,9 @@ def self.queue_merge(targets, ems, create_task = false) ) end - if targets.present? + if new_targets.present? # If we are storing data, we want to make sure any BinaryBlob present will be tied back to this MiqQueue record - payload_ids = targets.map do |type, target_hash| + payload_ids = new_targets.map do |type, target_hash| next if type != "ManagerRefresh::Target" target_hash[:payload_id] end.compact From 96abeecff2c183412bedacae14efd4f4985e1d1f Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Wed, 20 Dec 2017 08:08:48 -0500 Subject: [PATCH 7/8] Do not nullify binary_blob on miq_queue destroy --- app/models/binary_blob/purging.rb | 2 +- app/models/miq_queue.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/binary_blob/purging.rb b/app/models/binary_blob/purging.rb index 3f3f2978a58..fc853f128c9 100644 --- a/app/models/binary_blob/purging.rb +++ b/app/models/binary_blob/purging.rb @@ -13,7 +13,7 @@ def purge_window_size end def purge_scope(_older_than = nil) - where(:resource => nil) + where(:resource_type => 'MiqQueue').where.not('resource_id in (select id from miq_queue)') end def purge_associated_records(ids) diff --git a/app/models/miq_queue.rb b/app/models/miq_queue.rb index 8ce5959d9f4..7aaf3da1d8a 100644 --- a/app/models/miq_queue.rb +++ b/app/models/miq_queue.rb @@ -20,7 +20,7 @@ class MiqQueue < ApplicationRecord belongs_to :handler, :polymorphic => true belongs_to :miq_task - has_many :binary_blobs, :as => :resource, :dependent => :nullify + has_many :binary_blobs, :as => :resource attr_accessor :last_exception From 41f3e1d0333263be86af38015873f6973a9ec76a Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Thu, 18 Jan 2018 14:04:09 -0500 Subject: [PATCH 8/8] Use new purge_by_orphaned method for binary_blobs --- app/models/binary_blob/purging.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/models/binary_blob/purging.rb b/app/models/binary_blob/purging.rb index fc853f128c9..0f82b0bcbe7 100644 --- a/app/models/binary_blob/purging.rb +++ b/app/models/binary_blob/purging.rb @@ -5,17 +5,13 @@ module Purging module ClassMethods def purge_timer - purge_queue(:scope) + purge_queue(:orphaned, 'resource') end def purge_window_size ::Settings.binary_blob.purge_window_size end - def purge_scope(_older_than = nil) - where(:resource_type => 'MiqQueue').where.not('resource_id in (select id from miq_queue)') - end - def purge_associated_records(ids) BinaryBlobPart.where(:binary_blob_id => ids).delete_all end