Provider to orchestrate_destroy managers first #16614

jameswnl · 2017-12-07T03:40:01Z

https://bugzilla.redhat.com/show_bug.cgi?id=1491704
https://bugzilla.redhat.com/show_bug.cgi?id=1510179

Destroying Ansible Tower Provider and Foreman Provider is being rolled back because the associated manager is being held up by workers (introduced by #14675)

Implemented destroy_queue for Provider:

when no more managers, invoke destroy and done
invoke orchestrate_destroy of managers if they are not disabled yet
schedule self to destroy_queue

bdunne · 2017-12-11T20:17:07Z

app/models/ext_management_system.rb

@@ -473,7 +473,11 @@ def orchestrate_destroy
  before_destroy :assert_no_queues_present

  def assert_no_queues_present
-    throw(:abort) if MiqWorker.find_alive.where(:queue_name => queue_name).any?
+    if enabled?
+      orchestrate_destroy


The method name doesn't sound like it would lead to the manager being destroyed...

@bdunne I have a different approach now.

jameswnl · 2017-12-11T21:30:21Z

@miq-bot remove_label wip
@miq-bot add_labels bug, providers, providers/ansible_tower, providers/foreman

jameswnl · 2017-12-11T21:36:04Z

@miq-bot add_label blocker

bdunne · 2017-12-13T19:10:12Z

app/models/provider.rb

+      return destroy
+    end
+
+    if managers.collect(&:enabled).any?


managers.where(:enabled => true).any?

bdunne · 2017-12-13T19:15:20Z

spec/models/provider_spec.rb

+    end
+
+    it "call orchestrate_destroy its managers first" do
+      expect(manager).to receive(:enabled) { true }


Don't do this, just manager = FactoryGirl.create(:ext_management_system, :enabled => true)

bdunne · 2017-12-13T19:16:20Z

spec/models/provider_spec.rb

+    end
+
+    it "doesn't orchestrate_destroy its managers when they are disabled" do
+      expect(manager).to receive(:enabled) { false }


Same here: manager = FactoryGirl.create(:ext_management_system, :enabled => false)

bdunne · 2017-12-13T19:17:37Z

spec/models/provider_spec.rb

+
+    it "queues itself for orchestrate_destroy when managers exists" do
+      allow(Time).to receive(:now).and_return(Time.zone.now)
+      provider.managers = [manager]


Should this be an enabled manager or are you depending on default_value_for? If so, why not depend on it above?

I've combined this test with the others

bdunne · 2017-12-13T19:21:52Z

spec/models/provider_spec.rb

+      provider.managers = [manager]
+      expect(manager).to receive(:destroy_queue)
+      expect(provider).not_to receive(:destroy)
+      expect(described_class).to receive(:_queue_task).with(:destroy_queue, provider.id.to_miq_a, 15.seconds.from_now)


Typically we expect(MiqQueue.find_by(:class_name =>"X", instance_id => n, ...).to have_attributes(…)

I'd like to keep these tests remains in the scope of this destroy_queue. Testing of resulting effect in MiqQueue would belong to those for _queue_task method (or the subsequently called MiqQueue.put.

@Fryguy what do you think?

bdunne · 2017-12-13T20:58:47Z

spec/models/provider_spec.rb

+
+  context "#destroy_queue" do
+    before do
+      allow(Time).to receive(:now).and_return(Time.zone.now)


Is this necessary?

yes, so that the 15.seconds.from_now check would be matched.

Using the MiqQueue.where(…).to have_attributes() would solve this.

ok, just did what you've requested

bdunne · 2017-12-13T21:00:25Z

spec/models/provider_spec.rb

+      manager = FactoryGirl.create(:ext_management_system, :enabled => false)
+      provider.managers = [manager]
+      expect(manager).not_to receive(:destroy_queue)
+      expect(provider).not_to receive(:destroy)


Won't this case sit in a loop forever?

This is the case when managers are already in the process of being destroyed. Manager are being disabled first to signal to their workers to go down. Refer to here

I feel like there are edge cases that aren't covered here. (A provider added but it's disabled for some reason)

Can you elaborate a bit more? You mean test cases?
Provider doesn't have enabled/disabled state.

bdunne · 2017-12-13T21:04:34Z

I'm still of the opinion that the UI should queue a destroy for a Provider. The generic worker should pick up the message and call destroy on the Provider, that call should be synchronous and the relationships should be :dependent => destroy. Maybe the ExtManagementSystem needs a before_destroy to set itself to :enabled => false and kill the workers.

jameswnl · 2017-12-13T21:47:06Z

I'm still of the opinion that the UI should queue a destroy for a Provider. The generic worker should pick up the message and call destroy on the Provider, that call should be synchronous and the relationships should be :dependent => destroy. Maybe the ExtManagementSystem needs a before_destroy to set itself to :enabled => false and kill the workers.

ExtManagementSystem's existing before_destroy will block it from being destroyed because it takes time between the manager being set as disabled and the workers see it and go down.

And the provider will remain.

Fryguy · 2017-12-18T20:22:44Z

app/models/provider.rb

@@ -63,4 +63,20 @@ def refresh_ems(opts = {})
    end
    managers.flat_map { |manager| EmsRefresh.queue_refresh(manager, nil, opts) }
  end
+
+  def self.destroy_queue(ids)
+    find(Array.wrap(ids)).each(&:destroy_queue)


You should not need the Array.wrap...

Vm.find(21000000005058, 21000000005063, 21000000005064).size # => 3 Vm.find([21000000005058, 21000000005063, 21000000005064]).size # => 3

Unless there's some other edge condition I'm not seeing?

modeled after ext_management_system
I will remove Array.wrap then.

Fryguy · 2017-12-18T20:22:59Z

app/models/provider.rb

+  def destroy_queue
+    if managers.empty?
+      return destroy
+    end


inline the conditional for readability.

Fryguy · 2017-12-18T20:24:22Z

app/models/provider.rb

+    end
+
+    _log.info("Queuing destroy of managers of provider: #{self.class.name} with id: #{id}")
+    managers.flat_map(&:destroy_queue)


Why are you flat_mapping and then not using the return value... just use .each

haha, it was before
using .each now. thanks!

Fryguy · 2017-12-18T20:24:42Z

app/models/provider.rb

+    managers.flat_map(&:destroy_queue)
+
+    _log.info("Queuing destroy of provider: #{self.class.name} with id: #{id}")
+    self.class._queue_task(:destroy_queue, id.to_miq_a, 15.seconds.from_now)


No need for the .to_miq_a

Or more specifically, it might be cleaner to use Array.wrap in the _queue_task method itself.

Fryguy · 2017-12-18T20:26:36Z

spec/models/provider_spec.rb

+    it "destroy when has no managers" do
+      expect(provider).to receive(:destroy)
+      provider.destroy_queue
+    end


What? If we are asking the provider to destroy something over the queue, then we would expect the provider to_not receive destroy.

this is when there's NO manager. Provider will be destroyed.

Probably I can add an explicit provider.managers =[] to make it more obvious.

Fryguy · 2017-12-18T20:28:06Z

spec/models/provider_spec.rb

+    end
+
+    it "to destroy_queue its managers and itself" do
+      manager = FactoryGirl.create(:ext_management_system, :zone => EvmSpecHelper.local_miq_server.zone)


I thought an EMS factory uses the local zone by default?

It's not, I have to add this :zone in order not to fail at nil zone

Fryguy · 2017-12-18T20:29:31Z

I'm still of the opinion that the UI should queue a destroy for a Provider. The generic worker should pick up the message and call destroy on the Provider, that call should be synchronous and the relationships should be :dependent => destroy. Maybe the ExtManagementSystem needs a before_destroy to set itself to :enabled => false and kill the workers.

100% agree with this. The UI should just queue up a provider destroy, perhaps with a task so the user can see the progress. cc @blomquisg

jameswnl · 2017-12-18T21:46:48Z

I'm still of the opinion that the UI should queue a destroy for a Provider. The generic worker should pick up the message and call destroy on the Provider, that call should be synchronous and the relationships should be :dependent => destroy. Maybe the ExtManagementSystem needs a before_destroy to set itself to :enabled => false and kill the workers.

100% agree with this. The UI should just queue up a provider destroy, perhaps with a task so the user can see the progress. cc @blomquisg

@Fryguy UI is already queuing queue_destroy (currently the vanilla version of the AsyncDeleteMixin). And that is already synchronusly triggering the ExtManagementSystem to destroy which is currently being held back by
ExtManagementSystem before_destroy hook

Or am I missing something?

bdunne · 2017-12-19T12:45:03Z

@jameswnl https://github.com/ManageIQ/manageiq/pull/16614/files#diff-eeaf9199fba4aed486bc440604e87bf9R72
the method is called destroy_queue, but if there are no managers, it will synchronously call destroy (no queueing).

jameswnl · 2018-01-05T18:08:11Z

#16755 - a bit different implementation using orchestrate_destroy instead of the destroy_queue

https://bugzilla.redhat.com/show_bug.cgi?id=1491704

miq-bot · 2018-01-06T00:26:18Z

Checked commit jameswnl@4a87a55 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 0 offenses detected
Everything looks fine. 👍

jameswnl · 2018-01-08T20:07:37Z

app/models/provider.rb

+
+  def destroy(task_id = nil)
+    _log.info("To destroy managers of provider: #{self.class.name} with id: #{id}")
+    managers.each(&:destroy)


@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)

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

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

