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

Issue:
Delay in machine reconcile for machineClasses without finalizer
**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 delivery
/kind regression
/priority 2

**What happened**:
With the new changes on [finalizers for machineClass](https://github.com/gardener/machine-controller-manager/pull/593). The machine creations are now delayed until a force re-enqueue of machines. 

**What you expected to happen**:
Machines to be created immediately after machine class finalizer is added. 

**How to reproduce it (as minimally and precisely as possible)**:
Create machineDeployment and machineClass. The creation now takes longer by about 10mins.

**Anything else we need to know?**:

**Environment**:

- Kubernetes version (use `kubectl version`): All
- Cloud provider or hardware configuration: All
- Others:

/assign

Pull request:
Fixes regressions from PR#593 and PR#612 
**What this PR does / why we need it**:
Fixes regressions from https://github.com/gardener/machine-controller-manager/pull/593 and https://github.com/gardener/machine-controller-manager/pull/612. 

**Which issue(s) this PR fixes**:
Fixes #625 

- MachineClass finalizer error to retry sooner 
- Enqueue machines on MC finalizer addition
- Annotate NotManagedByMCM only post creationTimeout

**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
-->
```other operator
None
```
/invite @himanshu-kun @AxiomSamarth 

Patch:
diff --git a/pkg/util/provider/machinecontroller/machine.go b/pkg/util/provider/machinecontroller/machine.go
index b8cde1815..764c5f24e 100644
--- a/pkg/util/provider/machinecontroller/machine.go
+++ b/pkg/util/provider/machinecontroller/machine.go
@@ -208,7 +208,12 @@ func (c *controller) addNodeToMachine(obj interface{}) {
 	}
 
 	machine, err := c.getMachineFromNode(key)
-	if err != nil {
+	if err != nil && err != errNoMachineMatch {
+		if err != errNoMachineMatch {
+			// errNoMachineMatch could mean that VM is still in creation hence ignoring it
+			return
+		}
+
 		klog.Errorf("Couldn't fetch machine %s, Error: %s", key, err)
 		return
 	}
@@ -259,6 +264,7 @@ func (c *controller) getMachineFromNode(nodeName string) (*v1alpha1.Machine, err
 	} else if len(machines) < 1 {
 		return nil, errNoMachineMatch
 	}
+
 	return machines[0], nil
 }
 
diff --git a/pkg/util/provider/machinecontroller/machine_safety.go b/pkg/util/provider/machinecontroller/machine_safety.go
index f97591c85..1ca95fdc6 100644
--- a/pkg/util/provider/machinecontroller/machine_safety.go
+++ b/pkg/util/provider/machinecontroller/machine_safety.go
@@ -52,7 +52,6 @@ func (c *controller) reconcileClusterMachineSafetyOrphanVMs(key string) error {
 	defer klog.V(3).Infof("reconcileClusterMachineSafetyOrphanVMs: End, reSync-Period: %v", reSyncAfter)
 
 	retryPeriod, err := c.checkMachineClasses()
-
 	if err != nil {
 		klog.Errorf("reconcileClusterMachineSafetyOrphanVMs: Error occurred while checking for orphan VMs: %s", err)
 		c.machineSafetyOrphanVMsQueue.AddAfter("", time.Duration(retryPeriod))
@@ -170,7 +169,7 @@ func (c *controller) isAPIServerUp() bool {
 
 // AnnotateNodesUnmanagedByMCM checks for nodes which are not handled by MCM and annotes them
 func (c *controller) AnnotateNodesUnmanagedByMCM() (machineutils.RetryPeriod, error) {
-	//list all the nodes on target cluster
+	// list all the nodes on target cluster
 	nodes, err := c.nodeLister.List(labels.Everything())
 	if err != nil {
 		klog.Errorf("Safety-Net: Error getting nodes")
@@ -182,16 +181,25 @@ func (c *controller) AnnotateNodesUnmanagedByMCM() (machineutils.RetryPeriod, er
 			if err == errMultipleMachineMatch {
 				klog.Errorf("Couldn't fetch machine, Error: %s", err)
 			} else if err == errNoMachineMatch {
-				//if no backing machine for a node,means annotate it
-				if _, annotationPresent := node.ObjectMeta.Annotations[machineutils.NotManagedByMCM]; annotationPresent {
+
+				if !node.CreationTimestamp.Time.Before(time.Now().Add(c.safetyOptions.MachineCreationTimeout.Duration * -1)) {
+					// node creationTimestamp is NOT before now() - machineCreationTime
+					// meaning creationTimeout has not occurred since node creation
+					// hence don't tag such nodes
+					klog.V(3).Infof("Node %q is still too young to be tagged with NotManagedByMCM", node.Name)
+					continue
+				} else if _, annotationPresent := node.ObjectMeta.Annotations[machineutils.NotManagedByMCM]; annotationPresent {
+					// annotation already exists, ignore this node
 					continue
 				}
+
+				// if no backing machine object for a node, annotate it
 				nodeCopy := node.DeepCopy()
 				annotations := map[string]string{
 					machineutils.NotManagedByMCM: "1",
 				}
 
-				//err is returned only when node update fails
+				// err is returned only when node update fails
 				if err := c.updateNodeWithAnnotation(nodeCopy, annotations); 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 515dcdbb4..faf59d516 100644
--- a/pkg/util/provider/machinecontroller/machine_safety_util.go
+++ b/pkg/util/provider/machinecontroller/machine_safety_util.go
@@ -18,11 +18,11 @@ func (c *controller) updateNodeWithAnnotation(node *v1.Node, annotations map[str
 	}
 
 	_, err := c.targetCoreClient.CoreV1().Nodes().Update(node)
-
 	if err != nil {
 		klog.Errorf("Couldn't patch the node %q , 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 3bc408925..48fa454a6 100644
--- a/pkg/util/provider/machinecontroller/machine_util.go
+++ b/pkg/util/provider/machinecontroller/machine_util.go
@@ -124,10 +124,13 @@ func (c *controller) ValidateMachineClass(classSpec *v1alpha1.ClassSpec) (*v1alp
 	}
 
 	if finalizers := sets.NewString(machineClass.Finalizers...); !finalizers.Has(MCMFinalizerName) {
-		klog.Errorf("The machine class %s has no finalizers set. So not reconciling the machine.", machineClass.Name)
 		c.machineClassQueue.Add(machineClass.Name)
-		err := errors.New("The machine class %s has no finalizers set. So not reconciling the machine." + machineClass.Name)
-		return nil, nil, retry, err
+
+		errMessage := fmt.Sprintf("The machine class %s has no finalizers set. So not reconciling the machine.", machineClass.Name)
+		err := errors.New(errMessage)
+		klog.Warning(errMessage)
+
+		return nil, nil, machineutils.ShortRetry, err
 	}
 
 	return machineClass, secretData, retry, nil
diff --git a/pkg/util/provider/machinecontroller/machineclass.go b/pkg/util/provider/machinecontroller/machineclass.go
index 30781b3ac..d1d6fc53f 100644
--- a/pkg/util/provider/machinecontroller/machineclass.go
+++ b/pkg/util/provider/machinecontroller/machineclass.go
@@ -146,24 +146,34 @@ func (c *controller) reconcileClusterMachineClass(class *v1alpha1.MachineClass)
 		return err
 	}
 
-	// fetch all machines referring the machineClass
+	// Fetch all machines referring the machineClass
 	machines, err := c.findMachinesForClass(machineutils.MachineClassKind, class.Name)
 	if err != nil {
 		return err
 	}
 
-	// Add finalizer to avoid losing machineClass object
 	if class.DeletionTimestamp == nil && len(machines) > 0 {
-		err = c.addMachineClassFinalizers(class)
-		if err != nil {
-			return err
+		// If deletionTimestamp is not set and at least one machine is referring this machineClass
+
+		if finalizers := sets.NewString(class.Finalizers...); !finalizers.Has(MCMFinalizerName) {
+			// Add machineClassFinalizer as if doesn't exist
+			err = c.addMachineClassFinalizers(class)
+			if err != nil {
+				return err
+			}
+
+			// Enqueue all machines once finalizer is added to machineClass
+			// This is to allow processing of such machines
+			for _, machine := range machines {
+				c.enqueueMachine(machine)
+			}
 		}
 
 		return nil
 	}
 
 	if len(machines) > 0 {
-		// machines are still referring the machine class, please wait before deletion
+		// Machines are still referring the machine class, please wait before deletion
 		klog.V(3).Infof("Cannot remove finalizer on %s because still (%d) machines are referencing it", class.Name, len(machines))
 
 		for _, machine := range machines {
@@ -173,8 +183,12 @@ func (c *controller) reconcileClusterMachineClass(class *v1alpha1.MachineClass)
 		return fmt.Errorf("Retry as machine objects are still referring the machineclass")
 	}
 
-	// delete machine class finalizer if exists
-	return c.deleteMachineClassFinalizers(class)
+	if finalizers := sets.NewString(class.Finalizers...); finalizers.Has(MCMFinalizerName) {
+		// Delete finalizer if exists on machineClass
+		return c.deleteMachineClassFinalizers(class)
+	}
+
+	return nil
 }
 
 /*
@@ -183,23 +197,15 @@ func (c *controller) reconcileClusterMachineClass(class *v1alpha1.MachineClass)
 */
 
 func (c *controller) addMachineClassFinalizers(class *v1alpha1.MachineClass) error {
-	clone := class.DeepCopy()
-
-	if finalizers := sets.NewString(clone.Finalizers...); !finalizers.Has(MCMFinalizerName) {
-		finalizers.Insert(MCMFinalizerName)
-		return c.updateMachineClassFinalizers(clone, finalizers.List())
-	}
-	return nil
+	finalizers := sets.NewString(class.Finalizers...)
+	finalizers.Insert(MCMFinalizerName)
+	return c.updateMachineClassFinalizers(class, finalizers.List())
 }
 
 func (c *controller) deleteMachineClassFinalizers(class *v1alpha1.MachineClass) error {
-	clone := class.DeepCopy()
-
-	if finalizers := sets.NewString(clone.Finalizers...); finalizers.Has(MCMFinalizerName) {
-		finalizers.Delete(MCMFinalizerName)
-		return c.updateMachineClassFinalizers(clone, finalizers.List())
-	}
-	return nil
+	finalizers := sets.NewString(class.Finalizers...)
+	finalizers.Delete(MCMFinalizerName)
+	return c.updateMachineClassFinalizers(class, finalizers.List())
 }
 
 func (c *controller) updateMachineClassFinalizers(class *v1alpha1.MachineClass, finalizers []string) error {


Test patch:
diff --git a/pkg/util/provider/machinecontroller/machine_safety_test.go b/pkg/util/provider/machinecontroller/machine_safety_test.go
index d93f5af5f..b94591408 100644
--- a/pkg/util/provider/machinecontroller/machine_safety_test.go
+++ b/pkg/util/provider/machinecontroller/machine_safety_test.go
@@ -383,7 +383,7 @@ var _ = Describe("safety_logic", func() {
 				Expect(updatedNodeObject2.Annotations).Should(Equal(data.expect.node2.Annotations))
 			},
 
-			Entry("when annotations are updated in node-object", &data{
+			Entry("Annotate node object when creation time is older than machineCreationTimeout (21mins old)", &data{
 				setup: setup{
 					node: &corev1.Node{
 						TypeMeta: metav1.TypeMeta{
@@ -395,6 +395,7 @@ var _ = Describe("safety_logic", func() {
 							Annotations: map[string]string{
 								"anno1": "value1",
 							},
+							CreationTimestamp: metav1.NewTime(metav1.Now().Add(-21 * time.Minute)),
 						},
 					},
 				},
@@ -441,7 +442,65 @@ var _ = Describe("safety_logic", func() {
 					err:   nil,
 				},
 			}),
-			Entry("when node already not handled by MCM", &data{
+			Entry("Don't annotate node object when creation time is less than machineCreationTimeout (19mins old)", &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",
+							},
+							CreationTimestamp: metav1.NewTime(metav1.Now().Add(-19 * time.Minute)),
+						},
+					},
+				},
+				action: action{},
+				expect: expect{
+					node0: &corev1.Node{
+						TypeMeta: metav1.TypeMeta{
+							APIVersion: "v1",
+							Kind:       "Node",
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "test-node-0",
+							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,
+				},
+			}),
+			Entry("Node already has NotManagedByMCM annotation", &data{
 				setup: setup{
 					node: &corev1.Node{
 						TypeMeta: metav1.TypeMeta{