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

Issue:
NotManagedByMCM annotation incorrectly added by MCM to Node 
**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 quality
/kind bug
/priority 2

**What happened**:
Node associated with machines that are managed by the `machine-controller` are being annotated with the `NotManagedByMCM` (`node.machine.sapcloud.io/not-managed-by-mcm="1"`)  annotation.  This can occur plane migration when the Node creation time stamps *predate* the Machine creation time stamps after control plane migration. 

```
kubectl get no -o custom-columns="NAME":.metadata.name,"CREATION":.metadata.creationTimestamp
NAME                                                        CREATION
shoot--di-xm8--dh-tt23jtpe4f2f-static-dynamic-76487-89zwb   2023-08-16T16:42:15Z
shoot--di-xm8--dh-tt23jtpe4f2f-system-57c59-g2f8q           2023-08-16T16:42:17Z
shoot--di-xm8--dh-tt23jtpe4f2f-system-57c59-k9jwh           2023-08-16T16:42:13Z
```
```
k get mc  -o custom-columns="NAME":.metadata.name,"CREATION":.metadata.creationTimestamp
NAME                                                        CREATION
shoot--di-xm8--dh-tt23jtpe4f2f-static-dynamic-76487-89zwb   2023-10-22T05:56:53Z
shoot--di-xm8--dh-tt23jtpe4f2f-system-57c59-g2f8q           2023-10-22T05:56:53Z
shoot--di-xm8--dh-tt23jtpe4f2f-system-57c59-k9jwh           2023-10-22T05:56:53Z
```

**What you expected to happen**:
Ideally, Nodes should not be incorrectly tagged with this annotation. If they have this annotation, the annotation must be removed. A proper fix is described at https://github.com/gardener/machine-controller-manager/issues/774 . Unfortunately, the solution proposed there will take more time since now even gardener code shoot health check is relying on the process of this `NotManagedByMCM` annotation.

*Solution Proposal*
1. In `AnnotateNodesUnmanagedByMCM` (invoked by the safety reconciler),  remove annotation if the `Node` is already associated with `Machine` , yet has this annotation set.
1. Fix needs vendoring and release in all providers.
1. Current default period of safety reconciler job is `30m`. Consider reducing to `5m` after checking that all calls go through informer cache. 

**How to reproduce it (as minimally and precisely as possible)**:

**Anything else we need to know?**:

**Environment**:
all mcm versions
- Kubernetes version (use `kubectl version`): all
- Cloud provider or hardware configuration: all
- Others:

**What you expected to happen**:



Pull request:
Remove NotManagedByMCM when not applicable
**What this PR does / why we need it**:

See https://github.com/gardener/machine-controller-manager/issues/863

**Which issue(s) this PR fixes**:
Fixes #863 

