Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/resources/virtual_environment_vm.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<datastore_id>:import/<file_name>`, 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
Expand Down
17 changes: 11 additions & 6 deletions proxmoxtf/resource/vm/disk/disk.go
Original file line number Diff line number Diff line change
Expand Up @@ -583,8 +583,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 {
Expand All @@ -596,7 +597,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
Expand All @@ -612,7 +613,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 {
Expand All @@ -624,8 +625,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
Expand All @@ -647,5 +652,5 @@ func Update(
}
}

return rebootRequired, nil
return shutdownBeforeUpdate, rebootRequired, nil
}
71 changes: 70 additions & 1 deletion proxmoxtf/resource/vm/disk/disk_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,19 +210,23 @@
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{
AIO: &aio2,
Cache: &cache2,
Size: size2,
DatastoreID: &datastore2,
ImportFrom: &importFrom2,
}

// Test identical disks
Expand All @@ -247,6 +251,17 @@
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.
Expand Down Expand Up @@ -339,8 +354,9 @@
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
Expand All @@ -350,6 +366,59 @@
// 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, []interface{}{

Check failure on line 388 in proxmoxtf/resource/vm/disk/disk_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint

any: interface{} can be replaced by any (modernize)
map[string]interface{}{

Check failure on line 389 in proxmoxtf/resource/vm/disk/disk_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint

any: interface{} can be replaced by any (modernize)
mkDiskInterface: "scsi1",
mkDiskDatastoreID: "local",
mkDiskSize: 5, // Old size
mkDiskSpeed: []interface{}{},

Check failure on line 393 in proxmoxtf/resource/vm/disk/disk_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint

any: interface{} can be replaced by any (modernize)
},
})
require.NoError(t, err)

err = resourceData.Set(MkDisk, []interface{}{

Check failure on line 398 in proxmoxtf/resource/vm/disk/disk_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint

any: interface{} can be replaced by any (modernize)
map[string]interface{}{

Check failure on line 399 in proxmoxtf/resource/vm/disk/disk_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint

any: interface{} can be replaced by any (modernize)
mkDiskInterface: "scsi1",
mkDiskDatastoreID: "local",
mkDiskSize: 20, // New size
mkDiskSpeed: []interface{}{},

Check failure on line 403 in proxmoxtf/resource/vm/disk/disk_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint

any: interface{} can be replaced by any (modernize)
},
})
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) {
Expand Down
17 changes: 12 additions & 5 deletions proxmoxtf/resource/vm/vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
Loading