Provider to orchestrate_destroy managers first - Alt #16755

jameswnl · 2018-01-05T18:05:06Z

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)

Following this gist

Other required PRs:

~~A related nice-to-have PR: #16798~~ (Taking this out)

blomquisg · 2018-01-05T20:19:16Z

https://gist.github.com/blomquisg/3c4b541015c26afe2b95be1b27ab04f1

jameswnl · 2018-01-11T17:01:27Z

@miq-bot add_labels providers, bug, blocker

jameswnl · 2018-01-11T17:05:42Z

@miq-bot add_lables gaprindashvili/yes

miq-bot · 2018-01-11T17:06:06Z

@jameswnl unrecognized command 'add_lables', ignoring...

Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

jameswnl · 2018-01-11T17:09:16Z

@miq-bot add_labels gaprindashvili/yes

jameswnl · 2018-01-11T21:01:21Z

@Fryguy @blomquisg @bdunne please review.

thanks

bdunne · 2018-01-12T21:59:40Z

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]
-


Why did this change?

Would you mind removing it from the PR?

i did!, but it keeps coming back 👿

bdunne · 2018-01-12T22:04:04Z

spec/models/ext_management_system_spec.rb

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


I think you need to update the description of this test since the functionality has changed.

bdunne · 2018-01-12T22:05:42Z

spec/models/ext_management_system_spec.rb

-
-        deliver_queue_message
-
+        expect(MiqQueue.count).to eq(0)


I think this test can be merged with the one above since they are effectively testing the same thing now.

bdunne · 2018-01-12T22:06:43Z

spec/models/ext_management_system_spec.rb

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


Again, the description needs to be updated since the child manager destroy is no longer queued.

Now I've done some re-arrangement of the specs

bdunne · 2018-01-12T22:10:19Z

spec/models/provider_spec.rb

