Preservation bugfix to put PreserveExpiryTime whenever machine transitions to Failed phase by r4mek · Pull Request #1147 · gardener/machine-controller-manager

r4mek · 2026-09-01T09:59:00Z

What this PR does / why we need it:

Now we add the PreserveExpiryTime on the machine if the machine has annotation: node.machine.sapcloud.io/preserve: "when-failed" and it transitions to Failed phase.
For auto-preservation flow, we add the PreserveExpiryTime on the machine if the failed machine gets the annotation: node.machine.sapcloud.io/preserve: "auto-preserved"
Miscellaneous code fix:
- Ensure objects retrieved from the lister cache are never mutated directly.
- Machine fetch in UpdateMachineWithRetries() from the lister cache, which may return a stale object.
- On update failure, requeue the object for the next reconciliation rather than retrying with a potentially stale cached value.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
All the miscellaneous code fix are much needed to fix the aforementioned bug without surfacing/introducing new bugs.

Release note:

Fixed the scenario where a machine to be preserved can for brief moment not have PreserveExpiryTime set.

gardener-prow · 2026-09-01T09:59:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign takoverflow for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

r4mek · 2026-09-01T10:00:24Z

/kind bug

thiyyakat

Thanks for the changes and the refactoring! I haven't looked at the full PR yet. I had one case in mind for which I thought the new code may not behave as expected. Once you address that, I will look at the rest as well.

thiyyakat · 2026-09-03T07:17:34Z

 				defer trackers.Stop()
 				waitForCacheSync(stop, c)
