Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Disk size in Kibibyte #305

Merged
merged 9 commits into from
Feb 5, 2024
Merged

Conversation

Tinyblargon
Copy link
Collaborator

Changes the size of all disk to be in Kibibytes instead of Gibibytes. Closes #304

Work done:

  • Added test for new functions.
  • Added a resize step to the Qemu create and update workflow.
  • Renamed Size variable (big commit at the end)

@Tinyblargon
Copy link
Collaborator Author

Okay testing it now in the terraform provider, getting the following error lvcreate --help

@mleone87
Copy link
Collaborator

same

the PUT generated from https://github.com/Telmate/terraform-provider-proxmox/blob/b052f99b533c7165a6698102165f188d9418f4c3/proxmox/resource_vm_qemu.go#L1245 seems broken

virtio0: nvmepool:0,format=raw,replicate=0
I left the size without the suffix and it's parsed to 20KiB from provider

@Tinyblargon
Copy link
Collaborator Author

Yeah, the way It's intended to work is:

If size is exactly x amount of gigabytes, create disk with storage_name:disk_size, else we crate it like storage_name:0 and later we rezize the disk to the correct size.

I think i only tested this code with the file backend. Gonna try the volume backed as well.

@mleone87
Copy link
Collaborator

I only adapted the provide to digest the new fields in the struct and it's not working like that, I guess some adjustment is needed

@Tinyblargon
Copy link
Collaborator Author

Tinyblargon commented Jan 31, 2024

@mleone87 I've figured it out, when the backend is file the minimum size is 1K but when the backend is LVM the minimum size is 4097K. I'm gonna create a custom type type QemuDiskSize uint and enforce a minimum size of 4097.

@Tinyblargon
Copy link
Collaborator Author

@mleone87 Okay so now when the disk size isn't a nice gibibyte number we will create a disk the size of 0.001 gibibyte, as disk_size in storage_name:disk_size is a float. This initial created drive will be 1048 which is smaller than our minimum of 4097. After disk creation we will resize it to the user provided size.

The most important code is in proxmox/config_qemu.go.
For selecting which newly created disk have to be resized the unit tests in proxmox/config_qemu_disk_test.go will have to do.

Worst case scenarios are:

  1. disk is created as 0.001 gibigyte and didn't get resized (no data loss).
  2. disk will be selected for resize and it will already be bigger (will trigger an api error, no data loss).

Kinda happy when everything with the disk stuff is done. Never imagined that implementing every edge case would take this long. Hopefully we can wrap-up #187 soon.

@mleone87
Copy link
Collaborator

mleone87 commented Feb 2, 2024

@Tinyblargon Not able to adapt the terraform code anymore, my changes are there Telmate/terraform-provider-proxmox#928

provider crashes

panic: interface conversion: interface {} is int, not uint

@Tinyblargon
Copy link
Collaborator Author

@mleone87 Sorry, I forgot to create the PR to fix the Terraform side of things.

@mleone87
Copy link
Collaborator

mleone87 commented Feb 2, 2024

@Tinyblargon nevermind I got it and it's working, the only thing missing is change the schema definition for the size of disks to accept strings representing the size in kib or a size in KMGBT

I guess this part can be merged?

@Tinyblargon
Copy link
Collaborator Author

@mleone87 The code for the KMGT is ready. I'll make a pull tomorrow.

@Tinyblargon Tinyblargon merged commit ede76ba into Telmate:master Feb 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Disk size in Kibibyte
2 participants