Add support for "G", "M" etc. disk size unit spec

[bpg]

Currently the unit of virtual_environment_vm.disk.size parameter is "gigabytes". Add ability to specify different units using G/GB, M/MB notation.

│ Error: Incorrect attribute value type
│ 
│   on main.tf line 117, in resource "proxmox_virtual_environment_vm" "example_vm":
│  117:     size         = "300G"
│ 
│ Inappropriate value for attribute "size": a number is required.

[bpg]

Hmmmm... The parsing code already exists:

terraform-provider-proxmox/proxmoxtf/utils.go

Lines 461 to 492 in 380f07c

	func parseDiskSize(size *string) (int, error) {
	var diskSize int
	var err error
	if size != nil {
	if strings.HasSuffix(*size, "T") {
	diskSize, err = strconv.Atoi(strings.TrimSuffix(*size, "T"))
	if err != nil {
	return -1, err
	}
	diskSize = int(math.Ceil(float64(diskSize) * 1024))
	} else if strings.HasSuffix(*size, "G") {
	diskSize, err = strconv.Atoi(strings.TrimSuffix(*size, "G"))
	if err != nil {
	return -1, err
	}
	} else if strings.HasSuffix(*size, "M") {
	diskSize, err = strconv.Atoi(strings.TrimSuffix(*size, "M"))
	if err != nil {
	return -1, err
	}
	diskSize = int(math.Ceil(float64(diskSize) / 1024))
	} else {
	return -1, fmt.Errorf("Cannot parse storage size \"%s\"", *size)
	}
	}
	return diskSize, err
	}

[github-actions]

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!

[bpg]

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

[zeddD1abl0]

@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:

terraform-provider-proxmox/proxmox/utils_test.go

Lines 11 to 41 in 5ffb7d5

	func TestParseDiskSize(t *testing.T) {
	t.Parallel()
	tests := []struct {
	name string
	size *string
	want int
	wantErr bool
	}{
	{"handle null size", nil, 0, false},
	{"parse terabytes", strPtr("2T"), 2048, false},
	{"parse gigabytes", strPtr("2G"), 2, false},
	{"parse megabytes", strPtr("2048M"), 2, false},
	{"error on arbitrary string", strPtr("something"), -1, true},
	{"error on missing unit", strPtr("12345"), -1, true},
	}
	for _, test := range tests {
	tt := test
	t.Run(tt.name, func(t *testing.T) {
	t.Parallel()
	got, err := ParseDiskSize(tt.size)
	if (err != nil) != tt.wantErr {
	t.Errorf("ParseDiskSize() error = %v, wantErr %v", err, tt.wantErr)
	return
	}
	if got != tt.want {
	t.Errorf("ParseDiskSize() got = %v, want %v", got, tt.want)
	}
	})
	}
	}

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:

terraform-provider-proxmox/proxmoxtf/resource/vm.go

Line 2217 in 5ffb7d5

`qemu-img resize -f "$file_format" "$file_path_tmp" "${disk_size}G"`,

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.

[bpg]

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

terraform-provider-proxmox/proxmoxtf/resource/vm.go

Line 2505 in 8a2a3f7

diskDevice.FileVolume = fmt.Sprintf("%s:%d", datastoreID, size)

or in

terraform-provider-proxmox/proxmoxtf/resource/container.go

Line 1067 in 8a2a3f7

Volume: fmt.Sprintf("%s:%d", diskDatastoreID, diskSize),

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;

[zeddD1abl0]

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.

[bpg]

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.

[github-actions]

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!