Enable VM reconfigure disks for supported rhevm version #11971

masayag · 2016-10-15T14:03:50Z

The VM reconfigure disks is supported only by v4 of the API
which was introduced in RHEVM 4.0. The functionality will not
be available for an earlier versions in the 'VM reconfigure' dialog.
Also the feature requires the vm to have a store type associated
with it, so it can be used as a target storage for the new disks.

http://bugzilla.redhat.com/1373916

masayag · 2016-10-15T14:09:52Z

@miq-bot add_label providers/rhevm

masayag · 2016-10-15T14:11:09Z

@miq-bot add_label euwe/yes

masayag · 2016-10-15T14:53:07Z

@miq-bot assign @durandom

masayag · 2016-10-15T14:53:37Z

@borod108 please review

miq-bot · 2016-10-19T17:16:20Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

miq-bot · 2016-10-24T19:16:26Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

miq-bot · 2016-10-25T16:16:26Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

borod108 · 2016-10-26T06:34:13Z

@masayag looks good to me.

durandom · 2016-10-26T10:30:33Z

app/controllers/application_controller/ci_processing.rb

@@ -892,6 +892,7 @@ def reconfigure
    @force_no_grid_xml   = true
    @view, @pages = get_view(Vm, :view_suffix => "VmReconfigureRequest", :where_clause => ["vms.id IN (?)", @reconfigure_items])  # Get the records (into a view) and the paginator
    get_reconfig_limits
+    @supports_reconfigure_disks = supports_reconfigure_disks?


Dont know much about the UI, but cant we do this without a instance variable? Why no call the method directly?

durandom · 2016-10-26T10:31:33Z

app/models/manageiq/providers/vmware/infra_manager/vm.rb

@@ -9,6 +9,8 @@ class ManageIQ::Providers::Vmware::InfraManager::Vm < ManageIQ::Providers::Infra
    unsupported_reason_add(:clone, _('Clone operation is not supported')) if blank? || orphaned? || archived?
  end

+  supports :reconfigure_disks


there should be a validate_reconfigure_disks somewhere in the code if this is not a new feature

I haven't find any. Before adding support for reconfigure_disks by rhevm, only vmware supported that feature and the dialog was the responsible to block any other provider from doing so. I didn't find any previous validation of it in code.

durandom · 2016-10-26T10:32:58Z

app/models/manageiq/providers/redhat/infra_manager/vm.rb

+  supports :reconfigure_disks do
+    if storage.blank?
+      unsupported_reason_add(:reconfigure_disks, _('storage is missing'))
+    elsif ext_management_system.blank?


there is supports :control maybe you can use this? Its checking similar stuff

The 'supports :control' cover more extensive cases than required. However, I'll have to share some of this logic with with #10505 (add quick_stats)

Fine, then leave it just like this. I actually prefer having all checks inside this block/

durandom · 2016-10-26T10:34:07Z

app/models/manageiq/providers/redhat/infra_manager/vm.rb

@@ -6,6 +6,16 @@ class ManageIQ::Providers::Redhat::InfraManager::Vm < ManageIQ::Providers::Infra

  supports_not :migrate, :reason => _("Migrate operation is not supported.")

+  supports :reconfigure_disks do


~~can you move that to app/models/vm.rb~~

durandom · 2016-10-26T10:39:44Z

@masayag is reconfigure_disks something that is supported by vmware too?
If so, I dont see how this PR would hook into the same UI as vmware does.

durandom · 2016-10-27T09:24:05Z

@miq-bot add_label ui

@AparnaKarve could you have a look at the UI part?

durandom · 2016-10-27T09:24:36Z

@miq-bot add_label providers/vmware/infra

@agrare could you have a look at vmware part?

miq-bot · 2016-10-27T09:25:10Z

@durandom Cannot apply the following label because they are not recognized: providers/vmware

durandom · 2016-10-27T09:25:26Z

I'm fine with the supports part for now, as we are missing some pieces of the puzzle to properly move those blocks to the base class

masayag · 2016-10-31T07:02:15Z

@durandom reconfigure_disks is supported by both vmware and rhevm. It enables the UI to display the 'Disks' section by the result of 'supports_reconfigure_disks?' which returns 'true' for vmware due to change in app/models/manageiq/providers/vmware/infra_manager/vm.rb

durandom · 2016-10-31T15:31:10Z

spec/controllers/application_controller/ci_processing_spec.rb

+    let(:rec_ids) { [vm.id] }
+    let(:supports_reconfigure_disks) { true }
+    before(:each) do
+      allow_any_instance_of(ManageIQ::Providers::Redhat::InfraManager::Vm)


I think you can also just mock the ems that is attached to vm

allow(vm.ext_management_system).to receive..

The test was updated to reflect the removal of the instance variable from previous patch which stored the value of the tested method.

durandom · 2016-10-31T15:31:52Z

spec/models/manageiq/providers/redhat/infra_manager/vm_spec.rb

+        let(:vm) { FactoryGirl.create(:vm_redhat, :storage => storage) }
+
+        before(:each) do
+          allow_any_instance_of(ManageIQ::Providers::Redhat::InfraManager)


