Add Snapshot Code for Azure Managed Disks #117

jerryk55 · 2017-08-24T14:21:17Z

Add code to snapshot VMs with Managed Disks,
as well as the code to delete the snapshots upon
completion of SSA (or aborts).
Also check that the disk requires a snapshot, which for now
is just for Managed Disks. This will change in a subsequent PR.

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:
https://github.com/ManageIQ/azure-armrest#299

A PR for the ManageIQ repo has been added as well to work with this -
https://github.com/ManageIQ/manageiq#15865

Also a PR for the manageiq-smartstate gem is required for this -
ManageIQ/manageiq-smartstate#23

# Links

https://bugzilla.redhat.com/show_bug.cgi?id=1475540
https://bugzilla.redhat.com/show_bug.cgi?id=1459612
ManageIQ/azure-armrest#299
ManageIQ/manageiq#15865
ManageIQ/manageiq-smartstate#23

# Steps for Testing/QA

Add an Azure VM with a Managed Disk.
Run SmartState Analysis against the VM.

@roliveri @hsong-rh please review for SSA sanity.
@bronaghs @blomquisg please review or assign an appropriate providers person to do so.
Thanks!

miq-bot · 2017-08-24T14:28:08Z

This pull request is not mergeable. Please rebase and repush.

roliveri · 2017-08-24T14:42:36Z

app/models/manageiq/providers/azure/cloud_manager/vm_or_template_shared/scanning.rb

@@ -56,4 +56,18 @@ def has_proxy?
  def requires_storage_for_scan?
    false
  end
+
+  def require_snapshot_for_scan?


@jerryk55 So, when you add snapshots for non-managed disks, this method will always return true?

@roliveri no, when you check to see if you need to add a snapshot for non-managed disks, it will always return false, won't it?

@jerryk55 I thought you said that they suggested that both, managed and non-managed disks, always be snapshotted.

Oh yes I misunderstood your original question. Yes - when I add the other code for non-managed disk snapshots - this method will simply return true all the time.

Add code to snapshot VMs with Managed Disks, as well as the code to delete the snapshots upon completion of SSA (or aborts). Also check that the disk requires a snapshot, which for now is just for Managed Disks. This will change in a subsequent PR.

Fix issues called out by RuboCop.

Previously moved method was deleted by another PR but still being added backed in here (incorrectly). Test caught the issue.

jerryk55 · 2017-08-30T19:36:21Z

@bronaghs @blomquisg @djberg96 Can someone on the Providers team review and merge please? We have outstanding BZs waiting for this as well as other PRs that have been merged waiting for this set of changes. Thanks.

bdunne · 2017-08-30T19:53:10Z

@jerryk55 Are you going to address the rubocop comments first?

jerryk55 · 2017-08-30T19:57:48Z

@bdunne - ah, the RuboCop comments. The first go-round RuboCop complained because there weren't parentheses around the arguments to the log message calls. So I put them in. And now its complaining because there ARE parentheses around the log message calls. So I would like to throw Rubocop in the garbage, thank you very much. And it is worse than that. Look at the three warnings for line 17 of scanning.rb. It is both complaining that there are and aren't parentheses - there are. If you can provide insight as to how to shut it the heck up I will gladly take advantage.

Spaces before parens are a bad thing. Also ending braces after a hash definition need to be on a separate line.

jerryk55 · 2017-08-30T20:27:36Z

@bdunne excuse my rant. :-) I've been absolved of all warnings by the almighty Rubocop.

blomquisg · 2017-08-30T20:51:37Z

app/models/manageiq/providers/azure/cloud_manager.rb

+      end
+    end
+    _log.error ("SSA Snapshot #{snap_name} already exists.")
+    raise "Snapshot #{snap_name} already exists. Another SSA request for this VM is in progress or a previous one failed to clean up properly."


Who calls this method? I'm seeing the possibility of a caller having to react to two different error conditions in possibly two different ways. One error condition is when it can't create the snapshot because of an error (probably in Azure). And another error condition when an SSA request is already being processed for this VM.

Maybe the "snapshot already exists" situation could be a specific error type? Then, the caller could rescue that error type in its own block with a special handler. And all other errors could be assumed to be an actual failure to process the request by the provider.

