Create Snapshot for Azure #15865
Create Snapshot for Azure #15865
Conversation
| @@ -102,6 +102,8 @@ def call_snapshot_create | |||
| if vm.kind_of?(ManageIQ::Providers::Openstack::CloudManager::Vm) || | |||
| vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) | |||
| return unless create_snapshot(vm) | |||
| elsif vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan? | |||
There was a problem hiding this comment.
why not just have each type of Vm have a require_snapshot_for_scan? method and remove all this complex if/else/elsif logic?
There was a problem hiding this comment.
@chessbyte I agree. It does open up a larger can of worms because there is similar logic for the snapshot removal and it seems that there is VMware logic built in to the code here in both cases. Doing some cleanup in this file would be helpful. For the purposes of fixing this BZ however, I'm not sure this is the right place to have it done. I can add it as a follow-on if you like. Thoughts?
There was a problem hiding this comment.
Follow-up PRs to address this make sense. Please put it on your team's radar.
There was a problem hiding this comment.
Ideally, we'd have different scan job classes based on VM type. The jobs for the types of VMs that don't require snapshots won't even have a snapshot state.
app/models/vm_scan.rb
Outdated
| @@ -242,7 +244,8 @@ def call_snapshot_delete | |||
| # or, make type-specific Job classes. | |||
| 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) | |||
| elsif vm.kind_of?(ManageIQ::Providers::Microsoft::InfraManager::Vm) || | |||
| vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) | |||
There was a problem hiding this comment.
The condition for creating the snapshot is:
vm.kind_of?(ManageIQ::Providers::Azure::CloudManager::Vm) && vm.require_snapshot_for_scan?Shouldn't that be the same condition for deleting the snapshot? The same question applies to the delete snapshot in the about code path.
For Azure VMs check if a snapshot is required for SSA and if so call the snapshot code. On this go-round the snapshot will only be required for Managed Disks.
Add Snapshot delete calls on abort and success.
Multiline condition indentation fixed.
Review comment pointed out that while we are validating that a VM requires a snapshot (for a managed disk) before taking the snapshot, we are not checking that the snapshot is required before deleting it either in the success or abort case.
jerryk55
force-pushed
the
snapshot_azure_for_ssa
branch
from
dc883e4 to
9c80330
Compare
|
Checked commits jerryk55/manageiq@8d6454d~...9c80330 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@miq-bot add_label fine/yes |
Create Snapshot for Azure (cherry picked from commit ab36e54) https://bugzilla.redhat.com/show_bug.cgi?id=1488967
|
Fine backport details: |
Create Snapshot for Azure (cherry picked from commit ab36e54) https://bugzilla.redhat.com/show_bug.cgi?id=1488967
For Azure VMs check if a snapshot is required for SSA and if
so call the snapshot code.
On this go-round the snapshot will only be required for Managed Disks.
This is one of several PRs required to add support for SSA for Managed Disks,
that is is response to the following BZs:
https://bugzilla.redhat.com/show_bug.cgi?id=1475540
and
https://bugzilla.redhat.com/show_bug.cgi?id=1459612
A PR for the azure-armrest gem has already been merged in advance of this:
ManageIQ/azure-armrest#299
A PR for the manageiq-smartstate gem has been submitted as well:
ManageIQ/manageiq-smartstate#23
In addition a PR for the manageiq-providers-azure repo was added:
ManageIQ/manageiq-providers-azure#117
Links
Steps for Testing/QA
Add an Azure VM with a Managed Disk.
Run SmartState Analysis against the VM.
@roliveri @hsong-rh please review.