Repository: gardener/machine-controller-manager (go)

Issue:
[machineset controller] Do not update machine status if there is an error adding `deletionTimestamp`
**How to categorize this issue?**
<!--
Please select area, kind, and priority for this issue. This helps the community categorizing it.
Replace below TODOs or exchange the existing identifiers with those that fit best in your opinion.
If multiple identifiers make sense you can also state the commands multiple times, e.g.
  /area control-plane
  /area auto-scaling
  ...

"/area" identifiers:     audit-logging|auto-scaling|backup|certification|control-plane-migration|control-plane|cost|delivery|dev-productivity|disaster-recovery|documentation|high-availability|logging|metering|monitoring|networking|open-source|ops-productivity|os|performance|quality|robustness|scalability|security|storage|testing|usability|user-management
"/kind" identifiers:     api-change|bug|cleanup|discussion|enhancement|epic|impediment|poc|post-mortem|question|regression|task|technical-debt|test
"/priority" identifiers: 1|2|3|4|5 (ordered from greatest to least)
-->
/area control-plane
/kind bug
/priority 3

**What happened**:
There was a bug reported where a machine had its's `status.currentStatus.phase` set to `Terminating` even though the machine did not have a `deletionTimestamp` set.
This can happen when the `DeleteMachine()`call [here](https://github.com/gardener/machine-controller-manager/blob/master/pkg/controller/machineset.go#L679) fails (for whatever reason). We do not return from this place, instead we continue with a status update

The machineset controller will not consider this machine as a stale machine anymore and hence will never reattempt to add a `deletionTimestamp`. The lack of a `deletionTimestamp` will also not cause the machine controller to run the deletion flow for the machine. Machines in this state will therefore be stuck indefinitely in this state

**What you expected to happen**:
The machineset controller should update the status of stale machines only if it has successfully added a `deletionTimestamp` to it

**How to reproduce it (as minimally and precisely as possible)**:

**Anything else we need to know?**:

**Environment**:

- Kubernetes version (use `kubectl version`):
- Cloud provider or hardware configuration:
- Others:


Pull request description:
Don't update machine status to terminating on deletion failure
**What this PR does / why we need it**:
Upon failure in deletion of a machine, it should not transition to `Terminating` phase.

**Which issue(s) this PR fixes**:
Fixes #984 

**Special notes for your reviewer**:

**Release note**:
<!--  Write your release note:
1. Enter your release note in the below block.
2. If no release note is required, just write "NONE" within the block.

Format of block header: <category> <target_group>
Possible values:
- category:       breaking|feature|bugfix|doc|other
- target_group:   user|operator|developer|dependency
-->
```bugfix operator
Fixed a bug in the MachineSet controller where the machine status was set to `Terminating` even if attempt to delete the machine object failed.
```


Patch:
diff --git a/README.md b/README.md
index 5e3ea28be..3fd15f98b 100644
--- a/README.md
+++ b/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.
diff --git a/pkg/controller/machineset.go b/pkg/controller/machineset.go
index 05f9e569e..536d47cc8 100644
--- a/pkg/controller/machineset.go
+++ b/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)
 	}
diff --git a/pkg/util/provider/drain/drain.go b/pkg/util/provider/drain/drain.go
index c8085c593..dee9a1bc4 100644
--- a/pkg/util/provider/drain/drain.go
+++ b/pkg/util/provider/drain/drain.go
@@ -416,7 +416,7 @@ func (o *Options) evictPod(ctx context.Context, pod *corev1.Pod, policyGroupVers
 		DeleteOptions: deleteOptions,
 	}
 	klog.V(3).Infof("Attempting to evict the pod:%q from node %q", pod.Name, o.nodeName)
-	// TODO: Remember to change the URL manipulation func when Evction's version change
+	// TODO: Remember to change the URL manipulation func when Eviction's version change
 	return o.client.PolicyV1beta1().Evictions(eviction.Namespace).Evict(ctx, eviction)
 }
 
@@ -682,7 +682,7 @@ func (o *Options) evictPodsWithPv(ctx context.Context, attemptEvict bool, pods [
 }
 
 // checkAndDeleteWorker is a helper method that check if volumeAttachmentHandler
-// is supported and delete's the worker from the list of event handlers
+// is supported and deletes the worker from the list of event handlers
 func (o *Options) checkAndDeleteWorker(volumeAttachmentEventCh chan *storagev1.VolumeAttachment) {
 	if o.volumeAttachmentHandler != nil {
 		o.volumeAttachmentHandler.DeleteWorker(volumeAttachmentEventCh)
@@ -695,11 +695,11 @@ func (o *Options) evictPodsWithPVInternal(
 	pods []*corev1.Pod,
 	podVolumeInfoMap map[string]PodVolumeInfo,
 	policyGroupVersion string,
-	_ func(namespace, name string) (*corev1.Pod, error),
+	getPodFn func(namespace, name string) (*corev1.Pod, error),
 	returnCh chan error,
 ) (remainingPods []*corev1.Pod, fastTrack bool) {
 	var (
-		retryPods []*corev1.Pod
+		retryPods, pendingPods []*corev1.Pod
 	)
 
 	for i, pod := range pods {
@@ -817,7 +817,18 @@ func (o *Options) evictPodsWithPVInternal(
 		returnCh <- nil
 	}
 
-	return retryPods, false
+	for _, pod := range retryPods {
+		p, err := getPodFn(pod.Namespace, pod.Name)
+		if apierrors.IsNotFound(err) || (p != nil && p.ObjectMeta.UID != pod.ObjectMeta.UID) {
+			returnCh <- nil
+			continue
+		} else if err != nil {
+			klog.Errorf("error fetching pod %s status. Retrying. Err: %v", pod.Name, err)
+		}
+		pendingPods = append(pendingPods, pod)
+	}
+
+	return pendingPods, false
 }
 
 func (o *Options) getPersistentVolumeNamesForPod(pod *corev1.Pod) []string {
diff --git a/pkg/util/provider/machinecontroller/machine.go b/pkg/util/provider/machinecontroller/machine.go
index fbb2e5ab5..34520128e 100644
--- a/pkg/util/provider/machinecontroller/machine.go
+++ b/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.


Test patch:
diff --git a/pkg/controller/controller_suite_test.go b/pkg/controller/controller_suite_test.go
index ba8246ded..6ad214e7f 100644
--- a/pkg/controller/controller_suite_test.go
+++ b/pkg/controller/controller_suite_test.go
@@ -465,6 +465,7 @@ func createController(
 
 	controller.machineControl = FakeMachineControl{
 		controlMachineClient: fakeTypedMachineClient,
+		Recorder:             controller.recorder,
 	}
 
 	return controller, fakeObjectTrackers
diff --git a/pkg/controller/machineset_test.go b/pkg/controller/machineset_test.go
index b58c9f47a..3bdd70c23 100644
--- a/pkg/controller/machineset_test.go
+++ b/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() {