Merge pull request #201 from wanyufe/bugfix-finalizer

Add finalizer only when deployVM no error

Author: maxdrib

controllers/cloudstackmachine_controller.go

@@ -223,11 +223,14 @@ func (r *CloudStackMachineReconciliationRunner) GetOrCreateVMInstance() (retRes
 		r.Recorder.Eventf(r.ReconciliationSubject, "Warning", "Creating", CSMachineCreationFailed, err.Error())
 	}
 	if err == nil && !controllerutil.ContainsFinalizer(r.ReconciliationSubject, infrav1.MachineFinalizer) { // Fetched or Created?
+		// Adding a finalizer will make reconcile-delete try to destroy the associated VM through instanceID.
+		// If err is not nil, it means CAPC could not get an associated VM through instanceID or name, so we should not add a finalizer to this CloudStackMachine,
+		// Otherwise, reconcile-delete will be stuck trying to wait for instanceID to be available.
+		controllerutil.AddFinalizer(r.ReconciliationSubject, infrav1.MachineFinalizer)
 		r.Recorder.Eventf(r.ReconciliationSubject, "Normal", "Created", CSMachineCreationSuccess)
 		r.Log.Info(CSMachineCreationSuccess, "instanceStatus", r.ReconciliationSubject.Status)
 	}
-	// Always add the finalizer regardless. It can't be added twice anyway.
-	controllerutil.AddFinalizer(r.ReconciliationSubject, infrav1.MachineFinalizer)
+
 	return ctrl.Result{}, err
 }
 

controllers/cloudstackmachine_controller_test.go

@@ -72,7 +72,7 @@ var _ = Describe("CloudStackMachineReconciler", func() {
 				key := client.ObjectKey{Namespace: dummies.ClusterNameSpace, Name: dummies.CSMachine1.Name}
 				if err := k8sClient.Get(ctx, key, tempMachine); err == nil {
 					if tempMachine.Status.Ready == true {
-						return true
+						return len(tempMachine.ObjectMeta.Finalizers) > 0
 					}
 				}
 				return false

pkg/cloud/instance.go

@@ -275,15 +275,17 @@ func (c *client) GetOrCreateVMInstance(
 		listVirtualMachineParams.SetZoneid(fd.Spec.Zone.ID)
 		listVirtualMachineParams.SetNetworkid(fd.Spec.Zone.Network.ID)
 		listVirtualMachineParams.SetName(csMachine.Name)
-		if listVirtualMachinesResponse, err2 := c.cs.VirtualMachine.ListVirtualMachines(listVirtualMachineParams); err2 == nil && listVirtualMachinesResponse.Count > 0 {
-			csMachine.Spec.InstanceID = pointer.StringPtr(listVirtualMachinesResponse.VirtualMachines[0].Id)
-		} else {
+		listVirtualMachinesResponse, err2 := c.cs.VirtualMachine.ListVirtualMachines(listVirtualMachineParams)
+		if err2 != nil || listVirtualMachinesResponse.Count <= 0 {
 			c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err2)
+			return err
 		}
-		return err
+		csMachine.Spec.InstanceID = pointer.StringPtr(listVirtualMachinesResponse.VirtualMachines[0].Id)
+		csMachine.Status.InstanceState = listVirtualMachinesResponse.VirtualMachines[0].State
+	} else {
+		csMachine.Spec.InstanceID = pointer.StringPtr(deployVMResp.Id)
+		csMachine.Status.Status = pointer.String(metav1.StatusSuccess)
 	}
-	csMachine.Spec.InstanceID = pointer.StringPtr(deployVMResp.Id)
-	csMachine.Status.Status = pointer.String(metav1.StatusSuccess)
 	// Resolve uses a VM metrics request response to fill cloudstack machine status.
 	// The deployment response is insufficient.
 	return c.ResolveVMInstanceDetails(csMachine)