diff --git a/internal/controller/nodereadinessrule_controller.go b/internal/controller/nodereadinessrule_controller.go index 46127232..207df590 100644 --- a/internal/controller/nodereadinessrule_controller.go +++ b/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,6 +154,7 @@ 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 } } @@ -158,12 +162,14 @@ func (r *RuleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. // 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) } } } diff --git a/internal/controller/nodereadinessrule_controller_test.go b/internal/controller/nodereadinessrule_controller_test.go index b551ebb3..416c54a7 100644 --- a/internal/controller/nodereadinessrule_controller_test.go +++ b/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")) + }) }) }) diff --git a/internal/controller/warning_events_test.go b/internal/controller/warning_events_test.go new file mode 100644 index 00000000..337d887d --- /dev/null +++ b/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) + } +}