fix(vm): handle update of disks moved during VM clone (#1849)

* fix(vm): handle update of disks moved during VM clone

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

.github/workflows/golangci-lint.yml

@@ -43,4 +43,5 @@ jobs:
         uses: golangci/golangci-lint-action@v7
         with: 
           version: v2.0.2 # renovate: depName=golangci/golangci-lint datasource=github-releases
+          skip-cache: true
           args: -v --timeout=10m

Makefile

@@ -112,7 +112,8 @@ testacc:
 .PHONY: lint
 lint:
 	# NOTE: This target is for local runs only. For linting in CI see .github/workflows/golangci-lint.yml
-	@docker run -t --rm -v $$(pwd):/app -v ~/.cache/golangci-lint/$(GOLANGCI_LINT_VERSION):/root/.cache -w /app golangci/golangci-lint:$(GOLANGCI_LINT_VERSION) golangci-lint format run --fix
+	@docker run -t --rm -v $$(pwd):/app -v ~/.cache/golangci-lint/$(GOLANGCI_LINT_VERSION):/root/.cache -w /app golangci/golangci-lint:$(GOLANGCI_LINT_VERSION) golangci-lint fmt
+	@docker run -t --rm -v $$(pwd):/app -v ~/.cache/golangci-lint/$(GOLANGCI_LINT_VERSION):/root/.cache -w /app golangci/golangci-lint:$(GOLANGCI_LINT_VERSION) golangci-lint run
 
 .PHONY: release-build
 release-build:

fwprovider/test/resource_vm_disks_test.go

@@ -422,7 +422,7 @@ func TestAccResourceVMDisks(t *testing.T) {
 		
 					disk {
 						interface    = "scsi0"
-						//size = 10
+						size = 10
 					}
 				}`),
 				Check: ResourceAttributes("proxmox_virtual_environment_vm.test_disk", map[string]string{
@@ -432,7 +432,7 @@ func TestAccResourceVMDisks(t *testing.T) {
 					"disk.0.interface":         "scsi0",
 					"disk.0.iothread":          "true",
 					"disk.0.path_in_datastore": `base-\d+-disk-\d+`,
-					"disk.0.size":              "8",
+					"disk.0.size":              "10",
 					"disk.0.ssd":               "true",
 				}),
 			},
@@ -535,6 +535,85 @@ func TestAccResourceVMDisks(t *testing.T) {
 				RefreshState: true,
 			},
 		}},
+		{"clone with updating disk attributes", []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",
+				}),
+			),
+		}}},
+		{"clone with moving disk", []resource.TestStep{{
+			Config: te.RenderConfig(`
+				resource "proxmox_virtual_environment_vm" "template" {
+					node_name = "{{.NodeName}}"
+					started   = false
+					efi_disk {
+						datastore_id = "local-lvm"
+						type = "4m"
+					}
+					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 = "tank"
+						interface    = "virtio0"
+						file_format  = "raw"
+						size         = 20
+					}
+				}`),
+			Check: resource.ComposeTestCheckFunc(
+				ResourceAttributes("proxmox_virtual_environment_vm.clone", map[string]string{
+					"disk.0.datastore_id": "tank",
+				}),
+			),
+		}}},
 	}
 
 	for _, tt := range tests {

fwprovider/test/resource_vm_test.go

@@ -704,50 +704,6 @@ 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 {

proxmox/nodes/vms/custom_storage_device.go

@@ -25,13 +25,17 @@ var StorageInterfaces = []string{"ide", "sata", "scsi", "virtio"}
 
 // CustomStorageDevice handles QEMU SATA device parameters.
 type CustomStorageDevice struct {
+	// FileVolume is the path to the storage device in format
+	// "STORAGE_ID:SIZE_IN_GiB" or "STORAGE_ID:PATH_TO_FILE".
+	// This is a required field.
+	FileVolume string `json:"file" url:"file"`
+
 	AIO                     *string           `json:"aio,omitempty"         url:"aio,omitempty"`
 	Backup                  *types.CustomBool `json:"backup,omitempty"      url:"backup,omitempty,int"`
 	BurstableReadSpeedMbps  *int              `json:"mbps_rd_max,omitempty" url:"mbps_rd_max,omitempty"`
 	BurstableWriteSpeedMbps *int              `json:"mbps_wr_max,omitempty" url:"mbps_wr_max,omitempty"`
 	Cache                   *string           `json:"cache,omitempty"       url:"cache,omitempty"`
 	Discard                 *string           `json:"discard,omitempty"     url:"discard,omitempty"`
-	FileVolume              string            `json:"file"                  url:"file"`
 	Format                  *string           `json:"format,omitempty"      url:"format,omitempty"`
 	IopsRead                *int              `json:"iops_rd,omitempty"     url:"iops_rd,omitempty"`
 	IopsWrite               *int              `json:"iops_wr,omitempty"     url:"iops_wr,omitempty"`

proxmoxtf/provider/provider.go

@@ -80,7 +80,7 @@ func providerConfigure(ctx context.Context, d *schema.ResourceData) (interface{}
 		csrfPreventionToken = v.(string)
 	}
 
-	//nolint:staticcheck //using GetOkExists to check if the value is set, as it can be an empty string in tests
+	//nolint:staticcheck
 	if v, ok := d.GetOkExists(mkProviderAPIToken); ok {
 		apiToken = v.(string)
 	}
@@ -89,12 +89,12 @@ func providerConfigure(ctx context.Context, d *schema.ResourceData) (interface{}
 		otp = v.(string)
 	}
 
-	///nolint:staticcheck //using GetOkExists to check if the value is set, as it can be an empty string in tests
+	///nolint:staticcheck
 	if v, ok := d.GetOkExists(mkProviderUsername); ok {
 		username = v.(string)
 	}
 
-	//nolint:staticcheck //using GetOkExists to check if the value is set, as it can be an empty string in tests
+	//nolint:staticcheck
 	if v, ok := d.GetOkExists(mkProviderPassword); ok {
 		password = v.(string)
 	}

proxmoxtf/resource/vm/disk/disk.go

@@ -82,6 +82,17 @@ func UpdateClone(
 			)
 		}
 
+		// update other disk parameters
+		// we have to do it before moving the disk, because the disk volume and location may change
+		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)
+			}
+		}
+
 		moveDisk := false
 
 		if *planDisk.DatastoreID != "" {
@@ -98,6 +109,8 @@ func UpdateClone(
 				TargetStorage:      *planDisk.DatastoreID,
 			}
 
+			// Note: after disk move, the actual disk volume ID will be different: both datastore id *and*
+			// path in datastore will change.
 			err := vmAPI.MoveVMDisk(ctx, diskMoveBody)
 			if err != nil {
 				return fmt.Errorf("disk move fails: %w", err)
@@ -115,16 +128,6 @@ func UpdateClone(
 				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