Fix for Archived Openstack instances #5410

romanblanco · 2015-11-12T15:36:41Z

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

chessbyte · 2015-12-22T14:37:00Z

app/models/miq_report/search.rb

@@ -104,6 +104,8 @@ def paged_view_search(options = {})

    if options[:parent]
      targets = get_parent_targets(options)
+      # filter out archived VMs
+      targets = targets.select { |target| !Vm.find_by(:id => target).archived? unless Vm.find_by(:id => target).nil? }


@matthewd @Fryguy something looks off here (making the same SQL query twice on one line). Suggestions?

@chessbyte as I'm looking on it now, I'm considering to use try Rails method , so this line would become

targets = targets.select { |target| !Vm.find_by(:id => target).try(:archived?) }

but that would mean, I'd need to do the same with condition below, checking if target variable is empty?.

Any better suggestions? @matthewd @Fryguy

chessbyte · 2016-01-07T22:17:58Z

@dclarizio please review

dclarizio · 2016-01-07T23:00:58Z

@Ladas could you verify this fix please?
@gtanzillo please review the model changes and specs.
Thx, Dan

dclarizio · 2016-01-14T18:29:28Z

@Ladas @gtanzillo re-ping :)

gtanzillo · 2016-01-14T18:53:59Z

app/models/miq_report/search.rb

@@ -104,7 +104,9 @@ def paged_view_search(options = {})

    if options[:parent]
      targets = get_parent_targets(options)
-      if targets.empty?
+      # filter out archived VMs
+      targets = targets.select { |target| !Vm.find_by(:id => target).try(:archived?) }


I think this change is a problem for a couple of reasons. First, paged_view_search is generically used for searching on any kind of object. Not just VMs. So, there's no guarantee that the ids in targets will be the ids of Vm objects. Second, looking up each Vm by id one at a time will create an n+1 query performance issue.

I see that you are filtering out the archived Vms here https://github.com/ManageIQ/manageiq/pull/5410/files#diff-6e332326f5467e2bd25aa3441a9cba33R36, anyway. So, maybe it's ok to just remove this change. Otherwise, additional filtering options would need to be passed down through the callers.

What @gtanzillo says, you need to pass the correct sql conditions (!archived) from above, this code should not mention any specific classes. I assume that adding of that condition should depend on your position in the tree?

E.g. when you are on all VMs or archived, you should see archived. When you click on provider, you should not see them?

romanblanco · 2016-01-27T12:09:59Z

@gtanzillo @Ladas PR updated, can you review if it's OK now? 😊

gtanzillo · 2016-02-02T12:02:09Z

👍 This looks good to me now. @Ladas can you please review as well when you can?

miq-bot · 2016-02-03T10:28:38Z

Checked commits romanblanco/manageiq@69113a1~...c41c431 with ruby 2.2.3, rubocop 0.34.2, and haml-lint 0.13.0
4 files checked, 0 offenses detected
Everything looks good. 🍪

Ladas · 2016-02-03T14:25:50Z

I will try to get to it on Monday, I am a bit swamped now :-)

Ladas · 2016-02-10T15:13:43Z

