add a customizable disk_size field to vm post-provision #839
Conversation
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
from
54a80f4
to
750effb
Compare
|
any chance we could overlook these "similar code" issues? the issues here too were "inherited" from the original code |
app/models/manageiq/providers/vmware/infra_manager/provision/configuration.rb
Outdated
Show resolved
Hide resolved
| @@ -107,8 +107,7 @@ def get_next_device_idx | |||
| @new_device_idx -= 1 | |||
| end | |||
|
|
|||
| def set_cpu_and_memory_allocation(vm) | |||
| config_spec = VimHash.new("VirtualMachineConfigSpec") do |vmcs| | |||
| def set_cpu_and_memory_allocation(vmcs) | |||
| vmcs.cpuAllocation = VimHash.new("ResourceAllocationInfo") do |rai| | |||
There was a problem hiding this comment.
It looks like the indentation is off here now that we dropped the config_spec = VimHash.new("VirtualMachineConfigSpec") do |vmcs| above
| # Do we need to perform a post-clone hardware reconfigure on the new VM? | ||
| [:cpu_limit, :memory_limit, :cpu_reserve, :memory_reserve].any? do |k| | ||
| return false unless options.key?(k) | ||
| destination.send(k) != options[k] | ||
| end | ||
| end | ||
|
|
||
| def reconfigure_disk_on_destination?(destination) |
There was a problem hiding this comment.
I don't think you need to pass destination in since it is an association on this class (and it wasn't passed in for the old reconfigure_hardware_on_destination?)
There was a problem hiding this comment.
I think we should return false unless options.key?(:disk_size) otherwise you'll be comparing nil to default_size if it isn't passed
|
@OrGur1987 overall looking very good, and don't worry about the codeclimate/rubocop warnings I'll let you know if there are issues that need to be fixed. |
|
I gave this a quick test on a live VC and confirmed that this will reconfigure the resulting VM to resize the disk prior to powering the VM on. @OrGur1987 I'm thinking we'll want to add the |
| # Do we need to perform a post-clone hardware reconfigure on the new VM? | ||
| [:cpu_limit, :memory_limit, :cpu_reserve, :memory_reserve].any? do |k| | ||
| return false unless options.key?(k) | ||
| destination.send(k) != options[k] | ||
| end | ||
| end | ||
|
|
||
| def reconfigure_disk_on_destination?(destination) | ||
| default_size = destination.hardware.disks.find_by(:device_type => "disk").size / (1024**3) |
There was a problem hiding this comment.
We're going to need to decide how we want to handle multiple disks.
I could see by default this only works with the boot disk, but we could allow passing additional disk sizes by specifying the device_name/location
There was a problem hiding this comment.
I gave this a quick test on a live VC and confirmed that this will reconfigure the resulting VM to resize the disk prior to powering the VM on.
@OrGur1987 I'm thinking we'll want to add the
:disk_sizeoption to the othercontent/miq_dialogsand in the future we can have it as an option on the normal provision dialog as well.
I added it to the rest of the yamls, but not sure how to check that they work.
do these yamls have their own hamls that must be adjusted, or do the changes I already made in ui-classic cover this?
We're going to need to decide how we want to handle multiple disks.
I could see by default this only works with the boot disk, but we could allow passing additional disk sizes by specifying the device_name/location
do you want me to add this in this PR or is it for a future enhancement?
There was a problem hiding this comment.
I think we need to deal with it in this PR, since the return order of records isn't guaranteed using find_by with multiple matching records can produce erratic results and we could end up making a second or third disk dramatically smaller by accident.
I think the quickest fix for this in the short term is to only allow changing the disk size if there is 1 disk on the template. We can doc this and work on improving it going forward.
The intermediate fix would be to sort disks by Hard Disk X and apply the disk_size to the "first" disk only.
Longer term allowing users to specify different disk sizes for each of the disks on the template would be nice.
There was a problem hiding this comment.
thanks!
I'll work on the short term fix, can't promise I'll be available to contribute to the intermediate and long term ones :)
There was a problem hiding this comment.
Yeah of course not a problem this is a huge start we can pick it up from there
There was a problem hiding this comment.
reconfigure_disk_on_destination? now checks if there's more than one hard disk, and if so logs that it's not currently supported and returns false.
provision_spec.rb tests this condition
There was a problem hiding this comment.
hi @agrare
is the recent update ok?
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
6 times, most recently
from
21261d4
to
54a49ef
Compare
… including refactoring to separate between container reconfiguration (memory, CPU) and disk resize
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
from
54a49ef
to
ec33f2e
Compare
|
Checked commit Autosde@ec33f2e with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint app/models/manageiq/providers/vmware/infra_manager/provision/configuration.rb
app/models/manageiq/providers/vmware/infra_manager/provision/configuration/container.rb
|
add a customizable disk_size field to vm post-provision, including refactoring to separate between container reconfiguration (memory, CPU) and disk resize.
related PRs:
manageiq: ManageIQ/manageiq#22251
ui-classic: ManageIQ/manageiq-ui-classic#8547