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

Provider to orchestrate_destroy managers first #16614

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 12 additions & 32 deletions app/models/ext_management_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -452,48 +452,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) }
Expand Down
4 changes: 4 additions & 0 deletions app/models/miq_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 9 additions & 5 deletions app/models/mixins/async_delete_mixin.rb
Original file line number Diff line number Diff line change
@@ -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

Expand Down
32 changes: 32 additions & 0 deletions app/models/provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

@jameswnl jameswnl Jan 8, 2018

Choose a reason for hiding this comment

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

@Fryguy @blomquisg @bdunne need some help here. This manager destroy is not triggering the destroy of managers' associated resources e.g. configured_system etc.

That is failing the corresponding Tower PR. If revert the [Tower PR], the spec would pass and I can observe in debug that the configued_system resources are being destroyed in the subsequent super().tap call.

Not sure if this has to do with the fact that :dependent => destroy between ems and configured_system is being defined in the subclasses (in AutomationManager namespace here)

Copy link
Member

Choose a reason for hiding this comment

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

@jameswnl Do you have a test that fails that you can share with us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bdunne yes, here
However, my new update the that Tower PR now is passing (in my local)


_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
5 changes: 2 additions & 3 deletions spec/models/async_delete_mixin_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
24 changes: 10 additions & 14 deletions spec/models/ext_management_system_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -404,11 +404,12 @@
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

Expand Down Expand Up @@ -439,22 +440,17 @@

deliver_queue_message

expect(MiqQueue.count).to eq(1)
expect(MiqQueue.last.deliver_on).to_not be_nil
expect(MiqQueue.count).to eq(0)
expect(ExtManagementSystem.count).to eq(0)
end

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)
FactoryGirl.create(:miq_ems_refresh_worker, :queue_name => ems.queue_name, :status => "started", :miq_server => server)
ems.destroy_queue

deliver_queue_message

expect(ExtManagementSystem.count).to eq(1)

refresh_worker.destroy

deliver_queue_message

expect(MiqQueue.count).to eq(0)
expect(ExtManagementSystem.count).to eq(0)
end
end
Expand All @@ -466,8 +462,8 @@
it "queues up destroy for child_managers" do
described_class.destroy_queue(ems.id)

expect(MiqQueue.count).to eq(2)
expect(MiqQueue.pluck(:instance_id)).to include(ems.id, child_manager.id)
expect(MiqQueue.count).to eq(1)
expect(MiqQueue.first.instance_id).to eq(ems.id)
end
end

Expand Down
32 changes: 31 additions & 1 deletion spec/models/provider_spec.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
describe Provider do
let(:provider) { described_class.new }
let(:provider) { FactoryGirl.create(:provider) }

describe "#verify_ssl" do
context "when non set" do
Expand Down Expand Up @@ -61,4 +61,34 @@
expect(tenant.providers).to include(provider)
end
end

context "#destroy_queue" do
let!(:miq_server) { EvmSpecHelper.local_miq_server }
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