**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
Removes `node.machine.sapcloud.io/not-managed-by-mcm` annotation from nodes managed by the MCM.
```
```other operator
The default `machine-safety-orphan-vms-period` has been reduced from 30m to 15m.
```


Patch:
diff --git a/pkg/util/provider/app/options/options.go b/pkg/util/provider/app/options/options.go
index 0a84289e0..1f0ace6d6 100644
--- a/pkg/util/provider/app/options/options.go
+++ b/pkg/util/provider/app/options/options.go
@@ -70,7 +70,7 @@ func NewMCServer() *MCServer {
 				MaxEvictRetries:                          drain.DefaultMaxEvictRetries,
 				PvDetachTimeout:                          metav1.Duration{Duration: 2 * time.Minute},
 				PvReattachTimeout:                        metav1.Duration{Duration: 90 * time.Second},
-				MachineSafetyOrphanVMsPeriod:             metav1.Duration{Duration: 30 * time.Minute},
+				MachineSafetyOrphanVMsPeriod:             metav1.Duration{Duration: 15 * time.Minute},
 				MachineSafetyAPIServerStatusCheckPeriod:  metav1.Duration{Duration: 1 * time.Minute},
 				MachineSafetyAPIServerStatusCheckTimeout: metav1.Duration{Duration: 30 * time.Second},
 			},
diff --git a/pkg/util/provider/machinecontroller/machine_safety.go b/pkg/util/provider/machinecontroller/machine_safety.go
index d29133a58..23ce2e132 100644
--- a/pkg/util/provider/machinecontroller/machine_safety.go
+++ b/pkg/util/provider/machinecontroller/machine_safety.go
@@ -170,7 +170,7 @@ func (c *controller) AnnotateNodesUnmanagedByMCM(ctx context.Context) (machineut
 		return machineutils.LongRetry, err
 	}
 	for _, node := range nodes {
-		_, err := c.getMachineFromNode(node.Name)
+		machine, err := c.getMachineFromNode(node.Name)
 		if err != nil {
 			if err == errMultipleMachineMatch {
 				klog.Errorf("Couldn't fetch machine, Error: %s", err)
@@ -193,11 +193,23 @@ func (c *controller) AnnotateNodesUnmanagedByMCM(ctx context.Context) (machineut
 					machineutils.NotManagedByMCM: "1",
 				}
 
+				klog.V(3).Infof("Adding NotManagedByMCM annotation to Node %q", node.Name)
 				// err is returned only when node update fails
-				if err := c.updateNodeWithAnnotation(ctx, nodeCopy, annotations); err != nil {
+				if err := c.updateNodeWithAnnotations(ctx, nodeCopy, annotations); err != nil {
 					return machineutils.MediumRetry, err
 				}
 			}
+		} else {
+			_, hasAnnot := node.Annotations[machineutils.NotManagedByMCM]
+			if !hasAnnot {
+				continue
+			}
+			klog.V(3).Infof("Removing NotManagedByMCM annotation from Node %q associated with Machine %q", node.Name, machine.Name)
+			nodeCopy := node.DeepCopy()
+			delete(nodeCopy.Annotations, machineutils.NotManagedByMCM)
+			if err := c.updateNodeWithAnnotations(ctx, nodeCopy, nil); err != nil {
+				return machineutils.MediumRetry, err
+			}
 		}
 	}
 
diff --git a/pkg/util/provider/machinecontroller/machine_safety_util.go b/pkg/util/provider/machinecontroller/machine_safety_util.go
index 0ab682f95..eb42593de 100644
--- a/pkg/util/provider/machinecontroller/machine_safety_util.go
+++ b/pkg/util/provider/machinecontroller/machine_safety_util.go
@@ -22,7 +22,7 @@ import (
 	"k8s.io/klog/v2"
 )
 
-func (c *controller) updateNodeWithAnnotation(ctx context.Context, node *v1.Node, annotations map[string]string) error {
+func (c *controller) updateNodeWithAnnotations(ctx context.Context, node *v1.Node, annotations map[string]string) error {
 
 	// Initialize node annotations if empty
 	if node.Annotations == nil {
@@ -37,10 +37,9 @@ func (c *controller) updateNodeWithAnnotation(ctx context.Context, node *v1.Node
 	_, err := c.targetCoreClient.CoreV1().Nodes().Update(ctx, node, metav1.UpdateOptions{})
 
 	if err != nil {
-		klog.Errorf("Couldn't patch the node %q , Error: %s", node.Name, err)
+		klog.Errorf("Failed to update annotations for Node %q due to error: %s", node.Name, err)
 		return err
 	}
-	klog.V(2).Infof("Annotated node %q was annotated with NotManagedByMCM successfully", node.Name)
 
 	return nil
 }
diff --git a/pkg/util/provider/machinecontroller/machine_util.go b/pkg/util/provider/machinecontroller/machine_util.go
index 22be3ff6f..a9b75e750 100644
--- a/pkg/util/provider/machinecontroller/machine_util.go
+++ b/pkg/util/provider/machinecontroller/machine_util.go
@@ -1022,7 +1022,13 @@ func (c *controller) drainNode(ctx context.Context, deleteMachineRequest *driver
 			}
 		}
 
-		klog.V(3).Infof("(drainNode) For node %q, machine %q, nodeReadyCondition: %s, readOnlyFileSystemCondition: %s", nodeName, machine.Name, nodeReadyCondition, readOnlyFileSystemCondition)
+		// verify and log node object's existence
+		if _, err := c.nodeLister.Get(nodeName); err == nil {
+			klog.V(3).Infof("(drainNode) For node %q, machine %q, nodeReadyCondition: %s, readOnlyFileSystemCondition: %s", nodeName, machine.Name, nodeReadyCondition, readOnlyFileSystemCondition)
+		} else if apierrors.IsNotFound(err) {
+			klog.Warningf("(drainNode) Node %q for machine %q doesn't exist, so drain will finish instantly", nodeName, machine.Name)
+		}
+
 		if !isConditionEmpty(nodeReadyCondition) && (nodeReadyCondition.Status != v1.ConditionTrue) && (time.Since(nodeReadyCondition.LastTransitionTime.Time) > nodeNotReadyDuration) {
 			message := "Setting forceDeletePods & forceDeleteMachine to true for drain as machine is NotReady for over 5min"
 			forceDeleteMachine = true
@@ -1502,12 +1508,13 @@ func (c *controller) canMarkMachineFailed(machineDeployName, machineName, namesp
 		}
 	}
 
-	klog.V(2).Infof("machineDeployName=%q for machine=%q , terminating=%d , failed=%d , pending=%d , noPhase=%d , crashLooping=%d , extraCountedProgress=%d", machineDeployName, machineName, terminating, failed, pending, noPhase, crashLooping, terminating)
+	klog.V(2).Infof("Performing rate-limit check for machine=%q. Under machineDeployment=%q : terminating=%d , failed=%d , pending=%d , noPhase=%d , crashLooping=%d , extraCountedProgress=%d", machineName, machineDeployName, terminating, failed, pending, noPhase, crashLooping, terminating)
 
 	if inProgress < maxReplacements {
 		klog.V(2).Infof("Number of goroutines now %d\n", runtime.NumGoroutine())
 		return true, nil
 	}
+	klog.V(2).Infof("Cannot mark `Unknown` machine=%q as `Failed` as max rate-limit reached for machineDeployment=%q. maxAllowedReplacements=%d and inProgressReplacements=%d", machineName, machineDeployName, maxReplacements, inProgress)
 	return false, nil
 }
 


Test patch:
diff --git a/pkg/util/provider/machinecontroller/machine_safety_test.go b/pkg/util/provider/machinecontroller/machine_safety_test.go
index e820584a7..dd7f917f7 100644
--- a/pkg/util/provider/machinecontroller/machine_safety_test.go
+++ b/pkg/util/provider/machinecontroller/machine_safety_test.go
@@ -418,7 +418,8 @@ var _ = Describe("safety_logic", func() {
 	Describe("#AnnotateNodesUnmanagedByMCM", func() {
 
 		type setup struct {
-			node *corev1.Node
+			node                     *corev1.Node
+			associateNodeWithMachine bool
 		}
 		type action struct {
 		}
@@ -441,6 +442,25 @@ var _ = Describe("safety_logic", func() {
 				targetCoreObjects := []runtime.Object{}
 				controlMachineObjects := []runtime.Object{}
 
+				if data.setup.associateNodeWithMachine {
+					//optional machine object for test-node-1 ie data.setup.node
+					testMachineObject := &v1alpha1.Machine{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "testmachine_0",
+							Namespace: testNamespace,
+							Labels: map[string]string{
+								v1alpha1.NodeLabelKey: data.setup.node.Name,
+							},
+						},
+						Status: v1alpha1.MachineStatus{
+							CurrentStatus: v1alpha1.CurrentStatus{
+								Phase: v1alpha1.MachineRunning,
+							},
+						},
+					}
+					controlMachineObjects = append(controlMachineObjects, testMachineObject)
+				}
+
 				//machine object for test-node-1
 				testMachineObject := &v1alpha1.Machine{
 					ObjectMeta: metav1.ObjectMeta{
@@ -726,6 +746,66 @@ var _ = Describe("safety_logic", func() {
 					err:   nil,
 				},
 			}),
+			Entry("Node incorrectly assigned NotManagedByMCM annotation", &data{
+				setup: setup{
+					node: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-0",
+							Annotations: map[string]string{
+								"anno1":                      "value1",
+								machineutils.NotManagedByMCM: "1",
+							},
+						},
+					},
+					associateNodeWithMachine: true,
+				},
+				action: action{},
+				expect: expect{
+					node0: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-0",
+							// node0 no longer has NotManagedByMCM since it has a backing machine object.
+							Annotations: map[string]string{
+								"anno1": "value1",
+							},
+						},
+					},
+					node1: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-1",
+							Annotations: map[string]string{
+								"anno1": "value1",
+							},
+						},
+					},
+					node2: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-2",
+							Annotations: map[string]string{
+								"anno1": "value1",
+							},
+						},
+					},
+					retry: machineutils.LongRetry,
+					err:   nil,
+				},
+			}),
 		)
 	})
 })
diff --git a/pkg/util/provider/machinecontroller/machine_safety_util_test.go b/pkg/util/provider/machinecontroller/machine_safety_util_test.go
index 4d67e02d6..23e300f6d 100644
--- a/pkg/util/provider/machinecontroller/machine_safety_util_test.go
+++ b/pkg/util/provider/machinecontroller/machine_safety_util_test.go
@@ -17,13 +17,14 @@ package controller
 import (
 	"context"
 
-	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
 )
 
 var _ = Describe("machine_safety_util", func() {
@@ -56,7 +57,7 @@ var _ = Describe("machine_safety_util", func() {
 				testNode := data.action.node
 				expectedNode := data.expect.node
 
-				err := c.updateNodeWithAnnotation(context.TODO(), testNode, data.action.annotations)
+				err := c.updateNodeWithAnnotations(context.TODO(), testNode, data.action.annotations)
 
 				waitForCacheSync(stop, c)