Seems it works when you delete a provider, then the archived instance is displayed correctly. (If it shouldn't be in Orphaned state?)

But when you retire the instance, it's not displayed correctly. And seems like retired instance is deleted in the provider, so it disappears entirely after refresh. But in the meantime, it's displayed among the active VMs

chessbyte · 2016-02-11T03:48:45Z

@romanblanco please review the comments from @Ladas

romanblanco · 2016-02-22T12:40:00Z

@Ladas I've tried to reproduce what you have described, but it seems that there is some bug in retire_now method. I'm getting this error in evm.log:

[----] I, [2016-02-22T13:33:29.050136 #19788:2b05fa5bd968]  INFO -- : Raising Retirement Event for [cloudformation-spec]
[----] I, [2016-02-22T13:33:29.054524 #19788:2b05fa5bd968]  INFO -- : <AutomationEngine> MiqAeEvent.build_evm_event >> event=<"request_orchestration_stack_retire"> inputs=<{:retirement_initiator=>"system", :orchestration_stack=>#<ManageIQ::Providers::Amazon::CloudManager::OrchestrationStack id: 10000000000010, name: "cloudformation-spec", type: "ManageIQ::Providers::Amazon::CloudManager::Orchest...", description: "AWS CloudFormation Sample Template vpc_single_inst...", status: "CREATE_FAILED", ems_ref: "arn:aws:cloudformation:us-east-1:200278856672:stac...", ancestry: nil, ems_id: 10000000000009, orchestration_template_id: 10000000000001, created_at: "2015-10-22 09:12:49", updated_at: "2015-10-30 13:14:31", retired: false, retires_on: "2015-11-01", retirement_warn: 7, retirement_last_warn: "2015-10-30 13:14:31", retirement_state: nil, retirement_requester: nil, status_reason: "The following resource(s) failed to create: [WebSe...", cloud_tenant_id: nil, resource_group: nil>, :type=>"ManageIQ::Providers::Amazon::CloudManager::OrchestrationStack", "MiqEvent::miq_event"=>10000000285949, :miq_event_id=>10000000285949, "EventStream::event_stream"=>10000000285949, :event_stream_id=>10000000285949}>
[----] E, [2016-02-22T13:33:29.055063 #19788:2b05fa5bd968] ERROR -- : [NoMethodError]: undefined method `tenant_identity' for #<ManageIQ::Providers::Amazon::CloudManager::OrchestrationStack:0x00560bfaab3360>  Method:[rescue in retire_now]
[----] E, [2016-02-22T13:33:29.055241 #19788:2b05fa5bd968] ERROR -- : /home/rblanco/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/activemodel-5.0.0.beta2/lib/active_model/attribute_methods.rb:433:in `method_missing'
/home/rblanco/devel/manageiq/lib/miq_automation_engine/engine/miq_ae_event.rb:114:in `call_automate'
/home/rblanco/devel/manageiq/lib/miq_automation_engine/engine/miq_ae_event.rb:41:in `raise_evm_event'
/home/rblanco/devel/manageiq/app/models/miq_event.rb:43:in `raise_evm_event'
/home/rblanco/devel/manageiq/app/models/mixins/retirement_mixin.rb:168:in `raise_retirement_event'
/home/rblanco/devel/manageiq/app/models/mixins/retirement_mixin.rb:115:in `retire_now'
/home/rblanco/devel/manageiq/app/models/mixins/retirement_mixin.rb:103:in `retirement_check'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta2/lib/active_record/relation/delegation.rb:39:in `each'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/activerecord-5.0.0.beta2/lib/active_record/relation/delegation.rb:39:in `each'
/home/rblanco/devel/manageiq/app/models/orchestration_stack/retirement_management.rb:13:in `retirement_check'
/home/rblanco/devel/manageiq/app/models/miq_queue.rb:335:in `block in deliver'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/2.2.0/timeout.rb:89:in `block in timeout'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/2.2.0/timeout.rb:34:in `block in catch'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/2.2.0/timeout.rb:34:in `catch'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/2.2.0/timeout.rb:34:in `catch'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/2.2.0/timeout.rb:104:in `timeout'
/home/rblanco/devel/manageiq/app/models/miq_queue.rb:331:in `deliver'
/home/rblanco/devel/manageiq/app/models/miq_queue_worker_base/runner.rb:106:in `deliver_queue_message'
/home/rblanco/devel/manageiq/app/models/miq_queue_worker_base/runner.rb:134:in `deliver_message'
/home/rblanco/devel/manageiq/app/models/miq_queue_worker_base/runner.rb:151:in `block in do_work'
/home/rblanco/devel/manageiq/app/models/miq_queue_worker_base/runner.rb:145:in `loop'
/home/rblanco/devel/manageiq/app/models/miq_queue_worker_base/runner.rb:145:in `do_work'
/home/rblanco/devel/manageiq/app/models/miq_worker/runner.rb:327:in `block in do_work_loop'
/home/rblanco/devel/manageiq/app/models/miq_worker/runner.rb:324:in `loop'
/home/rblanco/devel/manageiq/app/models/miq_worker/runner.rb:324:in `do_work_loop'
/home/rblanco/devel/manageiq/app/models/miq_worker/runner.rb:146:in `run'
/home/rblanco/devel/manageiq/app/models/miq_worker/runner.rb:121:in `start'
/home/rblanco/devel/manageiq/app/models/miq_worker/runner.rb:21:in `start_worker'
/home/rblanco/devel/manageiq/app/models/miq_worker.rb:346:in `block in start'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/nakayoshi_fork-0.0.3/lib/nakayoshi_fork.rb:24:in `fork'
/home/rblanco/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/nakayoshi_fork-0.0.3/lib/nakayoshi_fork.rb:24:in `fork'
/home/rblanco/devel/manageiq/app/models/miq_worker.rb:344:in `start'
/home/rblanco/devel/manageiq/app/models/miq_worker.rb:274:in `start_worker'
/home/rblanco/devel/manageiq/app/models/miq_worker.rb:154:in `block in sync_workers'
/home/rblanco/devel/manageiq/app/models/miq_worker.rb:154:in `times'
/home/rblanco/devel/manageiq/app/models/miq_worker.rb:154:in `sync_workers'
/home/rblanco/devel/manageiq/app/models/miq_server/worker_management/monitor.rb:49:in `block in sync_workers'
/home/rblanco/devel/manageiq/app/models/miq_server/worker_management/monitor.rb:47:in `each'
/home/rblanco/devel/manageiq/app/models/miq_server/worker_management/monitor.rb:47:in `sync_workers'
/home/rblanco/devel/manageiq/app/models/miq_server.rb:186:in `start'
/home/rblanco/devel/manageiq/app/models/miq_server.rb:273:in `start'
/home/rblanco/devel/manageiq/lib/workers/evm_server.rb:60:in `start'
/home/rblanco/devel/manageiq/lib/workers/evm_server.rb:79:in `start'
/home/rblanco/devel/manageiq/lib/workers/bin/evm_server.rb:4:in `<main>'

Ladas · 2016-02-22T12:44:15Z

@romanblanco ah, please fill an issue for that and mention @blomquisg

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

martinpovolny · 2016-03-09T08:30:18Z

@romanblanco , @Ladas : can we merge this one?

Ladas · 2016-03-09T14:28:14Z

@martinpovolny I think @romanblanco is still working on this one

dclarizio · 2016-03-16T17:34:00Z

@romanblanco Is there more to do on this?

romanblanco · 2016-03-16T17:47:38Z

@dclarizio yes, i think it's still not working properly

romanblanco · 2016-03-21T18:53:11Z

@dclarizio @Ladas checked again, seems to work.

martinpovolny · 2016-03-21T18:59:39Z

If we are done, then remove the WIP and let's merge it!

Fix for Archived Openstack instances

chessbyte added bug wip providers/openstack labels Nov 15, 2015

romanblanco force-pushed the BZ1254636 branch from 33970c3 to d21c443 Compare November 16, 2015 15:37

romanblanco changed the title ~~[WIP] Fix for Archived Openstack instances~~ Fix for Archived Openstack instances Nov 16, 2015

romanblanco force-pushed the BZ1254636 branch 2 times, most recently from 554c165 to 69fa030 Compare November 21, 2015 16:59

miq-bot removed the wip label Nov 26, 2015

chessbyte reviewed Dec 22, 2015
View reviewed changes

romanblanco force-pushed the BZ1254636 branch from 69fa030 to 6bada05 Compare January 7, 2016 15:20

chessbyte added the ui label Jan 7, 2016

gtanzillo reviewed Jan 14, 2016
View reviewed changes

romanblanco force-pushed the BZ1254636 branch 5 times, most recently from 61c530d to 83b498d Compare January 27, 2016 10:10

romanblanco force-pushed the BZ1254636 branch from 83b498d to c41c431 Compare February 3, 2016 10:23

romanblanco mentioned this pull request Feb 22, 2016

Retiring instance does not work #6853

Closed

romanblanco added 4 commits March 7, 2016 13:18

Archived Openstack instances only in "Archived" tree

c7e2540

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

Optimalization of SQL query for loading target VMs

6c6cace

Modified specs

b3b3839

Review response

7806ad2

romanblanco force-pushed the BZ1254636 branch from c41c431 to 7806ad2 Compare March 7, 2016 12:18

dclarizio added the wip label Mar 17, 2016

dclarizio changed the title ~~Fix for Archived Openstack instances~~ [WIP] Fix for Archived Openstack instances Mar 17, 2016

romanblanco changed the title ~~[WIP] Fix for Archived Openstack instances~~ Fix for Archived Openstack instances Mar 21, 2016

miq-bot removed the wip label Mar 21, 2016

martinpovolny added this to the Sprint 38 Ending Mar 28, 2016 milestone Mar 21, 2016

martinpovolny pushed a commit that referenced this pull request Mar 21, 2016

Merge pull request #5410 from romanblanco/BZ1254636

22cf82a

Fix for Archived Openstack instances

martinpovolny merged commit 22cf82a into ManageIQ:master Mar 21, 2016

romanblanco deleted the BZ1254636 branch March 21, 2016 19:08

romanblanco mentioned this pull request Apr 11, 2016

Corrected condition for loading VMs #7864

Merged

Fryguy added the providers/cloud label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Archived Openstack instances #5410

Fix for Archived Openstack instances #5410

romanblanco commented Nov 12, 2015

chessbyte Dec 22, 2015

romanblanco Dec 22, 2015

chessbyte commented Jan 7, 2016

dclarizio commented Jan 7, 2016

dclarizio commented Jan 14, 2016

gtanzillo Jan 14, 2016

Ladas Jan 15, 2016

romanblanco commented Jan 27, 2016

gtanzillo commented Feb 2, 2016

miq-bot commented Feb 3, 2016

Ladas commented Feb 3, 2016

Ladas commented Feb 10, 2016

chessbyte commented Feb 11, 2016

romanblanco commented Feb 22, 2016

Ladas commented Feb 22, 2016

martinpovolny commented Mar 9, 2016

Ladas commented Mar 9, 2016

dclarizio commented Mar 16, 2016

romanblanco commented Mar 16, 2016

romanblanco commented Mar 21, 2016

martinpovolny commented Mar 21, 2016

Fix for Archived Openstack instances #5410

Fix for Archived Openstack instances #5410

Conversation

romanblanco commented Nov 12, 2015

chessbyte Dec 22, 2015

Choose a reason for hiding this comment

romanblanco Dec 22, 2015

Choose a reason for hiding this comment

chessbyte commented Jan 7, 2016

dclarizio commented Jan 7, 2016

dclarizio commented Jan 14, 2016

gtanzillo Jan 14, 2016

Choose a reason for hiding this comment

Ladas Jan 15, 2016

Choose a reason for hiding this comment

romanblanco commented Jan 27, 2016

gtanzillo commented Feb 2, 2016

miq-bot commented Feb 3, 2016

Ladas commented Feb 3, 2016

Ladas commented Feb 10, 2016

chessbyte commented Feb 11, 2016

romanblanco commented Feb 22, 2016

Ladas commented Feb 22, 2016

martinpovolny commented Mar 9, 2016

Ladas commented Mar 9, 2016

dclarizio commented Mar 16, 2016

romanblanco commented Mar 16, 2016

romanblanco commented Mar 21, 2016

martinpovolny commented Mar 21, 2016