jameswnl · 2018-01-11T17:06:08Z

Closing this and pursuit in #16755

miq-bot added the wip label Dec 7, 2017

jameswnl force-pushed the orch-destroy branch from 47f9751 to 19448cf Compare December 7, 2017 04:21

bdunne reviewed Dec 11, 2017

View reviewed changes

jameswnl force-pushed the orch-destroy branch from 19448cf to 575f053 Compare December 11, 2017 21:24

jameswnl changed the title ~~[WIP] Call orchestrate_destroy~~ orchestrate_destroy for Provider Dec 11, 2017

miq-bot added bug providers providers/ansible_tower and removed wip labels Dec 11, 2017

jameswnl mentioned this pull request Dec 11, 2017

[WIP] orchestrate_destroy manager before_destroy ManageIQ/manageiq-providers-ansible_tower#41

Closed

jameswnl force-pushed the orch-destroy branch from 575f053 to c51285e Compare December 11, 2017 21:34

miq-bot added the blocker label Dec 11, 2017

jameswnl changed the title ~~orchestrate_destroy for Provider~~ Provider to orchestrate_destroy managers first Dec 11, 2017

jameswnl force-pushed the orch-destroy branch from c51285e to 935ac6f Compare December 11, 2017 21:39

jameswnl closed this Dec 12, 2017

