Preservation bugfix to put PreserveExpiryTime whenever machine transitions to Failed phase

cmd/machine-controller-manager/app/options/options.go

@@ -70,6 +70,7 @@ func NewMCMServer() *MCMServer {
 				SafetyUp:                        2,
 				SafetyDown:                      1,
 				MachineSafetyOvershootingPeriod: metav1.Duration{Duration: 1 * time.Minute},
+				MachinePreserveTimeout:          metav1.Duration{Duration: 96 * time.Hour},
 			},
 		},
 	}
@@ -98,6 +99,7 @@ func (s *MCMServer) AddFlags(fs *pflag.FlagSet) {
 	fs.Int32Var(&s.SafetyOptions.SafetyDown, "safety-down", s.SafetyOptions.SafetyDown, "Upper-limit minus safety-down value gives the lower-limit. This is the limits below which any temporarily frozen machineSet/machineDeployment object is unfrozen. lower-limit = desired + maxSurge (if applicable) + safetyUp - safetyDown.")
 
 	fs.DurationVar(&s.SafetyOptions.MachineSafetyOvershootingPeriod.Duration, "machine-safety-overshooting-period", s.SafetyOptions.MachineSafetyOvershootingPeriod.Duration, "Time period (in duration) used to poll for overshooting of machine objects backing a machineSet by safety controller.")
+	fs.DurationVar(&s.SafetyOptions.MachinePreserveTimeout.Duration, "machine-preserve-timeout", s.SafetyOptions.MachinePreserveTimeout.Duration, "Duration for which a failed machine should be preserved if it has the appropriate preserve annotation set.")
 
 	fs.BoolVar(&s.AutoscalerScaleDownAnnotationDuringRollout, "autoscaler-scaledown-annotation-during-rollout", true, "Add cluster autoscaler scale-down disabled annotation during roll-out.")
 

pkg/controller/deployment.go

@@ -558,7 +558,7 @@ func (dc *controller) reconcileClusterMachineDeployment(key string) error {
 		return err
 	}
 
-	err = dc.updateMachineAndMachineDeploymentDeletionAnnotations(ctx, d)
+	d, err = dc.updateMachineAndMachineDeploymentDeletionAnnotations(ctx, d, machineMap)
 	if err != nil {
 		return err
 	}
@@ -659,10 +659,10 @@ func (dc *controller) updateMachineDeploymentFinalizers(ctx context.Context, mac
 	}
 }
 