-				err := c.updateMachineAndMachineDeploymentDeletionAnnotations(context.TODO(), testMachineDeployment)
+				err := func() error {


This may be a stupid question, but what is the point of the closure here? Can you not simply have just L2361?

thiyyakat · 2026-09-03T07:30:34Z

+			node.Annotations[PreserveMachineAnnotationKey]; ok &&
+			AllowedPreserveAnnotationValues.Has(val) {
+			return val, true


Please print a warning here if the value is not valid.

thiyyakat · 2026-09-03T07:32:38Z

+	if val, ok :=
+		machine.Annotations[PreserveMachineAnnotationKey]; ok &&
+		AllowedPreserveAnnotationValues.Has(val) {
+		return val, true


Please log a warning if the value is not valid.

thiyyakat · 2026-09-03T07:36:29Z

+	preserveAnnotationValue, exists := machineutils.GetPreserveAnnotationValue(node, machine)
+	if !exists {
 		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
 	}


Please correct me if I'm wrong, but what about the case where preservation was being manipulated by only the machine object's annotations? If a machine was preserved, and the annotation was deleted to indicate that preservation should be stopped, you will end up returning early here instead of stopping preservation.

Good catch! Addressed in the latest commit ab6bf44

gagan16k

Thanks for the changes! Have some comments, PTAL

gagan16k · 2026-09-07T08:30:33Z

-				})
+			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


This will force a reconcile instead of retry on returning 409 errors, so the unit test for LDRCBST could flake even more now. Might need to find another way to correct that.

After our offline discussions, the error is very low and honestly I am not really sure why the LDRCBST test is failed. More investigation would be required for fixing it and not sure whether we want to tackle it now.

@aaronfern

gagan16k · 2026-09-07T09:46:24Z

+	// Deep-copy to avoid mutating cache objects downstream.
+	for i, m := range filteredMachines {
+		filteredMachines[i] = m.DeepCopy()
+	}
+


Great change, but I was thinking if we could apply this while creating the filteredMachines slices, in claimMachines().
Argument for: Saves extra work on the slice, and does it in the same loop while the slice is getting constructed.
Argument against: The other callers for the function, do not seem to write to the slice like this reconciler does, and thus do not really need a deep copy. WDYT?

I did not implement it in claimMachines() for the reason mentioned in Argument against. But if the team sees differently, I can implement it.

aaronfern

General feedback

Newly introduced flag should be mentioned in description and release notes
Error strings should not be capitalised
In general it's not nice that this huge bunch of miscellaneous code fixes are part of this bug fix. They are good pieces, but not required to fix the bug this PR talks about. It's fine for now as the work is done already, but keep this in mind for future reference

aaronfern · 2026-09-08T10:48:48Z

 	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.")


The issue description does not mention any new flag. A new flag like this is important enough to be mentioned as a release note

aaronfern · 2026-09-08T12:03:44Z

+			klog.Errorf("Error annotating and setting PreserveExpiryTime on machine %q for auto-preservation: %v", machine.Name, err)
+			return nil, err


Returning here stops patching of further machines. Consider pooling all error messages and returning an aggregate error at the end of the loop

aaronfern · 2026-09-09T02:34:02Z

+// nodeTemplateChanged returns true if machine's nodeTemplate is changed.
+func nodeTemplateChanged(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool {


This func name is semantically incorrect. Generally a machine's nodeTemplate won't change but would rather get outdated and needs to be re-synced.
Consider names like nodeTemplateOutOfSync or machineNodeTemplateMismatch or feel free to choose another name that you feel is more accutare

aaronfern · 2026-09-09T02:35:23Z

+// configChanged returns true if machine's config is changed.
+func configChanged(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool {


Same as above, name is not accurate

aaronfern · 2026-09-09T02:36:09Z

+// classKindChanged returns true if machine's class.Kind is changed.
+func classKindChanged(machineset *v1alpha1.MachineSet, machine *v1alpha1.Machine) bool {


This naming is not accurate too

aaronfern · 2026-09-09T03:06:17Z

+						machineCurrentStatus := v1alpha1.CurrentStatus{
+							Phase:          v1alpha1.MachineFailed,
+							LastUpdateTime: metav1.Now(),
+						}


Does having this out and assigned to another var help much?

Yes. I update the PreserveExpiryTime below, based on certain criteria. If there is a better way, I can change it.

aaronfern · 2026-09-09T05:08:00Z

 	} else {
-		delete(clone.Annotations, machineutils.PreserveMachineAnnotationKey)
+		klog.V(3).Infof(
+			"Syncing machine %q 's annotation:%q=%q to its node %q 's annotation:%q=%q",


This is worded incorrectly. We sync the node's annotation to the machine

aaronfern · 2026-09-09T05:19:06Z

 					PreserveExpiryTime: machine.Status.CurrentStatus.PreserveExpiryTime,
 				}
+				// check if preservation is needed for the failed machine
+				if val, shouldHandlePreservation := machineutils.GetPreserveAnnotationValue(node, machine); shouldHandlePreservation && val == machineutils.PreserveMachineAnnotationValueWhenFailed {
+					clone.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.getEffectiveMachinePreserveTimeout(machine).Duration)}
+				}


This snippet could potentially overwrite existing PreserveExpiryTime on the machine that is set a line above

Although, @thiyyakat do you know why we might already have a PreserveExpiryTime on the machine? We enter this block only when the machine is in the Pending phase so I wonder if line 1142 is ever relevant

Correct me if I am wrong @thiyyakat

Only for preserve=now PreserveExpiryTime is set else it is nil. I am setting PreserveExpiryTime only for preserve=when-failed, so they should not conflict.

Yes. As long as we're checking for the annotation value == when-failed, it should be fine.

aaronfern · 2026-09-09T05:20:06Z

 func (c *controller) updateMachineToFailedState(ctx context.Context, description string, machine, clone *v1alpha1.Machine) (bool, error) {
 	// Log the error message for machine failure
 	klog.Error(description)
+	node, _ := c.nodeLister.Get(machine.Annotations[v1alpha1.NodeLabelKey])


Please don't ignore a potential error

After offline discussions, we decided to log the error.

Signed-off-by: r4mek <vivek.ram688@gmail.com>

thiyyakat

Hi Vivek. Few more comments from me. PTAL. Thanks

thiyyakat · 2026-09-17T05:25:04Z

+var (
+	// DefaultMachinePreserveTimeout is the default time for which the machine is preserved
+	// when [MachineConfiguration.MachinePreserveTimeout] is not specified.
+	DefaultMachinePreserveTimeout = metav1.Duration{Duration: 96 * time.Hour}


Why not change this to a constant?

DefaultMachinePreserveTimeout = 96 * time.Hour

And, in manageAutoPreservationOfFailedMachines

m.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(v1alpha1.DefaultMachinePreserveTimeout)}

thiyyakat · 2026-09-17T08:47:46Z


-	filteredMachines = c.manageAutoPreservationOfFailedMachines(ctx, filteredMachines, machineSet)
+	filteredMachines, err = c.manageAutoPreservationOfFailedMachines(ctx, filteredMachines, machineSet)
+	if err != nil {


Since we don't update AutoPreserveFailedMachineCount until calculateMachineSetStatus is called in reconcileClusterMachineSet, there is a chance that a few machines would be successfully annotated with the auto-preserve annotation, but in the next reconciliation, since AutoPreserveFailedMachineCount is stale, manageAutoPreservationOfFailedMachines will end up preserving additional machines above AutoPreserveFailedMachineMax.

However, this will self-heal because the annotation update on the machine object will requeue the machineset object as well.

@gagan16k , I think your fix will reduce the likelihood of this happening, right? If we do not rely on AutoPreserveFailedMachineCount set on the status? We need to make sure both fixes go in together.

thiyyakat · 2026-09-17T09:10:30Z

 					result: false,
 				},
 			}),
-			Entry("should return true if machine is annotated with preserve=false", testCase{


Could you please add a test case for when preserveExpiryTime is not set on a Failed machine?

thiyyakat · 2026-09-17T09:35:14Z

+
+		preservedMachine, err := machineutils.PatchMachine(ctx, c.controlMachineClient.Machines(annotatedMachine.Namespace), annotatedMachine, func(m *v1alpha1.Machine) error {
+			if annotatedMachine.Spec.MachineConfiguration != nil && annotatedMachine.Spec.MachineConfiguration.MachinePreserveTimeout != nil {
+				m.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(annotatedMachine.Spec.MachineConfiguration.MachinePreserveTimeout.Duration)}


In the current state of the code in your PR, is there a possibility that a machine is annotated with auto-preserved, causing the machine and its owner object (the machineset) to get reconciled again, but the PreserveExpiryTime is not set, so in the latest reconciliation, the machine is found to be without PreserveExpiryTime and is thus deleted?

I was wondering if we could instead set the PreserveExpiryTime first and then add the auto-preserved annotation(just for tracking). But if there is an error during the status update, it would be difficult to add this annotation correctly to the same machine. Plus semantically it seems odd because we are using the annotation as an intent here.

Instead, in the shouldFailedMachineBeTerminated, what if we check for either preserve expiry time or auto-preserved annotation, and not just preserve expiry time?

thiyyakat · 2026-09-17T09:42:35Z

-		return
+	node, err := c.nodeLister.Get(nodeName)
+	if err != nil {
+		klog.V(3).Infof("Error fetching node %q . Will check the machine %q for annotation:%q", nodeName, machine.Name, machineutils.PreserveMachineAnnotationKey)


This log will get printed every reconciliation until a node object registers and the lister cache gets updated. Even for non-preservation-related machines. I would suggesting doing away with the log or increasing the level.

thiyyakat · 2026-09-17T09:44:11Z

-	effectivePreserveValue := getEffectivePreservationAnnotations(&preserveInfo, getErr)

-	var removeAnnotations bool
 	clone := machine.DeepCopy()


Why not use updatedMachine here? Is it because you don't want to return nil on error? Can that not be taken care of in the defer ?

thiyyakat · 2026-09-17T09:57:07Z

-	_, err := c.controlMachineClient.Machines(clone.Namespace).Update(ctx, clone, metav1.UpdateOptions{})
-	return err
+	klog.V(3).Infof(
+		"Removing machine %q 's annotation:%q=%q as node %q 's has annotation:%q=%q",


Suggested change

"Removing machine %q 's annotation:%q=%q as node %q 's has annotation:%q=%q",

"Removing machine %q 's annotation:%q=%q as node %q has annotation:%q=%q",

Nit.

thiyyakat · 2026-09-17T10:07:21Z

 					PreserveExpiryTime: machine.Status.CurrentStatus.PreserveExpiryTime,
 				}
+				// check if preservation is needed for the failed machine
+				if val, shouldHandlePreservation := machineutils.GetPreserveAnnotationValue(node, machine); shouldHandlePreservation && val == machineutils.PreserveMachineAnnotationValueWhenFailed {
+					clone.Status.CurrentStatus.PreserveExpiryTime = &metav1.Time{Time: metav1.Now().Add(c.getEffectiveMachinePreserveTimeout(machine).Duration)}
+				}


Yes. As long as we're checking for the annotation value == when-failed, it should be fine.

thiyyakat · 2026-09-17T10:12:00Z

+			AllowedPreserveAnnotationValues.Has(val) {
+			return val, true
+		}
+		klog.Warningf(


This would get printed for every un-annotated node, every reconciliation, right? 😅 I think you should only print if it is an invalid value. No point printing otherwise.

thiyyakat · 2026-09-17T10:13:04Z

+		AllowedPreserveAnnotationValues.Has(val) {
+		return val, true
+	}
+	klog.Warningf(


This too would get printed for every un-annotated machine, every reconciliation, until a node joins?

r4mek requested a review from a team as a code owner September 1, 2026 09:59

gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 1, 2026

gardener-prow Bot added kind/bug Bug and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 1, 2026

thiyyakat reviewed Sep 3, 2026

View reviewed changes

r4mek marked this pull request as draft September 3, 2026 09:40

gardener-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 3, 2026

r4mek marked this pull request as ready for review September 4, 2026 07:19

gardener-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2026

gagan16k requested a review from thiyyakat September 6, 2026 16:26

gagan16k reviewed Sep 7, 2026

View reviewed changes

aaronfern reviewed Sep 9, 2026

View reviewed changes

r4mek force-pushed the preserve-bug-3 branch from 1fa4282 to f28bbed Compare September 10, 2026 09:41

thiyyakat mentioned this pull request Sep 10, 2026

Fix handling of NotFound errors when deciding if a Failed machine should be terminated #1135

Open

r4mek added 9 commits September 16, 2026 14:54

bugfix

e2f1e22

Signed-off-by: r4mek <vivek.ram688@gmail.com>

make check

95a5297

Signed-off-by: r4mek <vivek.ram688@gmail.com>

introducing PatchMachine()

0526932

Signed-off-by: r4mek <vivek.ram688@gmail.com>

addressing review comments 1

1cc4fbe

Signed-off-by: r4mek <vivek.ram688@gmail.com>

addressing review comments 2

c9e869e

Signed-off-by: r4mek <vivek.ram688@gmail.com>

nits

7fc90b9

Signed-off-by: r4mek <vivek.ram688@gmail.com>

nits

94a2c0a

Signed-off-by: r4mek <vivek.ram688@gmail.com>

test fix

8bf8a0b

Signed-off-by: r4mek <vivek.ram688@gmail.com>

removing mcm flag

f46b8e9

Signed-off-by: r4mek <vivek.ram688@gmail.com>

r4mek force-pushed the preserve-bug-3 branch from cd95cde to f46b8e9 Compare September 16, 2026 09:28

thiyyakat reviewed Sep 17, 2026

View reviewed changes

		klog.Errorf("Error annotating and setting PreserveExpiryTime on machine %q for auto-preservation: %v", machine.Name, err)
		return nil, err

		// nodeTemplateChanged returns true if machine's nodeTemplate is changed.
		func nodeTemplateChanged(machineset v1alpha1.MachineSet, machine v1alpha1.Machine) bool {

		// configChanged returns true if machine's config is changed.
		func configChanged(machineset v1alpha1.MachineSet, machine v1alpha1.Machine) bool {

		// classKindChanged returns true if machine's class.Kind is changed.
		func classKindChanged(machineset v1alpha1.MachineSet, machine v1alpha1.Machine) bool {

	"Removing machine %q 's annotation:%q=%q as node %q 's has annotation:%q=%q",
	"Removing machine %q 's annotation:%q=%q as node %q has annotation:%q=%q",

Conversation

r4mek commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gardener-prow Bot commented Sep 1, 2026

Uh oh!

r4mek commented Sep 1, 2026

Uh oh!

thiyyakat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagan16k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronfern left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

r4mek commented Sep 1, 2026 •

edited

Loading