same here, only mock the ems attached to vm

Fixed, looks much better.

blomquisg · 2016-11-02T21:45:47Z

@masayag looks like this needs a rebase now ... assign back to me after you've rebased.

miq-bot · 2016-11-02T21:46:57Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

masayag · 2016-11-03T05:19:43Z

@miq-bot assign @blomquisg

blomquisg · 2016-11-03T14:10:10Z

close/reopen to re-kick tests ... brakeman failure looks like a failure to cache data in the tests

miq-bot · 2016-11-03T18:52:04Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

The VM reconfigure disks is supported only by API v4 of version which was introduced since rhevm 4.0. The functionality will not be available for lower levels in the 'VM reconfigure' dialog. Also the feature requires the vm to have a store type associated with it, so it can be used as a target storage for the new disks. http://bugzilla.redhat.com/1373916

miq-bot · 2016-11-03T19:08:35Z

Checked commit masayag@30aafd4 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
8 files checked, 0 offenses detected
Everything looks good. 🍪

chessbyte · 2016-11-07T20:40:45Z

Euwe BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1391711

chessbyte · 2016-11-07T20:41:50Z

Euwe Backport conflict:

$ git cherry-pick -e -x -m 1    3a72731 
error: could not apply 3a72731... Merge pull request #11971 from masayag/enable_vm_reconfigure_disk_for_supported_rhevm
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

$ git status
On branch euwe
Your branch is up-to-date with 'upstream/euwe'.
You are currently cherry-picking commit 3a72731.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

    modified:   app/controllers/application_controller/ci_processing.rb
    modified:   app/models/manageiq/providers/redhat/infra_manager/vm.rb
    modified:   app/views/vm_common/_reconfigure.html.haml
    modified:   spec/controllers/application_controller/ci_processing_spec.rb
    modified:   spec/models/manageiq/providers/redhat/infra_manager/vm_spec.rb

Unmerged paths:
  (use "git add <file>..." to mark resolution)

    both modified:   app/models/manageiq/providers/redhat/infra_manager/api_integration.rb
    both modified:   app/models/manageiq/providers/vmware/infra_manager/vm.rb
    both modified:   app/models/mixins/supports_feature_mixin.rb

$ git diff

diff --cc app/models/manageiq/providers/redhat/infra_manager/api_integration.rb
index 496b966,18bca2d..0000000
--- a/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb
+++ b/app/models/manageiq/providers/redhat/infra_manager/api_integration.rb
@@@ -211,7 -218,12 +211,16 @@@ module ManageIQ::Providers::Redhat::Inf
      end

      def api4_supported_features
++<<<<<<< HEAD
 +      [:quick_stats, :snapshots]
++=======
+       [
+         :migrate,
+         :quick_stats,
+         :reconfigure_disks,
+         :snapshots
+       ]
++>>>>>>> 3a72731... Merge pull request #11971 from masayag/enable_vm_reconfigure_disk_for_supported_rhevm
      end

      def api_features
diff --cc app/models/manageiq/providers/vmware/infra_manager/vm.rb
index 3a7a0b1,d3959d8..0000000
--- a/app/models/manageiq/providers/vmware/infra_manager/vm.rb
+++ b/app/models/manageiq/providers/vmware/infra_manager/vm.rb
@@@ -5,6 -5,12 +5,15 @@@ class ManageIQ::Providers::Vmware::Infr
    include_concern 'RemoteConsole'
    include_concern 'Reconfigure'

++<<<<<<< HEAD
++=======
+   supports :clone do
+     unsupported_reason_add(:clone, _('Clone operation is not supported')) if blank? || orphaned? || archived?
+   end
+ 
+   supports :reconfigure_disks
+ 
++>>>>>>> 3a72731... Merge pull request #11971 from masayag/enable_vm_reconfigure_disk_for_supported_rhevm
    def add_miq_alarm
      raise "VM has no EMS, unable to add alarm" unless ext_management_system
      ext_management_system.vm_add_miq_alarm(self)
diff --cc app/models/mixins/supports_feature_mixin.rb
index 64c8bf2,6fae596..0000000
--- a/app/models/mixins/supports_feature_mixin.rb
+++ b/app/models/mixins/supports_feature_mixin.rb
@@@ -86,6 -89,8 +86,11 @@@ module SupportsFeatureMixi
      :quick_stats                => 'Quick Stats',
      :reboot_guest               => 'Reboot Guest Operation',
      :reconfigure                => 'Reconfiguration',
++<<<<<<< HEAD
++=======
+     :reconfigure_disks          => 'Reconfigure Virtual Machines Disks',
+     :refresh_network_interfaces => 'Refresh Network Interfaces for a Host',
++>>>>>>> 3a72731... Merge pull request #11971 from masayag/enable_vm_reconfigure_disk_for_supported_rhevm
      :refresh_new_target         => 'Refresh non-existing record',
      :regions                    => 'Regions of a Provider',
      :remove_host                => 'Remove Host',

