Cleanup Machine controller (kubernetes-sigs#763)

Signed-off-by: Vince Prignano <[email protected]>

pkg/controller/machine/controller.go

@@ -57,7 +57,7 @@ func newReconciler(mgr manager.Manager, actuator Actuator) reconcile.Reconciler
 	}
 
 	if r.nodeName == "" {
-		klog.Warningf("environment variable %v is not set, this controller will not protect against deleting its own machine", NodeNameEnvVar)
+		klog.Warningf("Environment variable %q is not set, this controller will not protect against deleting its own machine", NodeNameEnvVar)
 	}
 
 	return r
@@ -72,16 +72,12 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 	}
 
 	// Watch for changes to Machine
-	err = c.Watch(&source.Kind{Type: &clusterv1.Machine{}}, &handler.EnqueueRequestForObject{})
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return c.Watch(
+		&source.Kind{Type: &clusterv1.Machine{}},
+		&handler.EnqueueRequestForObject{},
+	)
 }
 
-var _ reconcile.Reconciler = &ReconcileMachine{}
-
 // ReconcileMachine reconciles a Machine object
 type ReconcileMachine struct {
 	client.Client
@@ -99,6 +95,7 @@ type ReconcileMachine struct {
 func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Result, error) {
 	// TODO(mvladev): Can context be passed from Kubebuilder?
 	ctx := context.TODO()
+
 	// Fetch the Machine instance
 	m := &clusterv1.Machine{}
 	if err := r.Client.Get(ctx, request.NamespacedName, m); err != nil {
@@ -107,13 +104,14 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul
 			// For additional cleanup logic use finalizers.
 			return reconcile.Result{}, nil
 		}
+
 		// Error reading the object - requeue the request.
 		return reconcile.Result{}, err
 	}
 
 	// Implement controller logic here
 	name := m.Name
-	klog.Infof("Running reconcile Machine for %s\n", name)
+	klog.Infof("Reconciling Machine %q", name)
 
 	// Cluster might be nil as some providers might not require a cluster object
 	// for machine management.
@@ -149,7 +147,7 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul
 
 		if len(m.Finalizers) > finalizerCount {
 			if err := r.Client.Update(ctx, m); err != nil {
-				klog.Infof("failed to add finalizers to machine object %v due to error %v.", name, err)
+				klog.Infof("Failed to add finalizers to machine %q: %v", name, err)
 				return reconcile.Result{}, err
 			}
 
@@ -160,63 +158,67 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul
 
 	if !m.ObjectMeta.DeletionTimestamp.IsZero() {
 		// no-op if finalizer has been removed.
-		if !util.Contains(m.Finalizers, clusterv1.MachineFinalizer) {
-			klog.Infof("reconciling machine object %v causes a no-op as there is no finalizer.", name)
+		if !util.Contains(m.ObjectMeta.Finalizers, clusterv1.MachineFinalizer) {
+			klog.Infof("Reconciling machine %q causes a no-op as there is no finalizer", name)
 			return reconcile.Result{}, nil
 		}
 
 		if !r.isDeleteAllowed(m) {
-			klog.Infof("Skipping reconciling of machine object %v", name)
+			klog.Infof("Skipping reconciling of machine %q", name)
 			return reconcile.Result{}, nil
 		}
 
-		klog.Infof("reconciling machine object %v triggers delete.", name)
+		klog.Infof("Reconciling machine %q triggers delete", name)
 		if err := r.actuator.Delete(ctx, cluster, m); err != nil {
-			klog.Errorf("Error deleting machine object %v; %v", name, err)
 			if requeueErr, ok := err.(*controllerError.RequeueAfterError); ok {
 				klog.Infof("Actuator returned requeue-after error: %v", requeueErr)
 				return reconcile.Result{Requeue: true, RequeueAfter: requeueErr.RequeueAfter}, nil
 			}
+
+			klog.Errorf("Failed to delete machine %q: %v", name, err)
 			return reconcile.Result{}, err
 		}
 
 		// Remove finalizer on successful deletion.
-		klog.Infof("machine object %v deletion successful, removing finalizer.", name)
-		m.Finalizers = util.Filter(m.Finalizers, clusterv1.MachineFinalizer)
+		m.ObjectMeta.Finalizers = util.Filter(m.ObjectMeta.Finalizers, clusterv1.MachineFinalizer)
 		if err := r.Client.Update(context.Background(), m); err != nil {
-			klog.Errorf("Error removing finalizer from machine object %v; %v", name, err)
+			klog.Errorf("Failed to remove finalizer from machine %q: %v", name, err)
 			return reconcile.Result{}, err
 		}
 
+		klog.Infof("Machine %q deletion successful", name)
 		return reconcile.Result{}, nil
 	}
 
 	exist, err := r.actuator.Exists(ctx, cluster, m)
 	if err != nil {
-		klog.Errorf("Error checking existence of machine instance for machine object %v; %v", name, err)
+		klog.Errorf("Failed to check if machine %q exists: %v", name, err)
 		return reconcile.Result{}, err
 	}
 
 	if exist {
-		klog.Infof("Reconciling machine object %v triggers idempotent update.", name)
+		klog.Infof("Reconciling machine %q triggers idempotent update", name)
 		if err := r.actuator.Update(ctx, cluster, m); err != nil {
 			if requeueErr, ok := err.(*controllerError.RequeueAfterError); ok {
 				klog.Infof("Actuator returned requeue-after error: %v", requeueErr)
 				return reconcile.Result{Requeue: true, RequeueAfter: requeueErr.RequeueAfter}, nil
 			}
+
 			return reconcile.Result{}, err
 		}
+
 		return reconcile.Result{}, nil
 	}
 
 	// Machine resource created. Machine does not yet exist.
 	klog.Infof("Reconciling machine object %v triggers idempotent create.", m.ObjectMeta.Name)
 	if err := r.actuator.Create(ctx, cluster, m); err != nil {
-		klog.Warningf("unable to create machine %v: %v", name, err)
 		if requeueErr, ok := err.(*controllerError.RequeueAfterError); ok {
 			klog.Infof("Actuator returned requeue-after error: %v", requeueErr)
 			return reconcile.Result{Requeue: true, RequeueAfter: requeueErr.RequeueAfter}, nil
 		}
+
+		klog.Warningf("Failed to create machine %q: %v", name, err)
 		return reconcile.Result{}, err
 	}
 
@@ -246,15 +248,17 @@ func (r *ReconcileMachine) isDeleteAllowed(machine *clusterv1.Machine) bool {
 	if r.nodeName == "" || machine.Status.NodeRef == nil {
 		return true
 	}
+
 	if machine.Status.NodeRef.Name != r.nodeName {
 		return true
 	}
+
 	node := &corev1.Node{}
-	err := r.Client.Get(context.Background(), client.ObjectKey{Name: r.nodeName}, node)
-	if err != nil {
-		klog.Infof("unable to determine if controller's node is associated with machine '%v', error getting node named '%v': %v", machine.Name, r.nodeName, err)
+	if err := r.Client.Get(context.Background(), client.ObjectKey{Name: r.nodeName}, node); err != nil {
+		klog.Infof("Failed to determine if controller's node %q is associated with machine %q: %v", r.nodeName, machine.Name, err)
 		return true
 	}
+
 	// When the UID of the machine's node reference and this controller's actual node match then then the request is to
 	// delete the machine this machine-controller is running on. Return false to not allow machine controller to delete its
 	// own machine.

pkg/controller/machine/controller_test.go

@@ -28,6 +28,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
+var (
+	_ reconcile.Reconciler = &ReconcileMachine{}
+)
+
 func TestReconcileRequest(t *testing.T) {
 	machine1 := v1alpha1.Machine{
 		TypeMeta: metav1.TypeMeta{