Resize disk from reconfigure screen #164

evertmulder · 2017-12-21T12:21:59Z

PR to support resize disk from reconfigure screen.

agrare · 2017-12-21T13:31:35Z

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

+    controller_key, key = vim_obj.getDeviceKeysByBacking(options[:disk_name], hardware)
+    raise "resize_disk_config_spec: no virtual device associated with: #{options[:disk_name]}" unless key
+
+    device = vim_obj.getDeviceByBacking(options[:disk_name])


Ah here's an example of where we can pass in the hardware hash and save an API call. If you call just this method you can get the controller_key and key from the device returned.

Yup I will fix that

agrare · 2017-12-21T15:03:08Z

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

+      vdcs.device = VimHash.new("VirtualDisk") do |dev|
+        dev.key           = key
+        dev.capacityInKB  = options[:disk_size_in_mb].to_i * 1024
+        dev.controllerKey = controller_key


Do you need the rest of these? I think just key is required plus whatever you want to change (ref: https://www.vmware.com/support/developer/vc-sdk/visdk25pubs/ReferenceGuide/vim.vm.device.VirtualDevice.html)

I did some testing.
Removing dev.connectable --> No problem, removing this
Removing dev.backing --> Error: Incompatible device backing specified for device '0'., keeping this
Removing dev.unitNumber --> Error: Invalid configuration for device '0', keeping this
Removing dev.controllerKey --> Error: Device requires a controller., keeping this

Do you know a quicker way testing these option, currently I do a evm:stop / evm:start to test code changes in provider code, is there a quicker way (I tried killing some workers (generic, refesh or broker), they are restarted but I keep hitting the old code ...)

Interesting, so yes you can run in one console bin/rails s to run the UI and bin/rails c in another and run simulate_queue_worker in the rails c terminal. That way you can just ctrl-C the worker Sim and reload! the code when you change something.

Ah cool. I will try this!

evertmulder · 2018-01-12T12:38:19Z

@agrare Is it possible to restart travis? I think it should work now, because ManageIQ/manageiq#16711 is merged now...

agrare · 2018-01-15T13:48:46Z

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

+    add_device_config_spec(vmcs, VirtualDeviceConfigSpecOperation::Edit) do |vdcs|
+      vdcs.device = VimHash.new("VirtualDisk") do |dev|
+        dev.key           = device.key
+        dev.capacityInKB  = options[:disk_size_in_mb].to_i * 1024


Since you're using options[:disk_size_in_mb].to_i * 1024 twice can you just save it as a local var e.g. new_capacity_in_kb?

✔️ I will improve this

agrare · 2018-01-15T13:56:44Z

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

+        dev.controllerKey = device.controllerKey
+        dev.unitNumber    = device.unitNumber
+
+        dev.backing = VimHash.new("VirtualDiskFlatVer2BackingInfo") do |bck|


There's no guarantee that the backing type will be FlatVer2, you have the device so you could either do VimHash.new(device.backing.xsiType), or just use the backing dev.backing = device.backing since you aren't changing anything in the backing.

I assume you tried not passing the backing in at all and it failed?

Yes, not passing the backing results in --> Error: Incompatible device backing specified for device '0'.

agrare · 2018-01-15T13:57:22Z

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

+        dev.backing = VimHash.new("VirtualDiskFlatVer2BackingInfo") do |bck|
+          bck.diskMode        = device.backing.diskMode
+          bck.thinProvisioned = device.backing.thinProvisioned
+          bck.fileName        = device.backing.fileName


These are tricky because they aren't defined for all backing types, not even all types that extend VirtualDeviceFileBackingInfo. E.g. VirtualDiskRawDiskMappingVer1BackingInfo doesn't have thinProvisioned or fileName.

If we have to send a backing I would prefer sending the entire backing from the existing disk so we don't have to worry about these issues.

Yes without the backing the call fails. SO I think something like

dev.backing = device.backing

I will try this tomorrow.

agrare · 2018-01-15T13:58:36Z

@evertmulder this is coming along :) Couple of comments, can you also add some tests to https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/spec/models/manageiq/providers/vmware/infra_manager/vm/reconfigure_spec.rb ?

evertmulder · 2018-01-17T21:18:22Z

I will try to write a test for this tomorrow. This will me my first in ruby so be gentle...

agrare · 2018-01-17T21:20:42Z

Awesome, thanks for sticking with it @evertmulder ! I'm looking forward to this functionality

miq-bot · 2018-01-18T15:44:50Z

Checked commits evertmulder/manageiq-providers-vmware@dc852d2~...ca1c96d with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare · 2018-01-18T16:07:05Z

Looking good, let me test this against a live system later today

agrare

👍 LGTM, tested against a few live VMs and this worked perfectly

agrare · 2018-01-18T17:48:40Z

@evertmulder if you haven't gotten very far with specs I'll follow this up with some tests, it is difficult if you're not used to it

Resize disk from reconfigure screen

This adds spec tests for the new #resize_disks and #resize_disk_config_spec methods added here ManageIQ#164

evertmulder · 2018-01-19T07:44:56Z

@agrare Thanks for writing the tests and the merge!

simaishi · 2018-06-01T14:43:45Z

