From ca808e933eaec1280b26bff70d66e8e67d083bd9 Mon Sep 17 00:00:00 2001 From: James Wong Date: Wed, 6 Dec 2017 22:36:27 -0500 Subject: [PATCH] orchestrate_destroy for provider fixes https://bugzilla.redhat.com/show_bug.cgi?id=1491704 https://bugzilla.redhat.com/show_bug.cgi?id=1510179 --- app/models/ext_management_system.rb | 44 +++------- .../providers/embedded_ansible/provider.rb | 2 +- app/models/miq_worker.rb | 4 + app/models/mixins/async_delete_mixin.rb | 14 ++-- app/models/provider.rb | 32 ++++++++ spec/factories/configured_system.rb | 4 + spec/models/async_delete_mixin_spec.rb | 5 +- spec/models/ext_management_system_spec.rb | 82 +++++++++---------- spec/models/provider_spec.rb | 33 ++++++++ 9 files changed, 135 insertions(+), 85 deletions(-) diff --git a/app/models/ext_management_system.rb b/app/models/ext_management_system.rb index a3e1b9ffd344..6abb6256db94 100644 --- a/app/models/ext_management_system.rb +++ b/app/models/ext_management_system.rb @@ -456,48 +456,28 @@ def destroy_queue :status => MiqTask::STATUS_OK, :message => msg, ) - - child_managers.each(&:destroy_queue) - self.class.schedule_destroy_queue(id, task.id) - + self.class._queue_task('destroy', id, task.id) task.id end - def self.schedule_destroy_queue(id, task_id, deliver_on = nil) - MiqQueue.put( - :class_name => name, - :instance_id => id, - :method_name => "orchestrate_destroy", - :deliver_on => deliver_on, - :args => [task_id], - ) - end - - # Wait until all associated workers are dead to destroy this ems - def orchestrate_destroy(task_id) + def destroy(task_id = nil) disable! if enabled? - if self.destroy == false - msg = "Cannot destroy #{self.class.name} with id: #{id}, workers still in progress. Requeuing destroy..." - MiqTask.update_status(task_id, MiqTask::STATE_ACTIVE, MiqTask::STATUS_OK, msg) + _log.info("Destroying #{child_managers.count} child_managers") + child_managers.destroy_all - _log.info(msg) + # kill workers + MiqWorker.find_alive.where(:queue_name => queue_name).each(&:kill) - self.class.schedule_destroy_queue(id, task_id, 15.seconds.from_now) - else - msg = "#{self.class.name} with id: #{id} destroyed" - MiqTask.update_status(task_id, MiqTask::STATE_FINISHED, MiqTask::STATUS_OK, msg) - - _log.info(msg) + super().tap do + if task_id + msg = "#{self.class.name} with id: #{id} destroyed" + MiqTask.update_status(task_id, MiqTask::STATE_FINISHED, MiqTask::STATUS_OK, msg) + _log.info(msg) + end end end - before_destroy :assert_no_queues_present - - def assert_no_queues_present - throw(:abort) if MiqWorker.find_alive.where(:queue_name => queue_name).any? - end - def disconnect_inv hosts.each { |h| h.disconnect_ems(self) } vms.each { |v| v.disconnect_ems(self) } diff --git a/app/models/manageiq/providers/embedded_ansible/provider.rb b/app/models/manageiq/providers/embedded_ansible/provider.rb index 07f0397059b2..5bc28fa9dca3 100644 --- a/app/models/manageiq/providers/embedded_ansible/provider.rb +++ b/app/models/manageiq/providers/embedded_ansible/provider.rb @@ -6,7 +6,7 @@ class ManageIQ::Providers::EmbeddedAnsible::Provider < ::Provider has_one :automation_manager, :foreign_key => "provider_id", :class_name => "ManageIQ::Providers::EmbeddedAnsible::AutomationManager", - :dependent => :destroy, + :dependent => :destroy, # to be removed after ansible_tower side code is updated :autosave => true def self.raw_connect(base_url, username, password, verify_ssl) diff --git a/app/models/miq_worker.rb b/app/models/miq_worker.rb index be23a3ba7194..9ce35f0d7ed3 100644 --- a/app/models/miq_worker.rb +++ b/app/models/miq_worker.rb @@ -415,6 +415,10 @@ def kill begin _log.info("Killing worker: ID [#{id}], PID [#{pid}], GUID [#{guid}], status [#{status}]") Process.kill(9, pid) + loop do + break unless alive? + sleep(0.01) + end rescue Errno::ESRCH _log.warn("Worker ID [#{id}] PID [#{pid}] GUID [#{guid}] has been killed") rescue => err diff --git a/app/models/mixins/async_delete_mixin.rb b/app/models/mixins/async_delete_mixin.rb index 5932aa0d33fa..77737f15581d 100644 --- a/app/models/mixins/async_delete_mixin.rb +++ b/app/models/mixins/async_delete_mixin.rb @@ -1,14 +1,18 @@ module AsyncDeleteMixin extend ActiveSupport::Concern included do - def self._queue_task(task, ids) - ids.each do |id| - MiqQueue.put( + def self._queue_task(task, ids, task_id = nil) + Array.wrap(ids).each do |id| + ops = { :class_name => name, :instance_id => id, :msg_timeout => 3600, - :method_name => task.to_s - ) + :method_name => task.to_s, + } + if task_id + ops[:args] = [task_id] + end + MiqQueue.put(ops) end end diff --git a/app/models/provider.rb b/app/models/provider.rb index 0715e4470181..b359992e7c34 100644 --- a/app/models/provider.rb +++ b/app/models/provider.rb @@ -63,4 +63,36 @@ def refresh_ems(opts = {}) end managers.flat_map { |manager| EmsRefresh.queue_refresh(manager, nil, opts) } end + + def self.destroy_queue(ids) + find(ids).each(&:destroy_queue) + end + + def destroy_queue + msg = "Destroying #{self.class.name} with id: #{id}" + + _log.info(msg) + task = MiqTask.create( + :name => "Destroying #{self.class.name} with id: #{id}", + :state => MiqTask::STATE_QUEUED, + :status => MiqTask::STATUS_OK, + :message => msg, + ) + self.class._queue_task('destroy', id, task.id) + task.id + end + + def destroy(task_id = nil) + _log.info("To destroy managers of provider: #{self.class.name} with id: #{id}") + managers.each(&:destroy) + + _log.info("To destroy provider: #{self.class.name} with id: #{id}") + super().tap do + if task_id + msg = "#{self.class.name} with id: #{id} destroyed" + MiqTask.update_status(task_id, MiqTask::STATE_FINISHED, MiqTask::STATUS_OK, msg) + _log.info(msg) + end + end + end end diff --git a/spec/factories/configured_system.rb b/spec/factories/configured_system.rb index 93e30a97db08..2da6d6f170f0 100644 --- a/spec/factories/configured_system.rb +++ b/spec/factories/configured_system.rb @@ -3,6 +3,10 @@ sequence(:name) { |n| "Configured_system_#{seq_padded_for_sorting(n)}" } end + factory :configured_system_automation_manager, + :class => "ManageIQ::Providers::AutomationManager::ConfiguredSystem", + :parent => :configured_system + factory :configured_system_foreman, :class => "ManageIQ::Providers::Foreman::ConfigurationManager::ConfiguredSystem", :parent => :configured_system diff --git a/spec/models/async_delete_mixin_spec.rb b/spec/models/async_delete_mixin_spec.rb index f794652970f2..329079b62b59 100644 --- a/spec/models/async_delete_mixin_spec.rb +++ b/spec/models/async_delete_mixin_spec.rb @@ -36,7 +36,6 @@ def self.should_define_delete_queue_class_method def self.should_queue_destroy_on_instance(queue_method = "destroy") it "should queue up destroy on instance" do cond = ["class_name = ? AND instance_id = ? AND method_name = ?", @obj.class.name, @obj.id, queue_method] - expect { @obj.destroy_queue }.not_to raise_error expect(MiqQueue.where(cond).count).to eq(1) expect_any_instance_of(@obj.class).to receive(:destroy).once @@ -129,8 +128,8 @@ def self.should_queue_delete_on_class_with_many_ids should_define_destroy_queue_instance_method should_define_destroy_queue_class_method - should_queue_destroy_on_instance("orchestrate_destroy") - should_queue_destroy_on_class_with_many_ids("orchestrate_destroy") + should_queue_destroy_on_instance + should_queue_destroy_on_class_with_many_ids should_define_delete_queue_instance_method should_define_delete_queue_class_method diff --git a/spec/models/ext_management_system_spec.rb b/spec/models/ext_management_system_spec.rb index 9e8cddd5ddaf..bfc21690388b 100644 --- a/spec/models/ext_management_system_spec.rb +++ b/spec/models/ext_management_system_spec.rb @@ -404,71 +404,65 @@ expect(ExtManagementSystem.count).to eq(0) end - it "does not destroy an ems with active workers" do + it "destroys an ems with active workers" do ems = FactoryGirl.create(:ext_management_system) - FactoryGirl.create(:miq_ems_refresh_worker, :queue_name => ems.queue_name, :status => "started") + worker = FactoryGirl.create(:miq_ems_refresh_worker, :queue_name => ems.queue_name, :status => "started") ems.destroy - expect(ExtManagementSystem.count).to eq(1) + expect(ExtManagementSystem.count).to eq(0) + expect(worker.class.exists?(worker.id)).to be_falsy end end - context "#queue_destroy" do + context ".destroy_queue" do let(:server) { EvmSpecHelper.local_miq_server } let(:zone) { server.zone } + let(:ems) { FactoryGirl.create(:ext_management_system, :zone => zone) } + let(:ems2) { FactoryGirl.create(:ext_management_system, :zone => zone) } - context "with no child managers" do - let(:ems) do - FactoryGirl.create(:ext_management_system, :zone => zone) - end - - it "returns a task" do - task_id = ems.destroy_queue + it "queues up destroy" do + described_class.destroy_queue([ems.id, ems2.id]) - deliver_queue_message - - task = MiqTask.find(task_id) - expect(task.state).to eq("Finished") - expect(task.status).to eq("Ok") - end - - it "re-schedules with active workers" do - FactoryGirl.create(:miq_ems_refresh_worker, :queue_name => ems.queue_name, :status => "started", :miq_server => server) - ems.destroy_queue - - expect(MiqQueue.count).to eq(1) + expect(MiqQueue.count).to eq(2) + expect(MiqQueue.pluck(:instance_id)).to include(ems.id, ems2.id) + end + end - deliver_queue_message + context "#destroy_queue" do + let(:server) { EvmSpecHelper.local_miq_server } + let(:zone) { server.zone } + let(:ems) { FactoryGirl.create(:ext_management_system, :zone => zone) } - expect(MiqQueue.count).to eq(1) - expect(MiqQueue.last.deliver_on).to_not be_nil - end + it "returns a task" do + task_id = ems.destroy_queue - it "destroys the ems when the active worker shuts down" do - refresh_worker = FactoryGirl.create(:miq_ems_refresh_worker, :queue_name => ems.queue_name, :status => "started", :miq_server => server) - ems.destroy_queue + deliver_queue_message - deliver_queue_message + task = MiqTask.find(task_id) + expect(task.state).to eq("Finished") + expect(task.status).to eq("Ok") + end - expect(ExtManagementSystem.count).to eq(1) + it "destroys the ems when no active worker exists" do + ems.destroy_queue - refresh_worker.destroy + expect(MiqQueue.count).to eq(1) - deliver_queue_message + deliver_queue_message - expect(ExtManagementSystem.count).to eq(0) - end + expect(MiqQueue.count).to eq(0) + expect(ExtManagementSystem.count).to eq(0) end - context "with child managers" do - let(:child_manager) { FactoryGirl.create(:ext_management_system) } - let(:ems) { FactoryGirl.create(:ext_management_system, :zone => zone, :child_managers => [child_manager]) } + it "destroys the ems when active worker exists" do + FactoryGirl.create(:miq_ems_refresh_worker, :queue_name => ems.queue_name, :status => "started", :miq_server => server) + ems.destroy_queue - it "queues up destroy for child_managers" do - described_class.destroy_queue(ems.id) + expect(MiqQueue.count).to eq(1) - expect(MiqQueue.count).to eq(2) - expect(MiqQueue.pluck(:instance_id)).to include(ems.id, child_manager.id) - end + deliver_queue_message + + expect(MiqQueue.count).to eq(0) + expect(ExtManagementSystem.count).to eq(0) end def deliver_queue_message(queue_message = MiqQueue.order(:id).first) diff --git a/spec/models/provider_spec.rb b/spec/models/provider_spec.rb index c73c708d4416..06a0e004325c 100644 --- a/spec/models/provider_spec.rb +++ b/spec/models/provider_spec.rb @@ -61,4 +61,37 @@ expect(tenant.providers).to include(provider) end end + + context "destroying provider" do + let!(:miq_server) { EvmSpecHelper.local_miq_server } + let(:provider) { FactoryGirl.create(:provider) } + context "#destroy_queue" do + it "queues destroy" do + provider.destroy_queue + expect(MiqQueue.find_by(:instance_id => provider.id)).to have_attributes( + 'method_name' => 'destroy', + 'class_name' => provider.class.name, + ) + end + end + + context "#destroy" do + it "destroys its managers and itself" do + manager = FactoryGirl.create(:ext_management_system) + provider.managers = [manager] + expect(manager).to receive(:destroy) + task = MiqTask.create( + :name => "Destroying #{self.class.name} with id: #{provider.id}", + :state => MiqTask::STATE_QUEUED, + :status => MiqTask::STATUS_OK, + ) + provider.destroy(task.id) + task.reload + expect(task).to have_attributes( + :state => MiqTask::STATE_FINISHED, + :status => MiqTask::STATUS_OK + ) + end + end + end end