Added support for VM delete #184

VojkoR · 2018-02-19T10:57:55Z

Added support for VM delete

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

/cc @miha-plesko

VojkoR · 2018-02-19T12:22:35Z

Use "Power -> Delete" (shown in image) to delete VM.

miha-plesko

Thanks @VojkoR , I only have one tiny comment, otherwise LGTM 👍

miha-plesko · 2018-02-19T12:30:11Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+  def raw_destroy
+    raise "VM has no #{ui_lookup(:table => "ext_management_systems")}, unable to destroy VM" unless ext_management_system
+    with_provider_object(&:undeploy)
+


Nitpicking, but could you please remove this empty line?

Empty line removed

miha-plesko · 2018-02-19T12:51:06Z

@miq-bot assign @agrare
@miq-bot add_label enhancement,gaprindashvili/yes

agrare · 2018-02-20T15:01:46Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+  def raw_destroy
+    raise "VM has no #{ui_lookup(:table => "ext_management_systems")}, unable to destroy VM" unless ext_management_system
+    with_provider_object(&:undeploy)
+    ext_management_system.with_provider_connection do |service|


Since with_provider_object calls with_provider_connection can you do the :undeploy in this block so you only have to connect once?

@VojkoR I believe what @agrare is suggesting is not to use the with_provider_object to undeploy but rather use it like this

ext_management_system.with_provider_connection do |service| service.post_undeploy_vapp(ems_ref) service.delete_vapp(ems_ref) end

https://github.com/xlab-si/fog-vcloud-director/blob/master/lib/fog/vcloud_director/requests/compute/post_undeploy_vapp.rb#L36

agrare · 2018-02-20T15:05:28Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+    ext_management_system.with_provider_connection do |service|
+      service.delete_vapp(ems_ref)
+    end
+    update_attributes!(:raw_power_state => "DELETED")


Is DELETED a valid power state? How about "off" since it is a valid vcloud_director state and also recognized by the vcloud provider

VojkoR · 2018-02-22T08:57:18Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+    raise "VM has no #{ui_lookup(:table => "ext_management_systems")}, unable to destroy VM" unless ext_management_system
+    ext_management_system.with_provider_connection do |service|
+      begin
+        response = service.post_undeploy_vapp(ems_ref, :UndeployPowerAction => 'shutdown')


Before delete action is called we need to stop VM. In case VM is already stopped action will rise an error. To successfully delete VM we are catching this error and just log the message what happened.

VojkoR · 2018-02-22T09:02:19Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+      ensure
+        response = service.delete_vapp(ems_ref)
+        service.process_task(response.body)
+        update_attributes!(:raw_power_state => "terminated")


I think correct power state for deleted VM should be "terminated". Maybe I should set power state to "unknown" instead of "terminated"?

VojkoR · 2018-02-22T09:06:56Z

Hi @agrare I changed functionality as requested. I added some extra code, to correctly implement DELETE action. I wrote some comments for the code that was added and why it was added.

agrare · 2018-02-22T15:28:07Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+      begin
+        response = service.post_undeploy_vapp(ems_ref, :UndeployPowerAction => 'shutdown')
+        service.process_task(response.body)
+      rescue => err


Can you rescue the exact exception that is raised if the VM is already stopped?

agrare · 2018-02-22T15:30:14Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+      rescue => err
+        $vcloud_log.debug("vm=[#{name}], error: VM not running! Ignoring error: #{err}")
+      ensure
+        response = service.delete_vapp(ems_ref)


Don't like doing this in the ensure block assuming that you're rescuing a "vm already powered off" exception because it could be something completely different and we'll still try to do this.
I'd rather rescue the exact exception then do this outside the begin/rescue block like:

begin service.post_undeploy_vapp(ems_ref, :UndeployPowerAction => 'shutdown') rescue Vcloud::VmAlreadyPoweredOff /* not sure what this exception actually is, just for demonstration */ # this is expected if the vapp is already off end service.process_task(response.body) update_attributes!(:raw_power_state => "terminated")

This commit now adds support for removal of a stopped VM from vApp. Besides removal operation it overrides the `supports :terminate` feature because the VM must be powered off to allow to be deleted from VCD. Signed-off-by: Vojko Rozic <[email protected]> Signed-off-by: Gregor Berginc <[email protected]>

miq-bot · 2018-02-23T10:14:58Z

Checked commit https://github.com/VojkoR/manageiq-providers-vmware/commit/042780967d87c0bc9524a57d71cf174099398791 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

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

❗ - Line 86, Col 3 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

gberginc

@agrare We have simplified the code to handle removal of VM in a way VCD handles it. I've added two comments in the code to explain the rationale.

I hope this will be ok.

gberginc · 2018-02-23T10:13:52Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+
+  included do
+    supports :terminate do
+      unsupported_reason_add(:terminate, "The VM is powered on") unless current_state == "off"


@agrare we have modified the code to resemble the behaviour of VCD. Now, it is not possible to delete a VM that is running (powered on). To this end we had to redefine the supports_terminate? by looking at the current_state of the VM.

Ah I like that, we actually have a helper method for this that you can use, https://github.com/ManageIQ/manageiq/blob/master/app/models/vm/operations/power.rb#L22-L24

Thanks @agrare. I’ll make another PR to use this.