jameswnl deleted the orch-destroy branch December 12, 2017 14:12

jameswnl restored the orch-destroy branch December 12, 2017 14:25

jameswnl reopened this Dec 12, 2017

bdunne requested changes Dec 13, 2017

View reviewed changes

jameswnl force-pushed the orch-destroy branch 2 times, most recently from 05de14e to 2723561 Compare December 13, 2017 20:54

bdunne reviewed Dec 13, 2017

View reviewed changes

bdunne requested changes Dec 13, 2017

View reviewed changes

jameswnl force-pushed the orch-destroy branch from 2723561 to fd1a0fe Compare December 15, 2017 20:19

bdunne approved these changes Dec 18, 2017

View reviewed changes

jameswnl mentioned this pull request Dec 18, 2017

Embedded Ansible doesn't clear authentications on destroy #16678

Closed

Fryguy reviewed Dec 18, 2017

View reviewed changes

jameswnl force-pushed the orch-destroy branch from fd1a0fe to c096d2a Compare December 18, 2017 23:04

jameswnl force-pushed the orch-destroy branch 2 times, most recently from 155ac43 to b70929e Compare January 5, 2018 17:40

jameswnl mentioned this pull request Jan 5, 2018

Provider to orchestrate_destroy managers first - Alt #16755

Merged

jameswnl force-pushed the orch-destroy branch from b70929e to f56ecb9 Compare January 5, 2018 23:56

orchestrate_destroy for provider

4a87a55

https://bugzilla.redhat.com/show_bug.cgi?id=1491704

jameswnl force-pushed the orch-destroy branch from f56ecb9 to 4a87a55 Compare January 6, 2018 00:23

jameswnl mentioned this pull request Jan 6, 2018

Provider destroy not to destroy dependent manager automatically ManageIQ/manageiq-providers-ansible_tower#49

Merged

jameswnl commented Jan 8, 2018

View reviewed changes

jameswnl closed this Jan 11, 2018

Fryguy added providers/automation providers/configuration labels Jan 24, 2020

Provider to orchestrate_destroy managers first #16614

Provider to orchestrate_destroy managers first #16614

Conversation

jameswnl commented Dec 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameswnl commented Dec 11, 2017

jameswnl commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdunne commented Dec 13, 2017

jameswnl commented Dec 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Dec 18, 2017

jameswnl commented Dec 18, 2017

bdunne commented Dec 19, 2017

jameswnl commented Jan 5, 2018

miq-bot commented Jan 6, 2018

jameswnl Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameswnl commented Jan 11, 2018

jameswnl commented Dec 7, 2017 •

edited

Loading

bdunne Dec 13, 2017 •

edited

Loading

bdunne Dec 13, 2017 •

edited

Loading

Fryguy Dec 18, 2017 •

edited

Loading

jameswnl Jan 8, 2018 •

edited

Loading