Improve machine creation and deletion flow by listing the machines before actual creation/deletion (#432)

* Improve machine creation and deletion flow by verifying the VMs before actually creating or deleting the machine.

* Added unit-tests

* Address review comments

* Update pkg/controller/machine.go

Co-authored-by: Prashanth <prashanth@sap.com>

Author: hardikdr

pkg/controller/machine.go

@@ -266,14 +266,39 @@ func (c *controller) getMachineFromNode(nodeName string) (*v1alpha1.Machine, err
 }
 
 func (c *controller) updateMachineState(machine *v1alpha1.Machine) (*v1alpha1.Machine, error) {
+	nodeName := machine.Status.Node
 
-	if machine.Status.Node == "" {
-		// There are no objects mapped to this machine object
-		// Hence node status need not be propogated to machine object
-		return machine, nil
+	if nodeName == "" {
+		// Check if any existing node-object can be adopted.
+		nodeList, err := c.nodeLister.List(labels.Everything())
+		if err != nil {
+			klog.Errorf("Could not list the nodes due to error: %v", err)
+			return machine, err
+		}
+		for _, node := range nodeList {
+			nID, mID := decodeMachineID(node.Spec.ProviderID), decodeMachineID(machine.Spec.ProviderID)
+			if nID == mID {
+				klog.V(2).Infof("Adopting the node object %s for machine %s", node.Name, machine.Name)
+				nodeName = node.Name
+				clone := machine.DeepCopy()
+				clone.Status.Node = nodeName
+				clone, err = c.controlMachineClient.Machines(clone.Namespace).UpdateStatus(clone)
+				if err != nil {
+					klog.Errorf("Could not update status of the machine-object %s due to error %v", machine.Name, err)
+					return machine, err
+				}
+				break
+			}
+		}
+		// Couldnt adopt any node-object.
+		if nodeName == "" {
+			// There are no objects mapped to this machine object
+			// Hence node status need not be propogated to machine object
+			return machine, nil
+		}
 	}
 
-	node, err := c.nodeLister.Get(machine.Status.Node)
+	node, err := c.nodeLister.Get(nodeName)
 	if err != nil && apierrors.IsNotFound(err) {
 		// Node object is not found
 
@@ -325,7 +350,7 @@ func (c *controller) updateMachineState(machine *v1alpha1.Machine) (*v1alpha1.Ma
 		clone.Labels = make(map[string]string)
 	}
 
-	if _, ok := clone.Labels["node"]; !ok {
+	if n := clone.Labels["node"]; n == "" {
 		clone.Labels["node"] = machine.Status.Node
 		machine, err = c.controlMachineClient.Machines(clone.Namespace).Update(clone)
 		if err != nil {
@@ -344,6 +369,7 @@ func (c *controller) updateMachineState(machine *v1alpha1.Machine) (*v1alpha1.Ma
 
 func (c *controller) machineCreate(machine *v1alpha1.Machine, driver driver.Driver) error {
 	klog.V(2).Infof("Creating machine %q, please wait!", machine.Name)
+	var actualProviderID, nodeName string
 
 	err := c.addBootstrapTokenToUserData(machine.Name, driver)
 	if err != nil {
@@ -362,7 +388,22 @@ func (c *controller) machineCreate(machine *v1alpha1.Machine, driver driver.Driv
 		c.updateMachineStatus(machine, lastOperation, currentStatus)
 		return err
 	}
-	actualProviderID, nodeName, err := driver.Create()
+	// Before actually creating the machine, we should once check and adopt if the virtual machine already exists.
+	VMList, err := driver.GetVMs("")
+	if err != nil {
+		klog.Errorf("Failed to list VMs before creating machine %q %+v", machine.Name, err)
+		return err
+	}
+	for providerID, machineName := range VMList {
+		if machineName == machine.Name {
+			klog.V(2).Infof("Adopted an existing VM %s for machine object %s.", providerID, machineName)
+			actualProviderID = providerID
+		}
+	}
+	if actualProviderID == "" {
+		actualProviderID, nodeName, err = driver.Create()
+	}
+
 	if err != nil {
 		klog.Errorf("Error while creating machine %s: %s", machine.Name, err.Error())
 		lastOperation := v1alpha1.LastOperation{
@@ -385,7 +426,7 @@ func (c *controller) machineCreate(machine *v1alpha1.Machine, driver driver.Driv
 
 		return err
 	}
-	klog.V(2).Infof("Created machine: %q, MachineID: %s", machine.Name, actualProviderID)
+	klog.V(2).Infof("Created/Adopted machine: %q, MachineID: %s", machine.Name, actualProviderID)
 
 	for {
 		machineName := machine.Name
@@ -614,6 +655,27 @@ func (c *controller) machineDelete(machine *v1alpha1.Machine, driver driver.Driv
 			}
 
 			err = driver.Delete(machineID)
+		} else {
+			klog.V(2).Infof("Machine %q on deletion doesn't have a providerID attached to it. Checking for any VM linked to this machine object.", machine.Name)
+			// As MachineID is missing, we should check once if actual VM was created but MachineID was not updated on machine-object.
+			// We list VMs and check if any one them map with the given machine-object.
+			VMList, err := driver.GetVMs("")
+			if err != nil {
+				klog.Errorf("Failed to list VMs while deleting the machine %q %v", machine.Name, err)
+				return err
+			}
+			for providerID, machineName := range VMList {
+				if machineName == machine.Name {
+					klog.V(2).Infof("Deleting the VM %s backing the machine-object %s.", providerID, machine.Name)
+					err = driver.Delete(providerID)
+					if err != nil {
+						klog.Errorf("Error deleting the VM %s backing the machine-object %s due to error %v", providerID, machine.Name, err)
+						// Not returning error so that status on the machine object can be updated in the next step if errored.
+					} else {
+						klog.V(2).Infof("VM %s backing the machine-object %s is deleted successfully", providerID, machine.Name)
+					}
+				}
+			}
 		}
 
 		if err != nil {
@@ -952,3 +1014,9 @@ func shouldReconcileMachine(machine *v1alpha1.Machine, now time.Time) bool {
 
 	return true
 }
+
+// decodeMachineID is a generic way of decoding the Spec.ProviderID field of node-objects.
+func decodeMachineID(id string) string {
+	splitProviderID := strings.Split(id, "/")
+	return splitProviderID[len(splitProviderID)-1]
+}

pkg/controller/machine_test.go

@@ -511,7 +511,11 @@ var _ = Describe("machine", func() {
 					},
 					func(string) error {
 						return action.fakeError
-					}, nil))
+					}, nil,
+					func() (driver.VMs, error) {
+						return map[string]string{}, nil
+					},
+				))
 
 				if data.expect.err {
 					Expect(err).To(HaveOccurred())
@@ -733,6 +737,67 @@ var _ = Describe("machine", func() {
 					err: true,
 				},
 			}),
+			Entry("If ProviderID is available and node-name missing, ProviderID should be set back on machine object", &data{
+				setup: setup{
+					secrets: []*corev1.Secret{
+						{
+							ObjectMeta: *newObjectMeta(objMeta, 0),
+						},
+					},
+					aws: []*machinev1.AWSMachineClass{
+						{
+							ObjectMeta: *newObjectMeta(objMeta, 0),
+							Spec: machinev1.AWSMachineClassSpec{
+								SecretRef: newSecretReference(objMeta, 0),
+							},
+						},
+					},
+					machines: newMachines(1, &machinev1.MachineTemplateSpec{
+						ObjectMeta: *newObjectMeta(objMeta, 0),
+						Spec: machinev1.MachineSpec{
+							Class: machinev1.ClassSpec{
+								Kind: "AWSMachineClass",
+								Name: "machine-0",
+							},
+						},
+					}, nil, nil, nil, nil),
+					fakeResourceActions: &customfake.ResourceActions{
+						Machine: customfake.Actions{
+							Get: apierrors.NewGenericServerResponse(
+								http.StatusBadRequest,
+								"dummy method",
+								schema.GroupResource{},
+								"dummy name",
+								"Failed to GET machine",
+								30,
+								true,
+							),
+						},
+					},
+				},
+				action: action{
+					machine:        "machine-0",
+					fakeProviderID: "providerid-0",
+					fakeNodeName:   "",
+					fakeError:      nil,
+				},
+				expect: expect{
+					machine: newMachine(&machinev1.MachineTemplateSpec{
+						ObjectMeta: *newObjectMeta(objMeta, 0),
+						Spec: machinev1.MachineSpec{
+							Class: machinev1.ClassSpec{
+								Kind: "AWSMachineClass",
+								Name: "machine-0",
+							},
+							ProviderID: "providerid",
+						},
+					}, &machinev1.MachineStatus{
+						Node: "",
+						//TODO conditions
+					}, nil, nil, nil),
+					err: false,
+				},
+			}),
 		)
 	})
 
@@ -747,6 +812,7 @@ var _ = Describe("machine", func() {
 			machine                 string
 			fakeProviderID          string
 			fakeNodeName            string
+			fakeDriverGetVMs        func() (driver.VMs, error)
 			fakeError               error
 			forceDeleteLabelPresent bool
 			fakeMachineStatus       *machinev1.MachineStatus
@@ -791,6 +857,12 @@ var _ = Describe("machine", func() {
 				machine, err := controller.controlMachineClient.Machines(objMeta.Namespace).Get(action.machine, metav1.GetOptions{})
 				Expect(err).ToNot(HaveOccurred())
 
+				fakeDriverGetVMsTemp := func() (driver.VMs, error) { return nil, nil }
+
+				if action.fakeDriverGetVMs != nil {
+					fakeDriverGetVMsTemp = action.fakeDriverGetVMs
+				}
+
 				fakeDriver := driver.NewFakeDriver(
 					func() (string, string, error) {
 						_, err := controller.targetCoreClient.CoreV1().Nodes().Create(&v1.Node{
@@ -809,6 +881,7 @@ var _ = Describe("machine", func() {
 					func() (string, error) {
 						return action.fakeProviderID, action.fakeError
 					},
+					fakeDriverGetVMsTemp,
 				)
 
 				// Create a machine that is to be deleted later
@@ -899,6 +972,45 @@ var _ = Describe("machine", func() {
 					machineDeleted: true,
 				},
 			}),
+			Entry("Allow proper deletion of the machine object when providerID is missing but actual VM still exists in cloud", &data{
+				setup: setup{
+					secrets: []*corev1.Secret{
+						{
+							ObjectMeta: *newObjectMeta(objMeta, 0),
+						},
+					},
+					aws: []*machinev1.AWSMachineClass{
+						{
+							ObjectMeta: *newObjectMeta(objMeta, 0),
+							Spec: machinev1.AWSMachineClassSpec{
+								SecretRef: newSecretReference(objMeta, 0),
+							},
+						},
+					},
+					machines: newMachines(1, &machinev1.MachineTemplateSpec{
+						ObjectMeta: *newObjectMeta(objMeta, 0),
+						Spec: machinev1.MachineSpec{
+							Class: machinev1.ClassSpec{
+								Kind: "AWSMachineClass",
+								Name: "machine-0",
+							},
+						},
+					}, nil, nil, nil, nil),
+				},
+				action: action{
+					machine: "machine-0",
+					//fakeProviderID: "fakeID-0",
+					fakeNodeName: "fakeNode-0",
+					fakeError:    nil,
+					fakeDriverGetVMs: func() (driver.VMs, error) {
+						return map[string]string{"fakeID-0": "machine-0"}, nil
+					},
+				},
+				expect: expect{
+					errOccurred:    false,
+					machineDeleted: true,
+				},
+			}),
 			Entry("Machine deletion when drain fails", &data{
 				setup: setup{
 					secrets: []*corev1.Secret{
@@ -1422,7 +1534,7 @@ var _ = Describe("machine", func() {
 
 		machineName := "machine-0"
 
-		DescribeTable("##Different machine state update scenrios",
+		DescribeTable("##Different machine state update scenarios",
 			func(data *data) {
 				stop := make(chan struct{})
 				defer close(stop)
@@ -1668,6 +1780,42 @@ var _ = Describe("machine", func() {
 					),
 				},
 			}),
+			Entry("Machine object does not have status.node set and node exists then it should adopt node using providerID", &data{
+				setup: setup{
+					machines: newMachines(1, &machinev1.MachineTemplateSpec{
+						ObjectMeta: *newObjectMeta(objMeta, 0),
+						Spec: machinev1.MachineSpec{
+							ProviderID: "aws//fakeID",
+						},
+					}, &machinev1.MachineStatus{}, nil, nil, nil),
+					nodes: []*corev1.Node{
+						{
+							ObjectMeta: metav1.ObjectMeta{
+								Name: "node-0",
+							},
+							Spec: corev1.NodeSpec{
+								ProviderID: "aws//fakeID-0",
+							},
+						},
+					},
+				},
+				action: action{
+					machine: machineName,
+				},
+				expect: expect{
+					machine: newMachine(&machinev1.MachineTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "machine-0",
+						},
+					}, &machinev1.MachineStatus{
+						Node: "node",
+					}, nil, nil,
+						map[string]string{
+							"node": "node-0",
+						},
+					),
+				},
+			}),
 		)
 	})
 

pkg/driver/driver.go

@@ -106,5 +106,8 @@ func NewDriver(machineID string, secretRef *corev1.Secret, classKind string, mac
 		func() (string, error) {
 			return "fake", nil
 		},
+		func() (VMs, error) {
+			return nil, nil
+		},
 	)
 }

pkg/driver/driver_azure.go

@@ -74,7 +74,7 @@ func (d *AzureDriver) getNICParameters(vmName string, subnet *network.Subnet) ne
 					Name: &nicName,
 					InterfaceIPConfigurationPropertiesFormat: &network.InterfaceIPConfigurationPropertiesFormat{
 						PrivateIPAllocationMethod: network.Dynamic,
-						Subnet:                    subnet,
+						Subnet: subnet,
 					},
 				},
 			},
@@ -292,7 +292,7 @@ func (d *AzureDriver) GetVMs(machineID string) (result VMs, err error) {
 	return
 }
 
-//GetUserData return the used data whit which the VM will be booted
+//GetUserData return the user data with which the VM will be booted
 func (d *AzureDriver) GetUserData() string {
 	return d.UserData
 }

pkg/driver/driver_fake.go

@@ -25,15 +25,17 @@ type FakeDriver struct {
 	delete func(string) error
 	//existing func() (string, v1alpha1.MachinePhase, error)
 	existing    func() (string, error)
+	getVMs      func() (VMs, error)
 	getVolNames func([]corev1.PersistentVolumeSpec) ([]string, error)
 }
 
 // NewFakeDriver returns a new fakedriver object
-func NewFakeDriver(create func() (string, string, error), delete func(string) error, existing func() (string, error)) Driver {
+func NewFakeDriver(create func() (string, string, error), delete func(string) error, existing func() (string, error), getVMs func() (VMs, error)) Driver {
 	return &FakeDriver{
 		create:   create,
 		delete:   delete,
 		existing: existing,
+		getVMs:   getVMs,
 	}
 }
 
@@ -54,8 +56,7 @@ func (d *FakeDriver) GetExisting() (string, error) {
 
 // GetVMs returns a list of VMs
 func (d *FakeDriver) GetVMs(name string) (VMs, error) {
-	listOfVMs := make(map[string]string)
-	return listOfVMs, nil
+	return d.getVMs()
 }
 
 // GetVolNames parses volume names from pv specs