diff --git a/docs/resources/virtual_environment_vm.md b/docs/resources/virtual_environment_vm.md index 0fd67e4b9..c1565e605 100755 --- a/docs/resources/virtual_environment_vm.md +++ b/docs/resources/virtual_environment_vm.md @@ -305,7 +305,7 @@ output "ubuntu_vm_public_key" { `proxmox_virtual_environment_download_file` resource. *Deprecated*, use `import_from` instead. - `import_from` - (Optional) The file ID for a disk image to import into VM. The image must be of `import` content type. The ID format is `:import/`, for example `local:import/centos8.qcow2`. Can be also taken from - `proxmox_virtual_environment_download_file` resource. + a disk replacement operation, which will require a VM reboot. Your original disks will remain as detached disks. - `interface` - (Required) The disk interface for Proxmox, currently `scsi`, `sata` and `virtio` interfaces are supported. Append the disk index at the end, for example, `virtio0` for the first virtio disk, `virtio1` for diff --git a/proxmoxtf/resource/vm/disk/disk.go b/proxmoxtf/resource/vm/disk/disk.go index 163c4aa4b..3faf9b452 100644 --- a/proxmoxtf/resource/vm/disk/disk.go +++ b/proxmoxtf/resource/vm/disk/disk.go @@ -549,13 +549,19 @@ func Read( if len(currentDiskList) > 0 { currentDiskMap := utils.MapResourcesByAttribute(currentDiskList, mkDiskInterface) - // copy import_from from the current disk if it exists + // copy import_from and size from the current disk if it exists for k, v := range currentDiskMap { if disk, ok := v.(map[string]any); ok { - if importFrom, ok := disk[mkDiskImportFrom].(string); ok && importFrom != "" { - if _, exists := diskMap[k]; exists { + if _, exists := diskMap[k]; exists { + if importFrom, ok := disk[mkDiskImportFrom].(string); ok && importFrom != "" { diskMap[k].(map[string]any)[mkDiskImportFrom] = importFrom } + // preserve size from state when API returns zero size (for disks with import_from or file_id) + if currentSize, ok := disk[mkDiskSize].(int); ok && currentSize > 0 { + if apiSize, ok := diskMap[k].(map[string]any)[mkDiskSize].(int64); ok && apiSize == 0 { + diskMap[k].(map[string]any)[mkDiskSize] = currentSize + } + } } } } @@ -583,8 +589,9 @@ func Update( planDisks vms.CustomStorageDevices, currentDisks vms.CustomStorageDevices, updateBody *vms.UpdateRequestBody, -) (bool, error) { +) (bool, bool, error) { rebootRequired := false + shutdownBeforeUpdate := false if d.HasChange(MkDisk) { for iface, disk := range planDisks { @@ -596,7 +603,7 @@ func Update( // only disks with defined file ID are custom image disks that need to be created via import. err := createCustomDisk(ctx, client, nodeName, vmID, iface, *disk) if err != nil { - return false, fmt.Errorf("creating custom disk: %w", err) + return false, false, fmt.Errorf("creating custom disk: %w", err) } } else { // otherwise this is a blank disk that can be added directly via update API @@ -612,7 +619,7 @@ func Update( tmp = currentDisks[iface] default: // something went wrong - return false, fmt.Errorf("missing device %s", iface) + return false, false, fmt.Errorf("missing device %s", iface) } if tmp == nil || disk == nil { @@ -624,8 +631,12 @@ func Update( tmp.AIO = disk.AIO } - // Never send ImportFrom for existing disks - it triggers re-import which fails for boot disks - // ImportFrom is only for initial disk creation, not updates + if disk.ImportFrom != nil && *disk.ImportFrom != "" { + shutdownBeforeUpdate = true + tmp.DatastoreID = disk.DatastoreID + tmp.ImportFrom = disk.ImportFrom + tmp.Size = disk.Size + } tmp.Backup = disk.Backup tmp.BurstableReadSpeedMbps = disk.BurstableReadSpeedMbps @@ -647,5 +658,5 @@ func Update( } } - return rebootRequired, nil + return shutdownBeforeUpdate, rebootRequired, nil } diff --git a/proxmoxtf/resource/vm/disk/disk_test.go b/proxmoxtf/resource/vm/disk/disk_test.go index a483a1349..4f85b68f7 100644 --- a/proxmoxtf/resource/vm/disk/disk_test.go +++ b/proxmoxtf/resource/vm/disk/disk_test.go @@ -210,12 +210,15 @@ func TestDiskDevicesEqual(t *testing.T) { size2 := types.DiskSizeFromGigabytes(10) datastore1 := "local" datastore2 := "local" + importFrom1 := "local:import/test.qcow2" + importFrom2 := "local:import/test.qcow2" disk1 := &vms.CustomStorageDevice{ AIO: &aio1, Cache: &cache1, Size: size1, DatastoreID: &datastore1, + ImportFrom: &importFrom1, } disk2 := &vms.CustomStorageDevice{ @@ -223,6 +226,7 @@ func TestDiskDevicesEqual(t *testing.T) { Cache: &cache2, Size: size2, DatastoreID: &datastore2, + ImportFrom: &importFrom2, } // Test identical disks @@ -247,6 +251,17 @@ func TestDiskDevicesEqual(t *testing.T) { DatastoreID: &datastore2, } require.False(t, disk1.Equals(disk2SizeChanged)) + + // Test different ImportFrom values + importFrom2Changed := "local:import/test2.qcow2" + disk2ImportFromChanged := &vms.CustomStorageDevice{ + AIO: &aio2, + Cache: &cache2, + Size: size2, + ImportFrom: &importFrom2Changed, + DatastoreID: &datastore2, + } + require.False(t, disk1.Equals(disk2ImportFromChanged)) } // TestDiskUpdateSkipsUnchangedDisks tests that the Update function only updates changed disks. @@ -339,8 +354,9 @@ func TestDiskUpdateSkipsUnchangedDisks(t *testing.T) { require.NoError(t, err) // Call the Update function - _, err = Update(ctx, client, nodeName, vmID, resourceData, planDisks, currentDisks, updateBody) + shutdownBeforeUpdate, _, err := Update(ctx, client, nodeName, vmID, resourceData, planDisks, currentDisks, updateBody) require.NoError(t, err) + require.False(t, shutdownBeforeUpdate) // Check that only the changed disk (scsi1) is in the update body // scsi0 should NOT be in the update body since it hasn't changed @@ -350,6 +366,59 @@ func TestDiskUpdateSkipsUnchangedDisks(t *testing.T) { // This prevents the "can't unplug bootdisk 'scsi0'" error // Note: We can't directly inspect the updateBody content in this test framework, // but the fact that no error occurred means the logic worked correctly + + // Create plan disks (what terraform wants) + importFrom2 := "local:import/test2.qcow2" + planDisks2 := vms.CustomStorageDevices{ + "scsi0": &vms.CustomStorageDevice{ + Size: types.DiskSizeFromGigabytes(10), // Same as current + DatastoreID: &datastoreID, + ImportFrom: &importFrom2, // Different Import file. + }, + "scsi1": &vms.CustomStorageDevice{ + Size: types.DiskSizeFromGigabytes(5), // Same as current + DatastoreID: &datastoreID, + }, + } + + // Mock update body to capture what gets sent to the API + updateBody2 := &vms.UpdateRequestBody{} + + // Force HasChange to return true by setting old and new values + err = resourceData.Set(MkDisk, []any{ + map[string]any{ + mkDiskInterface: "scsi1", + mkDiskDatastoreID: "local", + mkDiskSize: 5, // Old size + mkDiskSpeed: []any{}, + }, + }) + require.NoError(t, err) + + err = resourceData.Set(MkDisk, []any{ + map[string]any{ + mkDiskInterface: "scsi1", + mkDiskDatastoreID: "local", + mkDiskSize: 20, // New size + mkDiskSpeed: []any{}, + }, + }) + require.NoError(t, err) + + // Call the Update function + shutdownBeforeUpdate, _, err = Update(ctx, client, nodeName, vmID, resourceData, planDisks2, currentDisks, updateBody2) + require.NoError(t, err) + require.True(t, shutdownBeforeUpdate) + + // Check that only the changed disk (scsi1) is in the update body + // scsi0 should NOT be in the update body since it hasn't changed + require.NotNil(t, updateBody2) + require.Contains(t, updateBody2.CustomStorageDevices, "scsi0", "Update body should contain the changed disk scsi0") + require.NotContains(t, updateBody2.CustomStorageDevices, "scsi1", "Update body should not contain the unchanged disk scsi1") + require.Equal(t, importFrom2, *updateBody2.CustomStorageDevices["scsi0"].ImportFrom) + + // The update body should only contain scsi0, not scsi1 + // Note: We can't directly inspect the updateBody content in this test framework, } func TestDiskDeletionDetectionInGetDiskDeviceObjects(t *testing.T) { diff --git a/proxmoxtf/resource/vm/vm.go b/proxmoxtf/resource/vm/vm.go index 19ba3436b..8aefbcc52 100644 --- a/proxmoxtf/resource/vm/vm.go +++ b/proxmoxtf/resource/vm/vm.go @@ -5575,11 +5575,17 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnosti } } - rr, err := disk.Update(ctx, client, nodeName, vmID, d, planDisks, allDiskInfo, updateBody) + stoppedBeforeUpdate, rr, err := disk.Update(ctx, client, nodeName, vmID, d, planDisks, allDiskInfo, updateBody) if err != nil { return diag.FromErr(err) } + if stoppedBeforeUpdate { + if er := vmShutdown(ctx, vmAPI, d); er != nil { + return er + } + } + rebootRequired = rebootRequired || rr // Prepare the new efi disk configuration. @@ -5610,7 +5616,6 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnosti } // Prepare the new cloud-init configuration. - stoppedBeforeUpdate := false cloudInitRebuildRequired := false if d.HasChange(mkInitialization) { @@ -5657,8 +5662,10 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnosti if mustMove || mustChangeDatastore || existingInterface == "" { // CloudInit must be moved, either from a device to another or from a datastore // to another (or both). This requires the VM to be stopped. - if er := vmShutdown(ctx, vmAPI, d); er != nil { - return er + if !stoppedBeforeUpdate { + if er := vmShutdown(ctx, vmAPI, d); er != nil { + return er + } } if er := deleteIdeDrives(ctx, vmAPI, initializationInterface, existingInterface); er != nil { @@ -5945,7 +5952,7 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnosti if diags := vmStart(ctx, vmAPI, d); diags != nil { return diags } - } else { + } else if !stoppedBeforeUpdate { if er := vmShutdown(ctx, vmAPI, d); er != nil { return er }