Fix handling of NotFound errors when deciding if a Failed machine should be terminated by thiyyakat · Pull Request #1135 · gardener/machine-controller-manager

thiyyakat · 2026-08-20T16:46:28Z

What this PR does / why we need it:

Fixes the handling of NotFound errors in shouldFailedMachineBeTerminated() and refactors the shared preservation helpers so both the machineset controller and the machine controller rely on the same logic.

Previously, shouldFailedMachineBeTerminated() treated any error while resolving the effective preserve value (including a NotFound on the backing node) as a signal to terminate the failed machine. As a result, a machine explicitly annotated for preservation (preserve=now / when-failed / auto-preserved) whose node is not found was terminated instead of being preserved.

This PR:

Moves the preservation state helpers (PreserveStateInfo, GetPreserveStateInfo, GetEffectivePreservationAnnotations) into pkg/util/provider/machineutils so they can be shared, and adds IsPositivePreserveValue to distinguish values that actually request preservation (now/when-failed/auto-preserved) from false/empty/unrecognized values.
Rewrites shouldFailedMachineBeTerminated() to:
- fall through on a NotFound node error (enforcing the machine's own annotation), and only terminate on other errors,
- terminate once preservation has expired,
- keep the machine only when the effective preserve value is a positive value.

Behavior notes:

A failed machine with preserve=false, an empty, or an unrecognized value is terminated (matching the prior default behavior).
A failed machine annotated for preservation whose node is NotFound is now preserved (per the documented T1–T4 recovery semantics), rather than terminated.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

NotFound handling has two sub-cases worth calling out:
- machine annotated with a positive preserve value + node NotFound → preserved (intended change from prior behavior).
- only last-applied-node-preserve-value set (no active machine annotation) + node NotFound → terminated, unchanged; this is outcome-consistent with manageMachinePreservation, which stops preservation for the same state. last-applied-node-preserve-value is used as chronological history, not live intent.
No functional change to manageMachinePreservation beyond consuming the relocated/exported helpers.
IT passed for provider-aws

Release note:

Fixed handling of `NotFound` errors in the failed-machine termination check: a failed machine annotated for preservation whose backing node no longer exists is now preserved instead of being terminated.

gardener-prow · 2026-08-20T16:46:32Z

[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 ashwani2k 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

…ted()`. - If the error is NotFound, the MCS controller takes the machine's annotation value into consideration. - Move helpers from machine.go to utils.go to prevent duplication of code. These helpers are now used in `shouldFailedMachineBeTerminated` - Corresponding tests for the helpers are moved to utils_test.go

thiyyakat · 2026-08-21T10:10:49Z

Fix was verified by doing the following:

In both the master and fix versions, the code in the machine controller that handles auto-preservation was commented out. This way, PreserveExpiryTime is never set on an auto-preserved machine.
Set autoPreserveFailedMachineMax to 1 on the MCD.
Set machineCreationTimeout to a small value to force a new machine to go to Pending and then Failed due to creation timing out. No node has registered. Therefore, this would result in a NotFound error when the node is fetched from the lister.
When machine fails, the MCS annotates it with auto-preserved.
On master, this machine is deleted because there is no PreserveExpiryTime set, and the node could not be found.
With the fix, the machine stays preserved.

Relevant logs

For the run with the fix:

W0821 15:29:55.836179   28925 machineset.go:922] node "ip-10-180-18-3.eu-west-1.compute.internal" not found for machine "shoot--xxx--ca-test1-34-sys-comp-z1-7f667-krs58".
I0821 15:29:55.836211   28925 machineset.go:935] Failed machine "shoot--xxx--ca-test1-34-sys-comp-z1-7f667-krs58" is either preserved or in the process of being preserved.

Machine stays in Failed phase.

For the run with master:

E0821 15:32:24.796181   31384 machineset.go:1031] error fetching node "ip-10-180-18-3.eu-west-1.compute.internal" for machine "shoot--xxx--ca-test1-34-sys-comp-z1-7f667-krs58": node "ip-10-180-18-3.eu-west-1.compute.internal" not found
E0821 15:32:24.796252   31384 machineset.go:919] error finding effective preserve value for machine "shoot--xxx--ca-test1-34-sys-comp-z1-7f667-krs58": node "ip-10-180-18-3.eu-west-1.compute.internal" not found. Proceeding with termination of the machine.

Machine moves to Terminating after this.

takoverflow

Thanks for the changes.

Please don't make further changes, still in the process of reviewing the PR. (More changes will make it difficult to recall what I've already looked at)

takoverflow · 2026-08-24T09:40:26Z

+	if nodeName != "" {
+		node, err = c.nodeLister.Get(nodeName)
+		if err != nil {
+			if !apierrors.IsNotFound(err) {


What if there was any other error for listing the node? Then we unconditionally don't honor preservation? Why is that, preservation state can still be inferred from the Machine object right?

The behavior isn't consistent across different usages.

As part of manageMachinePreservation, if there's an error getting the node and the error isn't NotFound error, we just log a warning but still compute effectivePreserveValue from the preserveInfo.

if nodeName != "" { node, err = c.nodeLister.Get(nodeName) // We don't return on error immediately because we need to determine whether the machine has valid preservation state } ... if err != nil { if !apierrors.IsNotFound(err) { return } err = nil klog.Warningf("Couldn't find node %q for machine %q", nodeName, machine.Name) } else { nodeFound = true } // 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 := machineutils.GetEffectivePreservationAnnotations(&preserveInfo, nodeFound)

But here we don't do the same.

Nevermind, brain fog!
We're actually returning here as well.
But I'd still like to understand why.

Just thinking aloud here:

If node is not found in the lister, it is usually not a transient error AFAIK. In that case, it makes sense to defer to the machine's value to manipulate preservation.

Trouble is, if it is a transient error (there's no way of knowing, I understand that), isn't it better to retry rather than change the current state? Or do we defer to the machine in that case too?

Also, if you see the rest of the preservation code, unless the nodename is "" or the node is not found, we do a lot of node updates. All of those would fail and cause returns. However, if the machine was marked with when-failed or now, the machine object at least would get preserved, even if the latter steps result in errors. So there is some value in deferring to the machine.

If we are confident that operators wouldn't leave stale annotations on the machine object causing an early, unintentional end to preservation, then we could defer to the machine object on all errors.

Also, what do you think the behaviour should be if the machine object is found to be un-annotated on a lister error? To me it seems like we should return the error, rather than assume preservation is not desired.

LMK what you think.

Trouble is, if it is a transient error (there's no way of knowing, I understand that), isn't it better to retry rather than change the current state?

But we aren't retrying here. ShouldFailedMachineBeTerminated returns true.

if !apierrors.IsNotFound(err) { klog.Errorf("error finding preservation state for machine %q: %v. Proceeding with termination of the machine.", machine.Name, err) return true }

However, if the machine was marked with when-failed or now, the machine object at least would get preserved, even if the latter steps result in errors. So there is some value in deferring to the machine.

Sure, that is being done in ManageMachinePreservation where we actually retry for non-NotFound errors. But the behavior is not the same for ShouldFailedMachineBeTerminated. There if its a transient error listing the node, the machine's preservation annotation is not even being checked, is that desirable? That is what I'm asking.

If we are confident that operators wouldn't leave stale annotations on the machine object causing an early, unintentional end to preservation, then we could defer to the machine object on all errors.

Also, what do you think the behaviour should be if the machine object is found to be un-annotated on a lister error? To me it seems like we should return the error, rather than assume preservation is not desired.

I think you misunderstood my question, I want preservation to be honored as well. #1135 (comment) #1135 (comment)

After offline discussion with @takoverflow and @r4mek, it was decided to defer to the machine's annotation value on all errors to allow operators to preserve machines whose backing nodes cannot be fetched (in this case it is currently only NotFound error). If the machine object is not annotated, we return true.

Addressed in 111e654

takoverflow · 2026-08-24T09:41:25Z

-	switch preserveValue {
-	case machineutils.PreserveMachineAnnotationValueWhenFailed, machineutils.PreserveMachineAnnotationValueNow, machineutils.PreserveMachineAnnotationValueAutoPreserved: // this is in case preservation process is not complete yet
+
+	preserveInfo := machineutils.GetPreserveStateInfo(node, machine)


In cases when lister returned NotFound error, the node object will be an empty one. I see that GetPreserveStateInfo then checks if node != nil, why can't the same be done in case of other errors when listing the node? Why take the destructive route of not honoring preservation?

takoverflow · 2026-08-24T10:28:41Z

+//
+//	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.


In this case, what was the reason the node annotation was removed?

If explicitly removed, isn't the intention to not preserve the machine anymore? I'm a bit confused by this example. Can you clarify?

In this case, what was the reason the node annotation was removed?
To stop preservation.

If explicitly removed, isn't the intention to not preserve the machine anymore? I'm a bit confused by this example. Can you clarify?

Yes. You're right. Without the LastAppliedNodeAnnotationValue annotation, we would end up continuing to preserve the machine instead of honouring the intent expressed by the removal of the annotation.

I think a better place for this docstring would be wherever LastAppliedNodeAnnotationValue is declared. The example explains why we need the annotation. Will rewrite to remove the ambiguity.

Moved the example to the docstring for LastAppliedNodeAnnotationValue, and also simplified the example. Changes made in 111e654

thiyyakat · 2026-08-24T10:52:27Z

Please don't make further changes, still in the process of reviewing the PR.

Do you mean I shouldn't make the changes you recommended yet? Or push any unrelated changes?

takoverflow · 2026-08-24T11:00:06Z

I would prefer both for now, because it's a bit hard to track what change has what kind of impact without revisiting the entire call stack 😅

Just give me one day to go through the entire thing. In between changes are difficult to reason about is all.

Thanks!

thiyyakat · 2026-08-24T11:01:37Z

I would prefer both for now, because it's a bit hard to track what change has what kind of impact without revisiting the entire call stack 😅

Sure 👍 Will hold off on making the changes you requested until you give me the go-ahead.

r4mek · 2026-08-21T10:20:18Z

+		if err != nil {
+			if !apierrors.IsNotFound(err) {
+				klog.Errorf("error finding preservation state for machine %q: %v. Proceeding with termination of the machine.", machine.Name, err)
+				return true


do we want to terminate the machine if we we get an error in Get() or just ignore it?

We can't ignore it unconditionally because the error may not be transient and preservation may not be desired for the machine+node. So, we can instead defer to the machine object like @takoverflow suggested in his comment. If the machine object carries no request for preservation then we terminate the machine.

r4mek · 2026-08-24T06:28:50Z

+
+	if err != nil {
+		if !apierrors.IsNotFound(err) {
 			return


should we return here or ignore this?

Ignoring error, as discussed offline. Addressed in 2f7ed54. PTAL

r4mek · 2026-08-24T06:43:38Z

+//	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, nodeFound bool) string {


it might be nice to have GetPreserveStateInfo() and GetEffectivePreservationAnnotations() be a single function.

There is some use to having them separate actually. If we only need to figure out if a machine has preservation state (I needed to do this for the preservation during updates issue), then we need only GetPreserveStateInfo() and isMachinePreservationBound(). We don't really care about what the effective value is in that case.

r4mek · 2026-08-24T06:49:08Z

+// IsPositivePreserveValue returns true when value is a preserve annotation value that requests
+// preservation (now/when-failed/auto-preserved). The value "false", empty, or any unrecognized
+// value is not a positive preserve value.
+func IsPositivePreserveValue(value string) bool {


ideally we should have a function which gives us the preserve annotation value by checking node and machine annotation. We should only get annotation values which are valid, else error. So this function would not be needed.

Positive values are different from valid values. By positive value(which has been renamed now ref: #1135 (comment)), I meant the annotation requested for preservation to happen (when-failed,now,auto-preserved), rather than stopping preservation ("" and false).

takoverflow · 2026-08-24T21:24:40Z

+		err       error
+	)
+	nodeName := machine.Labels[v1alpha1.NodeLabelKey]
+	// We don't return on error until it is determined whether the machine has valid preservation state


This comment is not correct, when there's an error, we return early and never bother checking machine's preservation state.

Removed in commit 111e654.

takoverflow · 2026-08-25T05:20:49Z

+	if nodeName != "" {
+		node, err = c.nodeLister.Get(nodeName)
+		if err != nil {
+			if !apierrors.IsNotFound(err) {


Trouble is, if it is a transient error (there's no way of knowing, I understand that), isn't it better to retry rather than change the current state?

But we aren't retrying here. ShouldFailedMachineBeTerminated returns true.

if !apierrors.IsNotFound(err) { klog.Errorf("error finding preservation state for machine %q: %v. Proceeding with termination of the machine.", machine.Name, err) return true }

However, if the machine was marked with when-failed or now, the machine object at least would get preserved, even if the latter steps result in errors. So there is some value in deferring to the machine.

Sure, that is being done in ManageMachinePreservation where we actually retry for non-NotFound errors. But the behavior is not the same for ShouldFailedMachineBeTerminated. There if its a transient error listing the node, the machine's preservation annotation is not even being checked, is that desirable? That is what I'm asking.

If we are confident that operators wouldn't leave stale annotations on the machine object causing an early, unintentional end to preservation, then we could defer to the machine object on all errors.

Also, what do you think the behaviour should be if the machine object is found to be un-annotated on a lister error? To me it seems like we should return the error, rather than assume preservation is not desired.

I think you misunderstood my question, I want preservation to be honored as well. #1135 (comment) #1135 (comment)

- Rename `IsMachinePreservationExpired` to `HasMachinePreservationExpired` - Fix bug where a machine with `nodeName=""` had `nodeFound` set to `true` - Rename `IsPositivePreserveValue` to `IsPreservationRequested` - Move example use case for `LastAppliedNodePreserveValueAnnotationKey` to docstring.

gagan16k · 2026-09-02T08:38:12Z

+}
+
+// GetEffectivePreservationAnnotations returns the effective preservation value.
+func GetEffectivePreservationAnnotations(info *PreserveStateInfo, nodeFound bool) string {


Can we fold nodeFound bool into PreserveStateInfo? Unless there is a use case where we will have to pass a different value for the same node, in which case it needs to be called something else.

I've been wondering if I should do this. However, is it really a part of preservation state though? Moreover, if say initially the node was not found because the cache was not updated and the node has only just registered. Towards the end of manageMachinePreservation we fetch the node again before removing the taint. If we instead use the value computed by GetEffectivePreservationAnnotations, then this value would be stale, and the taint may not get removed in that reconciliation. It is self-healing, but what I'm trying to ask is whether we can assume nodeFound to be a static value like the rest of preserveStateInfo within a reconciliation.

Looking at the code, I was thinking if we even need this flag in the first place. The only place where it is consumed is by getEffectivePreserveInfo()and it's whole purpose is to return info.MachineValue if node is not found. As an add-on to @r4mek's comment here, we could have an effectivePreservationValue in GetPreserveStateInfo(), which can be used/discarded as required.
This might solve both cases, where we still re-fetch the node for untainting-purposes

Regardless of error type returned while fetching node from cache, defer to the machine object's annotation value. Signed-off-by: thiyyakat <meghana.thiyyakat@sap.com>

thiyyakat requested a review from a team as a code owner August 20, 2026 16:46

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 Aug 20, 2026

thiyyakat added the kind/bug Bug label Aug 20, 2026

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

thiyyakat force-pushed the bug/not-found-mcs branch 2 times, most recently from ada7670 to 42c5202 Compare August 21, 2026 05:38

thiyyakat force-pushed the bug/not-found-mcs branch from 42c5202 to 33b6609 Compare August 21, 2026 10:11

takoverflow requested changes Aug 24, 2026

View reviewed changes

gagan16k reviewed Aug 24, 2026

View reviewed changes

Comment thread pkg/util/provider/machinecontroller/machine.go Outdated

Comment thread pkg/controller/machineset.go Outdated

r4mek reviewed Aug 25, 2026

View reviewed changes

takoverflow reviewed Aug 25, 2026

View reviewed changes

gagan16k reviewed Sep 2, 2026

View reviewed changes

Address review comments - part 2

2f7ed54

Regardless of error type returned while fetching node from cache, defer to the machine object's annotation value. Signed-off-by: thiyyakat <meghana.thiyyakat@sap.com>

Uh oh!

Conversation

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

Uh oh!

Uh oh!

gardener-prow Bot commented Aug 20, 2026

Uh oh!

thiyyakat commented Aug 21, 2026

Uh oh!

takoverflow 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!

thiyyakat Aug 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!

takoverflow Aug 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

takoverflow commented Aug 24, 2026

Uh oh!

thiyyakat commented Aug 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thiyyakat Sep 2, 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 Sep 2, 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!

takoverflow Aug 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 commented Aug 20, 2026 •

edited

Loading

thiyyakat Aug 25, 2026 •

edited

Loading

takoverflow Aug 25, 2026 •

edited

Loading

thiyyakat commented Aug 24, 2026 •

edited

Loading

thiyyakat commented Aug 24, 2026 •

edited

Loading

thiyyakat Sep 2, 2026 •

edited

Loading

thiyyakat Sep 2, 2026 •

edited

Loading

takoverflow Aug 25, 2026 •

edited

Loading