@agrare can this be gaprindashvili/yes? We need reconfigure disk feature in G-branch (ManageIQ/manageiq#16711 (comment))

agrare · 2018-06-01T14:47:35Z

@simaishi sure, the required vmware_web_service change is already included in the G branch

Resize disk from reconfigure screen (cherry picked from commit 5042944)

simaishi · 2018-06-01T14:55:40Z

Gaprindashvili backport details:

$ git log -1
commit ba3cd6a289bbaca120aba68a9d6b99dff7500cd8
Author: Adam Grare <[email protected]>
Date:   Thu Jan 18 12:49:08 2018 -0500

    Merge pull request #164 from evertmulder/ResizeDisk_ReconfigureScreen
    
    Resize disk from reconfigure screen
    (cherry picked from commit 5042944e5b5e9743b8ab7ad1c24299c13005a3c0)

dayleparker · 2018-07-16T05:19:02Z

Hi @simaishi, is there a PR to bring this functionality to RHV VMs as well, or is this limited to VMware VMs?

agrare · 2018-07-16T14:47:11Z

Hey @dayleparker this is just for VMware, if Ovirt wants to implement this they will need to add it to their repo.

dayleparker · 2018-07-17T03:09:43Z

Thanks for confirming @agrare! 👍

AWS DetachVolume event switchboard setting.

evertmulder mentioned this pull request Dec 21, 2017

Reconfigure VM: Option to resize disk (see linked prs) ManageIQ/manageiq-ui-classic#3127

Closed

agrare self-assigned this Dec 21, 2017

agrare reviewed Dec 21, 2017

View reviewed changes

agrare added the enhancement label Dec 21, 2017

himdel mentioned this pull request Jan 11, 2018

Resize disk reconfigure screen ManageIQ/manageiq-ui-classic#3128

Merged

agrare reviewed Jan 15, 2018

View reviewed changes

Evert Mulder added 3 commits January 18, 2018 16:36

Add resize disk to reconfigure screen

dc852d2

Reduce number of API calls for resize_disk_config_spec

e246dcf

Improvements based on review comments, use current disk backing info

ca1c96d

agrare approved these changes Jan 18, 2018

View reviewed changes

agrare merged commit ca1c96d into ManageIQ:master Jan 18, 2018

agrare added a commit that referenced this pull request Jan 18, 2018

Merge pull request #164 from evertmulder/ResizeDisk_ReconfigureScreen

5042944

Resize disk from reconfigure screen

agrare added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 18, 2018

agrare added a commit to agrare/manageiq-providers-vmware that referenced this pull request Jan 18, 2018

Add spec for the new #resize_disks method

c29740a

This adds spec tests for the new #resize_disks and #resize_disk_config_spec methods added here ManageIQ#164

agrare mentioned this pull request Jan 18, 2018

Add spec for the new #resize_disks method #176

Merged

gmcculloug mentioned this pull request Jun 1, 2018

Resize disk reconfigure screen ManageIQ/manageiq#16711

Merged

agrare added the gaprindashvili/yes label Jun 1, 2018

simaishi pushed a commit that referenced this pull request Jun 1, 2018

Merge pull request #164 from evertmulder/ResizeDisk_ReconfigureScreen

ba3cd6a

Resize disk from reconfigure screen (cherry picked from commit 5042944)

simaishi added blocker gaprindashvili/backported and removed gaprindashvili/yes labels Jun 1, 2018

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

Merge pull request ManageIQ#164 from bronaghs/aws_detachvolume

7e34525

AWS DetachVolume event switchboard setting.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resize disk from reconfigure screen #164

Resize disk from reconfigure screen #164

evertmulder commented Dec 21, 2017

agrare Dec 21, 2017

evertmulder Dec 22, 2017

agrare Dec 21, 2017

evertmulder Dec 22, 2017

agrare Dec 22, 2017

evertmulder Jan 17, 2018 •

edited

Loading

evertmulder commented Jan 12, 2018 •

edited

Loading

agrare Jan 15, 2018

evertmulder Jan 17, 2018

agrare Jan 15, 2018

evertmulder Jan 17, 2018

agrare Jan 17, 2018

agrare Jan 15, 2018

evertmulder Jan 17, 2018

agrare commented Jan 15, 2018

evertmulder commented Jan 17, 2018

agrare commented Jan 17, 2018

miq-bot commented Jan 18, 2018

agrare commented Jan 18, 2018

agrare left a comment

agrare commented Jan 18, 2018

evertmulder commented Jan 19, 2018

simaishi commented Jun 1, 2018

agrare commented Jun 1, 2018

simaishi commented Jun 1, 2018

dayleparker commented Jul 16, 2018

agrare commented Jul 16, 2018

dayleparker commented Jul 17, 2018

Resize disk from reconfigure screen #164

Resize disk from reconfigure screen #164

Conversation

evertmulder commented Dec 21, 2017

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

evertmulder Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

evertmulder commented Jan 12, 2018 • edited Loading

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 Jan 15, 2018

evertmulder commented Jan 17, 2018

agrare commented Jan 17, 2018

miq-bot commented Jan 18, 2018

agrare commented Jan 18, 2018

agrare left a comment

Choose a reason for hiding this comment

agrare commented Jan 18, 2018

evertmulder commented Jan 19, 2018

simaishi commented Jun 1, 2018

agrare commented Jun 1, 2018

simaishi commented Jun 1, 2018

dayleparker commented Jul 16, 2018

agrare commented Jul 16, 2018

dayleparker commented Jul 17, 2018

evertmulder Jan 17, 2018 •

edited

Loading

evertmulder commented Jan 12, 2018 •

edited

Loading