fix: stop reporting adoption for taints the controller applied itself

internal/controller/node_controller.go

@@ -152,6 +152,21 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context
 			"rule", rule.Name,
 			"ruleResourceVersion", rule.ResourceVersion)
 
+		// The cached rule carries a status snapshot taken the last time
+		// RuleReconciler ran. Node evaluations persisted since then are missing
+		// from it, and because the rule spec is immutable that reconcile rarely
+		// runs again, so the snapshot can stay stale for the lifetime of the
+		// process. Refresh the evaluations from the live object first, otherwise
+		// every pass looks like the node's first evaluation. This reads through
+		// the controller-runtime cache, so it is not an extra API call.
+		latestRule := &readinessv1alpha1.NodeReadinessRule{}
+		if err := r.Get(ctx, client.ObjectKey{Name: rule.Name}, latestRule); err != nil {
+			log.V(4).Info("Could not refresh rule status before evaluation, using cached copy",
+				"node", node.Name, "rule", rule.Name, "error", err.Error())
+		} else {
+			rule.Status.NodeEvaluations = latestRule.Status.NodeEvaluations
+		}
+
 		if err := r.evaluateRuleForNode(ctx, rule, node); err != nil {
 			log.Error(err, "Failed to evaluate rule for node",
 				"node", node.Name, "rule", rule.Name)

internal/controller/node_controller_test.go

@@ -19,6 +19,7 @@ package controller
 import (
 	"context"
 	"fmt"
+	"strings"
 	"sync/atomic"
 	"time"
 
@@ -1155,4 +1156,147 @@ var _ = Describe("Node Controller", func() {
 				"metrics.Failures{rule, EvaluationError} must increment when the node reconciler hits an evaluation error")
 		})
 	})
+
+	Context("when a node is re-evaluated while still unready", func() {
+		const (
+			adoptNodeName = "adopt-events-node"
+			adoptRuleName = "adopt-events-rule"
+			adoptTaintKey = "readiness.k8s.io/adopt-events"
+			adoptCondType = "AdoptEventsCondition"
+		)
+
+		var (
+			adoptRecorder *events.FakeRecorder
+			adoptCtrl     *RuleReadinessController
+			adoptNodeRec  *NodeReconciler
+		)
+
+		countAdoptedEvents := func() int {
+			count := 0
+			for {
+				select {
+				case e := <-adoptRecorder.Events:
+					if strings.Contains(e, "TaintAdopted") {
+						count++
+					}
+				default:
+					return count
+				}
+			}
+		}
+
+		BeforeEach(func() {
+			adoptRecorder = events.NewFakeRecorder(100)
+			adoptCtrl = &RuleReadinessController{
+				Client:        k8sClient,
+				Scheme:        k8sClient.Scheme(),
+				clientset:     fake.NewSimpleClientset(),
+				ruleCache:     make(map[string]*nodereadinessiov1alpha1.NodeReadinessRule),
+				EventRecorder: adoptRecorder,
+			}
+			adoptNodeRec = &NodeReconciler{
+				Client:     k8sClient,
+				Scheme:     k8sClient.Scheme(),
+				Controller: adoptCtrl,
+			}
+
+			node := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   adoptNodeName,
+					Labels: map[string]string{"env": "adopt-events"},
+				},
+			}
+			Expect(k8sClient.Create(ctx, node)).To(Succeed())
+
+			fetched := &corev1.Node{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: adoptNodeName}, fetched)).To(Succeed())
+			fetched.Status.Conditions = []corev1.NodeCondition{{
+				Type:               corev1.NodeConditionType(adoptCondType),
+				Status:             corev1.ConditionFalse,
+				LastTransitionTime: metav1.Now(),
+				LastHeartbeatTime:  metav1.Now(),
+				Reason:             "NotReady",
+				Message:            "still starting up",
+			}}
+			Expect(k8sClient.Status().Update(ctx, fetched)).To(Succeed())
+
+			rule := &nodereadinessiov1alpha1.NodeReadinessRule{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       adoptRuleName,
+					Finalizers: []string{finalizerName},
+				},
+				Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{
+					Conditions: []nodereadinessiov1alpha1.ConditionRequirement{
+						{Type: adoptCondType, RequiredStatus: corev1.ConditionTrue},
+					},
+					NodeSelector:    metav1.LabelSelector{MatchLabels: map[string]string{"env": "adopt-events"}},
+					Taint:           corev1.Taint{Key: adoptTaintKey, Effect: corev1.TaintEffectNoSchedule},
+					EnforcementMode: nodereadinessiov1alpha1.EnforcementModeContinuous,
+				},
+			}
+			Expect(k8sClient.Create(ctx, rule)).To(Succeed())
+
+			// Seed the cache the way RuleReconciler does on its first pass, before
+			// any node evaluation has been persisted.
+			adoptCtrl.updateRuleCache(ctx, rule)
+		})
+
+		AfterEach(func() {
+			node := &corev1.Node{}
+			if err := k8sClient.Get(ctx, types.NamespacedName{Name: adoptNodeName}, node); err == nil {
+				_ = k8sClient.Delete(ctx, node)
+			}
+			rule := &nodereadinessiov1alpha1.NodeReadinessRule{}
+			if err := k8sClient.Get(ctx, types.NamespacedName{Name: adoptRuleName}, rule); err == nil {
+				rule.Finalizers = nil
+				_ = k8sClient.Update(ctx, rule)
+				_ = k8sClient.Delete(ctx, rule)
+			}
+			adoptCtrl.removeRuleFromCache(ctx, adoptRuleName)
+		})
+
+		It("should not report adoption for a taint the controller applied itself", func() {
+			By("applying the taint on the first reconcile")
+			_, err := adoptNodeRec.Reconcile(ctx, reconcile.Request{
+				NamespacedName: types.NamespacedName{Name: adoptNodeName},
+			})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(countAdoptedEvents()).To(Equal(0), "applying a taint is not an adoption")
+
+			By("persisting the evaluation for that node")
+			persisted := &nodereadinessiov1alpha1.NodeReadinessRule{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: adoptRuleName}, persisted)).To(Succeed())
+			Expect(persisted.Status.NodeEvaluations).To(ContainElement(HaveField("NodeName", adoptNodeName)))
+
+			By("re-evaluating while the node is still unready")
+			for range 3 {
+				_, err = adoptNodeRec.Reconcile(ctx, reconcile.Request{
+					NamespacedName: types.NamespacedName{Name: adoptNodeName},
+				})
+				Expect(err).NotTo(HaveOccurred())
+			}
+
+			Expect(countAdoptedEvents()).To(Equal(0),
+				"the rule already owns this taint, so no further TaintAdopted events should be emitted")
+		})
+
+		It("should still report adoption for a taint that pre-existed the rule", func() {
+			By("putting the taint on the node before the rule ever evaluates it")
+			node := &corev1.Node{}
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: adoptNodeName}, node)).To(Succeed())
+			node.Spec.Taints = append(node.Spec.Taints, corev1.Taint{
+				Key:    adoptTaintKey,
+				Effect: corev1.TaintEffectNoSchedule,
+			})
+			Expect(k8sClient.Update(ctx, node)).To(Succeed())
+
+			_, err := adoptNodeRec.Reconcile(ctx, reconcile.Request{
+				NamespacedName: types.NamespacedName{Name: adoptNodeName},
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			Expect(countAdoptedEvents()).To(Equal(1),
+				"a taint the controller did not apply should still be reported as adopted once")
+		})
+	})
 })