Remove node.machine.sapcloud.io/trigger-deletion-by-mcm annotation (#1055)

Author: gagan16k

machine-controller-manager

@@ -0,0 +1 @@
+Subproject commit 0f18f3d7c1127c4d2daf1df4ce8310e697fb2d14

pkg/util/provider/machinecontroller/controller.go

@@ -47,6 +47,9 @@ const (
 	// MCMFinalizerName is the finalizer used to tag dependencies before deletion
 	// of the object. This finalizer is carried over from the MCM
 	MCMFinalizerName = "machine.sapcloud.io/machine-controller-manager"
+	// NodeFinalizerName is the finalizer used to tag node dependencies before deletion
+	// Added to resolve https://github.com/gardener/machine-controller-manager/issues/1051
+	NodeFinalizerName = "node.machine.sapcloud.io/machine-controller"
 	// MCFinalizerName is the finalizer created for the external
 	// machine controller to differentiate it from the MCMFinalizerName
 	// This finalizer is added only on secret-objects to avoid race between in-tree and out-of-tree controllers.
@@ -187,9 +190,9 @@ func NewController(
 	// Machine Controller's Informers
 	if targetCoreInformerFactory != nil {
 		_, _ = targetCoreInformerFactory.Core().V1().Nodes().Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
-			AddFunc:    controller.addNodeToMachine,
-			UpdateFunc: controller.updateNodeToMachine,
-			DeleteFunc: controller.deleteNodeToMachine,
+			AddFunc:    controller.addNode,
+			UpdateFunc: controller.updateNode,
+			DeleteFunc: controller.deleteNode,
 		})
 	}
 

pkg/util/provider/machinecontroller/machine.go

@@ -7,7 +7,6 @@ package controller
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"maps"
 	"slices"
@@ -17,8 +16,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/selection"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/cache"
@@ -302,182 +299,6 @@ func (c *controller) reconcileClusterMachineTermination(key string) error {
 	return nil
 }
 
-/*
-SECTION
-Machine controller - nodeToMachine
-*/
-var (
-	errMultipleMachineMatch = errors.New("multiple machines matching node")
-	errNoMachineMatch       = errors.New("no machines matching node found")
-)
-
-func (c *controller) addNodeToMachine(obj any) {
-	node := obj.(*corev1.Node)
-	if node == nil {
-		klog.Errorf("Couldn't convert to node from object")
-		return
-	}
-
-	// If NotManagedByMCM annotation is present on node, don't process this node object
-	if _, annotationPresent := node.ObjectMeta.Annotations[machineutils.NotManagedByMCM]; annotationPresent {
-		return
-	}
-
-	key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(obj)
-	if err != nil {
-		klog.Errorf("Couldn't get key for object %+v: %v", obj, err)
-		return
-	}
-
-	machine, err := c.getMachineFromNode(key)
-	if err != nil {
-		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
-	}
-
-	isMachineCrashLooping := machine.Status.CurrentStatus.Phase == v1alpha1.MachineCrashLoopBackOff
-	isMachineTerminating := machine.Status.CurrentStatus.Phase == v1alpha1.MachineTerminating
-	_, _, nodeConditionsHaveChanged := nodeConditionsHaveChanged(machine.Status.Conditions, node.Status.Conditions)
-
-	if !isMachineCrashLooping && !isMachineTerminating && nodeConditionsHaveChanged {
-		c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. conditions of backing node %q have changed", getNodeName(machine)))
-	}
-}
-
-func (c *controller) updateNodeToMachine(oldObj, newObj any) {
-	oldNode := oldObj.(*corev1.Node)
-	node := newObj.(*corev1.Node)
-	if node == nil {
-		klog.Errorf("Couldn't convert to node from object")
-		return
-	}
-
-	machine, err := c.getMachineFromNode(node.Name)
-	if err != nil {
-		klog.Errorf("Unable to handle update event for node %s, couldn't fetch associated machine. Error: %s", node.Name, err)
-		return
-	}
-
-	// check for the TriggerDeletionByMCM annotation on the node object
-	// if it is present then mark the machine object for deletion
-	if value, ok := node.Annotations[machineutils.TriggerDeletionByMCM]; ok && value == "true" {
-		if machine.DeletionTimestamp == nil {
-			klog.Infof("Node %s for machine %s is annotated to trigger deletion by MCM.", node.Name, machine.Name)
-			if err := c.controlMachineClient.Machines(c.namespace).Delete(context.Background(), machine.Name, metav1.DeleteOptions{}); err != nil {
-				klog.Errorf("Machine object %s backing the node %s could not be marked for deletion.", machine.Name, node.Name)
-				return
-			}
-			klog.Infof("Machine object %s backing the node %s marked for deletion.", machine.Name, node.Name)
-		}
-	}
-
-	// to reconcile on addition/removal of essential taints in machine lifecycle, example - critical component taint
-	if addedOrRemovedEssentialTaints(oldNode, node, machineutils.EssentialTaints) {
-		c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. atleast one of essential taints on backing node %q has changed", getNodeName(machine)))
-		return
-	}
-
-	if inPlaceUpdateLabelsChanged(oldNode, node) {
-		c.enqueueMachine(machine, fmt.Sprintf("handling node UPDATE event. in-place update label added or updated for node %q", getNodeName(machine)))
-		return
-	}
-
-	c.addNodeToMachine(newObj)
-}
-
-func (c *controller) deleteNodeToMachine(obj any) {
-	key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(obj)
-	if err != nil {
-		klog.Errorf("Couldn't get key for object %+v: %v", obj, err)
-		return
-	}
-
-	machine, err := c.getMachineFromNode(key)
-	if err != nil {
-		klog.Errorf("Couldn't fetch machine %s, Error: %s", key, err)
-		return
-	}
-
-	// donot respond if machine obj is already in termination flow
-	if machine.DeletionTimestamp == nil {
-		c.enqueueMachine(machine, fmt.Sprintf("backing node obj %q got deleted", key))
-	}
-}
-
-/*
-	SECTION
-	NodeToMachine operations
-*/
-
-func (c *controller) getMachineFromNode(nodeName string) (*v1alpha1.Machine, error) {
-	var (
-		list     = []string{nodeName}
-		selector = labels.NewSelector()
-		req, _   = labels.NewRequirement("node", selection.Equals, list)
-	)
-
-	selector = selector.Add(*req)
-	machines, _ := c.machineLister.List(selector)
-
-	if len(machines) > 1 {
-		return nil, errMultipleMachineMatch
-	} else if len(machines) < 1 {
-		return nil, errNoMachineMatch
-	}
-
-	return machines[0], nil
-}
-
-func inPlaceUpdateLabelsChanged(oldNode, node *corev1.Node) bool {
-	if oldNode == nil || node == nil {
-		return false
-	}
-
-	labelKeys := []string{
-		v1alpha1.LabelKeyNodeCandidateForUpdate,
-		v1alpha1.LabelKeyNodeSelectedForUpdate,
-		v1alpha1.LabelKeyNodeUpdateResult,
-	}
-
-	for _, key := range labelKeys {
-		oldVal, oldOk := oldNode.Labels[key]
-		newVal, newOk := node.Labels[key]
-		if (!oldOk && newOk) || (key == v1alpha1.LabelKeyNodeUpdateResult && oldVal != newVal) {
-			return true
-		}
-	}
-
-	return false
-}
-
-func addedOrRemovedEssentialTaints(oldNode, node *corev1.Node, taintKeys []string) bool {
-	mapOldNodeTaintKeys := make(map[string]bool)
-	mapNodeTaintKeys := make(map[string]bool)
-
-	for _, t := range oldNode.Spec.Taints {
-		mapOldNodeTaintKeys[t.Key] = true
-	}
-
-	for _, t := range node.Spec.Taints {
-		mapNodeTaintKeys[t.Key] = true
-	}
-
-	for _, tk := range taintKeys {
-		_, oldNodeHasTaint := mapOldNodeTaintKeys[tk]
-		_, newNodeHasTaint := mapNodeTaintKeys[tk]
-		if oldNodeHasTaint != newNodeHasTaint {
-			klog.V(2).Infof("Taint with key %q has been added/removed from the node %q", tk, node.Name)
-			return true
-		}
-	}
-	return false
-}
-
 /*
 	SECTION
 	Machine operations - Create, Delete
@@ -820,6 +641,9 @@ func (c *controller) triggerDeletionFlow(ctx context.Context, deleteMachineReque
 	case strings.Contains(machine.Status.LastOperation.Description, machineutils.InitiateVMDeletion):
 		return c.deleteVM(ctx, deleteMachineRequest)
 
+	case strings.Contains(machine.Status.LastOperation.Description, machineutils.RemoveNodeFinalizers):
+		return c.deleteNodeFinalizers(ctx, machine)
+
 	case strings.Contains(machine.Status.LastOperation.Description, machineutils.InitiateNodeDeletion):
 		return c.deleteNodeObject(ctx, machine)
 

pkg/util/provider/machinecontroller/machine_safety.go

@@ -170,28 +170,30 @@ func (c *controller) AnnotateNodesUnmanagedByMCM(ctx context.Context) (machineut
 				klog.Errorf("Couldn't fetch machine, Error: %s", err)
 			} else if err == errNoMachineMatch {
 
-				if !node.CreationTimestamp.Time.Before(time.Now().Add(c.safetyOptions.MachineCreationTimeout.Duration * -1)) {
-					// node creationTimestamp is NOT before now() - machineCreationTime
+				if time.Since(node.CreationTimestamp.Time) < c.safetyOptions.MachineCreationTimeout.Duration {
+					// node creationTimestamp is NOT before now() - machineCreationTimeout
 					// 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",
 				}
-
-				klog.V(3).Infof("Adding NotManagedByMCM annotation to Node %q", node.Name)
-				// err is returned only when node update fails
-				if err := c.updateNodeWithAnnotations(ctx, nodeCopy, annotations); err != nil {
+				// Remove MCM finalizer from orphan nodes to allow deletion
+				if err := c.removeNodeFinalizers(ctx, node); err != nil {
+					klog.Errorf("Failed to remove finalizer from orphan node %q: %v", node.Name, err)
 					return machineutils.MediumRetry, err
 				}
+				if _, annotationPresent := node.ObjectMeta.Annotations[machineutils.NotManagedByMCM]; !annotationPresent {
+					// if no backing machine object for a node, annotate it
+					nodeCopy := node.DeepCopy()
+					annotations := map[string]string{
+						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.updateNodeWithAnnotations(ctx, nodeCopy, annotations); err != nil {
+						return machineutils.MediumRetry, err
+					}
+				}
 			}
 		} else {
 			_, hasAnnot := node.Annotations[machineutils.NotManagedByMCM]

pkg/util/provider/machinecontroller/machine_safety_test.go

@@ -734,6 +734,70 @@ var _ = Describe("safety_logic", func() {
 					err:   nil,
 				},
 			}),
+			Entry("Annotate orphan node and remove its MCM finalizer", &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",
+							},
+							Finalizers: []string{
+								NodeFinalizerName,
+							},
+							CreationTimestamp: metav1.NewTime(metav1.Now().Add(-21 * 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",
+								machineutils.NotManagedByMCM: "1",
+							},
+							// Finalizer should be removed to allow deletion
+							Finalizers: []string{},
+						},
+					},
+					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 incorrectly assigned NotManagedByMCM annotation", &data{
 				setup: setup{
 					node: &corev1.Node{