Skip to content

fix(disk): restore updating boot disk. #2150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+94 −13
Open

fix(disk): restore updating boot disk. #2150

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
47abbdb
fix(disk): restore updating boot disk.
Vaneixus Sep 7, 2025
b48de3e
fix(vm): restart vm after update is done.
Vaneixus Sep 7, 2025
7d32487
fix(lint): curly bracket indentation.
Vaneixus Sep 7, 2025
2b72850
Merge branch 'main' into Update_Disk_Via_Import
bpg Sep 9, 2025
43d1733
fix(vm): shutdown vm before updating boot disk.
Vaneixus Sep 21, 2025
f9004e9
Merge branch 'main' into Update_Disk_Via_Import
Vaneixus Sep 22, 2025
c67b910
Remove blank line in vm.go
Vaneixus Sep 22, 2025
abe7fbd
Merge branch 'main' into Update_Disk_Via_Import
Vaneixus Nov 3, 2025
0c3e1ff
Merge branch 'bpg:main' into Update_Disk_Via_Import
Vaneixus Nov 8, 2025
911c591
fix(vm): vm start happening twice.
Vaneixus Nov 8, 2025
21b9694
fix(docs): update documentation for `import-from`.
Vaneixus Nov 9, 2025
58bb9c0
feat(disk): improved import-from disk testing.
Vaneixus Nov 10, 2025
a407f4f
Update docs/resources/virtual_environment_vm.md
Vaneixus Nov 10, 2025
45847b0
feat(disk): improved import-from disk testing.
Vaneixus Nov 10, 2025
bf9d532
Merge branch 'main' into Update_Disk_Via_Import
Vaneixus Nov 14, 2025
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
18 changes: 12 additions & 6 deletions proxmoxtf/resource/vm/disk/disk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,8 +582,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 @@ -595,7 +596,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 @@ -611,7 +612,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 @@ -623,8 +624,13 @@ 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 != "" {
rebootRequired = true
shutdownBeforeUpdate = true
tmp.DatastoreID = disk.DatastoreID
tmp.ImportFrom = disk.ImportFrom
tmp.Size = disk.Size
}
Comment on lines 627 to 632
Copy link
Owner

@bpg bpg Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this part was specifically removed here
so I think this change will break the VM update scenario, when VM has an imported disk.

What was the reason for adding this back?

Copy link
Contributor Author

@Vaneixus Vaneixus Sep 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello again @bpg, the reason this was re-added was that the boot disk cannot be changed while the vm is running, you can only change it while it isn't, at least that's what I remember being the case back when I opened the first PR. This PR is currently untested. I just opened it as to show my progress. I should have probably put it as a draft. It's also why I move the start-up/shutdown code, I probably have to change that part again. But basically, the whole idea is to shut down the vm to allow the boot disk to be changed. I just haven't had the opportunity to test this again. I will be working again on this PR on the weekend.


tmp.Backup = disk.Backup
tmp.BurstableReadSpeedMbps = disk.BurstableReadSpeedMbps
Expand All @@ -646,5 +652,5 @@ func Update(
}
}

return rebootRequired, nil
return shutdownBeforeUpdate, rebootRequired, nil
}
2 changes: 1 addition & 1 deletion proxmoxtf/resource/vm/disk/disk_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func TestDiskUpdateSkipsUnchangedDisks(t *testing.T) {
require.NoError(t, err)

// Call the Update function
_, err = Update(ctx, client, nodeName, vmID, resourceData, planDisks, currentDisks, updateBody)
_, _, err = Update(ctx, client, nodeName, vmID, resourceData, planDisks, currentDisks, updateBody)
require.NoError(t, err)

// Check that only the changed disk (scsi1) is in the update body
Expand Down
26 changes: 21 additions & 5 deletions proxmoxtf/resource/vm/vm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5426,6 +5426,7 @@
return diag.FromErr(err)
}


Check failure on line 5429 in proxmoxtf/resource/vm/vm.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

File is not properly formatted (gofmt)
// Handle disk deletion before applying other changes
if d.HasChange(disk.MkDisk) {
bootOrder := d.Get(mkBootOrder).([]interface{})
Expand All @@ -5449,11 +5450,17 @@
}
}

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 @@ -5484,7 +5491,6 @@
}

// Prepare the new cloud-init configuration.
stoppedBeforeUpdate := false
cloudInitRebuildRequired := false

if d.HasChange(mkInitialization) {
Expand Down Expand Up @@ -5531,8 +5537,10 @@
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 @@ -5782,7 +5790,7 @@
//nolint: nestif
if (d.HasChange(mkStarted) || stoppedBeforeUpdate) && !bool(template) {
started := d.Get(mkStarted).(bool)
if started {
if started && !stoppedBeforeUpdate {
if diags := vmStart(ctx, vmAPI, d); diags != nil {
return diags
}
Expand All @@ -5795,6 +5803,14 @@
}
}

if stoppedBeforeUpdate && d.Get(mkStarted).(bool) {
if diags := vmStart(ctx, vmAPI, d); diags != nil {
return diags
}
// The VM has been started, so a reboot is no longer required.
rebootRequired = false
}

if cloudInitRebuildRequired {
if er := vmAPI.RebuildCloudInitDisk(ctx); er != nil {
return diag.FromErr(err)
Expand Down
Loading