I could also see the "snapshot already exists" situation being a non-error, and just log a warning and return nil. But, that might be too ambiguous for the caller.

Thoughts?

blomquisg · 2017-08-30T20:53:07Z

app/models/manageiq/providers/azure/cloud_manager.rb

+      end
+    end
+    _log.error("SSA Snapshot #{snap_name} already exists.")
+    raise "Snapshot #{snap_name} already exists. Another SSA request for this VM is in progress or a previous one failed to clean up properly."


Who calls this method? I'm seeing the possibility of a caller having to react to two different error conditions in possibly two different ways. One error condition is when it can't create the snapshot because of an error (probably in Azure). And another error condition when an SSA request is already being processed for this VM.

Maybe the "snapshot already exists" situation could be a specific error type? Then, the caller could rescue that error type in its own block with a special handler. And all other errors could be assumed to be an actual failure to process the request by the provider.

I could also see the "snapshot already exists" situation being a non-error, and just log a warning and return nil. But, that might be too ambiguous for the caller.

Thoughts?

@blomquisg It is called by VmScan.create_snapshot. We have similar logic for the Scvmm code as well (although some of it is embedded in the manageiq-smartstate repo with the separation of code into different repos)

It really doesn't matter to VmScan if we can't perform the snapshot because of a provider error or because the snapshot already exists - the SSA request needs to fail in either case.

The "snapshot already exists" situation cannot be a non-error - if there really is another request to run SSA on the same instance/image that we somehow missed (which should not happen) a problem would arise when the first request finished and therefore deleted the snapshot. Since these requests are stateless and the snapshot disk isn't actually mounted or opened the second request would then run into an unexplained error.

Let me know what you think. @roliveri, feel free to chime in if you feel otherwise.

Historically, vm_create_evm_snapshot is not a general purpose snapshot creation method, evm snapshots are only created for SSA. SSA doesn't allow multiple evm (SSA) snapshots, hence the error.

Maybe there should be a general create_snapshot method, that is called by vm_create_evm_snapshot.

@roliveri you mean as a follow-on task for all the providers needing it, right?

Yes. If a provider needs a general create snapshot facility, then it makes sense to structure the code that way. As it stands now, vm_create_evm_snapshot is SSA specific, so it should be ok the way it is.

jerryk55 · 2017-08-31T11:46:41Z

@blomquisg so please let me know if there are any other questions or if we can get this merged. Thanks!

Fryguy · 2017-08-31T13:39:51Z

app/models/manageiq/providers/azure/cloud_manager.rb

+                         :sourceResourceId => os_disk.managed_disk.id
+                       }
+                     } }
+    snap_name = os_disk.name + "__EVM__SSA__SNAPSHOT"


Prefer interpolation over concatenation.

@Fryguy understood. Updated.

After review comments the snapshot name composition has been changed from string concatenation to string interpolation.

miq-bot · 2017-08-31T15:08:28Z

Checked commits jerryk55/manageiq-providers-azure@9b32203~...527507a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

jerryk55 · 2017-09-05T13:54:12Z

@miq-bot add_label fine/yes

Add Snapshot Code for Azure Managed Disks (cherry picked from commit 4492103) https://bugzilla.redhat.com/show_bug.cgi?id=1488967

simaishi · 2017-09-06T17:59:21Z

Fine backport details:

