fix: use optimistic locking on rule status patches

internal/controller/helper.go

@@ -19,9 +19,11 @@ package controller
 import (
 	"encoding/json"
 	"maps"
+	"sort"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
+
 	readinessv1alpha1 "sigs.k8s.io/node-readiness-controller/api/v1alpha1"
 )
 
@@ -133,3 +135,48 @@ func filterStatusForExistingNodes(
 func labelsEqual(a, b map[string]string) bool {
 	return maps.Equal(a, b)
 }
+
+// nodeStatusDelta captures the per-node NodeEvaluation/NodeFailure changes produced by a single
+// processAllNodesForRule sweep, keyed by node name. It excludes AppliedNodes, ObservedGeneration,
+// and DryRunResults, which have a single writer and are safe to overwrite directly.
+//
+// A nil value in failures clears any failure recorded for that node. evaluations only holds
+// entries for nodes freshly (re-)evaluated this sweep.
+type nodeStatusDelta struct {
+	evaluations map[string]readinessv1alpha1.NodeEvaluation
+	failures    map[string]*readinessv1alpha1.NodeFailure
+}
+
+// applyNodeStatusDelta merges delta into rule's NodeEvaluations/FailedNodes, replacing only the
+// entries for nodes present in delta and leaving every other node's entry untouched.
+func applyNodeStatusDelta(rule *readinessv1alpha1.NodeReadinessRule, delta nodeStatusDelta) {
+	if len(delta.evaluations) > 0 {
+		merged := make([]readinessv1alpha1.NodeEvaluation, 0, len(rule.Status.NodeEvaluations)+len(delta.evaluations))
+		for _, eval := range rule.Status.NodeEvaluations {
+			if _, changed := delta.evaluations[eval.NodeName]; !changed {
+				merged = append(merged, eval)
+			}
+		}
+		for _, eval := range delta.evaluations {
+			merged = append(merged, eval)
+		}
+		sort.Slice(merged, func(i, j int) bool { return merged[i].NodeName < merged[j].NodeName })
+		rule.Status.NodeEvaluations = merged
+	}
+
+	if len(delta.failures) > 0 {
+		merged := make([]readinessv1alpha1.NodeFailure, 0, len(rule.Status.FailedNodes)+len(delta.failures))
+		for _, failure := range rule.Status.FailedNodes {
+			if _, changed := delta.failures[failure.NodeName]; !changed {
+				merged = append(merged, failure)
+			}
+		}
+		for _, failure := range delta.failures {
+			if failure != nil {
+				merged = append(merged, *failure)
+			}
+		}
+		sort.Slice(merged, func(i, j int) bool { return merged[i].NodeName < merged[j].NodeName })
+		rule.Status.FailedNodes = merged
+	}
+}

internal/controller/node_controller.go

@@ -169,14 +169,7 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context
 
 		var successfullyPatchedRule *readinessv1alpha1.NodeReadinessRule
 
-		err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
-			latestRule := &readinessv1alpha1.NodeReadinessRule{}
-			if err := r.Get(ctx, client.ObjectKey{Name: rule.Name}, latestRule); err != nil {
-				return err
-			}
-
-			patch := client.MergeFrom(latestRule.DeepCopy())
-
+		err := r.patchRuleStatusWithOptimisticLock(ctx, rule.Name, func(latestRule *readinessv1alpha1.NodeReadinessRule) bool {
 			// update only this specific node evaluation status
 			currEval := readinessv1alpha1.NodeEvaluation{}
 			for _, eval := range rule.Status.NodeEvaluations {
@@ -215,12 +208,8 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context
 			}
 			latestRule.Status.FailedNodes = updatedFailedNodes
 
-			if err := r.Status().Patch(ctx, latestRule, patch); err != nil {
-				return err
-			}
-
 			successfullyPatchedRule = latestRule
-			return nil
+			return true
 		})
 
 		if err != nil {
@@ -446,7 +435,12 @@ func (r *RuleReadinessController) markBootstrapCompleted(ctx context.Context, no
 	deferred := false
 	annotationKey := bootstrapAnnotationKey(rule.GetUID())
 
-	// retry to handle conflict with concurrent node updates
+	// The optimistic lock here isn't guarding the annotation merge itself (that's map-valued
+	// and merges cleanly against concurrent writers, e.g. Kubelet). It guards the
+	// hasTaintBySpec check above: without it, a taint added between that check and the Patch
+	// below would go undetected, and we'd mark bootstrap complete on a node that still carries
+	// the taint. See the "should not mark bootstrap completed when the rule taints concurrently"
+	// test for the regression this prevents.
 	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
 		node := &corev1.Node{}
 		if err := r.Get(ctx, client.ObjectKey{Name: nodeName}, node); err != nil {
@@ -464,15 +458,15 @@ func (r *RuleReadinessController) markBootstrapCompleted(ctx context.Context, no
 		}
 		deferred = false
 
-		patch := client.MergeFromWithOptions(node.DeepCopy(), client.MergeFromWithOptimisticLock{})
+		stored := node.DeepCopy()
 
 		// Initialize annotations map if nil.
 		if node.Annotations == nil {
 			node.Annotations = make(map[string]string)
 		}
 
 		node.Annotations[annotationKey] = bootstrapAnnotationValue(rule.Name)
-		if err := r.Patch(ctx, node, patch); err != nil {
+		if err := r.Patch(ctx, node, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{})); err != nil {
 			return err
 		}