Change machine actuator to stop removing finalizer. (#366)

The removal of the finalizer should belong in the machine controller, as
the finalizer is inserted at object creation time.

The documentation for the actuator Delete interface is updated to
specify that if no error is returned, it is assumed all resources
dependent on the machine is cleaned up.

Author: k4leung4

cloud/google/machineactuator.go

@@ -354,12 +354,6 @@ func (gce *GCEClient) Delete(cluster *clusterv1.Cluster, machine *clusterv1.Mach
 			"error deleting GCE instance: %v", err))
 	}
 
-	if gce.v1Alpha1Client != nil {
-		// Remove the finalizer
-		machine.ObjectMeta.Finalizers = util.Filter(machine.ObjectMeta.Finalizers, clusterv1.MachineFinalizer)
-		_, err = gce.v1Alpha1Client.Machines(machine.Namespace).Update(machine)
-	}
-
 	return err
 }
 

cloud/vsphere/machineactuator.go

@@ -423,9 +423,8 @@ func (vc *VsphereClient) Delete(cluster *clusterv1.Cluster, machine *clusterv1.M
 
 	vc.cleanUpStagingDir(machine)
 
-	// Update annotation for the state and remove finalizer.
+	// Update annotation for the state.
 	machine.ObjectMeta.Annotations[StatusMachineTerraformState] = ""
-	machine.ObjectMeta.Finalizers = util.Filter(machine.ObjectMeta.Finalizers, clusterv1.MachineFinalizer)
 	_, err = vc.machineClient.Update(machine)
 
 	return err

pkg/controller/machine/actuator.go

@@ -25,7 +25,7 @@ import (
 type Actuator interface {
 	// Create the machine.
 	Create(*clusterv1.Cluster, *clusterv1.Machine) error
-	// Delete the machine.
+	// Delete the machine. If no error is returned, it is assumed that all dependent resources have been cleaned up.
 	Delete(*clusterv1.Cluster, *clusterv1.Machine) error
 	// Update the machine to the provided definition.
 	Update(*clusterv1.Cluster, *clusterv1.Machine) error

pkg/controller/machine/controller.go

@@ -84,13 +84,15 @@ func (c *MachineControllerImpl) Init(arguments sharedinformers.ControllerInitArg
 
 // Reconcile handles enqueued messages. The delete will be handled by finalizer.
 func (c *MachineControllerImpl) Reconcile(machine *clusterv1.Machine) error {
+	// Deep-copy otherwise we are mutating our cache.
+	m := machine.DeepCopy()
 	// Implement controller logic here
-	name := machine.Name
+	name := m.Name
 	glog.Infof("Running reconcile Machine for %s\n", name)
 
-	if !machine.ObjectMeta.DeletionTimestamp.IsZero() {
+	if !m.ObjectMeta.DeletionTimestamp.IsZero() {
 		// no-op if finalizer has been removed.
-		if !util.Contains(machine.ObjectMeta.Finalizers, clusterv1.MachineFinalizer) {
+		if !util.Contains(m.ObjectMeta.Finalizers, clusterv1.MachineFinalizer) {
 			glog.Infof("reconciling machine object %v causes a no-op as there is no finalizer.", name)
 			return nil
 		}
@@ -99,38 +101,38 @@ func (c *MachineControllerImpl) Reconcile(machine *clusterv1.Machine) error {
 			return nil
 		}
 		glog.Infof("reconciling machine object %v triggers delete.", name)
-		if err := c.delete(machine); err != nil {
+		if err := c.delete(m); err != nil {
 			glog.Errorf("Error deleting machine object %v; %v", name, err)
 			return err
 		}
 
 		// Remove finalizer on successful deletion.
 		glog.Infof("machine object %v deletion successful, removing finalizer.", name)
-		machine.ObjectMeta.Finalizers = util.Filter(machine.ObjectMeta.Finalizers, clusterv1.MachineFinalizer)
-		if _, err := c.clientSet.ClusterV1alpha1().Machines(machine.Namespace).Update(machine); err != nil {
+		m.ObjectMeta.Finalizers = util.Filter(m.ObjectMeta.Finalizers, clusterv1.MachineFinalizer)
+		if _, err := c.clientSet.ClusterV1alpha1().Machines(m.Namespace).Update(m); err != nil {
 			glog.Errorf("Error removing finalizer from machine object %v; %v", name, err)
 			return err
 		}
 		return nil
 	}
 
-	cluster, err := c.getCluster(machine)
+	cluster, err := c.getCluster(m)
 	if err != nil {
 		return err
 	}
 
-	exist, err := c.actuator.Exists(cluster, machine)
+	exist, err := c.actuator.Exists(cluster, m)
 	if err != nil {
 		glog.Errorf("Error checking existance of machine instance for machine object %v; %v", name, err)
 		return err
 	}
 	if exist {
 		glog.Infof("reconciling machine object %v triggers idempotent update.", name)
-		return c.update(machine)
+		return c.update(m)
 	}
 	// Machine resource created. Machine does not yet exist.
-	glog.Infof("reconciling machine object %v triggers idempotent create.", machine.ObjectMeta.Name)
-	return c.create(machine)
+	glog.Infof("reconciling machine object %v triggers idempotent create.", m.ObjectMeta.Name)
+	return c.create(m)
 }
 
 func (c *MachineControllerImpl) Get(namespace, name string) (*clusterv1.Machine, error) {

pkg/controller/machine/controller_test.go

@@ -26,10 +26,10 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"sigs.k8s.io/cluster-api/pkg/client/clientset_generated/clientset/typed/cluster/v1alpha1"
 
+	"k8s.io/client-go/tools/cache"
 	clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	"sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1/testutil"
 	"sigs.k8s.io/cluster-api/pkg/client/clientset_generated/clientset"
-	"k8s.io/client-go/tools/cache"
 )
 
 func machineControllerReconcile(t *testing.T, cs *clientset.Clientset, controller *MachineController, namespace string) {

pkg/controller/machine/reconcile_test.go

@@ -32,7 +32,7 @@ import (
 	"sigs.k8s.io/cluster-api/pkg/util"
 )
 
-func TestMachineSetControllerReconcileHandler(t *testing.T) {
+func TestMachineControllerReconcileHandler(t *testing.T) {
 	tests := []struct {
 		name                   string
 		objExists              bool
@@ -160,7 +160,14 @@ func TestMachineSetControllerReconcileHandler(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			finalizerRemoved := machineUpdated && !util.Contains(machineToTest.ObjectMeta.Finalizers, v1alpha1.MachineFinalizer)
+			finalizerRemoved := false
+			if machineUpdated {
+				updatedMachine, err := fakeClient.ClusterV1alpha1().Machines(machineToTest.Namespace).Get(machineToTest.Name, metav1.GetOptions{})
+				if err != nil {
+					t.Fatalf("failed to get updated machine.")
+				}
+				finalizerRemoved = !util.Contains(updatedMachine.ObjectMeta.Finalizers, v1alpha1.MachineFinalizer)
+			}
 
 			if finalizerRemoved != test.expectFinalizerRemoved {
 				t.Errorf("Got finalizer removed %v, expected finalizer removed %v", finalizerRemoved, test.expectFinalizerRemoved)