Fix Deleting Snapshot on Smartstate Cancel #16885

jerryk55 · 2018-01-25T16:42:00Z

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.

@roliveri @hsong-rh please review this blocker fix.

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

Links

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

Steps for Testing/QA [Optional]

Run Smartstate Analysis on either an Azure or SCVMM VM. Once the snapshot has been taken and scanning has commenced, select the "Cancel Job" button. The job should be cancelled and the snapshot should be deleted.
If you attempt to run another Smartstate Analysis job on the same VM it should not fail because the Snapshot already exists.

roliveri · 2018-01-25T18:18:51Z

app/models/vm_scan.rb

@@ -491,22 +492,26 @@ def call_abort_retry(*args)
    end
  end

+  def process_delete_evm_snapshot(vm)


It seems this method is only necessary because of the differences between vm_delete_evm_snapshot methods of different providers. Ultimately, I think there should be a delete_evm_snapshot method in the VM object. Then each type of VM can have their own implementation with a common signature. Then all we'd have to do is call vm.delete_evm_snapshot(mor) without all the if vm.kind_of? checks.

For now, I think this fix is ok the way it is, but we should consider cleaning it up in the future.

agrare

👍

agrare · 2018-01-25T18:32:59Z

Looks like openstack is the outlier in not accepting the same options, 👍 for unifying the snapshot operations

hsong-rh · 2018-01-25T18:34:11Z

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)


The replaced delete_snapshot method has some logics about user_event and cleaning up snapshot descriptions. They are all missed in your new defined process_delete_evm_snapshot method. You sure they don't need?

On further review you are correct @hsong-rh. Reworking this PR now and will re-test.

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

Rubocop caught a redundant assignment of the vm which is already being passed to a method.

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.

jerryk55 · 2018-01-25T20:07:33Z

@hsong-rh I reworked the fix based on your comments.

chessbyte · 2018-01-25T20:15:15Z

@agrare Wondering if, in a future PR, we cannot just have

vm.delete_evm_snapshot(mor) if vm.require_snapshot_for_scan?

instead of

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

roliveri · 2018-01-25T20:20:38Z

@chessbyte That's exactly what my comment suggested, before it became "outdated" and hidden :-)

roliveri · 2018-01-25T20:41:30Z

app/models/vm_scan.rb

+        mor = context[:snapshot_mor]
+        context[:snapshot_mor] = nil
+        set_status("Deleting snapshot before aborting job")
+        if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm)


Shouldn't you change this whole conditional block to:

delete_snapshot(mor, vm)

now that delete_snapshot performs those checks?

Yup. YAP (Yet Another Push) coming up.

We are able to simplify process_abort since the lines deleted are performed in the delete_snapshot method now.

roliveri · 2018-01-25T20:57:10Z

app/models/vm_scan.rb

-        else
-          delete_snapshot(mor)
-        end
+        delete_snapshot(mor)


Pass the vm so delete_snapshot won't have to look it up again:

delete_snapshot(mor, vm)

Review comments requested the above.

miq-bot · 2018-01-25T21:14:13Z

Checked commits jerryk55/manageiq@1540f97~...08e8b59 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

chessbyte · 2018-01-25T22:39:13Z

@roliveri waiting on someone to review before merging?

…a_cancel Fix Deleting Snapshot on Smartstate Cancel

Fix Deleting Snapshot on Smartstate Cancel (cherry picked from commit 4d26ddb) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1539756

simaishi · 2018-01-29T14:47:36Z

Gaprindashvili backport details:

$ git log -1
commit bc0a80d8e77aee7b1f1f14de2c0d60efa6a147ae
Author: Richard Oliveri <[email protected]>
Date:   Sat Jan 27 15:57:30 2018 -0500

    Merge pull request #16885 from jerryk55/fix_delete_snap_on_ssa_cancel
    
    Fix Deleting Snapshot on Smartstate Cancel
    (cherry picked from commit 4d26ddb64ad31361a19ce75def14f8b9decc296d)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1539756

