Support create snapshot for VM #189

sasoc · 2018-02-23T13:41:56Z

Add vm_create_snapshot method that calls vCloud API
to create snapshot for VM.

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

sasoc · 2018-02-23T13:50:03Z

sasoc · 2018-02-23T13:51:07Z

sasoc · 2018-02-23T13:51:12Z

cc @miha-plesko

miha-plesko

Thanks @sasoc , I have some comments, please take a look.

miha-plesko · 2018-02-23T14:01:46Z

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

+  def vm_create_snapshot(vm, options = {})
+    options[:quiesce] = 'false'
+    with_provider_connection do |service|
+      service.post_create_snapshot(vm.uid_ems, options)


You can simply use ems_ref instead of vm.uid_ems, because we're on Cloud Provider anyway.

Also, this seems to be asynchronous task but we usually wait for tasks to complete. So could you please go with:

response = service.post_create_snapshot(ems_ref, options) service.process_task(response.body)

(see here)

miha-plesko · 2018-02-23T14:06:24Z

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

@@ -84,4 +84,11 @@ def vm_restart(vm, _options = {})
  def self.display_name(number = 1)
    n_('Cloud Provider (VMware vCloud)', 'Cloud Providers (VMware vCloud)', number)
  end
+
+  def vm_create_snapshot(vm, options = {})
+    options[:quiesce] = 'false'


I would be safer if you do it like it's done on the infra manager: https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager.rb#L334-L340

So basically you set all default options and then only override those that were specifically passed. Also, I'm not sure why would you pass 'false' (string) and not false (boolean), is there a reason?

I agree with @miha-plesko. Set defaults for memory and quiesce (both false) and then merge with given options to allow users to override them.

miha-plesko · 2018-02-23T14:09:55Z

spec/models/manageiq/providers/vmware/cloud_manager_spec.rb

+      let(:connection) { double("connection", :post_create_snapshot => "post_create_snapshot") }
+
+      it 'creates a snapshot' do
+        expect(@ems).to receive(:with_provider_connection).and_yield(connection)


Please move this into before do

miha-plesko · 2018-02-23T14:12:06Z

spec/models/manageiq/providers/vmware/cloud_manager_spec.rb

+    let(:vm) { FactoryGirl.create(:vm_vmware, :ext_management_system => @ems) }
+
+    context ".vm_create_snapshot" do
+      let(:snapshot_options) { {:name => 'name', :memory => false, :quiesce => false} }


Why are you passing :quiesce here? I think it's never used then, because it's hard-coded to "false"

miha-plesko · 2018-02-23T14:13:55Z

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

gberginc · 2018-02-23T16:09:32Z

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

@@ -84,4 +84,11 @@ def vm_restart(vm, _options = {})
  def self.display_name(number = 1)
    n_('Cloud Provider (VMware vCloud)', 'Cloud Providers (VMware vCloud)', number)
  end
+
+  def vm_create_snapshot(vm, options = {})
+    options[:quiesce] = 'false'


I agree with @miha-plesko. Set defaults for memory and quiesce (both false) and then merge with given options to allow users to override them.

gberginc · 2018-02-23T16:18:40Z

spec/models/manageiq/providers/vmware/cloud_manager_spec.rb

+      it 'creates a snapshot' do
+        expect(@ems).to receive(:with_provider_connection).and_yield(connection)
+
+        expect(connection).to receive(:post_create_snapshot)


When you add the process_task as suggested by @miha-plesko check the example here on how to mock the response.

gberginc · 2018-02-23T16:26:49Z

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

@@ -22,4 +22,6 @@ def self.calculate_power_state(raw_power_state)
  def self.display_name(number = 1)
    n_('Instance (VMware vCloud)', 'Instances (VMware vCloud)', number)
  end
+
+  supports :snapshots


Move this to line 3 (add empty lines) as otherwise it is really hard to spot that the VM supports this.

Add `vm_create_snapshot` method that calls vCloud API to create snapshot for VM.

miq-bot · 2018-02-26T10:39:30Z

Checked commit sasoc@54eb5b0 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

agrare

👍 looks great thanks @sasoc

miha-plesko · 2018-03-02T08:49:03Z

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

@simaishi ^

Support create snapshot for VM (cherry picked from commit 04ff0cd) https://bugzilla.redhat.com/show_bug.cgi?id=1552686

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

Gaprindashvili backport details:

$ git log -1
commit 1153155d392bec7ff863e4951e1d1b93944c6e5b
Author: Adam Grare <[email protected]>
Date:   Mon Feb 26 08:48:04 2018 -0500

    Merge pull request #189 from sasoc/support-create-snapshot-for-vm
    
    Support create snapshot for VM
    (cherry picked from commit 04ff0cd0553d028a92716a902939ec01531e7689)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552686

…ement_typo Fixed typo in Service Retirement method.

miha-plesko suggested changes Feb 23, 2018

View reviewed changes

miq-bot assigned agrare Feb 23, 2018

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

gberginc suggested changes Feb 23, 2018

View reviewed changes

sasoc force-pushed the support-create-snapshot-for-vm branch from 8c9df02 to 1482014 Compare February 26, 2018 08:36

Support create snapshot for VM

54eb5b0

Add `vm_create_snapshot` method that calls vCloud API to create snapshot for VM.

sasoc force-pushed the support-create-snapshot-for-vm branch from 1482014 to 54eb5b0 Compare February 26, 2018 10:39

agrare approved these changes Feb 26, 2018

View reviewed changes

agrare merged commit 04ff0cd into ManageIQ:master Feb 26, 2018

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

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

Merge pull request #189 from sasoc/support-create-snapshot-for-vm

1153155

Support create snapshot for VM (cherry picked from commit 04ff0cd) https://bugzilla.redhat.com/show_bug.cgi?id=1552686

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#189 from billfitzgerald0120/service_retir…

5c1d10f

…ement_typo Fixed typo in Service Retirement method.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support create snapshot for VM #189

Support create snapshot for VM #189

sasoc commented Feb 23, 2018 •

edited by simaishi

Loading

sasoc commented Feb 23, 2018

sasoc commented Feb 23, 2018

sasoc commented Feb 23, 2018

miha-plesko left a comment

miha-plesko Feb 23, 2018

miha-plesko Feb 23, 2018

miha-plesko Feb 23, 2018

gberginc Feb 23, 2018

miha-plesko Feb 23, 2018

miha-plesko Feb 23, 2018

miha-plesko commented Feb 23, 2018

gberginc Feb 23, 2018

gberginc Feb 23, 2018

gberginc Feb 23, 2018

miq-bot commented Feb 26, 2018

agrare left a comment

miha-plesko commented Mar 2, 2018

simaishi commented Mar 7, 2018

Support create snapshot for VM #189

Support create snapshot for VM #189

Conversation

sasoc commented Feb 23, 2018 • edited by simaishi Loading

sasoc commented Feb 23, 2018

sasoc commented Feb 23, 2018

sasoc commented Feb 23, 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

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

miha-plesko commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 26, 2018

agrare left a comment

Choose a reason for hiding this comment

miha-plesko commented Mar 2, 2018

simaishi commented Mar 7, 2018

sasoc commented Feb 23, 2018 •

edited by simaishi

Loading