[rel-v0.62] Fix preservation reconciliation to prevent deviation from regular flow for non-preservation-bound machines and prevent machines getting stuck in creation flow beyond CreationTimeout 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.

… reduced when possible

…edByMCM` to `PreserveMachineAnnotationValueAutoPreserved`

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!

- 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

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.

…encing happens in future edits. Add removed test.

…ue object.

aaronfern

/lgtm

gardener-prow · 2026-07-01T07:34:14Z

LGTM label has been added.

Details

Git tree hash: 880cd48b1d8ae1ea643961376d69f163bd383428

elankath · 2026-07-01T07:37:52Z

/lgtm with the understanding that code-simplification changes suggested will be deferred to another PR along with integration tests.

gardener-prow · 2026-07-01T08:03:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aaronfern

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [aaronfern]

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

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

Comment thread pkg/util/provider/machineutils/utils.go Outdated

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.

thiyyakat changed the title ~~Fix preservation reconciliation to prevent deviation from regular flow for non-preservation-bound machines~~ [rel-v0.62] Fix preservation reconciliation to prevent deviation from regular flow for non-preservation-bound machines Jun 29, 2026

thiyyakat mentioned this pull request Jun 29, 2026

Machine stuck with no phase past CreationTimeout #1110

Open

Address review comment: modify getPreserveStateInfo to return a val…

8cb22d2

…ue object.

aaronfern reviewed Jul 1, 2026

View reviewed changes

gardener-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 1, 2026

aaronfern approved these changes Jul 1, 2026

View reviewed changes

gardener-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2026

gardener-prow Bot merged commit 19f5500 into gardener:rel-v0.62 Jul 1, 2026
14 checks passed

	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!

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

r4mek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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