fix(vm): fix and improve disk management for cloned VMs (#1840)

Allow to set disk speed and set / update other attributes of existing disks when cloning a VM

Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>

example/variables.tf

@@ -28,5 +28,5 @@ variable "release_20240725_ubuntu_24_noble_lxc_img_url" {
 variable "release_20240725_ubuntu_24_noble_lxc_img_checksum" {
   type        = string
   description = "The checksum for the Ubuntu 24.04 LXC image"
-  default     = "10331782a01cd2348b421a261f0e15ba041358bd540f66f2432b162e70b90d28"
+  default     = "d767d38cb25b2c25d84edc31a80dd1c29a8c922b74188b0e14768b2b2fb6df8e"
 }

fwprovider/test/resource_vm_test.go

@@ -704,6 +704,50 @@ func TestAccResourceVMClone(t *testing.T) {
 				}`),
 			ExpectError: regexp.MustCompile(`storage 'doesnotexist' does not exist`),
 		}}},
+		{"update disk speed and resize in a clone", []resource.TestStep{{
+			Config: te.RenderConfig(`
+				resource "proxmox_virtual_environment_vm" "template" {
+					node_name = "{{.NodeName}}"
+					started   = false
+					disk {
+						datastore_id = "local-lvm"
+						interface    = "virtio0"
+						file_format  = "raw"
+						size         = 20
+					}
+				}
+				resource "proxmox_virtual_environment_vm" "clone" {
+					node_name = "{{.NodeName}}"
+					started   = false
+					clone {
+						vm_id = proxmox_virtual_environment_vm.template.vm_id
+					}
+					disk {
+						datastore_id = "local-lvm"
+						interface    = "virtio0"
+						iothread     = true
+						discard      = "on"
+						size         = 30
+						speed {
+						  iops_read = 100
+						  iops_read_burstable = 1000
+						  iops_write = 400
+						  iops_write_burstable = 800
+						}
+					}
+				}`),
+			Check: resource.ComposeTestCheckFunc(
+				ResourceAttributes("proxmox_virtual_environment_vm.clone", map[string]string{
+					"disk.0.iothread":                     "true",
+					"disk.0.discard":                      "on",
+					"disk.0.size":                         "30",
+					"disk.0.speed.0.iops_read":            "100",
+					"disk.0.speed.0.iops_read_burstable":  "1000",
+					"disk.0.speed.0.iops_write":           "400",
+					"disk.0.speed.0.iops_write_burstable": "800",
+				}),
+			),
+		}}},
 	}
 
 	for _, tt := range tests {

fwprovider/vm/resource.go

@@ -378,7 +378,7 @@ func (r *Resource) Delete(ctx context.Context, req resource.DeleteRequest, resp
 		resp.Diagnostics.AddError("Failed to get VM status", err.Error())
 	}
 
-	if resp.Diagnostics.HasError() {
+	if resp.Diagnostics.HasError() || status == nil {
 		return
 	}
 

proxmox/helpers/ptr/ptr.go

@@ -32,3 +32,14 @@ func Eq[T comparable](a, b *T) bool {
 
 	return *a == *b
 }
+
+// UpdateIfChanged updates dst with src if src is not nil and different from dst.
+// Returns true if an update was made.
+func UpdateIfChanged[T comparable](dst **T, src *T) bool {
+	if src != nil && !Eq(*dst, src) {
+		*dst = src
+		return true
+	}
+
+	return false
+}

proxmox/nodes/vms/custom_storage_device.go

@@ -14,6 +14,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/bpg/terraform-provider-proxmox/proxmox/helpers/ptr"
 	"github.com/bpg/terraform-provider-proxmox/proxmox/types"
 )
 
@@ -360,6 +361,40 @@ func (d *CustomStorageDevice) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
+// MergeWith merges attributes of the given CustomStorageDevice with the current one.
+// It will overwrite the current attributes with the given ones if they are not nil.
+// The attributes that are not merged are:
+//   - DatastoreID
+//   - FileID
+//   - FileVolume
+//   - Format
+//   - Size
+//
+// It will return true if any attribute of the current CustomStorageDevice was changed.
+func (d *CustomStorageDevice) MergeWith(m CustomStorageDevice) bool {
+	updated := false
+
+	updated = ptr.UpdateIfChanged(&d.AIO, m.AIO) || updated
+	updated = ptr.UpdateIfChanged(&d.Backup, m.Backup) || updated
+	updated = ptr.UpdateIfChanged(&d.BurstableReadSpeedMbps, m.BurstableReadSpeedMbps) || updated
+	updated = ptr.UpdateIfChanged(&d.BurstableWriteSpeedMbps, m.BurstableWriteSpeedMbps) || updated
+	updated = ptr.UpdateIfChanged(&d.Cache, m.Cache) || updated
+	updated = ptr.UpdateIfChanged(&d.Discard, m.Discard) || updated
+	updated = ptr.UpdateIfChanged(&d.IOThread, m.IOThread) || updated
+	updated = ptr.UpdateIfChanged(&d.IopsRead, m.IopsRead) || updated
+	updated = ptr.UpdateIfChanged(&d.IopsWrite, m.IopsWrite) || updated
+	updated = ptr.UpdateIfChanged(&d.Media, m.Media) || updated
+	updated = ptr.UpdateIfChanged(&d.MaxIopsRead, m.MaxIopsRead) || updated
+	updated = ptr.UpdateIfChanged(&d.MaxIopsWrite, m.MaxIopsWrite) || updated
+	updated = ptr.UpdateIfChanged(&d.MaxReadSpeedMbps, m.MaxReadSpeedMbps) || updated
+	updated = ptr.UpdateIfChanged(&d.MaxWriteSpeedMbps, m.MaxWriteSpeedMbps) || updated
+	updated = ptr.UpdateIfChanged(&d.Replicate, m.Replicate) || updated
+	updated = ptr.UpdateIfChanged(&d.SSD, m.SSD) || updated
+	updated = ptr.UpdateIfChanged(&d.Serial, m.Serial) || updated
+
+	return updated
+}
+
 // Filter returns a map of CustomStorageDevices filtered by the given function.
 func (d CustomStorageDevices) Filter(fn func(*CustomStorageDevice) bool) CustomStorageDevices {
 	result := make(CustomStorageDevices)

proxmoxtf/resource/vm/disk/disk.go

@@ -54,81 +54,77 @@ func GetInfo(resp *vms.GetResponseData, d *schema.ResourceData) vms.CustomStorag
 	return storageDevices
 }
 
-// CreateClone creates disks for a cloned VM.
+// UpdateClone updates disks in a cloned VM.
-func CreateClone(
+func UpdateClone(
 	ctx context.Context,
-	d *schema.ResourceData,
 	planDisks vms.CustomStorageDevices,
 	allDiskInfo vms.CustomStorageDevices,
 	vmAPI *vms.Client,
 ) error {
-	disk := d.Get(MkDisk).([]interface{})
+	for diskInterface, planDisk := range planDisks {
-	for i := range disk {
+		currentDisk := allDiskInfo[diskInterface]
-		diskBlock := disk[i].(map[string]interface{})
-		diskInterface := diskBlock[mkDiskInterface].(string)
-		dataStoreID := diskBlock[mkDiskDatastoreID].(string)
-		diskSize := int64(diskBlock[mkDiskSize].(int))
 
-		currentDiskInfo := allDiskInfo[diskInterface]
+		if currentDisk == nil {
-		configuredDiskInfo := planDisks[diskInterface]
-
-		if currentDiskInfo == nil {
 			diskUpdateBody := &vms.UpdateRequestBody{}
+			diskUpdateBody.AddCustomStorageDevice(diskInterface, *planDisk)
 
-			diskUpdateBody.AddCustomStorageDevice(diskInterface, *configuredDiskInfo)
+			if err := vmAPI.UpdateVM(ctx, diskUpdateBody); err != nil {
-
-			err := vmAPI.UpdateVM(ctx, diskUpdateBody)
-			if err != nil {
 				return fmt.Errorf("disk update fails: %w", err)
 			}
 
 			continue
 		}
 
-		if diskSize < currentDiskInfo.Size.InGigabytes() {
+		if planDisk.Size.InMegabytes() < currentDisk.Size.InMegabytes() {
-			return fmt.Errorf("disk resize fails requests size (%dG) is lower than current size (%d)",
+			return fmt.Errorf("disk resize failure: requested size (%s) is lower than current size (%s)",
-				diskSize,
+				planDisk.Size.String(),
-				*currentDiskInfo.Size,
+				currentDisk.Size.String(),
 			)
 		}
 
+		moveDisk := false
+
+		if *planDisk.DatastoreID != "" {
+			fileIDParts := strings.Split(currentDisk.FileVolume, ":")
+			moveDisk = *planDisk.DatastoreID != fileIDParts[0]
+		}
+
+		if moveDisk {
 			deleteOriginalDisk := types.CustomBool(true)
 
 			diskMoveBody := &vms.MoveDiskRequestBody{
 				DeleteOriginalDisk: &deleteOriginalDisk,
 				Disk:               diskInterface,
-			TargetStorage:      dataStoreID,
+				TargetStorage:      *planDisk.DatastoreID,
 			}
 
-		diskResizeBody := &vms.ResizeDiskRequestBody{
-			Disk: diskInterface,
-			Size: *types.DiskSizeFromGigabytes(diskSize),
-		}
-
-		moveDisk := false
-
-		if dataStoreID != "" {
-			moveDisk = true
-
-			if allDiskInfo[diskInterface] != nil {
-				fileIDParts := strings.Split(allDiskInfo[diskInterface].FileVolume, ":")
-				moveDisk = dataStoreID != fileIDParts[0]
-			}
-		}
-
-		if moveDisk {
 			err := vmAPI.MoveVMDisk(ctx, diskMoveBody)
 			if err != nil {
 				return fmt.Errorf("disk move fails: %w", err)
 			}
 		}
 
-		if diskSize > currentDiskInfo.Size.InGigabytes() {
+		if planDisk.Size.InMegabytes() > currentDisk.Size.InMegabytes() {
+			diskResizeBody := &vms.ResizeDiskRequestBody{
+				Disk: diskInterface,
+				Size: *planDisk.Size,
+			}
+
 			err := vmAPI.ResizeVMDisk(ctx, diskResizeBody)
 			if err != nil {
 				return fmt.Errorf("disk resize fails: %w", err)
 			}
 		}
+
+		// update other disk parameters
+		if currentDisk.MergeWith(*planDisk) {
+			diskUpdateBody := &vms.UpdateRequestBody{}
+			diskUpdateBody.AddCustomStorageDevice(diskInterface, *currentDisk)
+
+			if err := vmAPI.UpdateVM(ctx, diskUpdateBody); err != nil {
+				return fmt.Errorf("disk update fails: %w", err)
+			}
+		}
 	}
 
 	return nil

proxmoxtf/resource/vm/vm.go

@@ -2212,14 +2212,14 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d
 		return diag.FromErr(e)
 	}
 
-	allDiskInfo := disk.GetInfo(vmConfig, d) // from the cloned VM
+	clonedDiskInfo := disk.GetInfo(vmConfig, d) // from the cloned VM
 
 	planDisks, e := disk.GetDiskDeviceObjects(d, VM(), nil) // from the resource config
 	if e != nil {
 		return diag.FromErr(e)
 	}
 
-	e = disk.CreateClone(ctx, d, planDisks, allDiskInfo, vmAPI)
+	e = disk.UpdateClone(ctx, planDisks, clonedDiskInfo, vmAPI)
 	if e != nil {
 		return diag.FromErr(e)
 	}
@@ -2268,8 +2268,8 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d
 		if dataStoreID != "" {
 			moveDisk = true
 
-			if allDiskInfo[diskInterface] != nil {
+			if clonedDiskInfo[diskInterface] != nil {
-				fileIDParts := strings.Split(allDiskInfo[diskInterface].FileVolume, ":")
+				fileIDParts := strings.Split(clonedDiskInfo[diskInterface].FileVolume, ":")
 				moveDisk = dataStoreID != fileIDParts[0]
 			}
 		}
@@ -2319,8 +2319,8 @@ func vmCreateClone(ctx context.Context, d *schema.ResourceData, m interface{}) d
 		if dataStoreID != "" {
 			moveDisk = true
 
-			if allDiskInfo[diskInterface] != nil {
+			if clonedDiskInfo[diskInterface] != nil {
-				fileIDParts := strings.Split(allDiskInfo[diskInterface].FileVolume, ":")
+				fileIDParts := strings.Split(clonedDiskInfo[diskInterface].FileVolume, ":")
 				moveDisk = dataStoreID != fileIDParts[0]
 			}
 		}