-func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx context.Context, mcd *v1alpha1.MachineDeployment) (err error) {
+func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx context.Context, mcd *v1alpha1.MachineDeployment, machineMap map[types.UID]*v1alpha1.MachineList) (*v1alpha1.MachineDeployment, error) {
 	tgd := dc.computeMachineTriggerDeletionData(mcd)
 	if tgd == nil {
-		return nil
+		return mcd, nil
 	}
 
 	if tgd.triggerDeletionAnnotationValueChanged {
@@ -674,36 +674,47 @@ func (dc *controller) updateMachineAndMachineDeploymentDeletionAnnotations(ctx c
 		if mcdDeepCopy.Annotations[machineutils.TriggerDeletionByMCM] == "" {
 			delete(mcdDeepCopy.Annotations, machineutils.TriggerDeletionByMCM)
 		}
-		_, err = dc.controlMachineClient.MachineDeployments(mcd.Namespace).Update(ctx, mcdDeepCopy, metav1.UpdateOptions{})
+		updatedMCD, err := dc.controlMachineClient.MachineDeployments(mcd.Namespace).Update(ctx, mcdDeepCopy, metav1.UpdateOptions{})
 		if err != nil {
 			klog.Errorf("failed to update MachineDeployment %q with #%d machine names still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(tgd.markedMachines), mcdDeepCopy.Annotations[machineutils.TriggerDeletionByMCM])
-			return
+			return mcd, err
 		}
 		klog.V(3).Infof("Updated MachineDeployment %q with #%d machines still pending deletion, triggerDeletionAnnotValue=%q", mcd.Name, len(tgd.markedMachines), mcdDeepCopy.Annotations[machineutils.TriggerDeletionByMCM])
+		mcd = updatedMCD
 	}
 
 	for i, machine := range tgd.markedMachines {
 		if machine.Annotations[machineutils.MachinePriority] == "1" && machine.Annotations[machineutils.MarkedForDeletionTime] != "" {
 			klog.V(4).Infof("Machine %q of MachineDeployment %q already has MachinePriority=1 and MarkedForDeletionTime=%q annotation", machine.Name, mcd.Name, machine.Annotations[machineutils.MarkedForDeletionTime])
 			continue
 		}
-		machineDeepCopy := machine.DeepCopy()
-		if machineDeepCopy.Annotations == nil {
-			machineDeepCopy.Annotations = make(map[string]string)
-		}
-		machineDeepCopy.Annotations[machineutils.MachinePriority] = "1"
-		if machineDeepCopy.Annotations[machineutils.MarkedForDeletionTime] == "" {
-			machineDeepCopy.Annotations[machineutils.MarkedForDeletionTime] = tgd.markedMachineDeletionTimes[i]
-		}
-		_, err = dc.controlMachineClient.Machines(machine.Namespace).Update(ctx, machineDeepCopy, metav1.UpdateOptions{})
+		deletionTime := tgd.markedMachineDeletionTimes[i]
+		updatedMachine, err := machineutils.PatchMachine(ctx, dc.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error {
+			if m.Annotations == nil {
+				m.Annotations = make(map[string]string)
+			}
+			m.Annotations[machineutils.MachinePriority] = "1"
+			if m.Annotations[machineutils.MarkedForDeletionTime] == "" {
+				m.Annotations[machineutils.MarkedForDeletionTime] = deletionTime
+			}
+			return nil
+		}, true)
 		if err != nil {
 			klog.Errorf("failed to set MachinePriority=1 annotation on Machine %q of MachineDeployment %q: %v", machine.Name, mcd.Name, err)
-			return
+			return mcd, err
+		}
+		// TODO: not neat. refactor later.
+		for _, machineList := range machineMap {
+			for i, machine := range machineList.Items {
+				if machine.Name == updatedMachine.Name && machine.Namespace == updatedMachine.Namespace {
+					machineList.Items[i] = *updatedMachine
+				}
+			}
 		}
 		klog.V(3).Infof("Machine %q of MachineDeployment %q marked with MachinePriority=1 annotation successfully", machine.Name, mcd.Name)
 	}
 
-	return
+	return mcd, nil
 }
 
 // computeMachineTriggerDeletionData computes the data related to machines that are triggered for deletion based on the annotation on the MachineDeployment.

pkg/controller/deployment_sync.go

@@ -191,7 +191,7 @@ func (dc *controller) addHashKeyToISAndMachines(ctx context.Context, is *v1alpha
 	}
 
 	// 2. Update all machines managed by the rs to have the new hash label, so they will be correctly adopted.
-	if err := LabelMachinesWithHash(ctx, machineList, dc.controlMachineClient, dc.machineLister, is.Namespace, is.Name, hash); err != nil {
+	if err := LabelMachinesWithHash(ctx, machineList, dc.controlMachineClient, is.Namespace, is.Name, hash); err != nil {
 		return nil, fmt.Errorf("error in adding template hash label %s to machines %+v: %s", hash, machineList, err)
 	}
 

pkg/controller/deployment_test.go

@@ -21,6 +21,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 )
 
@@ -2356,7 +2357,10 @@ var _ = Describe("machineDeployment", func() {
 
 				defer trackers.Stop()
 				waitForCacheSync(stop, c)
-				err := c.updateMachineAndMachineDeploymentDeletionAnnotations(context.TODO(), testMachineDeployment)
+				err := func() error {
+					_, err := c.updateMachineAndMachineDeploymentDeletionAnnotations(context.TODO(), testMachineDeployment, map[types.UID]*machinev1.MachineList{})
+					return err
+				}()
 				Expect(err).To(BeNil())
 
 				waitForCacheSync(stop, c)

pkg/controller/deployment_util.go

@@ -41,7 +41,6 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/apimachinery/pkg/util/errors"
 	intstrutil "k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/klog/v2"
@@ -963,26 +962,22 @@ func WaitForMachinesHashPopulated(ctx context.Context, c v1alpha1listers.Machine
 }
 
 // LabelMachinesWithHash labels all machines in the given machineList with the new hash label.
-func LabelMachinesWithHash(ctx context.Context, machineList *v1alpha1.MachineList, c v1alpha1client.MachineV1alpha1Interface, machineLister v1alpha1listers.MachineLister, namespace, name, hash string) error {
-	for _, machine := range machineList.Items {
+func LabelMachinesWithHash(ctx context.Context, machineList *v1alpha1.MachineList, c v1alpha1client.MachineV1alpha1Interface, namespace, name, hash string) error {
+	for i, machine := range machineList.Items {
 		// Ignore inactive Machines.
 		if !machineutils.IsMachineActive(&machine) {
 			continue
 		}
 		// Only label the machine that doesn't already have the new hash
 		if machine.Labels[v1alpha1.DefaultMachineDeploymentUniqueLabelKey] != hash {
-			_, err := machineutils.UpdateMachineWithRetries(ctx, c.Machines(machine.Namespace), machineLister, machine.Namespace, machine.Name,
-				func(machineToUpdate *v1alpha1.Machine) error {
-					// Precondition: the machine doesn't contain the new hash in its label.
-					if machineToUpdate.Labels[v1alpha1.DefaultMachineDeploymentUniqueLabelKey] == hash {
-						return errors.ErrPreconditionViolated
-					}
-					machineToUpdate.Labels = labelsutil.AddLabel(machineToUpdate.Labels, v1alpha1.DefaultMachineDeploymentUniqueLabelKey, hash)
-					return nil
-				})
+			updatedMachine, err := machineutils.PatchMachine(ctx, c.Machines(machine.Namespace), &machine, func(m *v1alpha1.Machine) error {
+				m.Labels = labelsutil.AddLabel(m.Labels, v1alpha1.DefaultMachineDeploymentUniqueLabelKey, hash)
+				return nil
+			}, true)
 			if err != nil {
 				return fmt.Errorf("error in adding template hash label %s to machine %q: %v", hash, machine.Name, err)
 			}
+			machineList.Items[i] = *updatedMachine
 			klog.V(4).Infof("Labeled machine %s/%s of MachineSet %s/%s with hash %s.", machine.Namespace, machine.Name, namespace, name, hash)
 		}
 	}

pkg/controller/machineset.go

@@ -41,7 +41,7 @@ import (
 	"k8s.io/klog/v2"
 	"k8s.io/utils/integer"
 
-	"github.com/gardener/machine-controller-manager/pkg/apis/machine"
+	machineapi "github.com/gardener/machine-controller-manager/pkg/apis/machine"
 	"github.com/gardener/machine-controller-manager/pkg/apis/machine/v1alpha1"
 	"github.com/gardener/machine-controller-manager/pkg/apis/machine/validation"
 	"github.com/gardener/machine-controller-manager/pkg/util/provider/machineutils"
@@ -506,7 +506,7 @@ func (c *controller) reconcileClusterMachineSet(key string) error {
 	}
 
 	// Validate MachineSet
-	internalMachineSet := &machine.MachineSet{}
+	internalMachineSet := &machineapi.MachineSet{}
 	err = c.internalExternalScheme.Convert(machineSet, internalMachineSet, nil)
 	if err != nil {
 		return err
@@ -534,19 +534,25 @@ func (c *controller) reconcileClusterMachineSet(key string) error {
 	// list all machines to include the machines that don't match the rs`s selector
 	// anymore but has the stale controller ref.
 	// TODO: Do the List and Filter in a single pass, or use an index.
-	filteredMachines, err := c.machineLister.List(labels.Everything())
+	machineList, err := c.machineLister.List(labels.Everything())
 	if err != nil {
 		return err
 	}
 
 	// NOTE: filteredMachines are pointing to objects from cache - if you need to
 	// modify them, you need to copy it first.
-	filteredMachines, err = c.claimMachines(ctx, machineSet, selector, filteredMachines)
+	filteredMachines, err := c.claimMachines(ctx, machineSet, selector, machineList)
 	if err != nil {
 		return err
 	}
 
+	// Deep-copy to avoid mutating cache objects downstream.
+	for i, m := range filteredMachines {
+		filteredMachines[i] = m.DeepCopy()
+	}
+
 	// syncMachinesNodeTemplates syncs the nodeTemplate with claimedMachines if any of the machine's nodeTemplate has changed.
+	// TODO: too many update calls for the same machine object. Try to reduce it.
 	err = c.syncMachinesNodeTemplates(ctx, filteredMachines, machineSet)
 	if err != nil {
 		return err
@@ -563,7 +569,10 @@ func (c *controller) reconcileClusterMachineSet(key string) error {
 		return err
 	}
 
-	filteredMachines = c.manageAutoPreservationOfFailedMachines(ctx, filteredMachines, machineSet)
+	filteredMachines, err = c.manageAutoPreservationOfFailedMachines(ctx, filteredMachines, machineSet)
+	if err != nil {
+		return err
+	}
 
 	// TODO: Fix working of expectations to reflect correct behaviour
 	// machineSetNeedsSync := c.expectations.SatisfiedExpectations(key)
@@ -932,19 +941,20 @@ func (c *controller) shouldFailedMachineBeTerminated(machine *v1alpha1.Machine)
 // manageAutoPreservationOfFailedMachines annotates failed machines with preserve=auto-preserved annotation
 // to trigger preservation of the machines, by the machine controller, up to the limit defined in the
 // MachineSet's AutoPreserveFailedMachineMax field. If the AutoPreserveFailedMachineMax limit is breached, it removes the preserve=auto-preserved annotation from the oldest annotated machines.
-func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context, machines []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) []*v1alpha1.Machine {
+func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context, machines []*v1alpha1.Machine, machineSet *v1alpha1.MachineSet) ([]*v1alpha1.Machine, error) {
 	// TODO@thiyyakat: if preservation is to be honoured across updates, capacity remaining should consider machines in all machinesets
 	autoPreservationCapacityRemaining := machineSet.Spec.AutoPreserveFailedMachineMax - machineSet.Status.AutoPreserveFailedMachineCount
 	if autoPreservationCapacityRemaining == 0 {
 		// no capacity remaining, nothing to do
-		return machines
+		return machines, nil
 	} else if autoPreservationCapacityRemaining < 0 { // when autoPreserveFailedMachineMax is decreased, it can be negative.
 		numStillExceeding := c.stopAutoPreservationForMachines(ctx, machines, int(-autoPreservationCapacityRemaining))
 		if numStillExceeding > 0 {
 			klog.V(2).Infof("Attempted to decrease count of auto-preserved machines, but there are still %d violations of AutoPreserveFailedMachineMax.", numStillExceeding)
 		}
-		return machines
+		return machines, nil
 	}
+
 	var autoPreservationCandidates []*v1alpha1.Machine
 	var others []*v1alpha1.Machine
 	for _, m := range machines {
@@ -955,24 +965,39 @@ func (c *controller) manageAutoPreservationOfFailedMachines(ctx context.Context,
 			others = append(others, m)
 		}
 	}
+
 	sort.Slice(autoPreservationCandidates, func(i, j int) bool {
 		return autoPreservationCandidates[i].CreationTimestamp.After(autoPreservationCandidates[j].CreationTimestamp.Time)
 	})
-	for index, m := range autoPreservationCandidates {
+
+	for index, machine := range autoPreservationCandidates {
 		if autoPreservationCapacityRemaining == 0 {
 			break
 		}
-		klog.V(2).Infof("Annotating failed machine %q for auto-preservation as part of machine set %q", m.Name, machineSet.Name)
-		updatedMachine, err := machineutils.UpdateMachineWithRetries(ctx, c.controlMachineClient.Machines(m.Namespace), c.machineLister, m.Namespace, m.Name, addAutoPreserveAnnotationOnMachine)
+
+		klog.V(2).Infof("Annotating failed machine %q for auto-preservation and setting PreserveExpiryTime as part of machine set %q", machine.Name, machineSet.Name)
+		updatedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error {
+			if m.Annotations == nil {
+				m.Annotations = make(map[string]string)
+			}
+			m.Annotations[machineutils.PreserveMachineAnnotationKey] = machineutils.PreserveMachineAnnotationValueAutoPreserved
+			if m.Spec.MachineConfiguration != nil && m.Spec.MachineConfiguration.MachinePreserveTimeout != nil {
+				m.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(m.Spec.MachineConfiguration.MachinePreserveTimeout.Duration)}
+			} else {
+				m.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.safetyOptions.MachinePreserveTimeout.Duration)}
+			}
+			return nil
+		}, true)
 		if err != nil {
-			klog.V(2).Infof("Error annotating machine %q for auto-preservation: %v", m.Name, err)
-			// since addAutoPreserveAnnotation uses retries internally, on error we can continue with other machines
-			continue
+			klog.Errorf("Error annotating and setting PreserveExpiryTime on machine %q for auto-preservation: %v", machine.Name, err)
+			return nil, err
 		}
+
 		autoPreservationCandidates[index] = updatedMachine
 		autoPreservationCapacityRemaining--
 	}
-	return append(autoPreservationCandidates, others...)
+
+	return append(autoPreservationCandidates, others...), nil
 }
 
 func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machines []*v1alpha1.Machine, numToStop int) int {
@@ -991,14 +1016,18 @@ func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machin
 			return autoPreservedMachines[i].CreationTimestamp.Before(&autoPreservedMachines[j].CreationTimestamp)
 		})
 	}
-	for index, m := range autoPreservedMachines {
+
+	for index, machine := range autoPreservedMachines {
 		if numToStop == 0 {
 			break
 		}
-		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)
+		klog.V(2).Infof("Removing auto-preservation annotation from machine %q as AutoPreserveFailedMachineMax is breached", machine.Name)
+		updatedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(machine.Namespace), machine, func(m *v1alpha1.Machine) error {
+			delete(m.Annotations, machineutils.PreserveMachineAnnotationKey)
+			return nil
+		}, true)
 		if err != nil {
-			klog.Warningf("Error removing %q=%q annotation from machine %q: %v.", machineutils.PreserveMachineAnnotationKey, machineutils.PreserveMachineAnnotationValueAutoPreserved, m.Name, err)
+			klog.Warningf("Error removing %q=%q annotation from machine %q: %v.", machineutils.PreserveMachineAnnotationKey, machineutils.PreserveMachineAnnotationValueAutoPreserved, machine.Name, err)
 			continue
 		}
 		autoPreservedMachines[index] = updatedMachine
@@ -1007,19 +1036,6 @@ func (c *controller) stopAutoPreservationForMachines(ctx context.Context, machin
 	return numToStop
 }
 
-func addAutoPreserveAnnotationOnMachine(machineToUpdate *v1alpha1.Machine) error {
-	if machineToUpdate.Annotations == nil {
-		machineToUpdate.Annotations = make(map[string]string)
-	}
-	machineToUpdate.Annotations[machineutils.PreserveMachineAnnotationKey] = machineutils.PreserveMachineAnnotationValueAutoPreserved
-	return nil
-}
-
-func removeAutoPreserveAnnotationFromMachine(machineToUpdate *v1alpha1.Machine) error {
-	delete(machineToUpdate.Annotations, machineutils.PreserveMachineAnnotationKey)
-	return nil
-}
-
 func (c *controller) findEffectivePreserveValue(machine *v1alpha1.Machine) (string, error) {
 	var nodeAnnotationValue, machineAnnotationValue, lANodeAnnotationValue string
 	machineAnnotationValue = machine.Annotations[machineutils.PreserveMachineAnnotationKey]

pkg/controller/machineset_test.go

@@ -2156,7 +2156,8 @@ var _ = Describe("machineset", func() {
 			waitForCacheSync(stop, c)
 			machinesList := []*machinev1.Machine{testMachine1, testMachine2, testMachine3, testMachine4}
 			machinesList = append(machinesList, tc.setup.additionalMachines...)
-			c.manageAutoPreservationOfFailedMachines(context.TODO(), machinesList, testMachineSet)
+			_, err := c.manageAutoPreservationOfFailedMachines(context.TODO(), machinesList, testMachineSet)
+			Expect(err).To(BeNil())
 			waitForCacheSync(stop, c)
 			updatedMachine1, _ := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), testMachine1.Name, metav1.GetOptions{})
 			updatedMachine2, _ := c.controlMachineClient.Machines(testNamespace).Get(context.TODO(), testMachine2.Name, metav1.GetOptions{})