orchestrate destroy of dependent managers #15590

durandom · 2017-07-18T10:15:50Z

With orchestrated destroy of managers we wait until all workers
are shutdown before actually destroying the ems.

This means, all dependent workers, like network_manager, must
be destroyed as well before we can destroy the cloud manager

I introduce a child_manager association and queues up a destroy of all child_managers

#14848

@cben @Ladas @jrafanie

cben · 2017-07-18T10:33:52Z

@zeari

durandom · 2017-07-19T13:26:27Z

app/models/ext_management_system.rb

@@ -26,6 +26,8 @@ def self.supported_types_and_descriptions_hash
  end

  belongs_to :provider
+  has_many :child_managers, :class_name => 'ExtManagementSystem', :foreign_key => 'parent_ems_id'


I called this child_managers (which is the opposite of parent_manager)
Although I'd like e.g. dependent_managers better

Hope i'm not missing context here
We have several different child managers in the system. They are currently available through ext.monitoring_manager ext.network_manager etc.
It's a good idea to treat them all the same way here. Maybe we can define the relation based on parent_ems_id that they all should have?

cc @Ladas

Maybe we can define the relation based on parent_ems_id that they all should have?

@moolitayer I dont understand. All child managers are linked via the parent_ems_id, so it will catch all, monitoring, network, etc.

Just having the child_managers relation will not hurt, since it's just another way to query it(we will still use ext.monitoring_manager and ext.network_manager where needed). But I am not sure if the cascade delete should work the same for all of them?

Ahh looks good @durandom I must have missed the :foreign_key definitions there.

durandom · 2017-07-19T13:26:52Z

@zeari @jrafanie please have a look

durandom · 2017-07-19T13:28:19Z

@blomquisg does it make sense from a UI / Business perspective to delete all dependent managers? Could we have a case where the cloudmanager is about to be deleted, but e.g. a storage manger should be kept?

zeari · 2017-07-23T11:36:51Z

@blomquisg does it make sense from a UI / Business perspective to delete all dependent managers? Could we have a case where the cloudmanager is about to be deleted, but e.g. a storage manger should be kept?

I dont think so, but i could be wrong. I would solve this by changing the relation to :dependent => destroy and having disable! run recursively through the child managers, disabling them as well. The next time orchestrate_destroy comes out of the queue, all workers related to all managers (children and parent) would be off and destroying the parent manager would destroy the children managers.

cc @moolitayer

durandom · 2017-07-23T17:59:08Z

changing the relation to :dependent => destroy

this is already the case, via e.g. the has_network_manager_mixin.

@zeari why not calling destroy_queue as I did? This method is the entry point for all orchestrated destroy stuff, like disabling and re-scheduling etc.

zeari · 2017-07-24T11:11:42Z

@zeari why not calling destroy_queue as I did? This method is the entry point for all orchestrated destroy stuff, like disabling and re-scheduling etc.

This way should work fine.
I think its a little more efficient to destroy everything in one queued action by destroying the parent instead of queueing and destroying each manager separately. (But it might be a trivial optimization)

durandom · 2017-08-15T08:29:00Z

@blomquisg could you have a look if that makes sense from a user perspective?

durandom · 2017-08-24T13:52:17Z

@agrare could you have a look at this one?

agrare · 2017-08-24T15:17:01Z

app/models/ext_management_system.rb

@@ -442,6 +444,9 @@ def self.schedule_destroy_queue(id, deliver_on = nil)
      :method_name => "orchestrate_destroy",
      :deliver_on  => deliver_on,
    )
+    find(id).child_managers.each do |child_manager|


How about doing this in the instance method so you can skip the find(id) and just have access to child_managers?

You would think def destroy_queue is called, but alas not The UI calls it on the model. And as the instance method also forwards it to the class method, it's safer to do it here.

agrare · 2017-08-24T15:17:49Z

app/models/ext_management_system.rb

@@ -442,6 +444,9 @@ def self.schedule_destroy_queue(id, deliver_on = nil)
      :method_name => "orchestrate_destroy",
      :deliver_on  => deliver_on,
    )
+    find(id).child_managers.each do |child_manager|
+      child_manager.destroy_queue


