fix(controller): emit Warning events on failure paths for taint operations and reconciler errors

internal/controller/nodereadinessrule_controller.go

@@ -117,6 +117,7 @@ func (r *RuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 
 	// Add finalizer first if not set to avoid the race condition between init and delete.
 	if finalizerAdded, err := r.ensureFinalizer(ctx, rule, finalizerName); err != nil {
+		r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "FinalizerError", "EnsureFinalizer", "Failed to ensure finalizer: %v", err)
 		return ctrl.Result{}, err
 	} else if finalizerAdded {
 		// Adding a finalizer modifies Metadata, not Spec, so the Generation is unchanged.
@@ -127,6 +128,7 @@ func (r *RuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 
 	nodeList := &corev1.NodeList{}
 	if err := r.List(ctx, nodeList); err != nil {
+		r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "ListNodesError", "ListNodes", "Failed to list nodes: %v", err)
 		return ctrl.Result{}, err
 	}
 
@@ -142,6 +144,7 @@ func (r *RuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 	if rule.Spec.DryRun {
 		if err := r.Controller.processDryRun(ctx, rule, nodeList); err != nil {
 			log.Error(err, "Failed to process dry run", "rule", rule.Name)
+			r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "DryRunError", "ProcessDryRun", "Failed to process dry run: %v", err)
 			return ctrl.Result{RequeueAfter: time.Minute}, err
 		}
 	} else {
@@ -151,19 +154,22 @@ func (r *RuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 		// Process all applicable nodes for this rule
 		if err := r.Controller.processAllNodesForRule(ctx, rule, nodeList); err != nil {
 			log.Error(err, "Failed to process nodes for rule", "rule", rule.Name)
+			r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "ProcessNodesError", "ProcessNodes", "Failed to process nodes: %v", err)
 			return ctrl.Result{RequeueAfter: time.Minute}, err
 		}
 	}
 
 	// Update rule status
 	if err := r.Controller.updateRuleStatus(ctx, rule); err != nil {
 		log.Error(err, "Failed to update rule status", "rule", rule.Name)
+		r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "StatusUpdateError", "UpdateStatus", "Failed to update rule status: %v", err)
 		return ctrl.Result{RequeueAfter: time.Minute}, err
 	}
 
 	// Clean up status for deleted nodes
 	if err := r.Controller.cleanupDeletedNodes(ctx, rule, nodeList); err != nil {
 		log.Error(err, "Failed to clean up deleted nodes", "rule", rule.Name)
+		r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "CleanupNodesError", "CleanupDeletedNodes", "Failed to clean up deleted nodes status: %v", err)
 		return ctrl.Result{RequeueAfter: time.Minute}, err
 	}
 
@@ -191,6 +197,7 @@ func (r *RuleReconciler) reconcileDelete(ctx context.Context, rule *readinessv1a
 	log.Info("Cleaning up taints for deleted rule", "rule", rule.Name)
 	if err := r.Controller.cleanupTaintsForRule(ctx, rule, nodeList); err != nil {
 		log.Error(err, "Failed to cleanup taints for rule", "rule", rule.Name)
+		r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "CleanupTaintsError", "CleanupTaints", "Failed to cleanup taints: %v", err)
 		return ctrl.Result{RequeueAfter: time.Minute}, err
 	}
 
