Fix preservation reconciliation to prevent deviation from regular flow for non-preservation-bound machines by thiyyakat · Pull Request #1111 · gardener/machine-controller-manager

thiyyakat · 2026-06-24T10:19:24Z

What this PR does / why we need it:
The previous implementation had several correctness issues in the machine preservation flow:

Unconditional uncordon on every reconcile: uncordonNodeIfCordoned was called whenever IsMachineActive returned true and a node name was present. This meant any Running machine with a cordoned backing node — even one cordoned for reasons unrelated to preservation (e.g. Cluster Autoscaler scale-down) — would be unconditionally uncordoned on every reconcile pass.
No gate for non-preservation-bound machines: Every machine was processed through the full preservation logic on every reconcile, even machines with no preservation state at all (no annotations, no PreserveExpiryTime). This caused unnecessary API calls on non-preserved machines.
Stale clone causing conflict errors: The LastAppliedNodePreserveValueAnnotation sync in the defer compared annotations on a clone that had already been updated via UpdateStatus calls inside preserveMachine/stopPreservationIfActive. Because UpdateStatus returns the server's stored metadata (not the in-memory mutation), the annotation diff was never detected and the sync was silently dropped. On paths where the sync did fire, it was using a stale ResourceVersion, producing spurious conflict errors.

This PR fixes all of the above by:

Introducing preserveStateInfo struct populated by getPreserveStateInfo at the start of each reconcile, reading node and machine annotation state in a single pass and storing it for the rest of the function. This eliminates the double lister read and decouples the annotation sync from the defer.
Adding an explicit isMachinePreservationBound gate: machines with no preservation state (no PreserveExpiryTime, no preserve annotation on machine or node, no LastAppliedNodePreserveValueAnnotation) skip the entire preservation flow and return LongRetry, nil immediately.
Moving the LastAppliedNodePreserveValueAnnotation sync to an explicit updatePreserveAnnotationOnMachine call at the end of the function via shouldAnnotationsBeUpdatedOnMachine, rather than a defer that compared potentially stale annotation maps.
Moving uncordon logic into stopPreservationIfActive (step 4), so it fires only when preservation is actively being stopped for a Running machine — not on every reconcile of any active machine. The manageMachinePreservation path retains a narrower uncordon call gated on !preservationStopped && PreserveExpiryTime != nil && Phase == Running to handle the case where preserveMachine is called on a Running machine (e.g. preserve=now set by user).
Adding IsNotFound handling in the defer so transient not-found errors during preservation (e.g. machine deleted mid-reconcile) return LongRetryrather than ShortRetry.
Propagating the updated node object from removePreservationRelatedAnnotationsOnNode (now returns *corev1.Node) so that the subsequent uncordon step in stopPreservationIfActive operates on a current node object rather than a potentially stale one.
Removing the redundant PreventAutoPreserveAnnotationValues set in machineutils (it was identical to AllowedPreserveAnnotationValues minus ""); manageAutoPreservationOfFailedMachines in machineset.go now uses AllowedPreserveAnnotationValues directly.

Which issue(s) this PR fixes:
Fixes #1110

Special notes for your reviewer:

Release note:

Fix preservation reconcile loop to avoid unconditionally uncordoning nodes unrelated to machine preservation, prevent spurious writes on non-preserved machines, and eliminate conflict errors caused by stale machine object comparison in the defer annotation sync.

…eservation state.

…hase. Ignore node not found errors.

…conflicts.

…ines is not modified by `manageMachinePreservation`

…nning

…ss function calls.

…n manageMachinePreservation: This call is made to uncordon nodes that are preserved with preserve=now and the machine has recovered to Running. Preservation is not stopped in this case.

elankath · 2026-06-24T10:54:04Z

after we changed literal value we should change the name to the constant to also PreserveMachineAnnotationValueAutoPreserved instead. This package will be exposed to consumers like DWD so best to get semantic name changes finalized.

Addressed in 200ce00

… reduced when possible

