Merge pull request #1298 from openshift-cherrypick-robot/cherry-pick-1223-to-release-4.16

[release-4.16] OCPBUGS-43433: Vsphere: Handle cloned instance with lost taskID

Author: openshift-merge-bot[bot]

pkg/controller/vsphere/reconciler.go

@@ -115,9 +115,37 @@ func (r *Reconciler) create() error {
 
 	// We only clone the VM template if we have no taskRef.
 	if r.providerStatus.TaskRef == "" {
+		klog.V(4).Infof("%v: ProviderStatus does not have TaskRef", r.machine.GetName())
 		if !r.machineScope.session.IsVC() {
 			return fmt.Errorf("%v: not connected to a vCenter", r.machine.GetName())
 		}
+
+		// Attempt to power on instance in situation where we alredy cloned the instance and lost taskRef.
+		klog.V(4).Infof("%v: InstanceState is: %q", r.machine.GetName(), ptr.Deref(r.machineScope.providerStatus.InstanceState, ""))
+		if types.VirtualMachinePowerState(ptr.Deref(r.machineScope.providerStatus.InstanceState, "")) == types.VirtualMachinePowerStatePoweredOff {
+			klog.Infof("Powering on cloned machine without taskID: %v", r.machine.Name)
+
+			task, err := powerOn(r.machineScope)
+			if err != nil {
+				metrics.RegisterFailedInstanceCreate(&metrics.MachineLabels{
+					Name:      r.machine.Name,
+					Namespace: r.machine.Namespace,
+					Reason:    "PowerOn task finished with error",
+				})
+
+				conditionFailed := conditionFailed()
+				conditionFailed.Message = err.Error()
+				statusError := setProviderStatus(task, conditionFailed, r.machineScope, nil)
+				if statusError != nil {
+					return fmt.Errorf("failed to set provider status: %w", err)
+				}
+
+				return fmt.Errorf("%v: failed to power on machine: %w", r.machine.GetName(), err)
+			}
+
+			return setProviderStatus(task, conditionSuccess(), r.machineScope, nil)
+		}
+
 		klog.Infof("%v: cloning", r.machine.GetName())
 		task, err := clone(r.machineScope)
 		if err != nil {
@@ -170,8 +198,12 @@ func (r *Reconciler) create() error {
 		} else {
 			return fmt.Errorf("failed to check task status: %w", err)
 		}
-	} else if !taskIsFinished {
-		return fmt.Errorf("%v task %v has not finished", moTask.Info.DescriptionId, moTask.Reference().Value)
+	} else {
+		if taskIsFinished {
+			klog.V(4).Infof("%v task %v has completed", moTask.Info.DescriptionId, moTask.Reference().Value)
+		} else {
+			return fmt.Errorf("%v task %v has not finished", moTask.Info.DescriptionId, moTask.Reference().Value)
+		}
 	}
 
 	// if clone task finished successfully, power on the vm
@@ -287,10 +319,9 @@ func (r *Reconciler) exists() (bool, error) {
 	}
 
 	// Check if machine was powered on after clone.
-	// If it is powered off and in "Provisioning" phase, treat machine as non-existed yet and requeue for proceed
-	// with creation procedure.
+	// If it is powered off and in "Provisioning" phase, treat machine as non-existed yet and proceed with creation procedure.
 	powerState := types.VirtualMachinePowerState(ptr.Deref(r.machineScope.providerStatus.InstanceState, ""))
-	if powerState == "" {
+	if powerState == "" || ptr.Deref(r.machine.Status.Phase, "") == machinev1.PhaseProvisioning {
 		vm := &virtualMachine{
 			Context: r.machineScope.Context,
 			Obj:     object.NewVirtualMachine(r.machineScope.session.Client.Client, vmRef),
@@ -303,7 +334,11 @@ func (r *Reconciler) exists() (bool, error) {
 	}
 
 	if ptr.Deref(r.machine.Status.Phase, "") == machinev1.PhaseProvisioning && powerState == types.VirtualMachinePowerStatePoweredOff {
-		klog.Infof("%v: already exists, but was not powered on after clone, requeue ", r.machine.GetName())
+		klog.Infof("%v: already exists, but was not powered on after clone", r.machine.GetName())
+		r.machineScope.providerStatus.InstanceState = ptr.To(string(powerState))
+		if err := r.machineScope.PatchMachine(); err != nil {
+			return false, fmt.Errorf("%v: failed to patch machine: %w", r.machine.GetName(), err)
+		}
 		return false, nil
 	}
 

pkg/controller/vsphere/reconciler_test.go

@@ -2839,17 +2839,24 @@ func TestUpdate(t *testing.T) {
 }
 
 func TestExists(t *testing.T) {
-	model, session, server := initSimulator(t)
+	withPoweredOffVMS := func() simulatorModelOption {
+		return func(m *simulator.Model) {
+			m.Autostart = false
+		}
+	}
+
+	model, server := initSimulatorCustom(t, withPoweredOffVMS())
+	session := getSimulatorSession(t, server)
 	defer model.Remove()
 	defer server.Close()
 	credentialsSecretUsername := fmt.Sprintf("%s.username", server.URL.Host)
 	credentialsSecretPassword := fmt.Sprintf("%s.password", server.URL.Host)
 
 	password, _ := server.URL.User.Password()
 	namespace := "test"
-	vm := simulator.Map.Any("VirtualMachine").(*simulator.VirtualMachine)
-	instanceUUID := "a5764857-ae35-34dc-8f25-a9c9e73aa898"
-	vm.Config.InstanceUuid = instanceUUID
+	VMs := simulator.Map.All("VirtualMachine")
+	poweredOffVM := VMs[0].(*simulator.VirtualMachine)
+	poweredOnVM := VMs[1].(*simulator.VirtualMachine)
 
 	credentialsSecret := corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
@@ -2862,7 +2869,7 @@ func TestExists(t *testing.T) {
 		},
 	}
 
-	vmObj := object.NewVirtualMachine(session.Client.Client, vm.Reference())
+	vmObj := object.NewVirtualMachine(session.Client.Client, poweredOnVM.Reference())
 	task, err := vmObj.PowerOn(context.TODO())
 	if err != nil {
 		t.Fatal(err)
@@ -2874,11 +2881,12 @@ func TestExists(t *testing.T) {
 		instanceState string
 		exists        bool
 		vmExists      bool
+		vm            *simulator.VirtualMachine
 	}{
 		{
 			name:          "VM doesn't exist",
 			machinePhase:  "Provisioning",
-			instanceState: string(types.VirtualMachinePowerStatePoweredOn),
+			instanceState: "",
 			exists:        false,
 			vmExists:      false,
 		},
@@ -2888,54 +2896,67 @@ func TestExists(t *testing.T) {
 			instanceState: string(types.VirtualMachinePowerStatePoweredOn),
 			exists:        true,
 			vmExists:      true,
+			vm:            poweredOnVM,
 		},
 		{
 			name:          "VM exists but didnt powered on after clone",
 			machinePhase:  "Provisioning",
 			instanceState: string(types.VirtualMachinePowerStatePoweredOff),
 			exists:        false,
 			vmExists:      true,
+			vm:            poweredOffVM,
 		},
 		{
 			name:          "VM exists, but powered off",
 			machinePhase:  "Provisioned",
 			instanceState: string(types.VirtualMachinePowerStatePoweredOff),
 			exists:        true,
 			vmExists:      true,
+			vm:            poweredOffVM,
 		},
 	}
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			machineScope := machineScope{
-				Context: context.TODO(),
-				machine: &machinev1.Machine{
-					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test",
-						Namespace: "test",
-						Labels: map[string]string{
-							machinev1.MachineClusterIDLabel: "CLUSTERID",
-						},
-					},
-					Status: machinev1.MachineStatus{
-						Phase: &tc.machinePhase,
+
+			var name, uuid string
+			if tc.vm != nil {
+				name = tc.vm.Name
+				uuid = tc.vm.Config.InstanceUuid
+			}
+
+			machineObj := &machinev1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test",
+					Labels: map[string]string{
+						machinev1.MachineClusterIDLabel: "CLUSTERID",
 					},
 				},
+				Status: machinev1.MachineStatus{
+					Phase: &tc.machinePhase,
+				},
+			}
+
+			machineScope := machineScope{
+				Context:            context.TODO(),
+				machine:            machineObj,
+				machineToBePatched: runtimeclient.MergeFrom(machineObj.DeepCopy()),
 				providerSpec: &machinev1.VSphereMachineProviderSpec{
-					Template: vm.Name,
+					Template: name,
 				},
 				session: session,
 				providerStatus: &machinev1.VSphereMachineProviderStatus{
 					TaskRef:       task.Reference().Value,
 					InstanceState: &tc.instanceState,
 				},
-				client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&credentialsSecret).Build(),
+				client: fake.NewClientBuilder().WithScheme(scheme.Scheme).WithRuntimeObjects(&credentialsSecret, machineObj).WithStatusSubresource(machineObj).Build(),
 			}
 
 			reconciler := newReconciler(&machineScope)
 
 			if tc.vmExists {
-				reconciler.machine.UID = apimachinerytypes.UID(instanceUUID)
+				reconciler.machine.UID = apimachinerytypes.UID(uuid)
 			}
 
 			exists, err := reconciler.exists()