@@ -202,6 +209,7 @@ func (r *RuleReconciler) reconcileDelete(ctx context.Context, rule *readinessv1a
 	controllerutil.RemoveFinalizer(rule, finalizerName)
 	err := r.Patch(ctx, rule, patch)
 	if err != nil {
+		r.Controller.EventRecorder.Eventf(rule, nil, corev1.EventTypeWarning, "FinalizerError", "RemoveFinalizer", "Failed to remove finalizer: %v", err)
 		return ctrl.Result{}, err
 	}
 
@@ -403,6 +411,7 @@ func (r *RuleReadinessController) evaluateRuleForNode(ctx context.Context, rule
 
 		if err = r.removeTaintBySpec(ctx, node, rule.Spec.Taint, rule.Name); err != nil {
 			metrics.Failures.WithLabelValues(rule.Name, string(metrics.FailureReasonRemoveTaintError)).Inc()
+			r.EventRecorder.Eventf(node, rule, corev1.EventTypeWarning, "RemoveTaintError", "RemoveTaint", "Failed to remove taint '%s:%s' by rule %q: %v", rule.Spec.Taint.Key, rule.Spec.Taint.Effect, rule.Name, err)
 			return fmt.Errorf("failed to remove taint: %w", err)
 		}
 
@@ -435,6 +444,7 @@ func (r *RuleReadinessController) evaluateRuleForNode(ctx context.Context, rule
 
 		if err = r.addTaintBySpec(ctx, node, rule.Spec.Taint, rule.Name); err != nil {
 			metrics.Failures.WithLabelValues(rule.Name, string(metrics.FailureReasonAddTaintError)).Inc()
+			r.EventRecorder.Eventf(node, rule, corev1.EventTypeWarning, "AddTaintError", "AddTaint", "Failed to add taint '%s:%s' by rule %q: %v", rule.Spec.Taint.Key, rule.Spec.Taint.Effect, rule.Name, err)
 			return fmt.Errorf("failed to add taint: %w", err)
 		}
 
@@ -685,6 +695,7 @@ func (r *RuleReadinessController) cleanupTaintsForRule(ctx context.Context, rule
 
 			if err := r.removeTaintBySpec(ctx, &node, rule.Spec.Taint, rule.Name); err != nil {
 				errors = append(errors, fmt.Sprintf("node %s: %v", node.Name, err))
+				r.EventRecorder.Eventf(&node, rule, corev1.EventTypeWarning, "RemoveTaintError", "RemoveTaint", "Failed to remove taint '%s:%s' by rule %q during cleanup: %v", rule.Spec.Taint.Key, rule.Spec.Taint.Effect, rule.Name, err)
 			}
 		}
 	}

internal/controller/nodereadinessrule_controller_test.go

@@ -2234,5 +2234,42 @@ var _ = Describe("NodeReadinessRule Controller", func() {
 			}
 			Expect(failedNames).NotTo(ContainElement("stale-recovery-node"))
 		})
+
+		It("should emit Warning events on failure paths for taint operations and node evaluation", func() {
+			fakeRecorder := events.NewFakeRecorder(100)
+			c := &RuleReadinessController{
+				Client:        k8sClient,
+				Scheme:        scheme,
+				clientset:     fakeClientset,
+				ruleCache:     make(map[string]*nodereadinessiov1alpha1.NodeReadinessRule),
+				EventRecorder: fakeRecorder,
+			}
+
+			nonExistentNode := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{Name: "non-existent-node-for-events"},
+			}
+			testRule := &nodereadinessiov1alpha1.NodeReadinessRule{
+				ObjectMeta: metav1.ObjectMeta{Name: "event-test-rule"},
+				Spec: nodereadinessiov1alpha1.NodeReadinessRuleSpec{
+					Conditions: []nodereadinessiov1alpha1.ConditionRequirement{
+						{Type: "Ready", RequiredStatus: corev1.ConditionTrue},
+					},
+					Taint: corev1.Taint{Key: "readiness.k8s.io/event-test", Effect: corev1.TaintEffectNoSchedule},
+				},
+			}
+
+			// Trigger evaluation failure on non-existent node.
+			err := c.evaluateRuleForNode(ctx, testRule, nonExistentNode)
+			Expect(err).To(HaveOccurred())
+
+			var eventList []string
+			for len(fakeRecorder.Events) > 0 {
+				eventList = append(eventList, <-fakeRecorder.Events)
+			}
+
+			Expect(eventList).To(HaveLen(1))
+			Expect(eventList[0]).To(ContainSubstring("AddTaintError"))
+			Expect(eventList[0]).To(ContainSubstring("Warning"))
+		})
 	})
 })

internal/controller/warning_events_test.go

