[master] Fix preservation reconciliation to prevent deviation from regular flow for non-preservation-bound machines and prevent machines getting stuck in creation flow beyond CreationTimeout

pkg/controller/controller_utils.go

@@ -747,7 +747,7 @@ func (s ActiveMachines) Less(i, j int) bool {
 	}
 	if isPreservedI { // both machines are preserved, in which case deprioritize explicitly preserved machines over auto-preserved
 		isAutoPreserved := func(m *v1alpha1.Machine) bool {
-			return m.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM
+			return m.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValueAutoPreserved
 		}
 		isAutoPreservedI := isAutoPreserved(s[i])
 		isAutoPreservedJ := isAutoPreserved(s[j])

pkg/controller/deployment_machineset_util.go

@@ -136,9 +136,9 @@ func calculateMachineSetStatus(is *v1alpha1.MachineSet, filteredMachines []*v1al
 			}
 			failedMachines = append(failedMachines, machineSummary)
 		}
-		// Count number of failed machines annotated with PreserveMachineAnnotationValuePreservedByMCM
+		// Count number of failed machines annotated with PreserveAnnotationValueAutoPreserved
 		// Cannot combine with above if block in case auto-preservation is not complete yet
-		if machine.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM {
+		if machine.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValueAutoPreserved {
 			autoPreserveFailedMachineCount++
 		}
 	}

pkg/controller/machineset.go