gberginc · 2018-02-23T10:15:03Z

app/models/manageiq/providers/vmware/cloud_manager/vm/operations.rb

+  def raw_destroy
+    raise "VM has no #{ui_lookup(:table => "ext_management_systems")}, unable to destroy VM" unless ext_management_system
+    ext_management_system.with_provider_connection do |service|
+      response = service.delete_vapp(ems_ref)


Instead of checking for exceptions we are simply passing to the underlying provider connection. Looking at OpenStack and Amazon, this is exactly how these two are handled. In case an exception occurs it will be propagated.

👍 perfect

agrare · 2018-02-23T14:34:42Z

This looks good, can you send a followup to use that vm_powered_on? helper? I don't want to hold this up for that though.

miha-plesko · 2018-03-02T07:20:38Z

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

@simaishi ^

miha-plesko · 2018-03-02T07:41:01Z

@agrare apologies for delay, but here comes the followup PR: #204

Added support for VM delete (cherry picked from commit 1dfa303) https://bugzilla.redhat.com/show_bug.cgi?id=1552683

simaishi · 2018-03-07T14:50:16Z

Gaprindashvili backport details:

$ git log -1
commit fa49cf6bbc6e66d5373f03ed62bc1e37f6127ec9
Author: Adam Grare <[email protected]>
Date:   Fri Feb 23 09:34:48 2018 -0500

    Merge pull request #184 from VojkoR/destroy_vm_for_vmware_provider
    
    Added support for VM delete
    (cherry picked from commit 1dfa303668712cc1ee028d640cfbbaed734ca5f3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552683

…h-based-on-events Do targeted refresh based on Openstack events.

VojkoR force-pushed the destroy_vm_for_vmware_provider branch from 087e465 to 7252c43 Compare February 19, 2018 12:16

miha-plesko approved these changes Feb 19, 2018

View reviewed changes

miq-bot assigned agrare Feb 19, 2018

miq-bot added enhancement gaprindashvili/yes labels Feb 19, 2018

VojkoR force-pushed the destroy_vm_for_vmware_provider branch from 7252c43 to 8b9b085 Compare February 19, 2018 12:53

agrare requested changes Feb 20, 2018

View reviewed changes

VojkoR force-pushed the destroy_vm_for_vmware_provider branch from 8b9b085 to 6daddc7 Compare February 22, 2018 08:15

VojkoR commented Feb 22, 2018

View reviewed changes

agrare reviewed Feb 22, 2018

View reviewed changes

gberginc force-pushed the destroy_vm_for_vmware_provider branch from 6daddc7 to 0427809 Compare February 23, 2018 10:12

gberginc reviewed Feb 23, 2018

View reviewed changes

agrare approved these changes Feb 23, 2018

View reviewed changes

agrare merged commit 1dfa303 into ManageIQ:master Feb 23, 2018

agrare added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 23, 2018

agrare added the bugzilla needed label Feb 23, 2018

agrare mentioned this pull request Feb 23, 2018

Fix failing test now that vCloud supports destroy ManageIQ/manageiq#17041

Merged

miha-plesko mentioned this pull request Mar 2, 2018

Use utility function vm_powered_on? instead manual comparison #204

Merged

simaishi removed the bugzilla needed label Mar 7, 2018

simaishi pushed a commit that referenced this pull request Mar 7, 2018

Merge pull request #184 from VojkoR/destroy_vm_for_vmware_provider

fa49cf6

Added support for VM delete (cherry picked from commit 1dfa303) https://bugzilla.redhat.com/show_bug.cgi?id=1552683

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Mar 7, 2018

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019

Merge pull request ManageIQ#184 from mansam/openstack-targeted-refres…

2c1d828

…h-based-on-events Do targeted refresh based on Openstack events.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for VM delete #184

Added support for VM delete #184

VojkoR commented Feb 19, 2018 •

edited by simaishi

Loading

VojkoR commented Feb 19, 2018

miha-plesko left a comment

miha-plesko Feb 19, 2018

VojkoR Feb 19, 2018

miha-plesko commented Feb 19, 2018

agrare Feb 20, 2018

gberginc Feb 21, 2018

agrare Feb 20, 2018

VojkoR Feb 22, 2018

VojkoR Feb 22, 2018

VojkoR commented Feb 22, 2018

agrare Feb 22, 2018

agrare Feb 22, 2018

miq-bot commented Feb 23, 2018

gberginc left a comment

gberginc Feb 23, 2018

agrare Feb 23, 2018

gberginc Feb 23, 2018

gberginc Feb 23, 2018

agrare Feb 23, 2018

agrare commented Feb 23, 2018

miha-plesko commented Mar 2, 2018

miha-plesko commented Mar 2, 2018

simaishi commented Mar 7, 2018

Added support for VM delete #184

Added support for VM delete #184

Conversation

VojkoR commented Feb 19, 2018 • edited by simaishi Loading

VojkoR commented Feb 19, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Feb 19, 2018

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

VojkoR commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 23, 2018

gberginc left a 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

agrare commented Feb 23, 2018

miha-plesko commented Mar 2, 2018

miha-plesko commented Mar 2, 2018

simaishi commented Mar 7, 2018

VojkoR commented Feb 19, 2018 •

edited by simaishi

Loading