From 1540f9724c9f264ce25c315c945ba4d3ceafafff Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Thu, 25 Jan 2018 11:34:59 -0500 Subject: [PATCH 1/5] Fix Deleting Snapshot on Smartstate Cancel Cancelling a Smartstate Analysis job is supposed to delete the snapshot created if necessary. The code attempted to call an incorrectly named method for the provider in the case of Azure and SCVMM. This resulted in the snapshot being left and consequently the next attempt to run Smartstate on the same VM failed because it seemed as if there was an active analysis job running already. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1538347 --- app/models/vm_scan.rb | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/app/models/vm_scan.rb b/app/models/vm_scan.rb index 217e81248335..f4090ec5eed4 100644 --- a/app/models/vm_scan.rb +++ b/app/models/vm_scan.rb @@ -466,7 +466,8 @@ def process_cancel(*args) _log.info("job canceling, #{options[:message]}") begin - delete_snapshot(context[:snapshot_mor]) + vm = VmOrTemplate.find_by(:id => target_id) + process_delete_evm_snapshot(vm) rescue => err _log.log_backtrace(err) end @@ -491,22 +492,27 @@ def call_abort_retry(*args) end end + def process_delete_evm_snapshot(vm) + vm = VmOrTemplate.find_by(:id => target_id) + unless context[:snapshot_mor].nil? + mor = context[:snapshot_mor] + context[:snapshot_mor] = nil + set_status("Deleting snapshot before cancelling / aborting job") + if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm) + vm.ext_management_system.vm_delete_evm_snapshot(vm, mor) + elsif vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) || + (vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?) + vm.ext_management_system.vm_delete_evm_snapshot(vm, :snMor => mor) + else + delete_snapshot(mor) + end + end + end + def process_abort(*args) begin vm = VmOrTemplate.find_by(:id => target_id) - unless context[:snapshot_mor].nil? - mor = context[:snapshot_mor] - context[:snapshot_mor] = nil - set_status("Deleting snapshot before aborting job") - if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm) - vm.ext_management_system.vm_delete_evm_snapshot(vm, mor) - elsif vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) || - (vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?) - vm.ext_management_system.vm_delete_evm_snapshot(vm, :snMor => mor) - else - delete_snapshot(mor) - end - end + process_delete_evm_snapshot(vm) if vm inputs = {:vm => vm, :host => vm.host} MiqEvent.raise_evm_job_event(vm, {:type => "scan", :suffix => "abort"}, inputs) From 2b953ef468d3064e70853868863f700a9190aa02 Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Thu, 25 Jan 2018 12:44:46 -0500 Subject: [PATCH 2/5] Fix redundant vm assignment caught by Rubocop Rubocop caught a redundant assignment of the vm which is already being passed to a method. --- app/models/vm_scan.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/vm_scan.rb b/app/models/vm_scan.rb index f4090ec5eed4..b894ce2f5e45 100644 --- a/app/models/vm_scan.rb +++ b/app/models/vm_scan.rb @@ -493,7 +493,6 @@ def call_abort_retry(*args) end def process_delete_evm_snapshot(vm) - vm = VmOrTemplate.find_by(:id => target_id) unless context[:snapshot_mor].nil? mor = context[:snapshot_mor] context[:snapshot_mor] = nil From 0277f998aeb8c7171051d499fd761ed69e5373d0 Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Thu, 25 Jan 2018 15:05:10 -0500 Subject: [PATCH 3/5] Rework delete_snapshot to check for proper providers Based on review comments the delete_snapshot method was modified to check for the appropriate providers and call the correct methods in the provider rather than totally avoiding the call to delete_snapshot altogether. --- app/models/vm_scan.rb | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/app/models/vm_scan.rb b/app/models/vm_scan.rb index b894ce2f5e45..76024656f92f 100644 --- a/app/models/vm_scan.rb +++ b/app/models/vm_scan.rb @@ -393,7 +393,14 @@ def delete_snapshot(mor, vm = nil) delete_snapshot_by_description(mor, vm) else user_event = end_user_event_message(vm, false) - vm.ext_management_system.vm_remove_snapshot(vm, :snMor => mor, :user_event => user_event) + if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm) + vm.ext_management_system.vm_delete_evm_snapshot(vm, mor) + elsif vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) || + (vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?) + vm.ext_management_system.vm_delete_evm_snapshot(vm, :snMor => mor) + else + vm.ext_management_system.vm_remove_snapshot(vm, :snMor => mor, :user_event => user_event) + end end else raise _("No Providers available to delete snapshot") @@ -466,8 +473,7 @@ def process_cancel(*args) _log.info("job canceling, #{options[:message]}") begin - vm = VmOrTemplate.find_by(:id => target_id) - process_delete_evm_snapshot(vm) + delete_snapshot(context[:snapshot_mor]) rescue => err _log.log_backtrace(err) end @@ -492,26 +498,22 @@ def call_abort_retry(*args) end end - def process_delete_evm_snapshot(vm) - unless context[:snapshot_mor].nil? - mor = context[:snapshot_mor] - context[:snapshot_mor] = nil - set_status("Deleting snapshot before cancelling / aborting job") - if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm) - vm.ext_management_system.vm_delete_evm_snapshot(vm, mor) - elsif vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) || - (vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?) - vm.ext_management_system.vm_delete_evm_snapshot(vm, :snMor => mor) - else - delete_snapshot(mor) - end - end - end - def process_abort(*args) begin vm = VmOrTemplate.find_by(:id => target_id) - process_delete_evm_snapshot(vm) + unless context[:snapshot_mor].nil? + mor = context[:snapshot_mor] + context[:snapshot_mor] = nil + set_status("Deleting snapshot before aborting job") + if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm) + vm.ext_management_system.vm_delete_evm_snapshot(vm, mor) + elsif vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) || + (vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?) + vm.ext_management_system.vm_delete_evm_snapshot(vm, :snMor => mor) + else + delete_snapshot(mor) + end + end if vm inputs = {:vm => vm, :host => vm.host} MiqEvent.raise_evm_job_event(vm, {:type => "scan", :suffix => "abort"}, inputs) From 4fb0b82b6c2000c59d890760b501eea56c9d848f Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Thu, 25 Jan 2018 15:49:08 -0500 Subject: [PATCH 4/5] Simplify process_abort We are able to simplify process_abort since the lines deleted are performed in the delete_snapshot method now. --- app/models/vm_scan.rb | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/models/vm_scan.rb b/app/models/vm_scan.rb index 76024656f92f..0f505e63f25a 100644 --- a/app/models/vm_scan.rb +++ b/app/models/vm_scan.rb @@ -505,14 +505,7 @@ def process_abort(*args) mor = context[:snapshot_mor] context[:snapshot_mor] = nil set_status("Deleting snapshot before aborting job") - if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm) - vm.ext_management_system.vm_delete_evm_snapshot(vm, mor) - elsif vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) || - (vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?) - vm.ext_management_system.vm_delete_evm_snapshot(vm, :snMor => mor) - else - delete_snapshot(mor) - end + delete_snapshot(mor) end if vm inputs = {:vm => vm, :host => vm.host} From 08e8b598559a7f1627823348948e43c9a8651b16 Mon Sep 17 00:00:00 2001 From: Jerry Keselman Date: Thu, 25 Jan 2018 16:02:11 -0500 Subject: [PATCH 5/5] Pass the VM so delete_snapshot won't need to look it up Review comments requested the above. --- app/models/vm_scan.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/vm_scan.rb b/app/models/vm_scan.rb index 0f505e63f25a..a2315bf6728f 100644 --- a/app/models/vm_scan.rb +++ b/app/models/vm_scan.rb @@ -248,7 +248,7 @@ def call_snapshot_delete (vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?) vm.ext_management_system.vm_delete_evm_snapshot(vm, :snMor => mor) else - delete_snapshot(mor) + delete_snapshot(mor, vm) end rescue => err _log.error(err.to_s) @@ -505,7 +505,7 @@ def process_abort(*args) mor = context[:snapshot_mor] context[:snapshot_mor] = nil set_status("Deleting snapshot before aborting job") - delete_snapshot(mor) + delete_snapshot(mor, vm) end if vm inputs = {:vm => vm, :host => vm.host}