@@ -920,7 +920,7 @@ func (c *controller) shouldFailedMachineBeTerminated(machine *v1alpha1.Machine)
 		return true
 	}
 	switch preserveValue {
-	case machineutils.PreserveMachineAnnotationValueWhenFailed, machineutils.PreserveMachineAnnotationValueNow, machineutils.PreserveMachineAnnotationValuePreservedByMCM: // this is in case preservation process is not complete yet
+	case machineutils.PreserveMachineAnnotationValueWhenFailed, machineutils.PreserveMachineAnnotationValueNow, machineutils.PreserveMachineAnnotationValueAutoPreserved: // this is in case preservation process is not complete yet
 		return false
 	case machineutils.PreserveMachineAnnotationValueFalse:
 		return true
@@ -949,7 +949,7 @@ func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context,
 	var others []*v1alpha1.Machine
 	for _, m := range machines {
 		// check if machine is already annotated for preservation, if yes, skip. Machine controller will take care of the rest.
-		if machineutils.IsMachineFailed(m) && !machineutils.PreventAutoPreserveAnnotationValues.Has(m.Annotations[machineutils.PreserveMachineAnnotationKey]) {
+		if machineutils.IsMachineFailed(m) && !machineutils.AllowedPreserveAnnotationValues.Has(m.Annotations[machineutils.PreserveMachineAnnotationKey]) {
 			autoPreservationCandidates = append(autoPreservationCandidates, m)
 		} else {
 			others = append(others, m)
@@ -978,7 +978,7 @@ func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context,
 func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machines []*v1alpha1.Machine, numToStop int) int {
 	var autoPreservedMachines []*v1alpha1.Machine
 	for _, m := range machines {
-		if m.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM {
+		if m.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValueAutoPreserved {
 			autoPreservedMachines = append(autoPreservedMachines, m)
 		}
 	}
@@ -998,7 +998,7 @@ func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machin
 		klog.V(2).Infof("Removing auto-preservation annotation from machine %q as AutoPreserveFailedMachineMax is breached", m.Name)
 		updatedMachine, err := machineutils.UpdateMachineWithRetries(ctx, c.controlMachineClient.Machines(m.Namespace), c.machineLister, m.Namespace, m.Name, removeAutoPreserveAnnotationFromMachine)
 		if err != nil {
-			klog.Warningf("Error removing %q=%q annotation from machine %q: %v.", machineutils.PreserveMachineAnnotationKey, machineutils.PreserveMachineAnnotationValuePreservedByMCM, m.Name, err)
+			klog.Warningf("Error removing %q=%q annotation from machine %q: %v.", machineutils.PreserveMachineAnnotationKey, machineutils.PreserveMachineAnnotationValueAutoPreserved, m.Name, err)
 			continue
 		}
 		autoPreservedMachines[index] = updatedMachine
@@ -1011,7 +1011,7 @@ func addAutoPreserveAnnotationOnMachine(machineToUpdate *v1alpha1.Machine) error
 	if machineToUpdate.Annotations == nil {
 		machineToUpdate.Annotations = make(map[string]string)
 	}
-	machineToUpdate.Annotations[machineutils.PreserveMachineAnnotationKey] = machineutils.PreserveMachineAnnotationValuePreservedByMCM
+	machineToUpdate.Annotations[machineutils.PreserveMachineAnnotationKey] = machineutils.PreserveMachineAnnotationValueAutoPreserved
 	return nil
 }
 

pkg/controller/machineset_test.go

@@ -2163,10 +2163,10 @@ var _ = Describe("machineset", func() {
 			updatedMachine3, _ := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), testMachine3.Name, metav1.GetOptions{})
 			updatedMachine4, _ := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), testMachine4.Name, metav1.GetOptions{})
 			preservedCount := 0
-			if updatedMachine1.Annotations != nil && updatedMachine1.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM {
+			if updatedMachine1.Annotations != nil && updatedMachine1.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValueAutoPreserved {
 				preservedCount++
 			}
-			if updatedMachine2.Annotations != nil && updatedMachine2.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM {
+			if updatedMachine2.Annotations != nil && updatedMachine2.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValueAutoPreserved {
 				preservedCount++
 			}
 			// Running machine should not be auto-preserved in any of the cases
@@ -2176,7 +2176,7 @@ var _ = Describe("machineset", func() {
 
 			for _, m := range tc.setup.additionalMachines {
 				updatedMachine, _ := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), m.Name, metav1.GetOptions{})
-				if updatedMachine.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValuePreservedByMCM {
+				if updatedMachine.Annotations[machineutils.PreserveMachineAnnotationKey] == machineutils.PreserveMachineAnnotationValueAutoPreserved {
 					preservedCount++
 				}
 			}
@@ -2237,7 +2237,7 @@ var _ = Describe("machineset", func() {
 								Name:      "machine-5",
 								Namespace: testNamespace,
 								Annotations: map[string]string{
-									machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValuePreservedByMCM,
+									machineutils.PreserveMachineAnnotationKey: machineutils.PreserveMachineAnnotationValueAutoPreserved,
 								},
 							},
 							Status: machinev1.MachineStatus{

pkg/util/provider/machinecontroller/machine.go

@@ -744,22 +744,27 @@ func (c *controller) isCreationProcessing(machine *v1alpha1.Machine) bool {
 	Machine Preservation operations
 */
 
+// preserveStateInfo encapsulates the preservation annotation values found
+// on the machine and node objects, along with the effective preservation value for the machine
+// and the last applied node preserve value by MCM.
+type preserveStateInfo struct {
+	nodeAnnotated         bool
+	machineAnnotated      bool
+	nodeValue             string
+	machineValue          string
+	lastAppliedNodeValue  string
+	preserveExpiryTimeSet bool
+}
+
 // manageMachinePreservation manages machine preservation based on the preserve annotation values on the node and machine objects.
 func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1alpha1.Machine) (retry machineutils.RetryPeriod, err error) {
-	machineAnnotationsSynced := false
-	clone := machine.DeepCopy()
 	defer func() {
-		// This needs to be done for cases when machine is neither preserved nor un-preserved (e.g. when preserve changes from now to when-failed on a Failed machine),
-		// but the LastAppliedNodePreserveValueAnnotation needs to be synced to the machine object.
-		// We compare annotation value in the clone with the original machine object to see if an update is required
-		if err == nil && !machineAnnotationsSynced && clone.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] != machine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] {
-			_, err = c.controlMachineClient.Machines(clone.Namespace).Update(ctx, clone, metav1.UpdateOptions{})
-			if err != nil {
-				klog.Errorf("error updating LastAppliedNodePreserveValueAnnotation value on machine %q: %v", machine.Name, err)
-			}
-		}
 		if err != nil {
-			if apierrors.IsConflict(err) {
+			if apierrors.IsNotFound(err) {
+				klog.Warningf("Error during preservation flow: %v", err.Error())
+				retry = machineutils.LongRetry
+				err = nil
+			} else if apierrors.IsConflict(err) {
 				retry = machineutils.ConflictRetry
 			} else {
 				retry = machineutils.ShortRetry
@@ -768,76 +773,182 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a
 			retry = machineutils.LongRetry
 		}
 	}()
-
-	nodeName := clone.Labels[v1alpha1.NodeLabelKey]
-	nodeAnnotationValue := ""
-	if nodeName != "" {
-		nodeAnnotationValue, err = c.getNodePreserveAnnotationValue(nodeName)
-		if err != nil {
-			return
-		}
+	nodeName := machine.Labels[v1alpha1.NodeLabelKey]
+	// We buffer the error returned here until we can tell if the machine is preservation-bound.
+	preserveInfo, getErr := c.getPreserveStateInfo(machine)
+	if preserveInfo.machineAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.machineValue) {
+		// If machine is annotated incorrectly, log and proceed as though machine is not annotated.
+		klog.Warningf("Preserve annotation %q=%q on machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.machineValue, machine.Name)
+		preserveInfo.machineAnnotated = false
+		preserveInfo.machineValue = ""
+	}
+	if preserveInfo.nodeAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.nodeValue) {
+		klog.Warningf("Preserve annotation %q=%q on node %q backing machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.nodeValue, nodeName, machine.Name)
+		return
 	}
-	var effectivePreserveValue string
-	effectivePreserveValue, clone.Annotations = getEffectivePreservationAnnotations(nodeAnnotationValue, clone.Annotations)
-	if !machineutils.AllowedPreserveAnnotationValues.Has(effectivePreserveValue) {
-		if effectivePreserveValue == nodeAnnotationValue {
-			klog.Warningf("Preserve annotation value %q on node %q is invalid", effectivePreserveValue, nodeName)
-		} else {
-			klog.Warningf("Preserve annotation value %q on machine %q is invalid", effectivePreserveValue, clone.Name)
-		}
+	preservationBound := isMachinePreservationBound(&preserveInfo)
+	if !preservationBound {
+		// We clear the error here to prevent preservation logic from interfering with non-preservation-bound machines.
+		err = nil
 		return
+	} else if getErr != nil {
+		if !apierrors.IsNotFound(getErr) {
+			err = getErr
+			return
+		}
+		klog.Warningf("Couldn't find node %q for machine %q", nodeName, machine.Name)
+		err = nil
 	}
+	// Note: when the backing node cannot be found, we assume the machine's annotation value needs to be enforced to enable
+	// preservation of the machine object.
+	effectivePreserveValue := getEffectivePreservationAnnotations(&preserveInfo, getErr)
+
+	var removeAnnotations bool
+	clone := machine.DeepCopy()
 	switch effectivePreserveValue {
 	case "", machineutils.PreserveMachineAnnotationValueFalse:
-		machineAnnotationsSynced, err = c.stopPreservationIfActive(ctx, clone, false)
+		clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
 	case machineutils.PreserveMachineAnnotationValueWhenFailed:
 		// on timing out, remove preserve annotation to prevent incorrect re-preservation
 		if machineutils.IsMachinePreservationExpired(clone) {
-			machineAnnotationsSynced, err = c.stopPreservationIfActive(ctx, clone, true)
+			removeAnnotations = true
+			clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
 		} else if !machineutils.IsMachineFailed(clone) {
-			machineAnnotationsSynced, err = c.stopPreservationIfActive(ctx, clone, false)
+			clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
 		} else {
-			err = c.preserveMachine(ctx, clone, effectivePreserveValue)
+			clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue)
 		}
 	case machineutils.PreserveMachineAnnotationValueNow:
 		if machineutils.IsMachinePreservationExpired(clone) {
 			// on timing out, remove preserve annotation to prevent incorrect re-preservation
-			machineAnnotationsSynced, err = c.stopPreservationIfActive(ctx, clone, true)
+			removeAnnotations = true
+			clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
 		} else {
-			err = c.preserveMachine(ctx, clone, effectivePreserveValue)
+			clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue)
 		}
-	case machineutils.PreserveMachineAnnotationValuePreservedByMCM:
+	case machineutils.PreserveMachineAnnotationValueAutoPreserved:
 		if !machineutils.IsMachineFailed(clone) || machineutils.IsMachinePreservationExpired(clone) {
 			// To prevent incorrect re-preservation of a recovered, previously auto-preserved machine on future failures
 			// (since the autoPreserveFailedMachineCount maintained by the machineSetController, may have changed),
 			// in addition to stopping preservation, we also remove the preservation annotation on the machine.
-			machineAnnotationsSynced, err = c.stopPreservationIfActive(ctx, clone, true)
+			removeAnnotations = true
+			clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
 		} else {
-			err = c.preserveMachine(ctx, clone, effectivePreserveValue)
+			clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue)
 		}
 	}
 	if err != nil {
 		return
 	}
-	// This is to handle the case where a preserved machine recovers from Failed to Running
-	// in which case, pods should be allowed to be scheduled onto the node
-	if machineutils.IsMachineActive(clone) && nodeName != "" {
-		err = c.uncordonNodeIfCordoned(ctx, nodeName)
+	// For the preserve=now path (preservation still active), uncordon the node if the machine
+	// has recovered to Running. For all other cases, stopPreservationIfActive path handles uncordon internally.
+	if clone.Status.CurrentStatus.PreserveExpiryTime != nil && clone.Status.CurrentStatus.Phase == v1alpha1.MachineRunning && nodeName != "" {
+		var node *corev1.Node
+		node, err = c.nodeLister.Get(nodeName)
+		if err != nil {
+			if apierrors.IsNotFound(err) {
+				err = nil
+				return
+			}
+			return
+		}
+		err = c.uncordonNodeIfCordoned(ctx, node)
+		if err != nil {
+			return
+		}
+	}
+
+	if shouldAnnotationsBeUpdatedOnMachine(removeAnnotations, &preserveInfo) {
+		err = c.updatePreserveAnnotationOnMachine(ctx, preserveInfo.nodeValue, clone)
 	}
 	return
 }
 
-// getEffectivePreservationAnnotations returns the effective preservation value, and updates the machine Annotations related to preservation
-func getEffectivePreservationAnnotations(nodeAnnotationValue string, machineAnnotations map[string]string) (string, map[string]string) {
+// getEffectivePreservationAnnotations returns the effective preservation value.
+//
+// If there is no active node annotation AND no previously-applied node annotation,
+// enforce machine's preserve annotation.
+// Otherwise, the node annotation takes precedence (even if now empty/removed).
+//
+// lastAppliedNodeValue is required to handle the following scenario:
+//
+//	T1: Node and Machine both have the same annotation with the same value. (MCM is up and running).
+//	T2 (T2 > T1): MCM went down.
+//	T3 (T3 > T2): Node annotation was removed.
+//	T4 (T4 > T3): MCM came back up.
+//	At T4 it sees a Node with no preserve annotation but a Machine with a preserve annotation.
+//	It continues to preserve the machine.
+func getEffectivePreservationAnnotations(info *preserveStateInfo, getPreserveStateErr error) string {
+	// If the node cannot be found, nodeValue is "".
+	// In this case, we want the machine's annotation value to be enforced.
+	if apierrors.IsNotFound(getPreserveStateErr) {
+		return info.machineValue
+	}
 	// If there is no active node annotation AND no previously-applied node annotation,
 	// enforce machine's preserve annotation.
 	// Otherwise, the node annotation takes precedence (even if now empty/removed).
-	if nodeAnnotationValue == "" && machineAnnotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] == "" {
-		return machineAnnotations[machineutils.PreserveMachineAnnotationKey], machineAnnotations
-	}
-	clonedMachineAnnotations := make(map[string]string)
-	maps.Copy(clonedMachineAnnotations, machineAnnotations)
-	delete(clonedMachineAnnotations, machineutils.PreserveMachineAnnotationKey)
-	clonedMachineAnnotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] = nodeAnnotationValue
-	return nodeAnnotationValue, clonedMachineAnnotations
+	if info.nodeValue == "" && info.lastAppliedNodeValue == "" {
+		return info.machineValue
+	}
+	return info.nodeValue
+}
+
+func isMachinePreservationBound(info *preserveStateInfo) bool {
+	// if machine has no preservation state, the machine is not preservation-bound
+	if !info.preserveExpiryTimeSet && !info.nodeAnnotated && !info.machineAnnotated && info.lastAppliedNodeValue == "" {
+		return false
+	}
+	return true
+}
+
+func (c *controller) getPreserveStateInfo(machine *v1alpha1.Machine) (preserveStateInfo, error) {
+	var info preserveStateInfo
+	if machine.Annotations != nil {
+		info.machineValue, info.machineAnnotated = machine.Annotations[machineutils.PreserveMachineAnnotationKey]
+		info.lastAppliedNodeValue = machine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey]
+	}
+	if machine.Status.CurrentStatus.PreserveExpiryTime != nil {
+		info.preserveExpiryTimeSet = true
+	}
+	nodeName := machine.Labels[v1alpha1.NodeLabelKey]
+	if nodeName != "" {
+		node, err := c.nodeLister.Get(nodeName)
+		if err != nil {
+			return info, err
+		}
+		info.nodeValue, info.nodeAnnotated = node.Annotations[machineutils.PreserveMachineAnnotationKey]
+	}
+	return info, nil
+}
+
+// shouldAnnotationsBeUpdatedOnMachine returns true when the machine's annotation tracking needs
+// to be synced after a preservation action.
+func shouldAnnotationsBeUpdatedOnMachine(removeAnnotations bool, preserveInfo *preserveStateInfo) bool {
+	// annotations were already removed by stopPreservationIfActive — nothing left to sync
+	if removeAnnotations {
+		return false
+	}
+	// node annotation is not in control — machine annotation prevails, no sync needed
+	if !preserveInfo.nodeAnnotated && preserveInfo.lastAppliedNodeValue == "" {
+		return false
+	}
+	// node value is unchanged and machine has no annotation to clear — nothing has changed
+	if preserveInfo.nodeValue == preserveInfo.lastAppliedNodeValue && !preserveInfo.machineAnnotated {
+		return false
+	}
+	return true
+}
+
+// updatePreserveAnnotationOnMachine clears the machine's PreserveMachineAnnotationKey and sets
+// [machineutils.LastAppliedNodePreserveValueAnnotationKey] to nodeValue.
+func (c *controller) updatePreserveAnnotationOnMachine(ctx context.Context, nodeValue string, machine *v1alpha1.Machine) error {
+	clone := machine.DeepCopy()
+	if clone.Annotations == nil {
+		clone.Annotations = make(map[string]string)
+	} else {
+		delete(clone.Annotations, machineutils.PreserveMachineAnnotationKey)
+	}
+	clone.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] = nodeValue
+	_, err := c.controlMachineClient.Machines(clone.Namespace).Update(ctx, clone, metav1.UpdateOptions{})
+	return err
 }