Use the Smartstate Snapshot Name Built by the Provider #44
Conversation
Since there is an 80 character limit on the length of the snapshot name constructed for managed disks use the name set by the Azure provider. If there is no name passed in the provider is using the previous code that built the name known here and use that name instead.
lib/MiqVm/miq_azure_vm.rb
Outdated
| if args[:snapshot] | ||
| @disk_name = args[:snapshot] | ||
| else | ||
| @disk_name = os_disk.name + "__EVM__SSA__SNAPSHOT" |
There was a problem hiding this comment.
@jerryk55 Under what conditions will we enter this else block? I don't understand when this code will be paired with older provider code.
If we can enter this else block, isn't it the same problem we had in the first place?
There was a problem hiding this comment.
@roliveri I was concerned that there could be a situation where the old provider code was running with the new Smartstate gem and if so then the name of the snapshot would need to be the same as it was previous to this update. (more on that below)
If we enter this block and the old name is being used, then yes we have an issue if the name is too long - but only if the name is too long - and then we are no worse off than we were before.
If we aren't passing in the name of the snapshot as an argument then the provider already snapshotted the disk using the old name - if we don't use that name then any attempt at running SSA here will fail. If we do use the old name then it will only fail if it is too long.
The above all being said, now that the provider PR has been merged before this PR, this line of execution is actually moot and can probably be removed. I'll take it out....
Instead of allowing an old style name to be used in the case of a gem mismatch between the Azure provider and this Gem, raise an exception if the required snapshot argument has not been passed into this code.
|
@roliveri the code to allow the old-style snapshot name has been removed and it will not be an exception if the snapshot name is not passed in as an argument. |
|
Checked commits jerryk55/manageiq-smartstate@b7758d8~...ce1dc48 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@jerryk55 I assume this should be |
|
@simaishi yes except that this gem can't be fine/yes since its a gem that gets published. |
|
@jerryk55 Please still mark as |
|
@miq-bot add_label fine/yes |
|
@miq-bot add_label gaprindashvili/yes |
|
@jerryk55 Cannot apply the following label because they are not recognized: gaprindashvili/yes |
|
@simaishi it let me apply fine/yes but not gaprindashvili/yes |
|
Gaprindashvili uses the released gem, so backport of this PR is needed only to Fine branch. The PR that bumps manageiq-smartstate gem version needs to be marked as |
|
Fine backport (to manageiq-gems-pending repo) details: |
|
@miq-bot add_label fine/backported |
|
@miq-bot remove_label fine/yes |
Upgrade to 0.2.3 to pull the fix for ManageIQ/manageiq-smartstate#44 required for BZ fix for https://bugzilla.redhat.com/show_bug.cgi?id=1508577
Since there is an 80 character limit on the length of the snapshot name
constructed for managed disks use the name set by the Azure provider.
This is one of two PRs required to fix https://bugzilla.redhat.com/show_bug.cgi?id=1508577.
The other will be against the Azure provider repo.
This change is required upstream, for Fine, and for Gaprindashvili. Once this gem is
pushed out the Gemfile for the Azure provider will be updated in all three places.
@roliveri @hsong-rh please review and merge. This PR should be merged asap since the
The Azure Provider PR is ManageIQ/manageiq-providers-azure#157.
has already been merged.