…edByMCM` to `PreserveMachineAnnotationValueAutoPreserved`

aaronfern · 2026-06-24T14:58:29Z

+	preservationBound := c.isMachinePreservationBound(preserveInfo)
+	if !preservationBound {
+		// we clear the error here to prevent preservation logic from interfering with non-preservation-bound machines
+		err = nil


Why do you need to set err to nil here?
I see err gets populated before this only when calling c.getPreserveStateInfo(), and even then if err != nil, we return

You're right. The earlier return defeats the purpose of this. I will probably need to buffer the error until we can determine whether or not the machine is preservation-bound. Thanks

Addressed in cb105e7. The error from getPreserveStateInfo is now buffered in getErr and is only returned if machine is preservation-bound.

aaronfern · 2026-06-24T14:59:29Z

+	} else if err != nil {
+		return
+	}


Similar here. err is populated only by c.getPreserveStateInfo(), and there we return if err != nil

Addressed in cb105e7

aaronfern · 2026-06-24T17:26:47Z

+	nodeValue             string
+	machineValue          string
+	lastAppliedNodeValue  string
+	PreserveExpiryTimeSet bool


Need not be exported

Suggested change

PreserveExpiryTimeSet bool

preserveExpiryTimeSet bool

Addressed in cb105e7.

aaronfern · 2026-06-24T17:43:01Z

-			_, err := c.clearMachinePreserveExpiryTime(ctx, machine)
+			klog.Warningf("Node %q of machine %q not found. Proceeding to stop preservation on machine.", nodeName, machine.Name)
+			// Node not found, proceed to delete annotations and clear preserveExpiryTime on machine
+			machine, err = c.removePreserveAnnotationOnMachine(ctx, machine)


Is there a reason this is not gated with removePreservationAnnotations? I see that every other invocation is removePreserveAnnotationOnMachine() within this function is gated

Addressed in cb105e7

aaronfern · 2026-06-24T17:44:06Z

-// PreventAutoPreserveAnnotationValues contains the values to check if a machine is already annotated for preservation,
-// in which case it should not be auto-preserved.
-var PreventAutoPreserveAnnotationValues = sets.New(PreserveMachineAnnotationValueNow, PreserveMachineAnnotationValueWhenFailed, PreserveMachineAnnotationValuePreservedByMCM, PreserveMachineAnnotationValueFalse)
+var AllowedPreserveAnnotationValues = sets.New(PreserveMachineAnnotationValueNow, PreserveMachineAnnotationValueWhenFailed, PreserveMachineAnnotationValueAutoPreserved, PreserveMachineAnnotationValueFalse)


Is "" removed intentionally?

Yes! There is no use for preserve="". The user can either delete the annotation, or set it to false.

aaronfern · 2026-06-24T17:51:41Z

-					machineAnnotationValue: machineutils.PreserveMachineAnnotationValuePreservedByMCM,
-				},
-			}),
-			Entry("when node is annotated and preservation times out, should stop preservation", testCase{


Why is this test removed? This looks like a valid case

The test following this one covers this test case and also checks for annotation removal.

aaronfern · 2026-06-24T17:51:56Z

 					retry:                   machineutils.LongRetry,
 				},
 			}),
-			Entry("when invalid preserve annotation is added on node of un-preserved machine, should do nothing  ", testCase{


This looks like a valid case too

The test following this one is a duplicate of this one.

aaronfern · 2026-06-24T17:53:54Z

+	return true
+}
+
+func (c *controller) getPreserveStateInfo(machine *v1alpha1.Machine) (*preserveStateInfo, error) {


Please consider writing some unit tests for the new helper methods that you introduced

Addressed in cb105e7

aaronfern · 2026-06-24T17:54:30Z

 		if err != nil {
-			if apierrors.IsConflict(err) {
+			if apierrors.IsNotFound(err) {
+				klog.Warningf("Error during preservation flow:%v", err.Error())


nit

Suggested change

klog.Warningf("Error during preservation flow:%v", err.Error())

klog.Warningf("Error during preservation flow: %v", err.Error())

Addressed in cb105e7

gagan16k

Thanks for the fixes!

gagan16k · 2026-06-24T13:11:21Z

-	if err != nil {
-		return err
-	}
+func (c *controller) uncordonNodeIfCordoned(ctx context.Context, node *corev1.Node) error {


Should there be a check for ClusterAutoscalerScaleDownDisabledAnnotationByMCMKey annotation before node operations, similar to how it is done in removePreservationRelatedAnnotationsOnNode()?

Do you mean to gate the uncordoning? Do you feel the preservation-bound check and Running phase checks are insufficient?

I think it might be worth adding, as the more we can limit CA interactions with this the better. However, could ask for a second opinion

The two may not be related though. If we do what you suggest, then there is a chance that the user has set scale-down-disabled on the node in which case we would never uncordon the node even after it recovers from Failed to Running.

The check in removePreservationRelatedAnnotationsOnNode was done to ensure that we don't clear the scale-down-disabled annotation if it has been set by the user.

We limit CA interaction by :

By-passing preservation logic if machine is not preservation-bound

Ensuring CA does not scale down a preserved node. We ensure preserved nodes have the scale-down-disabled annotation and in CA's ForceDeleteNode, we skip nodes if they have the scale-down-disabled annotation.

@aaronfern do you think this is sufficient?

gardener-prow · 2026-06-25T12:27:35Z

[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 ask for approval from thiyyakat. 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

- nits - if not found, machine annotation value must be enforced - if machine annotation value is invalid, log and continue assuming it does not exist - gate `removePreserveAnnotationOnMachine` when node is not found

gagan16k

Some more comments, PTAL

gagan16k · 2026-06-25T15:32:19Z

+	// preservation of the machine object.
+	effectivePreserveValue := getEffectivePreservationAnnotations(preserveInfo, getErr)
+
+	var removeAnnotations, preservationStopped bool


Are these two variables required?
preservationStopped can be removed, as CurrentStatus.PreserveExpiryTime is removed by stopPreservationIfActive, and the check for the time is enough in L853 IMO, but with removeAnnotations it is not as obvious and the changes might be more involved.

removeAnnotations serves 2 purposes:

It indicates to stopPreservationIfActive whether the machine and not should be unannotated - in the cases where machine preservation expires, or auto-preserved machine has recovered to Running.

if annotations have been removed by stopPreservationIfActive, updatePreserveAnnotationOnMachine should not run because it will again set LastAppliedNodePreserveValueAnnotation.

removeAnnotations should stay as, if it is removed, stopPreservationIfActive will need to actively check this.preservationStopped can be removed (Based on discussion)

gagan16k · 2026-06-25T20:12:26Z

-	if err != nil {
-		return err
-	}
+func (c *controller) uncordonNodeIfCordoned(ctx context.Context, node *corev1.Node) error {


I think it might be worth adding, as the more we can limit CA interactions with this the better. However, could ask for a second opinion

r4mek

Thanks for the PR! I have posted few comments, some of which are more related to code cleanup which you can choose to do now or defer to a later PR.

r4mek · 2026-06-26T03:55:20Z

+	machineValue          string
+	lastAppliedNodeValue  string
+	preserveExpiryTimeSet bool
+}


Since "" annotation value is not valid anymore, we can remove nodeAnnotated and machineAnnotated booleans. Annotation value is "" implies that annotation is not set.

Also, can we rename nodeValue, machineValue to nodeAnnotationValue, machineAnnotationValue resp.

The only way to distinguish between node/machine being annotated invalidly with "", or whether the annotation is unset(valid- means deletion of annotation), is by checking nodeAnnotated/machineAnnotated.

r4mek · 2026-06-26T04:20:50Z

+		}
+		klog.Warningf("Couldn't find node %q for machine %q", nodeName, machine.Name)
+		err = nil
 	}


You can move this check right after isMachinePreservationBound() call.

We want to check if a machine is preservation-bound at all before we return an error. This is being done so that we don't alter the flow for machines that are not preservation-bound. This is the current issue that the PR is trying to solve.

r4mek · 2026-06-26T04:29:19Z

-	delete(clonedMachineAnnotations, machineutils.PreserveMachineAnnotationKey)
-	clonedMachineAnnotations[machineutils.LastAppliedNodePreserveValueAnnotationKey] = nodeAnnotationValue
-	return nodeAnnotationValue, clonedMachineAnnotations
+	if info.nodeValue == "" && info.lastAppliedNodeValue == "" {


Why do we have to also check for lastAppliedNodeValue?

Please check the comment above (sorry, it's long). It gives an example scenario of why lastAppliedNodeValue is needed. If MCM goes down, we won't be able to tell if node was never annotated or if node annotation was deleted.

r4mek · 2026-06-26T04:52:04Z

 	// removal of preserveExpiryTime is the last step of stopping preservation
 	// therefore, if preserveExpiryTime is not set, machine is not preserved
 	nodeName := machine.Labels[v1alpha1.NodeLabelKey]
 	if machine.Status.CurrentStatus.PreserveExpiryTime == nil {


We have the info.preserveExpiryTimeSet and we already check for it in the parent function. So, no need for this.

If we still want to be check it again then shouldn't we clear the annotations before returning?

info.preserveExpiryTimeSet is only being used to check if a machine is preservation-bound. We do not pass it to stopPreservationIfActive.

r4mek · 2026-06-26T04:59:01Z

 		klog.V(2).Infof("Preservation of machine %q with no backing node has stopped.", machine.Name)
-		return true, nil
+		return machine, nil
 	}


We can extract this block outside, so that we always remove preserve annotation from machine and clear clear machine expiry time. Later we can return if nodeName == nil

Then we won't have to do the same operation again if node is NotFound.

Will raise a separate PR for any refactoring needed. Since the purpose of this is to play it safe and quickly deliver a fix without introducing any new regressions, I would like to leave this as-is for now.

r4mek · 2026-06-26T05:00:48Z

 	}
 	// Step 2: remove annotations from node
-	err = c.removePreservationRelatedAnnotationsOnNode(ctx, updatedNode, removePreservationAnnotations)
+	updatedNode, err = c.removePreservationRelatedAnnotationsOnNode(ctx, updatedNode, removePreservationAnnotations)


Can we combine this and the condition update on node as a single operation?

We cannot unfortunately. Annotations are updated using Update and conditions using UpdateStatus.

r4mek · 2026-06-26T05:18:40Z

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


We can split this function such that one handles the case where nodeName is set and other where nodeName is not set.

Will raise a separate PR for refactoring. Since the purpose of this is to play it safe and quickly deliver a fix without introducing any new regressions, I would like to leave this as-is for now.

r4mek · 2026-06-26T07:24:01Z

 			klog.Errorf("error draining preserved node %q for machine %q : %v", nodeName, machine.Name, drainErr)
-			return drainErr
+			return nil, drainErr
 		}


I don't think we will reach this codepath as we are gonna skip the if needsUpdate { if we have drainErr. So this block can be shifted after c.drainPreservedNode() call.

Hence, we would be returning without an error even if drain was unsuccessful.

Why would we skip if needsUpdate{ if we have drainErr that is not nil? In computeNewNodePreservedCondition we set needsUpdate to true if it is the first time that drain is failing.

…encing happens in future edits. Add removed test.

thiyyakat added 12 commits June 24, 2026 10:32

Add gate to ensure preservation code is only run for machines with pr…

4db7dfc

…eservation state.

Uncordon node only if Machine is preservation-bound, and in Running p…

60af619

…hase. Ignore node not found errors.

Return updated machine object from preservation functions to prevent …

cea40c2

…conflicts.

Handle IsNotFound error in the defer function.

de02bfd

Fix tests and defer function.

a3b7133

Add tests and code to ensure the flow for non-preservation bound mach…

daaa433

…ines is not modified by `manageMachinePreservation`

Add tests to cover uncordoning of machine when machines recover to Ru…

5f143c5

…nning

Uncordon machines in stopMachinePreservationIfActive to allow retry.

4188e83

Introduce struct preserveStateInfo to persist preservation state acro…

5a57dd4

…ss function calls.

Remove "" as a valid preservation value

0d7c29b

Add helper functions and simplify defer

5e258e0

Ensure machine preservation was not stopped before uncordoning node i…

0c147c9

…n manageMachinePreservation: This call is made to uncordon nodes that are preserved with preserve=now and the machine has recovered to Running. Preservation is not stopped in this case.

thiyyakat requested a review from a team as a code owner June 24, 2026 10:19

gardener-prow Bot added do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 24, 2026

thiyyakat changed the title ~~Preservation hot fix~~ Fix preservation reconciliation to prevent deviation from regular flow for non-preservation-bound machines Jun 24, 2026

thiyyakat added the kind/bug Bug label Jun 24, 2026

gardener-prow Bot removed the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jun 24, 2026

thiyyakat force-pushed the preservation-hot-fix branch from 063f8db to f8d53be Compare June 24, 2026 10:23

elankath reviewed Jun 24, 2026

View reviewed changes

thiyyakat added 2 commits June 24, 2026 16:24

Minor code changes and test refactoring to ensure conflict errors are…

e027119

… reduced when possible

Address review comment: Rename `PreserveMachineAnnotationValuePreserv…

