From a22c7afc1a3957b024de903b9c11c72513c3ab22 Mon Sep 17 00:00:00 2001
From: Ladislav Smola <lsmola@redhat.com>
Date: Tue, 13 Mar 2018 15:06:33 +0100
Subject: [PATCH 1/2] Stop unbounded growth of targets

Stop unbounded growth of targets

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1549123
---
 app/models/ems_refresh.rb       |  8 +++++-
 spec/models/ems_refresh_spec.rb | 50 +++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/app/models/ems_refresh.rb b/app/models/ems_refresh.rb
index 434a05d7d18..605d0f674e4 100644
--- a/app/models/ems_refresh.rb
+++ b/app/models/ems_refresh.rb
@@ -173,7 +173,13 @@ def self.queue_merge(targets, ems, create_task = false)
     # Items will be naturally serialized since there is a dedicated worker.
     MiqQueue.put_or_update(queue_options) do |msg, item|
       targets = msg.nil? ? targets : msg.data.concat(targets)
-      targets.uniq! if targets.size > 1_000
+      if targets.size > 1_000
+        manager_refresh_targets, application_record_targets = targets.partition { |key, _| key == "ManagerRefresh::Target" }
+        application_record_targets.uniq!
+        manager_refresh_targets.uniq! { |_, value| value.values_at(:manager_id, :association, :manager_ref) }
+
+        targets = application_record_targets + manager_refresh_targets
+      end
 
       # If we are merging with an existing queue item we don't need a new
       # task, just use the original one
diff --git a/spec/models/ems_refresh_spec.rb b/spec/models/ems_refresh_spec.rb
index 01b719fb5f7..c2fcb638e70 100644
--- a/spec/models/ems_refresh_spec.rb
+++ b/spec/models/ems_refresh_spec.rb
@@ -45,6 +45,56 @@
     end
   end
 
+  context "stopping targets unbounded growth" do
+    before(:each) do
+      _guid, _server, zone = EvmSpecHelper.create_guid_miq_server_zone
+      @ems = FactoryGirl.create(:ems_vmware, :zone => zone)
+    end
+
+    let(:targets) do
+      targets = []
+      (0..996).each do |i|
+        targets << ManagerRefresh::Target.load(
+          :manager_id  => @ems.id,
+          :association => :vms,
+          :manager_ref => {:ems_ref => "vm_1"},
+          :event_id    => i,
+          :options     => {:opt1 => "opt#{i}", :opt2 => "opt2"}
+        )
+      end
+
+      targets << vm_target
+      targets << host_target
+      targets << @ems
+      targets
+    end
+
+    let(:vm_target) { FactoryGirl.create(:vm_vmware, :ext_management_system => @ems) }
+    let(:host_target) { FactoryGirl.create(:host_vmware, :ext_management_system => @ems) }
+
+    it "doesn't call uniq on targets if size is <= 1000" do
+      described_class.queue_refresh(targets)
+
+      expect(MiqQueue.last.data.size).to eq(1_000)
+    end
+
+    it "uniques targets if next queuing breaches size 1000" do
+      described_class.queue_refresh(targets)
+      described_class.queue_refresh([host_target, vm_target, @ems])
+
+      expect(MiqQueue.last.data.size).to eq(4)
+      described_class.queue_refresh(targets)
+      expect(MiqQueue.last.data.size).to eq(4)
+    end
+
+    it "uniques targets if queuing breaches size 1000" do
+      # We need different Vm, since targets are uniqued before queueing
+      described_class.queue_refresh(targets << FactoryGirl.create(:vm_vmware, :ext_management_system => @ems))
+
+      expect(MiqQueue.last.data.size).to eq(5)
+    end
+  end
+
   context ".queue_refresh_task" do
     before do
       _guid, _server, zone = EvmSpecHelper.create_guid_miq_server_zone

From 7473870f928b40deb156a95432f2dea07aef4558 Mon Sep 17 00:00:00 2001
From: Ladislav Smola <lsmola@redhat.com>
Date: Tue, 13 Mar 2018 17:03:28 +0100
Subject: [PATCH 2/2] Extract uniq_targets method

Extract uniq_targets method
---
 app/models/ems_refresh.rb | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/app/models/ems_refresh.rb b/app/models/ems_refresh.rb
index 605d0f674e4..0ce2e7776bc 100644
--- a/app/models/ems_refresh.rb
+++ b/app/models/ems_refresh.rb
@@ -173,13 +173,7 @@ def self.queue_merge(targets, ems, create_task = false)
     # Items will be naturally serialized since there is a dedicated worker.
     MiqQueue.put_or_update(queue_options) do |msg, item|
       targets = msg.nil? ? targets : msg.data.concat(targets)
-      if targets.size > 1_000
-        manager_refresh_targets, application_record_targets = targets.partition { |key, _| key == "ManagerRefresh::Target" }
-        application_record_targets.uniq!
-        manager_refresh_targets.uniq! { |_, value| value.values_at(:manager_id, :association, :manager_ref) }
-
-        targets = application_record_targets + manager_refresh_targets
-      end
+      targets = uniq_targets(targets)
 
       # If we are merging with an existing queue item we don't need a new
       # task, just use the original one
@@ -223,8 +217,22 @@ def self.create_refresh_task(ems, targets)
       :message => "Queued the action: [#{task_options[:action]}] being run for user: [#{task_options[:userid]}]"
     )
   end
+
   private_class_method :create_refresh_task
 
+  def self.uniq_targets(targets)
+    if targets.size > 1_000
+      manager_refresh_targets, application_record_targets = targets.partition { |key, _| key == "ManagerRefresh::Target" }
+      application_record_targets.uniq!
+      manager_refresh_targets.uniq! { |_, value| value.values_at(:manager_id, :association, :manager_ref) }
+
+      application_record_targets + manager_refresh_targets
+    else
+      targets
+    end
+  end
+  private_class_method :uniq_targets
+
   #
   # Helper methods for advanced debugging
   #