chessbyte assigned roliveri Jan 25, 2018

chessbyte requested review from agrare and bronaghs January 25, 2018 17:50

chessbyte added bug blocker gaprindashvili/yes core/smart state labels Jan 25, 2018

roliveri approved these changes Jan 25, 2018

View reviewed changes

bronaghs approved these changes Jan 25, 2018

View reviewed changes

agrare approved these changes Jan 25, 2018

View reviewed changes

hsong-rh reviewed Jan 25, 2018

View reviewed changes

jerryk55 added 3 commits January 25, 2018 15:06

Fix redundant vm assignment caught by Rubocop

2b953ef

Rubocop caught a redundant assignment of the vm which is already being passed to a method.

Rework delete_snapshot to check for proper providers

0277f99

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.

jerryk55 force-pushed the fix_delete_snap_on_ssa_cancel branch from 0e3c401 to 0277f99 Compare January 25, 2018 20:06

roliveri suggested changes Jan 25, 2018

View reviewed changes

Simplify process_abort

4fb0b82

We are able to simplify process_abort since the lines deleted are performed in the delete_snapshot method now.

roliveri suggested changes Jan 25, 2018

View reviewed changes

Pass the VM so delete_snapshot won't need to look it up

08e8b59

Review comments requested the above.

roliveri approved these changes Jan 25, 2018

View reviewed changes

roliveri merged commit 4d26ddb into ManageIQ:master Jan 27, 2018

roliveri added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 27, 2018

paulslaby pushed a commit to paulslaby/manageiq that referenced this pull request Jan 29, 2018

Merge pull request ManageIQ#16885 from jerryk55/fix_delete_snap_on_ss…

79b7ed0

…a_cancel Fix Deleting Snapshot on Smartstate Cancel

simaishi pushed a commit that referenced this pull request Jan 29, 2018

Merge pull request #16885 from jerryk55/fix_delete_snap_on_ssa_cancel

bc0a80d

Fix Deleting Snapshot on Smartstate Cancel (cherry picked from commit 4d26ddb) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1539756

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Deleting Snapshot on Smartstate Cancel #16885

Fix Deleting Snapshot on Smartstate Cancel #16885

jerryk55 commented Jan 25, 2018

roliveri Jan 25, 2018

agrare left a comment

agrare commented Jan 25, 2018

hsong-rh Jan 25, 2018

jerryk55 Jan 25, 2018

jerryk55 commented Jan 25, 2018

chessbyte commented Jan 25, 2018 •

edited

Loading

roliveri commented Jan 25, 2018

roliveri Jan 25, 2018

jerryk55 Jan 25, 2018

roliveri Jan 25, 2018

miq-bot commented Jan 25, 2018

chessbyte commented Jan 25, 2018

simaishi commented Jan 29, 2018

Fix Deleting Snapshot on Smartstate Cancel #16885

Fix Deleting Snapshot on Smartstate Cancel #16885

Conversation

jerryk55 commented Jan 25, 2018

Links

Steps for Testing/QA [Optional]

roliveri Jan 25, 2018

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

agrare commented Jan 25, 2018

hsong-rh Jan 25, 2018

Choose a reason for hiding this comment

jerryk55 Jan 25, 2018

Choose a reason for hiding this comment

jerryk55 commented Jan 25, 2018

chessbyte commented Jan 25, 2018 • edited Loading

roliveri commented Jan 25, 2018

roliveri Jan 25, 2018

Choose a reason for hiding this comment

jerryk55 Jan 25, 2018

Choose a reason for hiding this comment

roliveri Jan 25, 2018

Choose a reason for hiding this comment

miq-bot commented Jan 25, 2018

chessbyte commented Jan 25, 2018

simaishi commented Jan 29, 2018

chessbyte commented Jan 25, 2018 •

edited

Loading