200ce00

…edByMCM` to `PreserveMachineAnnotationValueAutoPreserved`

thiyyakat force-pushed the preservation-hot-fix branch from f8d53be to 200ce00 Compare June 24, 2026 10:57

thiyyakat added 2 commits June 24, 2026 16:30

Correct docstring for PreserveMachineAnnotationValueFalse

08b6cd3

Log node name in uncordonNodeIfCordoned.

cf6d15f

thiyyakat mentioned this pull request Jun 24, 2026

🐛 Fix machine loop block when backing node does not exist #1109

Open

aaronfern reviewed Jun 24, 2026

View reviewed changes

gagan16k reviewed Jun 25, 2026

View reviewed changes

aaronfern assigned thiyyakat Jun 25, 2026

gardener-prow Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 25, 2026

Address review comments

cb105e7

- nits - if not found, machine annotation value must be enforced - if machine annotation value is invalid, log and continue assuming it does not exist - gate `removePreserveAnnotationOnMachine` when node is not found

thiyyakat force-pushed the preservation-hot-fix branch from 69b9eb6 to cb105e7 Compare June 25, 2026 13:08

gagan16k reviewed Jun 25, 2026

View reviewed changes

Address review comments - part 2

91b9476

thiyyakat force-pushed the preservation-hot-fix branch from d2f0d8f to 91b9476 Compare June 26, 2026 03:17

r4mek reviewed Jun 26, 2026

View reviewed changes

Return machine object even on error so no accidental nil ptr de-refer…

5ab7c92

…encing happens in future edits. Add removed test.

	klog.Warningf("Error during preservation flow:%v", err.Error())
	klog.Warningf("Error during preservation flow: %v", err.Error())

Uh oh!

Conversation

thiyyakat commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

thiyyakat Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

gardener-prow Bot commented Jun 25, 2026

Uh oh!

gagan16k left a comment

Choose a reason for hiding this comment

Uh oh!

thiyyakat commented Jun 24, 2026 •

edited

Loading

thiyyakat Jun 25, 2026 •

edited

Loading

thiyyakat Jun 25, 2026 •

edited

Loading

thiyyakat Jun 25, 2026 •

edited

Loading

thiyyakat Jun 25, 2026 •

edited

Loading

thiyyakat Jun 25, 2026 •

edited

Loading

thiyyakat Jun 25, 2026 •

edited

Loading

r4mek Jun 26, 2026 •

edited

Loading

thiyyakat Jun 26, 2026 •

edited

Loading