@@ -0,0 +1,244 @@
+/*
+Copyright The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controller
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/client-go/tools/events"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	readinessv1alpha1 "sigs.k8s.io/node-readiness-controller/api/v1alpha1"
+)
+
+type failingStatusWriter struct {
+	client.StatusWriter
+	patchError error
+}
+
+func (sw *failingStatusWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+	if sw.patchError != nil {
+		return sw.patchError
+	}
+	return sw.StatusWriter.Patch(ctx, obj, patch, opts...)
+}
+
+type failingClient struct {
+	client.Client
+	listError        error
+	statusPatchError error
+}
+
+func (c *failingClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
+	if c.listError != nil {
+		return c.listError
+	}
+	return c.Client.List(ctx, list, opts...)
+}
+
+func (c *failingClient) Status() client.StatusWriter {
+	return &failingStatusWriter{
+		StatusWriter: c.Client.Status(),
+		patchError:   c.statusPatchError,
+	}
+}
+
+func TestWarningEventsEmittedOnFailures(t *testing.T) {
+	ctx := context.Background()
+	scheme := runtime.NewScheme()
+	_ = corev1.AddToScheme(scheme)
+	_ = readinessv1alpha1.AddToScheme(scheme)
+
+	rule := &readinessv1alpha1.NodeReadinessRule{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "test-warning-rule",
+			Finalizers: []string{finalizerName},
+		},
+		Spec: readinessv1alpha1.NodeReadinessRuleSpec{
+			Conditions: []readinessv1alpha1.ConditionRequirement{
+				{Type: "Ready", RequiredStatus: corev1.ConditionTrue},
+			},
+			Taint:        corev1.Taint{Key: "readiness.k8s.io/warning-test", Effect: corev1.TaintEffectNoSchedule},
+			NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{"role": "worker"}},
+		},
+	}
+
+	node := &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{Name: "test-node", Labels: map[string]string{"role": "worker"}},
+		Status:     corev1.NodeStatus{Conditions: []corev1.NodeCondition{{Type: "Ready", Status: corev1.ConditionFalse}}},
+	}
+
+	fc := fakeclient.NewClientBuilder().WithScheme(scheme).WithObjects(rule).WithStatusSubresource(rule).Build()
+	fakeRecorder := events.NewFakeRecorder(100)
+
+	c := &RuleReadinessController{
+		Client:        fc,
+		Scheme:        scheme,
+		clientset:     fake.NewSimpleClientset(),
+		ruleCache:     map[string]*readinessv1alpha1.NodeReadinessRule{rule.Name: rule},
+		EventRecorder: fakeRecorder,
+	}
+
+	if err := c.evaluateRuleForNode(ctx, rule, node); err == nil {
+		t.Fatalf("expected error, got nil")
+	}
+
+	var eventsCaptured []string
+	for len(fakeRecorder.Events) > 0 {
+		eventsCaptured = append(eventsCaptured, <-fakeRecorder.Events)
+	}
+
+	if len(eventsCaptured) != 1 {
+		t.Fatalf("expected exactly 1 event, got: %v", eventsCaptured)
+	}
+
+	if !strings.Contains(eventsCaptured[0], "AddTaintError") || !strings.Contains(eventsCaptured[0], "Warning") {
+		t.Fatalf("expected Warning AddTaintError event, got: %v", eventsCaptured[0])
+	}
+}
+
+func TestReconcile_ListError(t *testing.T) {
+	ctx := context.Background()
+	scheme := runtime.NewScheme()
+	_ = corev1.AddToScheme(scheme)
+	_ = readinessv1alpha1.AddToScheme(scheme)
+
+	rule := &readinessv1alpha1.NodeReadinessRule{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "test-warning-rule",
+			Finalizers: []string{finalizerName},
+		},
+		Spec: readinessv1alpha1.NodeReadinessRuleSpec{
+			Conditions: []readinessv1alpha1.ConditionRequirement{
+				{Type: "Ready", RequiredStatus: corev1.ConditionTrue},
+			},
+			Taint:        corev1.Taint{Key: "readiness.k8s.io/warning-test", Effect: corev1.TaintEffectNoSchedule},
+			NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{"role": "worker"}},
+		},
+	}
+
+	baseClient := fakeclient.NewClientBuilder().WithScheme(scheme).WithObjects(rule).WithStatusSubresource(rule).Build()
+	fc := &failingClient{
+		Client:    baseClient,
+		listError: fmt.Errorf("fake listing error"),
+	}
+	fakeRecorder := events.NewFakeRecorder(100)
+
+	c := &RuleReadinessController{
+		Client:        fc,
+		Scheme:        scheme,
+		clientset:     fake.NewSimpleClientset(),
+		ruleCache:     map[string]*readinessv1alpha1.NodeReadinessRule{rule.Name: rule},
+		EventRecorder: fakeRecorder,
+	}
+
+	r := &RuleReconciler{
+		Client:     fc,
+		Scheme:     scheme,
+		Controller: c,
+	}
+
+	if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: rule.Name}}); err == nil {
+		t.Fatalf("expected error, got nil")
+	}
+
+	var eventsCaptured []string
+	for len(fakeRecorder.Events) > 0 {
+		eventsCaptured = append(eventsCaptured, <-fakeRecorder.Events)
+	}
+
+	hasListError := false
+	for _, evt := range eventsCaptured {
+		if strings.Contains(evt, "ListNodesError") && strings.Contains(evt, "Warning") {
+			hasListError = true
+		}
+	}
+	if !hasListError {
+		t.Fatalf("expected Warning ListNodesError event, got: %v", eventsCaptured)
+	}
+}
+
+func TestReconcile_StatusPatchError(t *testing.T) {
+	ctx := context.Background()
+	scheme := runtime.NewScheme()
+	_ = corev1.AddToScheme(scheme)
+	_ = readinessv1alpha1.AddToScheme(scheme)
+
+	rule := &readinessv1alpha1.NodeReadinessRule{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:       "test-warning-rule",
+			Finalizers: []string{finalizerName},
+		},
+		Spec: readinessv1alpha1.NodeReadinessRuleSpec{
+			Conditions: []readinessv1alpha1.ConditionRequirement{
+				{Type: "Ready", RequiredStatus: corev1.ConditionTrue},
+			},
+			Taint:        corev1.Taint{Key: "readiness.k8s.io/warning-test", Effect: corev1.TaintEffectNoSchedule},
+			NodeSelector: metav1.LabelSelector{MatchLabels: map[string]string{"role": "worker"}},
+		},
+	}
+
+	baseClient := fakeclient.NewClientBuilder().WithScheme(scheme).WithObjects(rule).WithStatusSubresource(rule).Build()
+	fc := &failingClient{
+		Client:           baseClient,
+		statusPatchError: fmt.Errorf("fake status patch error"),
+	}
+	fakeRecorder := events.NewFakeRecorder(100)
+
+	c := &RuleReadinessController{
+		Client:        fc,
+		Scheme:        scheme,
+		clientset:     fake.NewSimpleClientset(),
+		ruleCache:     map[string]*readinessv1alpha1.NodeReadinessRule{rule.Name: rule},
+		EventRecorder: fakeRecorder,
+	}
+
+	r := &RuleReconciler{
+		Client:     fc,
+		Scheme:     scheme,
+		Controller: c,
+	}
+
+	if _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: types.NamespacedName{Name: rule.Name}}); err == nil {
+		t.Fatalf("expected error, got nil")
+	}
+
+	var eventsCaptured []string
+	for len(fakeRecorder.Events) > 0 {
+		eventsCaptured = append(eventsCaptured, <-fakeRecorder.Events)
+	}
+
+	hasStatusError := false
+	for _, evt := range eventsCaptured {
+		if strings.Contains(evt, "StatusUpdateError") && strings.Contains(evt, "Warning") {
+			hasStatusError = true
+		}
+	}
+	if !hasStatusError {
+		t.Fatalf("expected Warning StatusUpdateError event, got: %v", eventsCaptured)
+	}
+}