fix(task.go): proxmox qmstart is inconsistent and leads to state failure

TaskFailure State is now only set if the action is not qmstart.
It looks impossible to detect a failure in qmstart. This fixes
the state machine transition in ReconcileVM.

Author: 65278

internal/service/taskservice/task.go

@@ -103,26 +103,36 @@ func checkAndRetryTask(scope *scope.MachineScope, task *proxmox.Task) (bool, err
 	case task.IsRunning:
 		logger.Info("task is still pending", "description", task.Type)
 		return true, nil
-	case task.IsSuccessful:
+	case task.IsSuccessful && task.IsCompleted:
 		logger.Info("task is a success", "description", task.Type)
 		scope.ProxmoxMachine.Status.TaskRef = nil
 		return false, nil
 	case task.IsFailed:
-		logger.Info("task failed", "description", task.Type)
-
-		// NOTE: When a task fails there is not simple way to understand which operation is failing (e.g. cloning or powering on)
-		// so we are reporting failures using a dedicated reason until we find a better solution.
-		var errorMessage string
+		// Failing tasks are actually red herrings. Some tasks fail, other
+		// tasks can fail successfully (like qmstart).
+		// We save the condition so the ReconcileVM statemachine keeps on working.
+		conditionReason := conditions.GetReason(scope.ProxmoxMachine, infrav1.ProxmoxMachineVirtualMachineProvisionedCondition)
+
+		// qmstart can fail and yet actually start the VM. We can not handle qmstart properly.
+		// In fact qmstart can find a machine already started, because proxmox's api is
+		// eventually consistent here.
+		// For all other jobs we do set the condition to failed.
+		if task.Type != "qmstart" {
+			logger.Info("task failed", "description", task.Type)
+			// We notify the user that intervention is required. This should stop the state machine.
+			conditionReason = infrav1.ProxmoxMachineVirtualMachineProvisionedTaskFailedReason
+		}
 
-		if task.ExitStatus != "OK" {
-			errorMessage = task.ExitStatus
-		} else {
-			errorMessage = "task failed but its exit status is OK; this should not happen"
+		errorMessage := fmt.Sprintf("%s: %s", task.Type, task.ExitStatus)
+		if task.ExitStatus == "OK" {
+			// If you end up here, file a bug with go-proxmox.
+			errorMessage = fmt.Sprintf("task %s failed but its exit status is OK; this should not happen", task.UPID)
 		}
+
 		conditions.Set(scope.ProxmoxMachine, metav1.Condition{
 			Type:    infrav1.ProxmoxMachineVirtualMachineProvisionedCondition,
 			Status:  metav1.ConditionFalse,
-			Reason:  infrav1.ProxmoxMachineVirtualMachineProvisionedTaskFailedReason,
+			Reason:  conditionReason,
 			Message: errorMessage,
 		})
 

internal/service/vmservice/helpers_test.go

@@ -544,7 +544,11 @@ func createBootstrapSecret(t *testing.T, c client.Client, machineScope *scope.Ma
 }
 
 func newTask() *proxmox.Task {
-	return &proxmox.Task{UPID: "result"}
+	return &proxmox.Task{
+		UPID:         "result",
+		IsSuccessful: true,
+		IsCompleted:  true,
+	}
 }
 
 func newVMResource() *proxmox.ClusterResource {

internal/service/vmservice/vm.go

@@ -115,7 +115,8 @@ func ReconcileVM(ctx context.Context, scope *scope.MachineScope) (infrav1.Virtua
 	} // VirtualMachineProvisioned reason is WaitingForBootstrapReady
 
 	// handle invalid state of the machine
-	if conditions.GetReason(scope.ProxmoxMachine, infrav1.ProxmoxMachineVirtualMachineProvisionedCondition) == infrav1.ProxmoxMachineVirtualMachineProvisionedVMProvisionFailedReason {
+	if conditions.GetReason(scope.ProxmoxMachine, infrav1.ProxmoxMachineVirtualMachineProvisionedCondition) == infrav1.ProxmoxMachineVirtualMachineProvisionedVMProvisionFailedReason ||
+		conditions.GetReason(scope.ProxmoxMachine, infrav1.ProxmoxMachineVirtualMachineProvisionedCondition) == infrav1.ProxmoxMachineVirtualMachineProvisionedTaskFailedReason {
 		scope.Logger.V(4).Info("invalid proxmoxmachine state", "state", conditions.GetReason(scope.ProxmoxMachine, infrav1.ProxmoxMachineVirtualMachineProvisionedCondition))
 		// If you end up here, please file a bug report.
 		return vm, errors.New("invalid state (failed and no error)")

internal/service/vmservice/vm_test.go

@@ -726,6 +726,8 @@ func TestReconcileVM_EndToEnd(t *testing.T) {
 		},
 	}
 
+	task := newTask()
+
 	// Round 0: no VM exists yet; CloneVM creates one and requeues for the task to complete.
 	proxmoxClient.EXPECT().GetReservableMemoryBytes(context.Background(), "node1", int64(100)).Return(0, nil).Once()
 	proxmoxClient.EXPECT().GetReservableMemoryBytes(context.Background(), "node2", int64(100)).Return(^uint64(0), nil).Once()
@@ -739,7 +741,7 @@ func TestReconcileVM_EndToEnd(t *testing.T) {
 		SnapName:    "snap",
 		Storage:     "storage",
 		Target:      "node2",
-	}).Return(proxmox.VMCloneResponse{NewID: 123, Task: newTask()}, nil).Once()
+	}).Return(proxmox.VMCloneResponse{NewID: 123, Task: task}, nil).Once()
 
 	result, err := ReconcileVM(context.Background(), machineScope)
 	require.NoError(t, err)
@@ -754,10 +756,9 @@ func TestReconcileVM_EndToEnd(t *testing.T) {
 	)
 
 	// Round 1: clone task complete; ConfigureVM sets VM options and requeues.
-	proxmoxClient.EXPECT().GetTask(context.Background(), "result").Return(&lutherproxmox.Task{UPID: "result", IsSuccessful: true}, nil).Once()
+	proxmoxClient.EXPECT().GetTask(context.Background(), "result").Return(task, nil).Once()
 	proxmoxClient.EXPECT().GetVM(context.Background(), "node2", int64(123)).Return(vm, nil).Once()
 
-	task := newTask()
 	expectedVMConfigureRequest := []interface{}{
 		proxmox.VirtualMachineOption{Name: optionSockets, Value: *machineScope.ProxmoxMachine.Spec.NumSockets},
 		proxmox.VirtualMachineOption{Name: optionCores, Value: *machineScope.ProxmoxMachine.Spec.NumCores},
@@ -782,7 +783,7 @@ func TestReconcileVM_EndToEnd(t *testing.T) {
 	// Round 2: ConfigureVM task has completed; reconcileDisks resizes the boot volume,
 	// then reconcileIPAddresses advances the state to WaitingForBootstrapData.
 	// We're not mocking the entirety of a network setup.
-	proxmoxClient.EXPECT().GetTask(context.Background(), "result").Return(&lutherproxmox.Task{UPID: "result", IsSuccessful: true}, nil).Once()
+	proxmoxClient.EXPECT().GetTask(context.Background(), "result").Return(task, nil).Once()
 	proxmoxClient.EXPECT().GetVM(context.Background(), "node2", int64(123)).Return(vm, nil).Once()
 	proxmoxClient.EXPECT().ResizeDisk(context.Background(), vm, "scsi0", "50G").Return(nil, nil).Once()
 
@@ -842,7 +843,7 @@ func TestReconcileVM_EndToEnd(t *testing.T) {
 	vm.Status = lutherproxmox.StatusVirtualMachineRunning
 	vm.QMPStatus = lutherproxmox.StatusVirtualMachineRunning
 
-	proxmoxClient.EXPECT().GetTask(context.Background(), "result").Return(&lutherproxmox.Task{UPID: "result", IsSuccessful: true}, nil).Once()
+	proxmoxClient.EXPECT().GetTask(context.Background(), "result").Return(task, nil).Once()
 	proxmoxClient.EXPECT().GetVM(context.Background(), "node2", int64(123)).Return(vm, nil).Once()
 	proxmoxClient.EXPECT().QemuAgentStatus(context.Background(), vm).Return(nil).Once()
 	proxmoxClient.EXPECT().CloudInitStatus(context.Background(), vm).Return(false, nil).Once()