vm: Modifying a stopped VM which's state should be started hangs on apply

[TobiPeterG]

Describe the bug
I noticed while testing #2260 that when a VM is stopped but should be started and I modify it (e.g. remove network devies as in this case), the apply hangs because the provider sends a "start" command to the VM (it was stopped) and immediately a "reboot" signal afterwards. This is a) not required, since the change was already applied when the VM was stopped and b) doesn't work as the VM doesn't receive the reboot signal and the apply just times out.

I think it should be detected that the VM is turned off (already known) and the reboot is not sent in this case, but only a start command.

To Reproduce
Steps to reproduce the behavior:

1. Create a config like the one in the linked PR
2. apply state
3. Turn off VM in proxmox
4. Delete network interface in opentofu config
5. apply state
6. See timeout and pending proxmox reboot task

Expected behavior
The reboot is not sent and the apply succeeds after starting the VM.

• Single or clustered Proxmox:
• Proxmox version: 9.0.10
• Provider version (ideally it should be the latest version): 0.85.1
• Terraform/OpenTofu version: 1.10.6
• OS (where you run Terraform/OpenTofu from): OpenSUSE Tumbleweed

[TobiPeterG]

@bpg Do I see it correctly that in vm.go we have:

terraform-provider-proxmox/proxmoxtf/resource/vm/vm.go

Lines 5790 to 5820 in 0c9879a

	// Determine if the state of the virtual machine state needs to be changed.
	//nolint: nestif
	if (d.HasChange(mkStarted) || stoppedBeforeUpdate) && !bool(template) {
	started := d.Get(mkStarted).(bool)
	if started {
	if diags := vmStart(ctx, vmAPI, d); diags != nil {
	return diags
	}
	} else {
	if er := vmShutdown(ctx, vmAPI, d); er != nil {
	return er
	}
	rebootRequired = false
	}
	}
	if cloudInitRebuildRequired {
	if er := vmAPI.RebuildCloudInitDisk(ctx); er != nil {
	return diag.FromErr(err)
	}
	}
	// Change the disk locations and/or sizes, if necessary.
	return vmUpdateDiskLocationAndSize(
	ctx,
	d,
	m,
	!bool(template) && rebootRequired,
	)
	}

Where we check if the current VM is not in the desired power state and if it should be started, we start it and if it should be stopped, we stop it. If it should be stopped, we also reset the rebootRequired variable since we shouldn't reboot the VM we just stopped (it should be stopped). We then pass the parameter if the VM should be rebooted to the function that modifies the disk.

However, in the following vmUpdateDiskLocationAndSize function we have this snippet at the end:

terraform-provider-proxmox/proxmoxtf/resource/vm/vm.go

Lines 6024 to 6047 in 0c9879a

	// Perform a regular reboot in case it's necessary and haven't already been done.
	if reboot {
	canReboot := d.Get(mkRebootAfterUpdate).(bool)
	if !canReboot {
	return []diag.Diagnostic{{
	Severity: diag.Warning,
	Summary: "a reboot is required to apply configuration changes, but automatic " +
	"reboots are disabled by 'reboot_after_update = false'. Please reboot the VM manually.",
	}}
	}
	vmStatus, err := vmAPI.GetVMStatus(ctx)
	if err != nil {
	return diag.FromErr(err)
	}
	if vmStatus.Status != "stopped" {
	rebootTimeoutSec := d.Get(mkTimeoutReboot).(int)
	if e := vmAPI.RebootVMAndWaitForRunning(ctx, rebootTimeoutSec); e != nil {
	return diag.FromErr(e)
	}
	}
	}

If I understand this correctly, if the VM hasn't been rebooted yet, we reboot it.

A) I think it's not very clean that we handle starting/stopping the VM and rebooting the VM at different places and
B) the reboot logic should certainly not be in a function with a name that suggests that only the disks are updated and
C) the reboot logic should not be in this function at all since now the function has two tasks (update disks + handle reboots) and
D) We send a reboot command to the just started VM since we reset the rebootRequired variable only if we turned off a VM that should be turned off.

I think the clean solution would be to either include this functionality at the end of the vmUpdate function (though this would not be very clean) or better, create a helper function to handle the power state of the VM.
I think it's nice that the function to update the disks is already seperate, though it should only do that.

Is there a reason why not first all changes should be applied (so network, storage, etc....) and in the very end, after everything was modified, we check which power state the VM should be in and determine the action based on that and based on which changes we made?

So we have these possible combinations:
Modifications can be applied when VM is powered off or powered on...
Do nothing; The changes have been applied no matter the power state of the VM.

Modifications can only be applied when VM is powered off and...
...VM is stopped...
...and should be stopped: Do nothing, all modifications were applied and VM is in correct power state.
...and should be started: Start the VM, all modifications were applied, but VM is not in the correct power state.

...VM is started...
...and should be stopped: Shutdown the VM, all modifications will be applied on shutdown and after the shutdown, the VM will also be in the correct power state
...and should be started: Reboot the VM, all modifications will be applied on reboot and after the reboot, the VM will also be in the correct state.

Do I miss a state or is there an error in my logic? I guess instead of suggesting that a reboot is required through the variable name, the variable should indicate whether the modification needs to happen while the VM is offline (e.g. rename rebootRequired to offlineModifiation. This makes it clearer that the VM needs to be offline for the modification to be applied correctly and not that the VM needs to be rebooted in all cases to apply the changes. This should be used in a helper function to determine based on the logic proposed above which power action is appropriate to a) apply the modification and b) bring the VM into the correct power state.

Am I allowed to implement it this way?

The VM power state is also handled is also handled when a VM is created, are there other places? I think some logic can be unified across functions.