add customizable disk_size to vm post-provision #22251
Conversation
|
the check failure here seems general and unrelated to the requested changes |
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
from
08304b3
to
2d35162
Compare
There was a problem hiding this comment.
@agrare - question around changing Hardware.used_disk_storage/allocated_disk_storage from referencing disks to hard_disks. hardware.rb
@OrGur1987 Question around changing to allocated_disk_storage or used
I did not have an opinion on the feature itself.
| @@ -5,7 +5,8 @@ def set_or_default_hardware_field_values(vm) | |||
| :cpu_limit => vm.cpu_limit, | |||
| :memory_limit => vm.memory_limit, | |||
| :cpu_reserve => vm.cpu_reserve, | |||
| :memory_reserve => vm.memory_reserve | |||
| :memory_reserve => vm.memory_reserve, | |||
| :disk_size => vm.hardware.disks.find_by(:device_type => "disk").size / (1024**3) | |||
There was a problem hiding this comment.
A few things.
In hardware.rb I notice that we have a number of different device_type values called out but none are "disk" in particular. I can't imagine that this is a new concept. Maybe we want to use hard_disks instead?
size is the count of the disks (e.g.: 1-4), and / (1024**3) is definitely talking about disk storage (e.g.: 3.5 * 10^9), so there is a disconnect here.
I'm thinking allocated_disk_storage or used_disk_storage would work here. The desire to not display CDRom values in disk size makes me wonder if used_disk_storage should not disks.sum(:used_disk_storage) but hard_disks.sum(:used_disk_storage). (I'm not sure if we've tested a virtual sum with a where() clause, so we probably want to put a test in here and over in virtual_attributes gem.
There was a problem hiding this comment.
@kbrock changed to allocated_disk_storage
There was a problem hiding this comment.
@kbrock I think allocated_disk_storage is confusing here, this is the size of a single disk that we're going to be creating and isn't intended to map to any columns.
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
3 times, most recently
from
1ab9741
to
9531be6
Compare
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
from
9531be6
to
99f8da7
Compare
| :memory_limit => vm.memory_limit, | ||
| :cpu_reserve => vm.cpu_reserve, | ||
| :memory_reserve => vm.memory_reserve, | ||
| :allocated_disk_storage => vm.hardware.disks.find_by(:device_type => "disk").size / (1024**3) |
There was a problem hiding this comment.
I know in the VMware PR we're discussing finding the "first" disk as an initial solution, that is relatively simple for a specific provider but it is going to be very tricky here since this code is common to all Infra providers.
In MiqProvisionVirtWorkflow#set_on_vm_id_changed we build a map of the NICs and LANs: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_provision_virt_workflow.rb#L137-L138
I wonder if we do the same with the template's disks and then we could more easily set a different size for each disk.
There was a problem hiding this comment.
I think we should change this, but speaking of this code specifically:
- Prefer
1.gigabyteover1024**3for readability - It would be better tot pass around
bytesif we have it rather thanGB, we can always convert to GB but not the other way around without losing precision (also if we're going to useGBshould be a float e.g. if a user wants1.5GBdisk)
There was a problem hiding this comment.
I changed the field to string and now the user can input a float-like value.
regarding bytes, the .size attribute of a disk is indeed in bytes, but is converted to a human readable GB to provide the default value, and is received from the user in GB scale as well.
afterwards, VirtualDisk reconfiguration expects it in KB for dev.capacityInKB (in ManageIQ::Providers::Vmware::InfraManager::Provision::Configuration::Disk#set_disk_allocation).
so I'm not sure where in that flow will it be effective to convert it to bytes
There was a problem hiding this comment.
I know in the VMware PR we're discussing finding the "first" disk as an initial solution, that is relatively simple for a specific provider but it is going to be very tricky here since this code is common to all Infra providers.
In
MiqProvisionVirtWorkflow#set_on_vm_id_changedwe build a map of the NICs and LANs: https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_provision_virt_workflow.rb#L137-L138I wonder if we do the same with the template's disks and then we could more easily set a different size for each disk.
in line with the short-term fix you suggested in the vmware repo PR, the code now adds the default :allocated_disk_storage only when there's exactly 1 hard-disk.
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
3 times, most recently
from
ae5f5da
to
d5c2511
Compare
… field to `set_or_default_hardware_field_values`
OrGur1987
force-pushed
the
411361_vmware_disk_size
branch
from
d5c2511
to
e82de23
Compare
|
Checked commit Autosde@e82de23 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint |
|
@OrGur1987 core tests were broken yesterday so was waiting for that fix for the tests to pass on this PR |
and does this PR pass the tests now? |
|
I still think allocated_disk_storage is overly confusing and disk_size would be simpler but we can do that in a follow-up |
add the field to
set_or_default_hardware_field_values, so that it would show the default template disk sizerelated PRs:
providers-vmware: ManageIQ/manageiq-providers-vmware#839
ui-classic: ManageIQ/manageiq-ui-classic#8547