Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
thiyyakat wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
33b6609
Fix handling of NotFound errors in `func shouldFailedMachineBeTermina…
thiyyakat Aug 20, 2026
111e654
Address review comments - part 1:
thiyyakat Aug 31, 2026
2f7ed54
Address review comments - part 2
thiyyakat Sep 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 26 additions & 37 deletions pkg/controller/machineset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"sync"
"time"

corev1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -904,29 +905,37 @@ func isMachineStatusEqual(s1, s2 v1alpha1.MachineStatus) bool {
// or if it is a candidate for auto-preservation. If none of these conditions are met, it returns true indicating
// that the failed machine should be terminated.
func (c *controller) shouldFailedMachineBeTerminated(machine *v1alpha1.Machine) bool {
// if preserve expiry time is set and is in the future, machine is already preserved
if machine.Status.CurrentStatus.PreserveExpiryTime != nil {
if machine.Status.CurrentStatus.PreserveExpiryTime.After(time.Now()) {
klog.V(3).Infof("Failed machine %q is preserved until %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime)
return false
var (
nodeFound bool
node *corev1.Node
err error
)
nodeName := machine.Labels[v1alpha1.NodeLabelKey]
// We don't return on error until it is determined whether the machine has valid preservation state

@takoverflow takoverflow Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thiyyakat thiyyakat Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in commit 111e654.

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

@takoverflow takoverflow Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@takoverflow takoverflow Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@takoverflow takoverflow Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thiyyakat thiyyakat Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@takoverflow takoverflow Aug 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@thiyyakat thiyyakat Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thiyyakat thiyyakat Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 111e654

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

@r4mek r4mek Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@thiyyakat thiyyakat Aug 31, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
klog.Warningf("node %q not found for machine %q.", nodeName, machine.Name)
} else {
nodeFound = true
}
klog.V(3).Infof("Preservation of failed machine %q has timed out at %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime)
return true
}
preserveValue, err := c.findEffectivePreserveValue(machine)
if err != nil {
// in case of error fetching node or annotations, we don't want to block deletion of failed machines, so we return true
klog.Errorf("error finding effective preserve value for machine %q: %v. Proceeding with termination of the machine.", machine.Name, err)

if machineutils.IsMachinePreservationExpired(machine) {
Comment thread
takoverflow marked this conversation as resolved.
Outdated
Comment thread
gagan16k marked this conversation as resolved.
Outdated
klog.V(3).Infof("Preservation of failed machine %q has timed out at %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime)
return true
}
switch preserveValue {
case machineutils.PreserveMachineAnnotationValueWhenFailed, machineutils.PreserveMachineAnnotationValueNow, machineutils.PreserveMachineAnnotationValueAutoPreserved: // this is in case preservation process is not complete yet

preserveInfo := machineutils.GetPreserveStateInfo(node, machine)

@takoverflow takoverflow Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if machineutils.IsPositivePreserveValue(machineutils.GetEffectivePreservationAnnotations(&preserveInfo, nodeFound)) {
klog.V(3).Infof("Failed machine %q is either preserved or in the process of being preserved.", machine.Name)
return false
case machineutils.PreserveMachineAnnotationValueFalse:
return true
default:
return true
}
return true
}

// manageAutoPreservationOfFailedMachines annotates failed machines with preserve=auto-preserved annotation
Expand Down Expand Up @@ -1019,23 +1028,3 @@ func removeAutoPreserveAnnotationFromMachine(machineToUpdate *v1alpha1.Machine)
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]
lANodeAnnotationValue = machine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey]
nodeName := machine.Labels[v1alpha1.NodeLabelKey]
if nodeName != "" {
node, err := c.nodeLister.Get(nodeName)
if err != nil {
klog.Errorf("error fetching node %q for machine %q: %v", nodeName, machine.Name, err)
return "", err
}
nodeAnnotationValue = node.Annotations[machineutils.PreserveMachineAnnotationKey]
}
if nodeAnnotationValue == "" && lANodeAnnotationValue == "" {
return machineAnnotationValue, nil
} else {
return nodeAnnotationValue, nil
}
}
110 changes: 30 additions & 80 deletions pkg/util/provider/machinecontroller/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,18 +756,6 @@ func (c *controller) isCreationProcessing(machine *v1alpha1.Machine) bool {
Machine Preservation operations
*/

// preserveStateInfo encapsulates the preservation annotation values found
// on the machine and node objects, along with the effective preservation value for the machine
// and the last applied node preserve value by MCM.
type preserveStateInfo struct {
nodeAnnotated bool
machineAnnotated bool
nodeValue string
machineValue string
lastAppliedNodeValue string
preserveExpiryTimeSet bool
}

// 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) {
defer func() {
Expand All @@ -785,35 +773,45 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a
retry = machineutils.LongRetry
}
}()
var (
nodeFound bool
node *corev1.Node
)
nodeName := machine.Labels[v1alpha1.NodeLabelKey]
// We buffer the error returned here until we can tell if the machine is preservation-bound.
preserveInfo, getErr := c.getPreserveStateInfo(machine)
if preserveInfo.machineAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.machineValue) {
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
}
preserveInfo := machineutils.GetPreserveStateInfo(node, machine)
if preserveInfo.MachineAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.MachineValue) {
// If machine is annotated incorrectly, log and proceed as though machine is not annotated.
klog.Warningf("Preserve annotation %q=%q on machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.machineValue, machine.Name)
preserveInfo.machineAnnotated = false
preserveInfo.machineValue = ""
klog.Warningf("Preserve annotation %q=%q on machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.MachineValue, machine.Name)
preserveInfo.MachineAnnotated = false
preserveInfo.MachineValue = ""
}
if preserveInfo.nodeAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.nodeValue) {
klog.Warningf("Preserve annotation %q=%q on node %q backing machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.nodeValue, nodeName, machine.Name)
if preserveInfo.NodeAnnotated && !machineutils.AllowedPreserveAnnotationValues.Has(preserveInfo.NodeValue) {
klog.Warningf("Preserve annotation %q=%q on node %q backing machine %q is invalid", machineutils.PreserveMachineAnnotationKey, preserveInfo.NodeValue, nodeName, machine.Name)
return
}
preservationBound := isMachinePreservationBound(&preserveInfo)
if !preservationBound {
// We clear the error here to prevent preservation logic from interfering with non-preservation-bound machines.
err = nil
return
} else if getErr != nil {
if !apierrors.IsNotFound(getErr) {
err = getErr
}

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

@r4mek r4mek Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we return here or ignore this?

@thiyyakat thiyyakat Sep 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
klog.Warningf("Couldn't find node %q for machine %q", nodeName, machine.Name)
err = nil
klog.Warningf("Couldn't find node %q for machine %q", nodeName, machine.Name)
} else {
nodeFound = true
Comment thread
gagan16k marked this conversation as resolved.
Outdated
}

// 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 := getEffectivePreservationAnnotations(&preserveInfo, getErr)
effectivePreserveValue := machineutils.GetEffectivePreservationAnnotations(&preserveInfo, nodeFound)

var removeAnnotations bool
clone := machine.DeepCopy()
Expand Down Expand Up @@ -872,81 +870,33 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a
}

if shouldAnnotationsBeUpdatedOnMachine(removeAnnotations, &preserveInfo) {
err = c.updatePreserveAnnotationOnMachine(ctx, preserveInfo.nodeValue, clone)
err = c.updatePreserveAnnotationOnMachine(ctx, preserveInfo.NodeValue, clone)
}
return
}

// getEffectivePreservationAnnotations returns the effective preservation value.
//
// If there is no active node annotation AND no previously-applied node annotation,
// enforce machine's preserve annotation.
// Otherwise, the node annotation takes precedence (even if now empty/removed).
//
// lastAppliedNodeValue is required to handle the following scenario:
//
// 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.
// 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, getPreserveStateErr error) string {
// If the node cannot be found, nodeValue is "".
// In this case, we want the machine's annotation value to be enforced.
if apierrors.IsNotFound(getPreserveStateErr) {
return info.machineValue
}
// If there is no active node annotation AND no previously-applied node annotation,
// enforce machine's preserve annotation.
// Otherwise, the node annotation takes precedence (even if now empty/removed).
if info.nodeValue == "" && info.lastAppliedNodeValue == "" {
return info.machineValue
}
return info.nodeValue
}

func isMachinePreservationBound(info *preserveStateInfo) bool {
// isMachinePreservationBound returns whether the machine carries any preservation state.
func isMachinePreservationBound(info *machineutils.PreserveStateInfo) bool {
// if machine has no preservation state, the machine is not preservation-bound
if !info.preserveExpiryTimeSet && !info.nodeAnnotated && !info.machineAnnotated && info.lastAppliedNodeValue == "" {
if !info.PreserveExpiryTimeSet && !info.NodeAnnotated && !info.MachineAnnotated && info.LastAppliedNodeValue == "" {
return false
}
return true
}

func (c *controller) getPreserveStateInfo(machine *v1alpha1.Machine) (preserveStateInfo, error) {
var info preserveStateInfo
if machine.Annotations != nil {
info.machineValue, info.machineAnnotated = machine.Annotations[machineutils.PreserveMachineAnnotationKey]
info.lastAppliedNodeValue = machine.Annotations[machineutils.LastAppliedNodePreserveValueAnnotationKey]
}
if machine.Status.CurrentStatus.PreserveExpiryTime != nil {
info.preserveExpiryTimeSet = true
}
nodeName := machine.Labels[v1alpha1.NodeLabelKey]
if nodeName != "" {
node, err := c.nodeLister.Get(nodeName)
if err != nil {
return info, err
}
info.nodeValue, info.nodeAnnotated = node.Annotations[machineutils.PreserveMachineAnnotationKey]
}
return info, nil
}

// shouldAnnotationsBeUpdatedOnMachine returns true when the machine's annotation tracking needs
// to be synced after a preservation action.
func shouldAnnotationsBeUpdatedOnMachine(removeAnnotations bool, preserveInfo *preserveStateInfo) bool {
func shouldAnnotationsBeUpdatedOnMachine(removeAnnotations bool, preserveInfo *machineutils.PreserveStateInfo) bool {
// annotations were already removed by stopPreservationIfActive — nothing left to sync
if removeAnnotations {
return false
}
// node annotation is not in control — machine annotation prevails, no sync needed
if !preserveInfo.nodeAnnotated && preserveInfo.lastAppliedNodeValue == "" {
if !preserveInfo.NodeAnnotated && preserveInfo.LastAppliedNodeValue == "" {
return false
}
// node value is unchanged and machine has no annotation to clear — nothing has changed
if preserveInfo.nodeValue == preserveInfo.lastAppliedNodeValue && !preserveInfo.machineAnnotated {
if preserveInfo.NodeValue == preserveInfo.LastAppliedNodeValue && !preserveInfo.MachineAnnotated {
return false
}
return true
Expand Down
Loading
Loading