diff --git a/controllers/cloudstackmachine_controller.go b/controllers/cloudstackmachine_controller.go index bafcd286..d9618cfb 100644 --- a/controllers/cloudstackmachine_controller.go +++ b/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 } diff --git a/controllers/cloudstackmachine_controller_test.go b/controllers/cloudstackmachine_controller_test.go index ab3d4b6e..36f17416 100644 --- a/controllers/cloudstackmachine_controller_test.go +++ b/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 diff --git a/pkg/cloud/instance.go b/pkg/cloud/instance.go index a83c73da..dbda5e98 100644 --- a/pkg/cloud/instance.go +++ b/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)