…_supported_rhevm Enable VM reconfigure disks for supported rhevm version (cherry picked from commit 3a72731) https://bugzilla.redhat.com/show_bug.cgi?id=1391711

chessbyte · 2016-11-07T20:46:57Z

Resolved conflicts.
Euwe Backport details:

$ git log -1
commit 52942b490b9b72979e3ed43735fa8a17385598d2
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Thu Nov 3 15:46:14 2016 -0400

    Merge pull request #11971 from masayag/enable_vm_reconfigure_disk_for_supported_rhevm

    Enable VM reconfigure disks for supported rhevm version
    (cherry picked from commit 3a72731ae15df71f2d066d1638fc7ffd969d3b3b)

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

miq-bot added the providers/rhevm label Oct 15, 2016

miq-bot added the euwe/yes label Oct 15, 2016

chessbyte assigned oourfali Oct 15, 2016

miq-bot assigned durandom and unassigned oourfali Oct 15, 2016

masayag force-pushed the enable_vm_reconfigure_disk_for_supported_rhevm branch 2 times, most recently from 16fd8ee to 13ffb7f Compare October 20, 2016 10:59

masayag force-pushed the enable_vm_reconfigure_disk_for_supported_rhevm branch 2 times, most recently from 20f08f4 to 58ab4b1 Compare October 25, 2016 14:44

masayag force-pushed the enable_vm_reconfigure_disk_for_supported_rhevm branch from 58ab4b1 to 513c749 Compare October 25, 2016 18:06

durandom suggested changes Oct 26, 2016

View reviewed changes

miq-bot added the ui label Oct 27, 2016

miq-bot added the providers/vmware/infra label Oct 27, 2016

masayag force-pushed the enable_vm_reconfigure_disk_for_supported_rhevm branch from 513c749 to 281258a Compare October 31, 2016 07:02

durandom reviewed Oct 31, 2016

View reviewed changes

miq-bot added the blocker label Nov 2, 2016

blomquisg added the bug label Nov 2, 2016

blomquisg assigned masayag and unassigned blomquisg Nov 2, 2016

masayag force-pushed the enable_vm_reconfigure_disk_for_supported_rhevm branch 2 times, most recently from 531be84 to 5c6f0d6 Compare November 3, 2016 05:16

miq-bot assigned blomquisg and unassigned masayag Nov 3, 2016

masayag force-pushed the enable_vm_reconfigure_disk_for_supported_rhevm branch from 5c6f0d6 to 7e4b318 Compare November 3, 2016 13:05

masayag mentioned this pull request Nov 3, 2016

Support delete backing in reconfigure vm disks #12388

Merged

blomquisg closed this Nov 3, 2016

blomquisg reopened this Nov 3, 2016

masayag force-pushed the enable_vm_reconfigure_disk_for_supported_rhevm branch from 7e4b318 to 30aafd4 Compare November 3, 2016 19:03

blomquisg merged commit 3a72731 into ManageIQ:master Nov 3, 2016

blomquisg added this to the Sprint 49 Ending Nov 14, 2016 milestone Nov 3, 2016

chessbyte added euwe/backported and removed euwe/yes labels Nov 7, 2016

masayag deleted the enable_vm_reconfigure_disk_for_supported_rhevm branch June 27, 2018 20:30

Fryguy added the providers/infrastructure label Jan 24, 2020

		@@ -6,6 +6,16 @@ class ManageIQ::Providers::Redhat::InfraManager::Vm < ManageIQ::Providers::Infra

		supports_not :migrate, :reason => _("Migrate operation is not supported.")

		supports :reconfigure_disks do

Enable VM reconfigure disks for supported rhevm version #11971

Enable VM reconfigure disks for supported rhevm version #11971

Conversation

masayag commented Oct 15, 2016 • edited by miq-bot Loading

masayag commented Oct 15, 2016

masayag commented Oct 15, 2016

masayag commented Oct 15, 2016

masayag commented Oct 15, 2016

miq-bot commented Oct 19, 2016

miq-bot commented Oct 24, 2016

miq-bot commented Oct 25, 2016

borod108 commented Oct 26, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

durandom commented Oct 26, 2016

durandom commented Oct 27, 2016

durandom commented Oct 27, 2016 • edited Loading

miq-bot commented Oct 27, 2016

durandom commented Oct 27, 2016

masayag commented Oct 31, 2016

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

blomquisg commented Nov 2, 2016 • edited Loading

miq-bot commented Nov 2, 2016

masayag commented Nov 3, 2016

blomquisg commented Nov 3, 2016

miq-bot commented Nov 3, 2016

miq-bot commented Nov 3, 2016

chessbyte commented Nov 7, 2016

chessbyte commented Nov 7, 2016

chessbyte commented Nov 7, 2016

masayag commented Oct 15, 2016 •

edited by miq-bot

Loading

durandom Oct 26, 2016 •

edited

Loading

durandom commented Oct 27, 2016 •

edited

Loading

blomquisg commented Nov 2, 2016 •

edited

Loading