Skip to content

Commit 0da0bcb

Browse files
Vaneixusgemini-code-assist[bot]bpg
authored
fix(disk): restore updating boot disk. (#2150)
* fix(disk): restore updating boot disk. Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * fix(vm): restart vm after update is done. Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * fix(lint): curly bracket indentation. Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * fix(vm): shutdown vm before updating boot disk. Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * Remove blank line in vm.go Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * fix(vm): vm start happening twice. Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * fix(docs): update documentation for `import-from`. Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * feat(disk): improved import-from disk testing. Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * Update docs/resources/virtual_environment_vm.md Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * feat(disk): improved import-from disk testing. Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> * fix linter errors Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> * fix state population if disk size is 0 Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> --------- Signed-off-by: Marco Attia <54147992+Vaneixus@users.noreply.github.com> Signed-off-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Pavel Boldyrev <627562+bpg@users.noreply.github.com>
1 parent d3b4847 commit 0da0bcb

File tree

4 files changed

+103
-16
lines changed

4 files changed

+103
-16
lines changed

‎docs/resources/virtual_environment_vm.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ output "ubuntu_vm_public_key" {
305305
`proxmox_virtual_environment_download_file` resource. *Deprecated*, use `import_from` instead.
306306
- `import_from` - (Optional) The file ID for a disk image to import into VM. The image must be of `import` content type.
307307
The ID format is `<datastore_id>:import/<file_name>`, for example `local:import/centos8.qcow2`. Can be also taken from
308-
`proxmox_virtual_environment_download_file` resource.
308+
a disk replacement operation, which will require a VM reboot. Your original disks will remain as detached disks.
309309
- `interface` - (Required) The disk interface for Proxmox, currently `scsi`,
310310
`sata` and `virtio` interfaces are supported. Append the disk index at
311311
the end, for example, `virtio0` for the first virtio disk, `virtio1` for

‎proxmoxtf/resource/vm/disk/disk.go‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -549,13 +549,19 @@ func Read(
549549

550550
if len(currentDiskList) > 0 {
551551
currentDiskMap := utils.MapResourcesByAttribute(currentDiskList, mkDiskInterface)
552-
// copy import_from from the current disk if it exists
552+
// copy import_from and size from the current disk if it exists
553553
for k, v := range currentDiskMap {
554554
if disk, ok := v.(map[string]any); ok {
555-
if importFrom, ok := disk[mkDiskImportFrom].(string); ok && importFrom != "" {
556-
if _, exists := diskMap[k]; exists {
555+
if _, exists := diskMap[k]; exists {
556+
if importFrom, ok := disk[mkDiskImportFrom].(string); ok && importFrom != "" {
557557
diskMap[k].(map[string]any)[mkDiskImportFrom] = importFrom
558558
}
559+
// preserve size from state when API returns zero size (for disks with import_from or file_id)
560+
if currentSize, ok := disk[mkDiskSize].(int); ok && currentSize > 0 {
561+
if apiSize, ok := diskMap[k].(map[string]any)[mkDiskSize].(int64); ok && apiSize == 0 {
562+
diskMap[k].(map[string]any)[mkDiskSize] = currentSize
563+
}
564+
}
559565
}
560566
}
561567
}
@@ -583,8 +589,9 @@ func Update(
583589
planDisks vms.CustomStorageDevices,
584590
currentDisks vms.CustomStorageDevices,
585591
updateBody *vms.UpdateRequestBody,
586-
) (bool, error) {
592+
) (bool, bool, error) {
587593
rebootRequired := false
594+
shutdownBeforeUpdate := false
588595

589596
if d.HasChange(MkDisk) {
590597
for iface, disk := range planDisks {
@@ -596,7 +603,7 @@ func Update(
596603
// only disks with defined file ID are custom image disks that need to be created via import.
597604
err := createCustomDisk(ctx, client, nodeName, vmID, iface, *disk)
598605
if err != nil {
599-
return false, fmt.Errorf("creating custom disk: %w", err)
606+
return false, false, fmt.Errorf("creating custom disk: %w", err)
600607
}
601608
} else {
602609
// otherwise this is a blank disk that can be added directly via update API
@@ -612,7 +619,7 @@ func Update(
612619
tmp = currentDisks[iface]
613620
default:
614621
// something went wrong
615-
return false, fmt.Errorf("missing device %s", iface)
622+
return false, false, fmt.Errorf("missing device %s", iface)
616623
}
617624

618625
if tmp == nil || disk == nil {
@@ -624,8 +631,12 @@ func Update(
624631
tmp.AIO = disk.AIO
625632
}
626633

627-
// Never send ImportFrom for existing disks - it triggers re-import which fails for boot disks
628-
// ImportFrom is only for initial disk creation, not updates
634+
if disk.ImportFrom != nil && *disk.ImportFrom != "" {
635+
shutdownBeforeUpdate = true
636+
tmp.DatastoreID = disk.DatastoreID
637+
tmp.ImportFrom = disk.ImportFrom
638+
tmp.Size = disk.Size
639+
}
629640

630641
tmp.Backup = disk.Backup
631642
tmp.BurstableReadSpeedMbps = disk.BurstableReadSpeedMbps
@@ -647,5 +658,5 @@ func Update(
647658
}
648659
}
649660

650-
return rebootRequired, nil
661+
return shutdownBeforeUpdate, rebootRequired, nil
651662
}

‎proxmoxtf/resource/vm/disk/disk_test.go‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -210,19 +210,23 @@ func TestDiskDevicesEqual(t *testing.T) {
210210
size2 := types.DiskSizeFromGigabytes(10)
211211
datastore1 := "local"
212212
datastore2 := "local"
213+
importFrom1 := "local:import/test.qcow2"
214+
importFrom2 := "local:import/test.qcow2"
213215

214216
disk1 := &vms.CustomStorageDevice{
215217
AIO: &aio1,
216218
Cache: &cache1,
217219
Size: size1,
218220
DatastoreID: &datastore1,
221+
ImportFrom: &importFrom1,
219222
}
220223

221224
disk2 := &vms.CustomStorageDevice{
222225
AIO: &aio2,
223226
Cache: &cache2,
224227
Size: size2,
225228
DatastoreID: &datastore2,
229+
ImportFrom: &importFrom2,
226230
}
227231

228232
// Test identical disks
@@ -247,6 +251,17 @@ func TestDiskDevicesEqual(t *testing.T) {
247251
DatastoreID: &datastore2,
248252
}
249253
require.False(t, disk1.Equals(disk2SizeChanged))
254+
255+
// Test different ImportFrom values
256+
importFrom2Changed := "local:import/test2.qcow2"
257+
disk2ImportFromChanged := &vms.CustomStorageDevice{
258+
AIO: &aio2,
259+
Cache: &cache2,
260+
Size: size2,
261+
ImportFrom: &importFrom2Changed,
262+
DatastoreID: &datastore2,
263+
}
264+
require.False(t, disk1.Equals(disk2ImportFromChanged))
250265
}
251266

252267
// TestDiskUpdateSkipsUnchangedDisks tests that the Update function only updates changed disks.
@@ -339,8 +354,9 @@ func TestDiskUpdateSkipsUnchangedDisks(t *testing.T) {
339354
require.NoError(t, err)
340355

341356
// Call the Update function
342-
_, err = Update(ctx, client, nodeName, vmID, resourceData, planDisks, currentDisks, updateBody)
357+
shutdownBeforeUpdate, _, err := Update(ctx, client, nodeName, vmID, resourceData, planDisks, currentDisks, updateBody)
343358
require.NoError(t, err)
359+
require.False(t, shutdownBeforeUpdate)
344360

345361
// Check that only the changed disk (scsi1) is in the update body
346362
// scsi0 should NOT be in the update body since it hasn't changed
@@ -350,6 +366,59 @@ func TestDiskUpdateSkipsUnchangedDisks(t *testing.T) {
350366
// This prevents the "can't unplug bootdisk 'scsi0'" error
351367
// Note: We can't directly inspect the updateBody content in this test framework,
352368
// but the fact that no error occurred means the logic worked correctly
369+
370+
// Create plan disks (what terraform wants)
371+
importFrom2 := "local:import/test2.qcow2"
372+
planDisks2 := vms.CustomStorageDevices{
373+
"scsi0": &vms.CustomStorageDevice{
374+
Size: types.DiskSizeFromGigabytes(10), // Same as current
375+
DatastoreID: &datastoreID,
376+
ImportFrom: &importFrom2, // Different Import file.
377+
},
378+
"scsi1": &vms.CustomStorageDevice{
379+
Size: types.DiskSizeFromGigabytes(5), // Same as current
380+
DatastoreID: &datastoreID,
381+
},
382+
}
383+
384+
// Mock update body to capture what gets sent to the API
385+
updateBody2 := &vms.UpdateRequestBody{}
386+
387+
// Force HasChange to return true by setting old and new values
388+
err = resourceData.Set(MkDisk, []any{
389+
map[string]any{
390+
mkDiskInterface: "scsi1",
391+
mkDiskDatastoreID: "local",
392+
mkDiskSize: 5, // Old size
393+
mkDiskSpeed: []any{},
394+
},
395+
})
396+
require.NoError(t, err)
397+
398+
err = resourceData.Set(MkDisk, []any{
399+
map[string]any{
400+
mkDiskInterface: "scsi1",
401+
mkDiskDatastoreID: "local",
402+
mkDiskSize: 20, // New size
403+
mkDiskSpeed: []any{},
404+
},
405+
})
406+
require.NoError(t, err)
407+
408+
// Call the Update function
409+
shutdownBeforeUpdate, _, err = Update(ctx, client, nodeName, vmID, resourceData, planDisks2, currentDisks, updateBody2)
410+
require.NoError(t, err)
411+
require.True(t, shutdownBeforeUpdate)
412+
413+
// Check that only the changed disk (scsi1) is in the update body
414+
// scsi0 should NOT be in the update body since it hasn't changed
415+
require.NotNil(t, updateBody2)
416+
require.Contains(t, updateBody2.CustomStorageDevices, "scsi0", "Update body should contain the changed disk scsi0")
417+
require.NotContains(t, updateBody2.CustomStorageDevices, "scsi1", "Update body should not contain the unchanged disk scsi1")
418+
require.Equal(t, importFrom2, *updateBody2.CustomStorageDevices["scsi0"].ImportFrom)
419+
420+
// The update body should only contain scsi0, not scsi1
421+
// Note: We can't directly inspect the updateBody content in this test framework,
353422
}
354423

355424
func TestDiskDeletionDetectionInGetDiskDeviceObjects(t *testing.T) {

‎proxmoxtf/resource/vm/vm.go‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5575,11 +5575,17 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnosti
55755575
}
55765576
}
55775577

5578-
rr, err := disk.Update(ctx, client, nodeName, vmID, d, planDisks, allDiskInfo, updateBody)
5578+
stoppedBeforeUpdate, rr, err := disk.Update(ctx, client, nodeName, vmID, d, planDisks, allDiskInfo, updateBody)
55795579
if err != nil {
55805580
return diag.FromErr(err)
55815581
}
55825582

5583+
if stoppedBeforeUpdate {
5584+
if er := vmShutdown(ctx, vmAPI, d); er != nil {
5585+
return er
5586+
}
5587+
}
5588+
55835589
rebootRequired = rebootRequired || rr
55845590

55855591
// Prepare the new efi disk configuration.
@@ -5610,7 +5616,6 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnosti
56105616
}
56115617

56125618
// Prepare the new cloud-init configuration.
5613-
stoppedBeforeUpdate := false
56145619
cloudInitRebuildRequired := false
56155620

56165621
if d.HasChange(mkInitialization) {
@@ -5657,8 +5662,10 @@ func vmUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnosti
56575662
if mustMove || mustChangeDatastore || existingInterface == "" {
56585663
// CloudInit must be moved, either from a device to another or from a datastore
56595664
// to another (or both). This requires the VM to be stopped.
5660-
if er := vmShutdown(ctx, vmAPI, d); er != nil {
5661-
return er
5665+
if !stoppedBeforeUpdate {
5666+
if er := vmShutdown(ctx, vmAPI, d); er != nil {
5667+
return er
5668+
}
56625669
}
56635670

56645671
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
59455952
if diags := vmStart(ctx, vmAPI, d); diags != nil {
59465953
return diags
59475954
}
5948-
} else {
5955+
} else if !stoppedBeforeUpdate {
59495956
if er := vmShutdown(ctx, vmAPI, d); er != nil {
59505957
return er
59515958
}

0 commit comments

Comments
 (0)