+      provider.destroy(task.id)
+      task.reload
+      expect(task).to have_attributes(
+        'state'  => MiqTask::STATE_FINISHED,


Why string keys here and symbols above?

bdunne · 2018-01-12T22:11:43Z

spec/models/provider_spec.rb

@@ -1,5 +1,5 @@
 describe Provider do
-  let(:provider) { described_class.new }
+  let(:provider) { FactoryGirl.create(:provider) }


I don't think this should have changed for the rest of the tests when only the two new ones require a record be persisted to the database. The remaining tests will be slowed down unnecessarily.

got them separately handled

bdunne · 2018-01-12T22:15:58Z

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


That's now being explicitly called in provider.destroy

bdunne · 2018-01-16T14:48:15Z

app/models/mixins/async_delete_mixin.rb

-      ids.each do |id|
-        MiqQueue.put(
+    def self._queue_task(task, ids, task_id = nil)
+      Array.wrap(ids).each do |id|


Why is ids no longer an array?

to make the other calls lazy like here line 81 and here line 459

hmm... I'm not a fan. But if you're going to do that, can you update the variable name accordingly? Maybe id_or_ids if we have to do this.

i am neutral to this. so i changed that to your favor.

bdunne · 2018-01-16T14:48:41Z

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


Why can't this line be removed now?

Removal of this will break the spec. We'll need ManageIQ/manageiq-providers-ansible_tower#54 (which is in circular dependency with this one).

If we know we'll merge both at the same(near) time, I can do this now.

I assume you meant to link ManageIQ/manageiq-providers-ansible_tower#49 but since that has its own has_one :automation_manager and it's a different subclass of Provider, I don't see how they're related.

No, specifically needed is this fix of the factory object https://github.com/ManageIQ/manageiq-providers-ansible_tower/pull/49/files#diff-58d459079c55b950f3e93005074af740R25

I'm very lost now..

Offline explained this to Brandon that this has to do with an existing bug in the shared Tower spec (linked in my last comment)

bdunne · 2018-01-16T14:54:29Z

app/models/provider.rb

+
+    _log.info(msg)
+    task = MiqTask.create(
+      :name    => "Destroying #{self.class.name} with id: #{id}",


Why not reuse msg since it's the same string?

bdunne · 2018-01-16T14:54:46Z

app/models/provider.rb

+  end
+
+  def destroy_queue
+    msg = "Destroying #{self.class.name} with id: #{id}"


Please spell it out

You mean hard code the class name of current provider? It would then always be the base class, right?

No, "message"

bdunne · 2018-01-16T14:55:51Z

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]
-


Would you mind removing it from the PR?

bdunne · 2018-01-16T15:00:04Z

spec/models/ext_management_system_spec.rb

-
-        expect(MiqQueue.count).to eq(1)
+      expect(MiqQueue.count).to eq(2)
+      expect(MiqQueue.pluck(:instance_id)).to include(ems.id, ems2.id)


Shouldn't this be match_array? I wouldn't expect any other instance_ids to be on the queue.

bdunne · 2018-01-16T15:00:22Z

spec/models/ext_management_system_spec.rb

+  context "#destroy_queue" do
+    let(:server) { EvmSpecHelper.local_miq_server }
+    let(:zone)   { server.zone }
+    let(:ems)    { FactoryGirl.create(:ext_management_system, :zone => zone) }


Sort alphabetically please

bdunne · 2018-01-16T15:02:07Z

spec/models/ext_management_system_spec.rb


-        deliver_queue_message
+      task = MiqTask.find(task_id)


This can be consolidated to a single expectation:

expect(MiqTask.find(task_id)).to have_attributes( :state => "Finished", :status => "Ok", )

bdunne · 2018-01-16T15:04:05Z

spec/models/ext_management_system_spec.rb

+      deliver_queue_message
+
+      expect(MiqQueue.count).to eq(0)
+      expect(ExtManagementSystem.count).to eq(0)


Do you also need an expectation that the worker is gone?

bdunne · 2018-01-16T15:06:21Z

spec/models/provider_spec.rb

+        expect(task).to have_attributes(
+          :state  => MiqTask::STATE_FINISHED,
+          :status => MiqTask::STATUS_OK
+        )


Expect that the manager is gone too?

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

miq-bot · 2018-01-16T20:28:03Z

Checked commit jameswnl@182b7ba with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
9 files checked, 0 offenses detected
Everything looks fine. ⭐

blomquisg · 2018-01-17T16:59:19Z

@bdunne can you re-review based on @jameswnl updates

Provider to orchestrate_destroy managers first - Alt (cherry picked from commit ef04837) https://bugzilla.redhat.com/show_bug.cgi?id=1536039 https://bugzilla.redhat.com/show_bug.cgi?id=1536041

simaishi · 2018-01-18T14:20:19Z

Gaprindashvili backport details:

$ git log -1
commit 70ea5cd6b9e38d282e51a2d6ec00baa11b800d6f
Author: Brandon Dunne <[email protected]>
Date:   Wed Jan 17 15:47:31 2018 -0500

    Merge pull request #16755 from jameswnl/orch-destroy2
    
    Provider to orchestrate_destroy managers first - Alt
    (cherry picked from commit ef04837a999cd942ac9ccf2c6cd6504f5752b6b5)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1536039
    https://bugzilla.redhat.com/show_bug.cgi?id=1536041

Change introduced in ManageIQ/manageiq#16755 is not compatible with OSP provider which uses cascade deletion of related providers. Nullification introduced in ManageIQ#198 breaks tests and possibly affects Infra provider deletion. Trying override destroy method with original from AR until we get better solution (preferably in main repo).

jameswnl force-pushed the orch-destroy2 branch from 834b521 to 3649541 Compare January 5, 2018 18:06

jameswnl mentioned this pull request Jan 5, 2018

Provider to orchestrate_destroy managers first #16614

Closed

jameswnl force-pushed the orch-destroy2 branch from 3649541 to 2d3cbcf Compare January 11, 2018 16:58

miq-bot added blocker bug providers labels Jan 11, 2018

jameswnl mentioned this pull request Jan 11, 2018

Using EMS.destroy instead of EMS.orchestrate_destroy ManageIQ/manageiq-providers-kubernetes#214

Merged

miq-bot added the gaprindashvili/yes label Jan 11, 2018

jameswnl force-pushed the orch-destroy2 branch 2 times, most recently from 179126c to eaee53a Compare January 11, 2018 19:54

bdunne reviewed Jan 12, 2018

View reviewed changes

bdunne requested changes Jan 12, 2018

View reviewed changes

jameswnl force-pushed the orch-destroy2 branch 4 times, most recently from d0a8511 to ca808e9 Compare January 16, 2018 14:01

bdunne requested changes Jan 16, 2018

View reviewed changes

jameswnl force-pushed the orch-destroy2 branch from ca808e9 to fe13fef Compare January 16, 2018 19:52

orchestrate_destroy for provider

182b7ba

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

jameswnl force-pushed the orch-destroy2 branch from fe13fef to 182b7ba Compare January 16, 2018 20:25

jameswnl mentioned this pull request Jan 16, 2018

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

Merged

bdunne merged commit ef04837 into ManageIQ:master Jan 17, 2018

bdunne added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 17, 2018

bdunne self-assigned this Jan 17, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jan 18, 2018

jameswnl mentioned this pull request Jan 18, 2018

Provider base class handles the managers' destroy now ManageIQ/manageiq-providers-openstack#198

Merged

aufi mentioned this pull request Jan 23, 2018

[WIP] Fix destroy provider ManageIQ/manageiq-providers-openstack#207

Closed

agrare mentioned this pull request Jan 25, 2018

[WIP] Set EMS status when during orchestrate_destroy #16574

Closed

cben mentioned this pull request Jul 30, 2019

Do not get stuck on destroy #19077

Merged

jrafanie mentioned this pull request Jun 23, 2020

Ensure ems workers are killed by their server/orchestrator pod #20290

Merged

Provider to orchestrate_destroy managers first - Alt #16755

Provider to orchestrate_destroy managers first - Alt #16755

Conversation

jameswnl commented Jan 5, 2018 • edited Loading

blomquisg commented Jan 5, 2018

jameswnl commented Jan 11, 2018

jameswnl commented Jan 11, 2018

miq-bot commented Jan 11, 2018

jameswnl commented Jan 11, 2018

jameswnl commented Jan 11, 2018

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jan 16, 2018

blomquisg commented Jan 17, 2018

simaishi commented Jan 18, 2018

jameswnl commented Jan 5, 2018 •

edited

Loading