Refactored code to fix unit tests

pkg/cmiclient/cmi-client.go

@@ -42,8 +42,8 @@ type VMs map[string]string
 // CMIClient is the client used to communicate with the CMIServer
 type CMIClient interface {
 	CreateMachine() (string, string, string, error)
-	GetMachineStatus() (string, string, string, error)
 	DeleteMachine() (string, error)
+	GetMachineStatus() (string, string, string, error)
 	ListMachines() (map[string]string, error)
 	ShutDownMachine() error
 	GetProviderID() string

pkg/cmiclient/fake-cmi-client.go

@@ -18,42 +18,54 @@ limitations under the License.
 package cmiclient
 
 import (
+	"fmt"
+
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	corev1 "k8s.io/api/core/v1"
 )
 
 // FakeCMIClient is a fake driver returned when none of the actual drivers match
 type FakeCMIClient struct {
-	MachineID string
-	NodeName  string
-	Err       error
+	VMExists       bool
+	ProviderID     string
+	NodeName       string
+	LastKnownState string
+	Err            error
 }
 
 // NewFakeCMIClient returns a new fakedriver object
-func NewFakeCMIClient(machineID string, nodeName string, err error) *FakeCMIClient {
-	return &FakeCMIClient{
-		MachineID: machineID,
-		NodeName:  nodeName,
-		Err:       err,
-	}
+func NewFakeCMIClient(fakeCMIClient *FakeCMIClient) *FakeCMIClient {
+	return fakeCMIClient
 }
 
 // CreateMachine makes a gRPC call to the driver to create the machine.
 func (c *FakeCMIClient) CreateMachine() (string, string, string, error) {
-	return c.MachineID, c.NodeName, "", c.Err
+	if c.Err == nil {
+		c.VMExists = true
+		return c.ProviderID, c.NodeName, c.LastKnownState, c.Err
+	}
+
+	return "", "", "", c.Err
 }
 
 // DeleteMachine make a grpc call to the driver to delete the machine.
 func (c *FakeCMIClient) DeleteMachine() (string, error) {
-	return "", c.Err
+	c.VMExists = false
+	return c.LastKnownState, c.Err
 }
 
 // GetMachineStatus makes a gRPC call to the driver to check existance of machine
 func (c *FakeCMIClient) GetMachineStatus() (string, string, string, error) {
-	if c.Err == nil {
-		return "", "", "", nil
+	switch {
+	case !c.VMExists:
+		errMessage := fmt.Sprintf("Fake plugin is returning no VM instances backing this machine object")
+		return "", "", "", status.Error(codes.NotFound, errMessage)
+	case c.Err != nil:
+		return "", "", "", c.Err
 	}
 
-	return "", "", "", c.Err
+	return c.ProviderID, c.NodeName, c.LastKnownState, nil
 }
 
 // ListMachines have to list machines
@@ -69,7 +81,7 @@ func (c *FakeCMIClient) ShutDownMachine() error {
 
 // GetProviderID returns the GetProviderID
 func (c *FakeCMIClient) GetProviderID() string {
-	return c.MachineID
+	return c.ProviderID
 }
 
 // GetVolumeIDs returns a list of VolumeIDs for the PV spec list supplied

pkg/controller/controller_suite_test.go

@@ -35,6 +35,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/watch"
 	coreinformers "k8s.io/client-go/informers"
 	v1core "k8s.io/client-go/kubernetes/typed/core/v1"
@@ -245,8 +246,9 @@ func newMachineFromMachineSet(
 	statusTemplate *v1alpha1.MachineStatus,
 	annotations map[string]string,
 	labels map[string]string,
+	addFinalizer bool,
 ) *v1alpha1.Machine {
-	return newMachinesFromMachineSet(1, machineSet, statusTemplate, annotations, labels)[0]
+	return newMachinesFromMachineSet(1, machineSet, statusTemplate, annotations, labels, addFinalizer)[0]
 }
 
 func newMachinesFromMachineSet(
@@ -255,6 +257,7 @@ func newMachinesFromMachineSet(
 	statusTemplate *v1alpha1.MachineStatus,
 	annotations map[string]string,
 	labels map[string]string,
+	addFinalizer bool,
 ) []*v1alpha1.Machine {
 	t := &machineSet.TypeMeta
 
@@ -280,6 +283,7 @@ func newMachinesFromMachineSet(
 		},
 		annotations,
 		finalLabels,
+		addFinalizer,
 	)
 }
 
@@ -289,8 +293,9 @@ func newMachine(
 	owner *metav1.OwnerReference,
 	annotations map[string]string,
 	labels map[string]string,
+	addFinalizer bool,
 ) *v1alpha1.Machine {
-	return newMachines(1, specTemplate, statusTemplate, owner, annotations, labels)[0]
+	return newMachines(1, specTemplate, statusTemplate, owner, annotations, labels, addFinalizer)[0]
 }
 
 func newMachines(
@@ -300,6 +305,7 @@ func newMachines(
 	owner *metav1.OwnerReference,
 	annotations map[string]string,
 	labels map[string]string,
+	addFinalizer bool,
 ) []*v1alpha1.Machine {
 	machines := make([]*v1alpha1.Machine, machineCount)
 
@@ -324,6 +330,12 @@ func newMachines(
 			},
 			Spec: *newMachineSpec(&specTemplate.Spec, i),
 		}
+		finalizers := sets.NewString(m.Finalizers...)
+
+		if addFinalizer {
+			finalizers.Insert(DeleteFinalizerName)
+		}
+		m.Finalizers = finalizers.List()
 
 		if statusTemplate != nil {
 			m.Status = *newMachineStatus(statusTemplate, i)
@@ -571,7 +583,7 @@ var _ = Describe("#createController", func() {
 	It("success", func() {
 		machine0 := newMachine(&v1alpha1.MachineTemplateSpec{
 			ObjectMeta: *objMeta,
-		}, nil, nil, nil, nil)
+		}, nil, nil, nil, nil, false)
 
 		stop := make(chan struct{})
 		defer close(stop)

pkg/controller/deployment_rollback_test.go

@@ -133,6 +133,7 @@ var _ = Describe("deployment_rollback", func() {
 						},
 						nil,
 						nil,
+						true,
 					),
 					nodes: newNodes(
 						1,

pkg/controller/deployment_rolling_test.go

@@ -131,6 +131,7 @@ var _ = Describe("deployment_rolling", func() {
 						},
 						nil,
 						nil,
+						true,
 					),
 					nodes: newNodes(
 						1,

pkg/controller/machine.go

@@ -185,12 +185,6 @@ func (c *controller) reconcileClusterMachine(machine *v1alpha1.Machine) (bool, e
 		return c.triggerDeletionFlow(machine, driver)
 	}
 
-	// Add finalizers if not present
-	retry, err := c.addMachineFinalizers(machine)
-	if err != nil {
-		return retry, err
-	}
-
 	if machine.Status.Node != "" {
 		// If reference to node object exists execute the below
 
@@ -313,6 +307,13 @@ func (c *controller) triggerCreationFlow(machine *v1alpha1.Machine, driver cmicl
 		err            error
 	)
 
+	// Add finalizers if not present
+	retry, err := c.addMachineFinalizers(machine)
+	if err != nil {
+		return retry, err
+	}
+
+	//
 	providerID, nodeName, lastKnownState, err = driver.GetMachineStatus()
 	if err == nil {
 		// Found VM with required machine name
@@ -387,7 +388,7 @@ func (c *controller) triggerCreationFlow(machine *v1alpha1.Machine, driver cmicl
 			glog.V(2).Infof("Machine labels/annotations UPDATE for %q", machine.Name)
 
 			// Return error even when machine object is updated
-			err = fmt.Errorf("Machine reconcilation in process")
+			err = fmt.Errorf("Machine creation in process. Machine UPDATE successful")
 		}
 		return RetryOp, err
 	}
@@ -416,7 +417,7 @@ func (c *controller) triggerCreationFlow(machine *v1alpha1.Machine, driver cmicl
 			glog.V(2).Infof("Machine/status UPDATE for %q during creation", machine.Name)
 
 			// Return error even when machine object is updated
-			err = fmt.Errorf("Machine reconcilation in process")
+			err = fmt.Errorf("Machine creation in process. Machine/Status UPDATE successful")
 		}
 
 		return RetryOp, err
@@ -464,18 +465,18 @@ func (c *controller) triggerUpdationFlow(machine *v1alpha1.Machine, actualProvid
 }
 
 func (c *controller) triggerDeletionFlow(machine *v1alpha1.Machine, driver cmiclient.CMIClient) (bool, error) {
-	if finalizers := sets.NewString(machine.Finalizers...); !finalizers.Has(DeleteFinalizerName) {
+	finalizers := sets.NewString(machine.Finalizers...)
+
+	switch {
+	case !finalizers.Has(DeleteFinalizerName):
 		// If Finalizers are not present on machine
 		err := fmt.Errorf("Machine %q is missing finalizers. Deletion cannot proceed", machine.Name)
-		glog.Error(err)
 		return DoNotRetryOp, err
-	}
 
-	switch {
 	case machine.Status.CurrentStatus.Phase != v1alpha1.MachineTerminating:
 		return c.setMachineTerminationStatus(machine, driver)
 
-	case machine.Status.LastOperation.Description == getVMStatus:
+	case strings.Contains(machine.Status.LastOperation.Description, getVMStatus):
 		return c.getVMStatus(machine, driver)
 
 	case strings.Contains(machine.Status.LastOperation.Description, initiateDrain):
@@ -495,19 +496,15 @@ func (c *controller) triggerDeletionFlow(machine *v1alpha1.Machine, driver cmicl
 			return RetryOp, err
 		}
 
-	case strings.Contains(machine.Status.LastOperation.Description, permanentError):
-		err := fmt.Errorf("Permanent Error has occurred for machine %q: \t%s", machine.Name, machine.Status.LastOperation.Description)
+	default:
+		err := fmt.Errorf("Unable to decode deletion flow state for machine %s", machine.Name)
 		glog.Error(err)
 		return DoNotRetryOp, err
-
-	default:
-		glog.Errorf("Unable to decode deletion flow state for machine %q", machine.Name)
-		return DoNotRetryOp, nil
 	}
 
 	/*
 		// Delete machine object
-		err = c.controlMachineClient.Machines(machine.Namespace).Delete(machine.Name, &metav1.DeleteOptions{})
+		err := c.controlMachineClient.Machines(machine.Namespace).Delete(machine.Name, &metav1.DeleteOptions{})
 		if err != nil && !apierrors.IsNotFound(err) {
 			// If its an error, and anyother error than object not found
 			glog.Errorf("Deletion of Machine Object %q failed due to error: %s", machine.Name, err)

pkg/controller/machine_safety_test.go

@@ -206,6 +206,7 @@ var _ = Describe("#machine_safety", func() {
 				&v1alpha1.MachineStatus{},
 				nil,
 				nil,
+				true,
 			)
 
 			controlMachineObjects := []runtime.Object{}