Fix Race conditions in Targeted Deletion of machines by CA (#965)

* read TriggerDeletionByMCM on MCD and set MachinePriority=1 annotation on machines

* fixed dup import issue

* improved logging+annotation clear

* addressed final review comments

* removed todo, added unit-test binary files to .gitignore

* changed to better var names

* reverted code review change - nil/empty check is necessary

Author: elankath

.gitignore

@@ -34,3 +34,6 @@ hack/tools/bin/
 
 # gosec
 gosec-report.sarif
+
+# ignore test binary fines
+**/*.test

pkg/controller/deployment.go

@@ -25,14 +25,16 @@ package controller
 import (
 	"context"
 	"fmt"
+	"github.com/gardener/machine-controller-manager/pkg/util/annotations"
+	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"reflect"
 	"sync"
 	"time"
 
 	"k8s.io/klog/v2"
 
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
@@ -422,7 +424,7 @@ func (dc *controller) reconcileClusterMachineDeployment(key string) error {
 		return err
 	}
 	deployment, err := dc.controlMachineClient.MachineDeployments(dc.namespace).Get(ctx, name, metav1.GetOptions{})
-	if errors.IsNotFound(err) {
+	if apierrors.IsNotFound(err) {
 		klog.V(4).Infof("Deployment %v has been deleted", key)
 		return nil
 	}
@@ -512,6 +514,11 @@ func (dc *controller) reconcileClusterMachineDeployment(key string) error {
 		return err
 	}
 
+	err = dc.setMachinePriorityAnnotationAndUpdateTriggeredForDeletion(ctx, d)
+	if err != nil {
+		return err
+	}
+
 	if d.Spec.Paused {
 		klog.V(3).Infof("Scaling detected for machineDeployment %s which is paused", d.Name)
 		return dc.sync(ctx, d, machineSets, machineMap)
@@ -604,3 +611,58 @@ func (dc *controller) updateMachineDeploymentFinalizers(ctx context.Context, mac
 		dc.updateMachineDeploymentFinalizers(ctx, machineDeployment, finalizers)
 	}
 }
+
+func (dc *controller) setMachinePriorityAnnotationAndUpdateTriggeredForDeletion(ctx context.Context, mcd *v1alpha1.MachineDeployment) error {
+	var triggerForDeletionMachineNames, skipTriggerForDeletionMachineNames []string
+	triggerForDeletionMachineNames = annotations.GetMachineNamesTriggeredForDeletion(mcd)
+	if len(triggerForDeletionMachineNames) == 0 {
+		return nil
+	}
+	klog.V(3).Infof("MachineDeployment %q has #%d machine(s) marked for deletion, triggerForDeletionMachineNames=%v", mcd.Name, len(triggerForDeletionMachineNames), triggerForDeletionMachineNames)
+	for _, machineName := range triggerForDeletionMachineNames {
+		mc, err := dc.machineLister.Machines(dc.namespace).Get(machineName)
+		if apierrors.IsNotFound(err) {
+			klog.V(4).Infof("Machine %q is not found in MachineDeployment %q - skip setting MachinePriority=1 annotation", machineName, mcd.Name)
+			skipTriggerForDeletionMachineNames = append(skipTriggerForDeletionMachineNames, machineName)
+			continue
+		}
+		if machineutils.IsMachineFailedOrTerminating(mc) {
+			klog.V(4).Infof("Machine %q of MachineDeployment %q is in Failed/Terminating state - skip setting MachinePriority=1 annotation", machineName, mcd.Name)
+			skipTriggerForDeletionMachineNames = append(skipTriggerForDeletionMachineNames, machineName)
+			continue
+		}
+		if mc.Annotations[machineutils.MachinePriority] == "1" {
+			klog.V(4).Infof("Machine %q of MachineDeployment %q already marked with MachinePriority=1 annotation", machineName, mcd.Name)
+			continue
+		}
+		mcAdjust := mc.DeepCopy()
+		mcAdjust.Annotations[machineutils.MachinePriority] = "1"
+		_, err = dc.controlMachineClient.Machines(mcAdjust.Namespace).Update(ctx, mcAdjust, metav1.UpdateOptions{})
+		if err != nil {
+			klog.Errorf("Failed to set MachinePriority=1 annotation on Machine %q of MachineDeployment %q: %v", machineName, mcd.Name, err)
+			return err
+		}
+		klog.V(3).Infof("Machine %q of MachineDeployment %q marked with MachinePriority=1 annotation successfully", machineName, mcd.Name)
+	}
+
+	if len(skipTriggerForDeletionMachineNames) == 0 {
+		return nil
+	}
+
+	triggerForDeletionMachineNames = sets.NewString(triggerForDeletionMachineNames...).Delete(skipTriggerForDeletionMachineNames...).List()
+	triggerDeletionAnnotValue := annotations.CreateMachinesTriggeredForDeletionAnnotValue(triggerForDeletionMachineNames)
+
+	mcdAdjust := mcd.DeepCopy()
+	if triggerDeletionAnnotValue != "" {
+		mcdAdjust.Annotations[machineutils.TriggerDeletionByMCM] = triggerDeletionAnnotValue
+	} else {
+		delete(mcdAdjust.Annotations, machineutils.TriggerDeletionByMCM)
+	}
+	_, err := dc.controlMachineClient.MachineDeployments(mcd.Namespace).Update(ctx, mcdAdjust, metav1.UpdateOptions{})
+	if err != nil {
+		klog.Errorf("Failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNames), triggerDeletionAnnotValue)
+		return err
+	}
+	klog.V(3).Infof("Updated MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(triggerForDeletionMachineNames), triggerDeletionAnnotValue)
+	return nil
+}

pkg/util/annotations/annotations.go

@@ -6,7 +6,11 @@
 package annotations
 
 import (
+	"github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
+	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
 	v1 "k8s.io/api/core/v1"
+	"slices"
+	"strings"
 )
 
 // AddOrUpdateAnnotation tries to add an annotation. Returns a new copy of updated Node and true if something was updated
@@ -68,3 +72,17 @@ func DeleteAnnotation(nodeAnnotations map[string]string, annotations map[string]
 	}
 	return newAnnotations, deleted
 }
+
+// GetMachineNamesTriggeredForDeletion returns the set of machine names contained within the machineutils.TriggerDeletionByMCM annotation on the given MachineDeployment
+func GetMachineNamesTriggeredForDeletion(mcd *v1alpha1.MachineDeployment) []string {
+	if mcd.Annotations == nil || mcd.Annotations[machineutils.TriggerDeletionByMCM] == "" {
+		return nil
+	}
+	return strings.Split(mcd.Annotations[machineutils.TriggerDeletionByMCM], ",")
+}
+
+// CreateMachinesTriggeredForDeletionAnnotValue constructs the annotation value for machineutils.TriggerDeletionByMCM from the given machine names.
+func CreateMachinesTriggeredForDeletionAnnotValue(machineNames []string) string {
+	slices.Sort(machineNames)
+	return strings.Join(machineNames, ",")
+}

pkg/util/provider/machineutils/utils.go

@@ -6,6 +6,7 @@
 package machineutils
 
 import (
+	"github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
 	"time"
 
 	v1 "k8s.io/api/core/v1"
@@ -49,6 +50,8 @@ const (
 	NotManagedByMCM = "node.machine.sapcloud.io/not-managed-by-mcm"
 
 	// TriggerDeletionByMCM annotation on the node would trigger the deletion of the corresponding machine object in the control cluster
+	// This annotation can also be set on the MachineDeployment and contains the machine names for which deletion should be triggered.
+	// This feature is leveraged by the CA-MCM cloud provider.
 	TriggerDeletionByMCM = "node.machine.sapcloud.io/trigger-deletion-by-mcm"
 
 	// NodeUnhealthy is a node termination reason for failed machines
@@ -86,3 +89,11 @@ const (
 // EssentialTaints are taints on node object which if added/removed, require an immediate reconcile by machine controller
 // TODO: update this when taints for ALT updation and PostCreate operations is introduced.
 var EssentialTaints = []string{TaintNodeCriticalComponentsNotReady}
+
+// IsMachineFailedOrTerminating returns true if machine is Failed or already being Terminated.
+func IsMachineFailedOrTerminating(machine *v1alpha1.Machine) bool {
+	if !machine.GetDeletionTimestamp().IsZero() || machine.Status.CurrentStatus.Phase == v1alpha1.MachineFailed {
+		return true
+	}
+	return false
+}