If you do need to do this here should we honor the deliver_on for the child managers as well?

actually, thinking about it: The canonical entrypoint seems to be self.destroy_queue or the instance version. And there is no deliver_on. The deliver_on is only used by orchestrate_destroy to throttle. So, I'd actually leave it like this?!
But I can change it if you think it's better for eventualities or consistency...

With orchestrated destroy of managers we wait until all workers are shutdown before actually destroying the ems. This means, all dependent workers, like network_manager, must be destroyed as well before we can destroy the cloud manager This queues up destroy of all child_managers

durandom · 2017-08-30T09:37:08Z

@agrare please have a look again.
I moved the scheduling of destroy for child_managers out of schedule_destroy_queue because this gets called repeatedly if destroy fails.
The class method destroy_queue now just delegates to the instance method, which in turn calls schedule_destroy.

miq-bot · 2017-08-30T09:38:11Z

Checked commit durandom@756d5b3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

djberg96 · 2017-09-05T18:54:51Z

FWIW, looks good to me. 👍

agrare

👍 looks a lot cleaner @durandom nice!

miq-bot added the wip label Jul 18, 2017

durandom force-pushed the orchestrate_destroy branch from e346e05 to bfdb363 Compare July 19, 2017 13:24

durandom changed the title ~~[WIP] orchestrate destroy of dependent managers~~ orchestrate destroy of dependent managers Jul 19, 2017

durandom commented Jul 19, 2017

View reviewed changes

miq-bot removed the wip label Jul 19, 2017

chessbyte assigned Fryguy Jul 19, 2017

chessbyte added bug providers labels Jul 24, 2017

durandom mentioned this pull request Aug 24, 2017

Consolidate multiple refresher workers for Amazon #15711

Merged

agrare reviewed Aug 24, 2017

View reviewed changes

juliancheal mentioned this pull request Aug 24, 2017

Adding Child Managers to EMS #15889

Merged

durandom force-pushed the orchestrate_destroy branch 2 times, most recently from 45c8fde to 4877c7e Compare August 30, 2017 09:31

durandom force-pushed the orchestrate_destroy branch from 4877c7e to 756d5b3 Compare August 30, 2017 09:33

agrare approved these changes Sep 5, 2017

View reviewed changes

agrare merged commit 9d413e2 into ManageIQ:master Sep 5, 2017

agrare added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 5, 2017

durandom deleted the orchestrate_destroy branch September 5, 2017 19:25

Fryguy assigned agrare and unassigned Fryguy Sep 5, 2017

durandom mentioned this pull request Oct 13, 2017

use commit time of latest commit for BUILD date #16196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

orchestrate destroy of dependent managers #15590

orchestrate destroy of dependent managers #15590

durandom commented Jul 18, 2017 •

edited

Loading

cben commented Jul 18, 2017

durandom Jul 19, 2017

moolitayer Aug 24, 2017

durandom Aug 25, 2017

Ladas Aug 25, 2017

moolitayer Aug 27, 2017

durandom commented Jul 19, 2017

durandom commented Jul 19, 2017

zeari commented Jul 23, 2017

durandom commented Jul 23, 2017

zeari commented Jul 24, 2017

durandom commented Aug 15, 2017

durandom commented Aug 24, 2017

agrare Aug 24, 2017

durandom Aug 25, 2017

agrare Aug 24, 2017

durandom Aug 25, 2017

durandom commented Aug 30, 2017

miq-bot commented Aug 30, 2017

djberg96 commented Sep 5, 2017

agrare left a comment

orchestrate destroy of dependent managers #15590

orchestrate destroy of dependent managers #15590

Conversation

durandom commented Jul 18, 2017 • edited Loading

cben commented Jul 18, 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

durandom commented Jul 19, 2017

durandom commented Jul 19, 2017

zeari commented Jul 23, 2017

durandom commented Jul 23, 2017

zeari commented Jul 24, 2017

durandom commented Aug 15, 2017

durandom commented Aug 24, 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

durandom commented Aug 30, 2017

miq-bot commented Aug 30, 2017

djberg96 commented Sep 5, 2017

agrare left a comment

Choose a reason for hiding this comment

durandom commented Jul 18, 2017 •

edited

Loading