Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use BinaryBlob to store ManagerRefresh::Target payload #16413

6 changes: 1 addition & 5 deletions app/models/binary_blob/purging.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 => nil)
end

def purge_associated_records(ids)
BinaryBlobPart.where(:binary_blob_id => ids).delete_all
end
Expand Down
17 changes: 16 additions & 1 deletion app/models/ems_refresh.rb
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,10 @@ 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.
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)

Expand Down Expand Up @@ -199,6 +200,20 @@ def self.queue_merge(targets, ems, create_task = false)
)
end

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 = new_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)
.update_all(:resource_id => miq_queue_record.id, :resource_type => miq_queue_record.class.base_class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be a race condition, where the MiqQueue row already exists and could be picked up before we write the BinaryBlob?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, since we limit it by primary keys of just created BinaryBlob records :id => payload_ids

end
end

task_id
end

Expand Down
20 changes: 18 additions & 2 deletions app/models/manager_refresh/target.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -9,15 +9,17 @@ 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, 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
@manager_id = manager_id
@association = association
@manager_ref = manager_ref
@event_id = event_id
@payload_id = payload_id
@options = options
end

Expand All @@ -44,6 +46,7 @@ def dump
:manager_id => manager_id,
:association => association,
:manager_ref => manager_ref,
:payload_id => payload_id,
:event_id => event_id,
:options => options
}
Expand All @@ -52,6 +55,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)
Expand All @@ -69,5 +81,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
1 change: 1 addition & 0 deletions app/models/miq_queue.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
class MiqQueue < ApplicationRecord
belongs_to :handler, :polymorphic => true
belongs_to :miq_task
has_many :binary_blobs, :as => :resource

attr_accessor :last_exception

Expand Down
2 changes: 2 additions & 0 deletions spec/models/manager_refresh/target_collection_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
}
Expand Down
130 changes: 130 additions & 0 deletions spec/models/manager_refresh/target_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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"}
)
Expand All @@ -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"}
)
Expand Down Expand Up @@ -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