NO-ISSUE: Prevent further reconciling after removing AgentMachine finalizer (#214)

When the AgentMachine's finalizer is removed, the
deletionTimestamp is also removed.

Previously, anytime the finalizer is successfully
removed, we allow the reconciliation to continue,
which is problematic since the AgentMachine will
have no indication that it is actually being deleted.
This leads to problems like when scaling down a
NodePool, the AgentMachine controller will unbind
the Agents, but then re-add the AgentMachineRef
label to those Agents later on in the reconcile logic.
The Agents will then not be able to be bound again
unless the AgentMachineRef label is removed manually.

This change ensures that whenever the AgentMachine
has successfully had its finalizer removed that we
end the reconciliation cycle.

Author: CrystalChun

controllers/agentmachine_controller.go

@@ -218,7 +218,7 @@ func (r *AgentMachineReconciler) handleDeletionHook(ctx context.Context, log log
 			log.Error(err)
 			return &ctrl.Result{}, err
 		}
-		return nil, nil
+		return &ctrl.Result{}, nil
 	}
 
 	// return if the machine is not waiting on this hook
@@ -236,7 +236,7 @@ func (r *AgentMachineReconciler) handleDeletionHook(ctx context.Context, log log
 			log.Error(err)
 			return &ctrl.Result{}, err
 		}
-		return nil, nil
+		return &ctrl.Result{}, nil
 	}
 
 	log.Infof("Machine is waiting on delete hook %s", clusterv1.PreTerminateDeleteHookSucceededCondition)
@@ -246,7 +246,7 @@ func (r *AgentMachineReconciler) handleDeletionHook(ctx context.Context, log log
 			log.Error(err)
 			return &ctrl.Result{}, err
 		}
-		return nil, nil
+		return &ctrl.Result{}, nil
 	}
 
 	agent, err := r.getAgent(ctx, log, agentMachine)
@@ -257,7 +257,7 @@ func (r *AgentMachineReconciler) handleDeletionHook(ctx context.Context, log log
 				log.Error(hookErr)
 				return &ctrl.Result{}, hookErr
 			}
-			return nil, nil
+			return &ctrl.Result{}, nil
 		} else {
 			log.WithError(err).Errorf("Failed to get agent %s", agentMachine.Status.AgentRef.Name)
 			return &ctrl.Result{}, err
@@ -294,12 +294,11 @@ func (r *AgentMachineReconciler) handleDeletionHook(ctx context.Context, log log
 			log.Error(err)
 			return &ctrl.Result{}, err
 		}
+		return &ctrl.Result{}, nil
 	} else {
 		log.Infof("Waiting for agent %s to reboot into discovery", agent.Name)
 		return &ctrl.Result{RequeueAfter: 5 * time.Second}, nil
 	}
-
-	return nil, nil
 }
 
 func (r *AgentMachineReconciler) getAgentCluster(ctx context.Context, log logrus.FieldLogger, machine *clusterv1.Machine) (*capiproviderv1.AgentCluster, error) {

controllers/agentmachine_controller_test.go

@@ -585,6 +585,8 @@ var _ = Describe("agentmachine reconcile", func() {
 			Expect(machine.GetAnnotations()).ToNot(HaveKey(machineDeleteHookName))
 			Expect(c.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "agentMachine-1"}, agentMachine)).To(Succeed())
 			Expect(controllerutil.ContainsFinalizer(agentMachine, AgentMachineFinalizerName)).To(BeFalse())
+			Expect(c.Get(ctx, types.NamespacedName{Namespace: agent.Namespace, Name: agent.Name}, agent)).To(Succeed())
+			Expect(agent.Labels).NotTo(HaveKey(AgentMachineRefLabelKey))
 		})
 
 		It("removes the delete hook and finalizer when the agent reaches unbinding pending user action", func() {
@@ -599,6 +601,8 @@ var _ = Describe("agentmachine reconcile", func() {
 			Expect(machine.GetAnnotations()).ToNot(HaveKey(machineDeleteHookName))
 			Expect(c.Get(ctx, types.NamespacedName{Namespace: testNamespace, Name: "agentMachine-1"}, agentMachine)).To(Succeed())
 			Expect(controllerutil.ContainsFinalizer(agentMachine, AgentMachineFinalizerName)).To(BeFalse())
+			Expect(c.Get(ctx, types.NamespacedName{Namespace: agent.Namespace, Name: agent.Name}, agent)).To(Succeed())
+			Expect(agent.Labels).NotTo(HaveKey(AgentMachineRefLabelKey))
 		})
 	})
 	Context("pausing agentmachine", func() {