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 2 commits
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
60 changes: 22 additions & 38 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,32 @@ 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
}
klog.V(3).Infof("Preservation of failed machine %q has timed out at %v", machine.Name, machine.Status.CurrentStatus.PreserveExpiryTime)
var (
nodeFound bool
node *corev1.Node
err error
)
if machineutils.HasMachinePreservationExpired(machine) {
Comment thread
gagan16k marked this conversation as resolved.
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)
return true

nodeName := machine.Labels[v1alpha1.NodeLabelKey]
if nodeName != "" {
node, err = c.nodeLister.Get(nodeName)
if err != nil {
klog.Warningf("Error fetching node %q of machine %q: %v", nodeName, machine.Name, err)
} else {
nodeFound = 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.IsPreservationRequested(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 +1023,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
}
}
117 changes: 34 additions & 83 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,46 @@ 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
if err == nil {
nodeFound = true
}
}
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)
}

// 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 All @@ -824,7 +823,7 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a
clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
case machineutils.PreserveMachineAnnotationValueWhenFailed:
// on timing out, remove preserve annotation to prevent incorrect re-preservation
if machineutils.IsMachinePreservationExpired(clone) {
if machineutils.HasMachinePreservationExpired(clone) {
removeAnnotations = true
clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
} else if !machineutils.IsMachineFailed(clone) {
Expand All @@ -833,15 +832,15 @@ func (c *controller) manageMachinePreservation(ctx context.Context, machine *v1a
clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue)
}
case machineutils.PreserveMachineAnnotationValueNow:
if machineutils.IsMachinePreservationExpired(clone) {
if machineutils.HasMachinePreservationExpired(clone) {
// on timing out, remove preserve annotation to prevent incorrect re-preservation
removeAnnotations = true
clone, err = c.stopPreservationIfActive(ctx, clone, removeAnnotations)
} else {
clone, err = c.preserveMachine(ctx, clone, effectivePreserveValue)
}
case machineutils.PreserveMachineAnnotationValueAutoPreserved:
if !machineutils.IsMachineFailed(clone) || machineutils.IsMachinePreservationExpired(clone) {
if !machineutils.IsMachineFailed(clone) || machineutils.HasMachinePreservationExpired(clone) {
// To prevent incorrect re-preservation of a recovered, previously auto-preserved machine on future failures
// (since the autoPreserveFailedMachineCount maintained by the machineSetController, may have changed),
// in addition to stopping preservation, we also remove the preservation annotation on the machine.
Expand Down Expand Up @@ -872,81 +871,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