Don't update machine status to terminating on deletion failure (#995)

Author: takoverflow

README.md

@@ -100,7 +100,7 @@ See [here](docs/documents/apis.md) for CRD API Documentation
             <td>There is a Safety Controller responsible for handling the unidentified or unknown behaviours from the cloud providers. Safety Controller:
                 <ul>
                     <li>
-                        freezes the MachineDeployment controller and MachineSet controller if the number of <code>Machine</code> objects goes beyond a certain threshold on top of <code>Spec.replicas</code>. It can be configured by the flag <code>--safety-up</code> or <code>--safety-down</code> and also <code>--machine-safety-overshooting-period`</code>.
+                        freezes the MachineDeployment controller and MachineSet controller if the number of <code>Machine</code> objects goes beyond a certain threshold on top of <code>Spec.replicas</code>. It can be configured by the flag <code>--safety-up</code> or <code>--safety-down</code> and also <code>--machine-safety-overshooting-period</code>.
                     </li>
                     <li>
                         freezes the functionality of the MCM if either of the <code>target-apiserver</code> or the <code>control-apiserver</code> is not reachable.

pkg/controller/controller_suite_test.go

@@ -465,6 +465,7 @@ func createController(
 
 	controller.machineControl = FakeMachineControl{
 		controlMachineClient: fakeTypedMachineClient,
+		Recorder:             controller.recorder,
 	}
 
 	return controller, fakeObjectTrackers

pkg/controller/machineset.go

@@ -528,7 +528,7 @@ func (c *controller) reconcileClusterMachineSet(key string) error {
 			return err
 		}
 	}
-	klog.V(3).Infof("Processing the machineset %q with replicas %d associated with machine class: %q", machineSet.Name, machineSet.Spec.Replicas, machineSet.Spec.MachineClass.Name)
+	klog.V(3).Infof("Processing the machineset %q with replicas %d associated with machine class: %q", machineSet.Name, machineSet.Spec.Replicas, machineSet.Spec.Template.Spec.Class.Name)
 
 	selector, err := metav1.LabelSelectorAsSelector(machineSet.Spec.Selector)
 	if err != nil {
@@ -713,6 +713,7 @@ func (c *controller) prepareMachineForDeletion(ctx context.Context, targetMachin
 		klog.V(2).Infof("Failed to delete %v, decrementing expectations for %v %s/%s", machineKey, machineSet.Kind, machineSet.Namespace, machineSet.Name)
 		c.expectations.DeletionObserved(machineSetKey, machineKey)
 		errCh <- err
+		return
 	} else {
 		// successful delete of a Failed phase machine due to unhealthiness for too long, increments staleMachinesRemoved counter
 		// note: call is blocking and thread safe as other worker threads might be updating the counter as well
@@ -733,7 +734,7 @@ func (c *controller) prepareMachineForDeletion(ctx context.Context, targetMachin
 		TimeoutActive:  false,
 		LastUpdateTime: metav1.Now(),
 	}
-	if _, err := c.updateMachineStatus(ctx, targetMachine, lastOperation, currentStatus); err != nil {
+	if _, err := c.updateMachineStatus(ctx, targetMachine, lastOperation, currentStatus); err != nil && !apierrors.IsNotFound(err) {
 		// TODO: proper error handling needs to happen here
 		klog.Errorf("failed to update machine status for machine %s: %v", targetMachine.Name, err)
 	}

pkg/controller/machineset_test.go

@@ -7,6 +7,7 @@ package controller
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sync"
 	"time"
 
@@ -15,9 +16,11 @@ import (
 	k8sError "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/testing"
 	"k8s.io/utils/pointer"
 
 	machinev1 "github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
+	faketyped "github.com/gardener/machine-controller-manager/pkg/client/clientset/versioned/typed/machine/v1alpha1/fake"
 	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
 )
 
@@ -1497,6 +1500,7 @@ var _ = Describe("machineset", func() {
 			testMachineSet     *machinev1.MachineSet
 			testFailedMachine2 *machinev1.Machine
 			testFailedMachine1 *machinev1.Machine
+			testRunningMachine *machinev1.Machine
 		)
 
 		BeforeEach(func() {
@@ -1558,6 +1562,22 @@ var _ = Describe("machineset", func() {
 					},
 				},
 			}
+
+			testRunningMachine = &machinev1.Machine{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "machine-t",
+					Namespace: testNamespace,
+					UID:       "1234560",
+					Labels: map[string]string{
+						"test-label": "test-label",
+					},
+				},
+				Status: machinev1.MachineStatus{
+					CurrentStatus: machinev1.CurrentStatus{
+						Phase: MachineRunning,
+					},
+				},
+			}
 		})
 
 		// Testcase: It should delete the inactive machines.
@@ -1582,6 +1602,40 @@ var _ = Describe("machineset", func() {
 			Expect(Err1).Should(Not(BeNil()))
 			Expect(Err2).Should(Not(BeNil()))
 		})
+
+		It("It should not mark a machine as terminating when deletion fails.", func() {
+			stop := make(chan struct{})
+			defer close(stop)
+
+			objects := []runtime.Object{}
+			objects = append(objects, testMachineSet, testFailedMachine1, testRunningMachine)
+			c, trackers := createController(stop, testNamespace, objects, nil, nil)
+			defer trackers.Stop()
+			waitForCacheSync(stop, c)
+
+			// Ref: https://estebangarcia.io/unit-testing-k8s-golang/
+			// Add a reactor that intercepts machine delete call and returns an error
+			// to simulate error when processing deletion request for a machine
+			machineDeletionError := "forced machine deletion error"
+			c.controlMachineClient.(*faketyped.FakeMachineV1alpha1).Fake.PrependReactor("delete", "machines", func(_ testing.Action) (handled bool, ret runtime.Object, err error) {
+				return true, &machinev1.Machine{}, errors.New(machineDeletionError)
+			})
+			targetMachines := []*machinev1.Machine{testFailedMachine1, testRunningMachine}
+			err := c.terminateMachines(context.TODO(), targetMachines, testMachineSet)
+
+			waitForCacheSync(stop, c)
+
+			mFailed, Err1 := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), targetMachines[0].Name, metav1.GetOptions{})
+			mRunning, Err2 := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), targetMachines[1].Name, metav1.GetOptions{})
+
+			Expect(err).To(Equal(fmt.Errorf("unable to delete machines: %s", machineDeletionError)))
+			Expect(Err1).Should(BeNil())
+			Expect(mFailed.ObjectMeta.DeletionTimestamp).Should(BeNil())
+			Expect(mFailed.Status.CurrentStatus.Phase).NotTo(Equal(machinev1.MachineTerminating))
+			Expect(Err2).Should(BeNil())
+			Expect(mRunning.ObjectMeta.DeletionTimestamp).Should(BeNil())
+			Expect(mRunning.Status.CurrentStatus.Phase).NotTo(Equal(machinev1.MachineTerminating))
+		})
 	})
 
 	Describe("#addMachineSetFinalizers", func() {

pkg/util/provider/machinecontroller/machine.go

@@ -539,7 +539,7 @@ func (c *controller) triggerCreationFlow(ctx context.Context, createMachineReque
 				nodeName = createMachineResponse.NodeName
 				providerID = createMachineResponse.ProviderID
 				// Creation was successful
-				klog.V(2).Infof("Created new VM for machine: %q with ProviderID: %q and backing node: %q", machine.Name, providerID, getNodeName(machine))
+				klog.V(2).Infof("Created new VM for machine: %q with ProviderID: %q and backing node: %q", machine.Name, providerID, nodeName)
 				// If a node obj already exists by the same nodeName, treat it as a stale node and trigger machine deletion.
 				// TODO: there is a case with Azure where the VM may join the cluster before the CreateMachine call is completed,
 				// and that would make an otherwise healthy node, be marked as stale.