Add support for "G", "M" etc. disk size unit spec about terraform-provider-proxmox HOT 8 CLOSED

Hmmmm... The parsing code already exists:

from terraform-provider-proxmox.

Marking this issue as stale due to inactivity in the past 180 days. This helps us focus on the active issues. If this issue is reproducible with the latest version of the provider, please comment. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

from terraform-provider-proxmox.

If anyone would like to contribute, this seems to be an easy one.

from terraform-provider-proxmox.

@bpg I'm happy to pick this one up, but I have a question regarding this code; why is the size stored in gigabytes? I understand that most HDDs would be of gigabyte size, but this function fails when you try to clone a VM with an efi disk that is measured in kilobytes.

Specifically, I have a number of templates that have a 528K EFI disk associated with them, and the provider chokes while trying to decode this drive because "K" is not a supported size extension. I attempted to quickly submit a pull request for this, but the tests for this function appear to expect the size to be gigabytes:

I tried to investigate how and why you'd go with that as a standard, and I can see you use qemu-img to grow the HDD to the requested size in this line:

Is that it? Have I missed anything in the logic? This appears to prevent the creation of anything that's not at least 1 gigabyte in size. After following through the logic, it seems to round up to the nearest whole gigabyte if M, G, or T, is specified, and dies if the value is anything but this.

If the above is all correct, I'd propose to base off of bytes, as qemu-img uses bytes by default, and expanding to bytes is super simple. Then, it's simply a case of changing the line above to remove the "G", changing the util function to multiply out to bytes, and adding in the newly expected values ("MB", "GB", "TB", "KB", "B", etc).

You could plausibly go to Kilobytes as the base unit, extending your limit from 512 Petabytes per disk to 512 Exabytes per disk...

As an aside, I notice that there is no way to provide an EFI disk for a VM. If I get the chance, I'll probably include that in another pull request and get it added.

from terraform-provider-proxmox.

Hey @zeddD1abl0 👋🏼 Thanks for taking a stab on this!

In addition to reference you've mentioned, there are also a few special cases in PVE where a size is expected to be set as an integer without any units. So my guess that was a main driving factor behind unification of all disk size values as "gigabytes" in the original provider.

For example, https://pve.proxmox.com/pve-docs/pve-admin-guide.html#qm_configuration, see "the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new volume" for params --ide[n], --sata[n], etc.
In provider we're using this approach in

So I'm not entirely sure how to proceed with this story. It seems like using units less than 1G won't work in all use cases. I would probably do something similar to this:

• if no units specified, then assume the value is in gigabytes;
• if units are specified, then convert to gigabytes rounding up;
• if the result value is less than 1G, then throw an error;

from terraform-provider-proxmox.

Cool. I can work with that. How do you feel about using Regex to do the matching? The current check allows for 80TG, which will break the process. I'd propose the following Regex-based solution to prevent erroneous sizes from being provided. This forces the value to be in the format of "<numbers><possibly a space><size designation>"

re := regexp.MustCompile(`(?i)^([\d\.]+)\s?(m|mb|mib|g|gb|gib|t|tb|tib)?$`)
matches := re.FindStringSubmatch(*size)
if len(matches) > 0 {
fsize, err := strconv.ParseFloat(matches[1], 64)
if err != nil {
	return -1, fmt.Errorf("cannot parse disk size \"%s\"", *size)
}
switch strings.ToLower(matches[2]) {
case "m", "mb", "mib":
	return int(math.Ceil(fsize / 1024)) , nil
case "g", "gb", "gib":
	return int(math.Ceil(fsize)), nil
case "t", "tb", "tib":
	return int(math.Ceil(fsize * 1024)), nil
case "p", "pb", "pib":
	return int(math.Ceil(fsize * 1024 * 1024)), nil
default:
	return int(math.Ceil(fsize)), nil
}
}

return -1, fmt.Errorf("cannot parse disk size \"%s\"", *size)

If you'd prefer me to follow the current format, I can do that too. I just wanted to double-check before I got too far ahead of myself.

from terraform-provider-proxmox.

I'm fine with regex :), just make sure it is cached and not compiled on each func invocation.

Also I'm not sure if we should go into the rabbit hole of MB vs MiB etc, as they mean different things. Have everything represented in power on 2 seems reasonable to me.

from terraform-provider-proxmox.

Marking this issue as stale due to inactivity in the past 180 days. This helps us focus on the active issues. If this issue is reproducible with the latest version of the provider, please comment. If this issue receives no comments in the next 30 days it will automatically be closed. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

from terraform-provider-proxmox.