Render snapshot for VM #190

sasoc · 2018-02-23T14:25:43Z

Refresh parser now renders snapshot for VM.
VCloud only returns creation time, size and
poweredOn, which tells if if the virtual machine was
powered on when the snapshot was created. So uid and
ems_ref are created from VM's id and creation time
of snapshot.

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

sasoc · 2018-02-23T14:39:10Z

cc @miha-plesko

gberginc · 2018-02-23T16:31:02Z

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

@@ -134,13 +134,30 @@ def parse_vm(vm)
 }
 end

+ snapshot_resp = vm.service.get_snapshot_section(vm.id).data[:body][:Snapshot]


Make a helper function to retrieve the snapshot

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

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

@@ -134,13 +134,30 @@ def parse_vm(vm)
 }
 end

+ snapshot_resp = vm.service.get_snapshot_section(vm.id).data[:body][:Snapshot]


Use @connection instead of vm.service.

gberginc · 2018-02-23T16:35:41Z

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

+ snapshot = [
+ {
+ :name => "#{vm.name}(snapshot)",
+ :uid => "#{vm.id}-#{snapshot_resp[:created]}",


Have you checked if the href attribute could be used (or part thereof) for the uid and ems_ref?

Yes I have checked, href only contains id of VM.

Maybe use a different seperator than '-' since that is part of the uuid already? That way you can split the uid apart with something like snapshot.uid.split("__")

gberginc · 2018-02-23T19:03:39Z

spec/models/manageiq/providers/vmware/cloud_manager/refresher_spec.rb

@@ -58,6 +58,18 @@
 end
 end

+ it "will perform refresh for snapshot" do
+ 2.times do # Run twice to verify that a second run with existing data does not change anything


Are you separating this to have a different VCR?

Yes, we talked about it with @miha-plesko and decided to create a new VCR.

agrare · 2018-02-26T13:49:03Z

@sasoc looks like this needs a new VCR recording cc @gberginc

agrare · 2018-02-26T13:49:51Z

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

@@ -217,4 +218,24 @@ def parse_vapp_template(vapp_template)

 return uid, new_result
 end
+
+ def parse_snapshot(vm)
+ snapshot_resp = @connection.get_snapshot_section(vm.id).data[:body][:Snapshot]


Use data.fetch_path(:body, :Snapshot) here so we fail the refresh with an exception if body is somehow nil

agrare · 2018-02-26T13:51:28Z

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

+ snapshot = [
+ {
+ :name => "#{vm.name}(snapshot)",
+ :uid => "#{vm.id}-#{snapshot_resp[:created]}",


Maybe use a different seperator than '-' since that is part of the uuid already? That way you can split the uid apart with something like snapshot.uid.split("__")

miha-plesko

@sasoc some minor comments. To make the old VCR spec green again, please mock parse_snapshot function there.

miha-plesko · 2018-02-27T12:51:00Z

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

@@ -141,6 +141,7 @@ def parse_vm(vm)
 :name => name,
 :vendor => "vmware",
 :raw_power_state => status,
+ :snapshots => parse_snapshot(vm),


:snapshots => [parse_snapshot(vm)].compact,

Nitpicking, but can you please use such format ^, it's annoying if we say "parse (single) snapshot" and then return a list :)

miha-plesko · 2018-02-27T12:56:03Z

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

+ ]
+ end
+
+ snapshot


We could simplify the whole function:

resp = @connection.get_snapshot_section(vm.id).data if (snapshot_resp = resp.fetch_path(:body, :Snapshot)) { :name => "#{vm.name}(snapshot)", :uid => "#{vm.id}-#{snapshot_resp[:created]}", ... } end

miha-plesko · 2018-02-27T13:16:51Z

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

+ end
+
+ snapshot
+ end


def parse_snapshot(vm) resp = @connection.get_snapshot_section(vm.id).data if (snapshot_resp = resp.fetch_path(:body, :Snapshot)) { :name => "#{vm.name} (snapshot)", :uid => "#{vm.id}_#{snapshot_resp[:created]}", :ems_ref => "#{vm.id}_#{snapshot_resp[:created]}", :parent_id => vm.id, :parent_uid => vm.id, :create_time => snapshot_resp[:created], :total_size => snapshot_resp[:size] } else nil end end

miha-plesko · 2018-02-27T13:20:39Z

spec/models/manageiq/providers/vmware/cloud_manager/refresher_spec.rb

@@ -40,6 +40,8 @@
 end

 it "will perform a full refresh" do
+ allow_any_instance_of(ManageIQ::Providers::Vmware::CloudManager::RefreshParser).to receive(:parse_snapshot).and_return(nil)


# Mock snapshot collection since VCR cassete does not contain those request yet. # TODO: record a new VCR cassette when provider development is done. allow_any_instance_of(described_class).to receive(:parse_snapshot).and_return(nil)

With this commit we support VM snapshot inventoring. Since vCloud is quite modes with information it reveals about VM snapshots (only creation time, size and poweredOn, which tells if if the virtual machine was powered on when the snapshot was created) we need to compose `uid` and `ems_ref` dynamically. Signed-off-by: Miha Pleško <miha.plesko@xlab.si> Signed-off-by: Sašo Cvitkovič <saso.cvitkovic@xlab.si>

miq-bot · 2018-02-27T15:31:16Z

Checked commit sasoc@a8eb345 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 1 offense detected

**

💣 💥 🔥 🚒 - Linter/Yaml - missing config files

miha-plesko · 2018-02-27T15:51:40Z

@miq-bot add_label enhancement,gaprindashvili/yes

agrare

👍 looks good, nice tests

Render snapshot for VM

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

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

@simaishi ^

Render snapshot for VM (cherry picked from commit 8b0d0e8) https://bugzilla.redhat.com/show_bug.cgi?id=1552686

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

Gaprindashvili backport details:

$ git log -1
commit 64c098ba52fc6f45935af2a017b9d24b2bec0513
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Feb 27 13:26:57 2018 -0500

    Merge pull request #190 from sasoc/render-snapshot-for-vm
    
    Render snapshot for VM
    (cherry picked from commit 8b0d0e8a9e6e9dbe4702b426f961745fc44ba7b2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552686

Sprint 70 update to changelog

sasoc force-pushed the render-snapshot-for-vm branch from b1d6c29 to b4c74c4 Compare February 23, 2018 17:37

gberginc suggested changes Feb 23, 2018

View reviewed changes

sasoc force-pushed the render-snapshot-for-vm branch from b4c74c4 to 2ac1225 Compare February 26, 2018 09:21

agrare self-assigned this Feb 26, 2018

agrare requested changes Feb 26, 2018

View reviewed changes

miha-plesko suggested changes Feb 27, 2018

View reviewed changes

sasoc force-pushed the render-snapshot-for-vm branch 2 times, most recently from 895e331 to c975370 Compare February 27, 2018 13:08

miha-plesko reviewed Feb 27, 2018

View reviewed changes

sasoc force-pushed the render-snapshot-for-vm branch 2 times, most recently from 87fe4cd to 8d7f84b Compare February 27, 2018 14:09

miha-plesko force-pushed the render-snapshot-for-vm branch 2 times, most recently from 9e84bcd to 678825c Compare February 27, 2018 14:43

miha-plesko force-pushed the render-snapshot-for-vm branch from 678825c to a8eb345 Compare February 27, 2018 15:28

miha-plesko approved these changes Feb 27, 2018

View reviewed changes

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

agrare approved these changes Feb 27, 2018

View reviewed changes

agrare merged commit a8eb345 into ManageIQ:master Feb 27, 2018

agrare added a commit that referenced this pull request Feb 27, 2018

Merge pull request #190 from sasoc/render-snapshot-for-vm

8b0d0e8

Render snapshot for VM

agrare modified the milestones: Sprint 80 Ending Feb 26, 2018, Sprint 81 Ending Mar 12, 2018 Feb 27, 2018

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

Merge pull request #190 from sasoc/render-snapshot-for-vm

64c098b

Render snapshot for VM (cherry picked from commit 8b0d0e8) 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#190 from JPrause/changelog70

3139fd3

Sprint 70 update to changelog

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render snapshot for VM #190

Render snapshot for VM #190

sasoc commented Feb 23, 2018 •

edited by simaishi

Loading

sasoc commented Feb 23, 2018

gberginc Feb 23, 2018

gberginc Feb 23, 2018

gberginc Feb 23, 2018

sasoc Feb 26, 2018

agrare Feb 26, 2018

gberginc Feb 23, 2018

sasoc Feb 26, 2018

agrare commented Feb 26, 2018

agrare Feb 26, 2018

agrare Feb 26, 2018

miha-plesko left a comment

miha-plesko Feb 27, 2018

miha-plesko Feb 27, 2018

miha-plesko Feb 27, 2018

miha-plesko Feb 27, 2018

miq-bot commented Feb 27, 2018

miha-plesko commented Feb 27, 2018

agrare left a comment

miha-plesko commented Mar 2, 2018

simaishi commented Mar 7, 2018

Render snapshot for VM #190

Render snapshot for VM #190

Conversation

sasoc commented Feb 23, 2018 • edited by simaishi Loading

sasoc 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

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 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

miq-bot commented Feb 27, 2018

miha-plesko commented Feb 27, 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