$ git log -1
commit 2dd69ef21edfc14356aca44bcbc6a254052e2d5a
Author: Greg Blomquist <[email protected]>
Date:   Fri Sep 1 10:15:38 2017 -0400

    Merge pull request #117 from jerryk55/managed_disk_snapshot_support
    
    Add Snapshot Code for Azure Managed Disks
    (cherry picked from commit 4492103ec7bfd198e184e144221cb2b398d023b6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1488967

miq-bot added unmergeable and removed unmergeable labels Aug 24, 2017

roliveri reviewed Aug 24, 2017

View reviewed changes

jerryk55 force-pushed the managed_disk_snapshot_support branch from 27e0f32 to e59502f Compare August 24, 2017 16:14

This was referenced Aug 25, 2017

Handle Managed Disk Snapshots ManageIQ/manageiq-smartstate#23

Merged

Create Snapshot for Azure ManageIQ/manageiq#15865

Merged

jerryk55 added 2 commits August 30, 2017 13:35

RuboCop Comments

cc6cc7e

Fix issues called out by RuboCop.

roliveri approved these changes Aug 30, 2017

View reviewed changes

Remove Vm.resource_group method

676448c

Previously moved method was deleted by another PR but still being added backed in here (incorrectly). Test caught the issue.

jerryk55 force-pushed the managed_disk_snapshot_support branch from e59502f to 676448c Compare August 30, 2017 19:22

Attempting to Silence Last set of Rubocop Complaints

8f8c68a

Spaces before parens are a bad thing. Also ending braces after a hash definition need to be on a separate line.

jerryk55 force-pushed the managed_disk_snapshot_support branch from f2ca02a to 8f8c68a Compare August 30, 2017 20:26

blomquisg reviewed Aug 30, 2017

View reviewed changes

bronaghs assigned blomquisg Aug 31, 2017

Fryguy reviewed Aug 31, 2017

View reviewed changes

jerryk55 force-pushed the managed_disk_snapshot_support branch from 82ae977 to c005b0d Compare August 31, 2017 14:43

Review Comment on String Interpolation

527507a

After review comments the snapshot name composition has been changed from string concatenation to string interpolation.

jerryk55 force-pushed the managed_disk_snapshot_support branch from c005b0d to 527507a Compare August 31, 2017 15:05

blomquisg merged commit 4492103 into ManageIQ:master Sep 1, 2017

blomquisg added this to the Sprint 68 Ending Sep 4, 2017 milestone Sep 1, 2017

miq-bot added the fine/yes label Sep 5, 2017

simaishi added the blocker label Sep 5, 2017

simaishi pushed a commit that referenced this pull request Sep 6, 2017

Merge pull request #117 from jerryk55/managed_disk_snapshot_support

2dd69ef

Add Snapshot Code for Azure Managed Disks (cherry picked from commit 4492103) https://bugzilla.redhat.com/show_bug.cgi?id=1488967

simaishi added fine/backported and removed fine/yes labels Sep 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Snapshot Code for Azure Managed Disks #117

Add Snapshot Code for Azure Managed Disks #117

jerryk55 commented Aug 24, 2017 •

edited

Loading

miq-bot commented Aug 24, 2017

roliveri Aug 24, 2017

jerryk55 Aug 24, 2017

roliveri Aug 24, 2017

jerryk55 Aug 24, 2017

jerryk55 commented Aug 30, 2017

bdunne commented Aug 30, 2017

jerryk55 commented Aug 30, 2017 •

edited

Loading

jerryk55 commented Aug 30, 2017

blomquisg Aug 30, 2017

blomquisg Aug 30, 2017 •

edited

Loading

jerryk55 Aug 30, 2017 •

edited

Loading

roliveri Aug 30, 2017

jerryk55 Aug 30, 2017

roliveri Aug 30, 2017

jerryk55 commented Aug 31, 2017

Fryguy Aug 31, 2017

jerryk55 Aug 31, 2017

miq-bot commented Aug 31, 2017

jerryk55 commented Sep 5, 2017

simaishi commented Sep 6, 2017

Add Snapshot Code for Azure Managed Disks #117

Add Snapshot Code for Azure Managed Disks #117

Conversation

jerryk55 commented Aug 24, 2017 • edited Loading

miq-bot commented Aug 24, 2017

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

jerryk55 commented Aug 30, 2017

bdunne commented Aug 30, 2017

jerryk55 commented Aug 30, 2017 • edited Loading

jerryk55 commented Aug 30, 2017

Choose a reason for hiding this comment

blomquisg Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

jerryk55 Aug 30, 2017 • edited Loading

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

jerryk55 commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Aug 31, 2017

jerryk55 commented Sep 5, 2017

simaishi commented Sep 6, 2017

jerryk55 commented Aug 24, 2017 •

edited

Loading

jerryk55 commented Aug 30, 2017 •

edited

Loading

blomquisg Aug 30, 2017 •

edited

Loading

jerryk55 Aug 30, 2017 •

edited

Loading