From 1c3d82c922f221197ccd5ae0028fc9ceb9745425 Mon Sep 17 00:00:00 2001 From: Luca Consalvi Date: Thu, 16 Jul 2026 17:26:14 +0200 Subject: [PATCH 1/5] OCPEDGE-2094: Add console notification for pacemaker podman-etcd failures Surface pacemaker health problems directly in the OpenShift web console via a ConsoleNotification banner. When etcd resources fail, fencing becomes unavailable, or pacemaker status goes stale, a red banner appears linking to the troubleshooting guide. The banner is removed automatically when health is restored. New controller shares the existing PacemakerCluster informer with the healthcheck and metrics controllers, following the same registration pattern in runPacemakerControllers(). Co-Authored-By: Claude Opus 4.6 --- pkg/tnf/operator/starter.go | 14 +- pkg/tnf/pkg/pacemaker/consolenotification.go | 210 ++++++++++++++++++ .../pkg/pacemaker/consolenotification_test.go | 196 ++++++++++++++++ 3 files changed, 418 insertions(+), 2 deletions(-) create mode 100644 pkg/tnf/pkg/pacemaker/consolenotification.go create mode 100644 pkg/tnf/pkg/pacemaker/consolenotification_test.go diff --git a/pkg/tnf/operator/starter.go b/pkg/tnf/operator/starter.go index 2d41174a1..eeb52f30f 100644 --- a/pkg/tnf/operator/starter.go +++ b/pkg/tnf/operator/starter.go @@ -80,7 +80,7 @@ func HandleDualReplicaClusters( if err := runTnfResourceController(ctx, controllerContext, kubeClient, dynamicClient, operatorClient, kubeInformersForNamespaces); err != nil { return false, err } - runPacemakerControllers(ctx, controllerContext, operatorClient, kubeClient, etcdInformer) + runPacemakerControllers(ctx, controllerContext, operatorClient, kubeClient, dynamicClient, etcdInformer) // we need node names for assigning auth and after-setup jobs to specific nodes controlPlaneNodeLister := corev1listers.NewNodeLister(controlPlaneNodeInformer.GetIndexer()) @@ -223,7 +223,7 @@ func runTnfResourceController(ctx context.Context, controllerContext *controller return nil } -func runPacemakerControllers(ctx context.Context, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, etcdInformer operatorv1informers.EtcdInformer) { +func runPacemakerControllers(ctx context.Context, controllerContext *controllercmd.ControllerContext, operatorClient v1helpers.StaticPodOperatorClient, kubeClient kubernetes.Interface, dynamicClient dynamic.Interface, etcdInformer operatorv1informers.EtcdInformer) { // Wait for external etcd transition before creating any Pacemaker controllers // This runs in a background goroutine to avoid blocking the main thread. go func() { @@ -327,6 +327,16 @@ func runPacemakerControllers(ctx context.Context, controllerContext *controllerc klog.Infof("starting Pacemaker metrics controller") go metricsController.Run(ctx, 1) + // Create and start the console notification controller, sharing the same informer + klog.Infof("creating Pacemaker console notification controller") + notificationController := pacemaker.NewConsoleNotificationController( + pacemakerInformer, + dynamicClient, + controllerContext.EventRecorder, + ) + klog.Infof("starting Pacemaker console notification controller") + go notificationController.Run(ctx, 1) + // Start the status collector CronJob klog.Infof("starting Pacemaker status collector CronJob") runPacemakerStatusCollectorCronJob(ctx, controllerContext, operatorClient, kubeClient) diff --git a/pkg/tnf/pkg/pacemaker/consolenotification.go b/pkg/tnf/pkg/pacemaker/consolenotification.go new file mode 100644 index 000000000..0fac49a83 --- /dev/null +++ b/pkg/tnf/pkg/pacemaker/consolenotification.go @@ -0,0 +1,210 @@ +package pacemaker + +import ( + "context" + "fmt" + "strings" + "time" + + consolev1 "github.com/openshift/api/console/v1" + pacmkrv1 "github.com/openshift/api/etcd/v1" + "github.com/openshift/library-go/pkg/controller/factory" + "github.com/openshift/library-go/pkg/operator/events" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" +) + +const ( + consoleNotificationName = "pacemaker-etcd-failure" + + // PatternFly danger palette for the banner. + notificationTextColor = "#fff" + notificationBackgroundColor = "#c9190b" + + troubleshootingURL = "https://docs.openshift.com/container-platform/latest/edge_computing/two-node-fencing/tnf-troubleshooting.html" + troubleshootingText = "Troubleshooting guide" +) + +var consoleNotificationGVR = schema.GroupVersionResource{ + Group: "console.openshift.io", + Version: "v1", + Resource: "consolenotifications", +} + +// consoleNotificationController manages a ConsoleNotification banner that surfaces +// pacemaker health problems directly in the OpenShift web console. It watches the +// PacemakerCluster CR (via a shared informer) and creates or removes the banner +// based on the health of etcd resources and fencing agents. +// +// Uses the dynamic client because the console client-go package is not vendored. +type consoleNotificationController struct { + dynamicClient dynamic.Interface + pacemakerInformer cache.SharedIndexInformer +} + +// NewConsoleNotificationController creates a controller that manages a ConsoleNotification +// banner for pacemaker podman-etcd failures. The controller shares the PacemakerCluster +// informer with the healthcheck and metrics controllers. +func NewConsoleNotificationController( + pacemakerInformer cache.SharedIndexInformer, + dynamicClient dynamic.Interface, + eventRecorder events.Recorder, +) factory.Controller { + c := &consoleNotificationController{ + dynamicClient: dynamicClient, + pacemakerInformer: pacemakerInformer, + } + + return factory.New(). + ResyncEvery(HealthCheckResyncInterval). + WithInformers(pacemakerInformer). + WithSync(c.sync). + ToController("ConsoleNotificationController", eventRecorder.WithComponentSuffix("console-notification")) +} + +func (c *consoleNotificationController) sync(ctx context.Context, _ factory.SyncContext) error { + klog.V(4).Infof("syncing console notification for pacemaker health") + + item, exists, err := c.pacemakerInformer.GetStore().GetByKey(PacemakerClusterResourceName) + if err != nil { + return err + } + + if !exists { + return c.deleteNotification(ctx) + } + + cr, ok := item.(*pacmkrv1.PacemakerCluster) + if !ok { + return fmt.Errorf("unexpected object type in informer store: %T", item) + } + + problems := evaluateHealth(cr) + if len(problems) == 0 { + return c.deleteNotification(ctx) + } + + return c.ensureNotification(ctx, problems) +} + +// evaluateHealth inspects the PacemakerCluster CR for conditions that warrant a +// console notification. Returns a list of human-readable problem descriptions, +// or nil when the cluster is healthy. +func evaluateHealth(cr *pacmkrv1.PacemakerCluster) []string { + var problems []string + + if !cr.Status.LastUpdated.IsZero() && time.Since(cr.Status.LastUpdated.Time) > StatusStalenessThreshold { + problems = append(problems, "Pacemaker status has not been updated recently — health monitoring may be offline") + } + + if getConditionStatus(cr.Status.Conditions, pacmkrv1.ClusterInServiceConditionType) == metav1.ConditionFalse { + problems = append(problems, "Pacemaker cluster is in maintenance mode") + } + + if cr.Status.Nodes == nil { + return problems + } + + for i := range *cr.Status.Nodes { + node := &(*cr.Status.Nodes)[i] + name := node.NodeName + + if getConditionStatus(node.Conditions, pacmkrv1.NodeOnlineConditionType) == metav1.ConditionFalse { + problems = append(problems, fmt.Sprintf("Node %s is offline", name)) + continue + } + + for j := range node.Resources { + res := &node.Resources[j] + if res.Name != pacmkrv1.PacemakerClusterResourceNameEtcd { + continue + } + if getConditionStatus(res.Conditions, pacmkrv1.ResourceHealthyConditionType) == metav1.ConditionFalse { + problems = append(problems, fmt.Sprintf("Etcd resource is unhealthy on node %s", name)) + } + } + + if getConditionStatus(node.Conditions, pacmkrv1.NodeFencingAvailableConditionType) == metav1.ConditionFalse { + problems = append(problems, fmt.Sprintf("Fencing is unavailable on node %s — automatic recovery is not possible", name)) + } + } + + return problems +} + +func (c *consoleNotificationController) ensureNotification(ctx context.Context, problems []string) error { + text := strings.Join(problems, "; ") + ". Check pacemaker status for details." + + notification := &consolev1.ConsoleNotification{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "console.openshift.io/v1", + Kind: "ConsoleNotification", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: consoleNotificationName, + Labels: map[string]string{ + "app": "cluster-etcd-operator", + "app.kubernetes.io/part-of": "cluster-etcd-operator", + "app.kubernetes.io/managed-by": "cluster-etcd-operator", + }, + }, + Spec: consolev1.ConsoleNotificationSpec{ + Text: text, + Location: consolev1.BannerTop, + Color: notificationTextColor, + BackgroundColor: notificationBackgroundColor, + Link: &consolev1.Link{ + Href: troubleshootingURL, + Text: troubleshootingText, + }, + }, + } + + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(notification) + if err != nil { + return fmt.Errorf("failed to convert ConsoleNotification to unstructured: %w", err) + } + u := &unstructured.Unstructured{Object: obj} + + existing, err := c.dynamicClient.Resource(consoleNotificationGVR).Get(ctx, consoleNotificationName, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + if _, err := c.dynamicClient.Resource(consoleNotificationGVR).Create(ctx, u, metav1.CreateOptions{}); err != nil { + return fmt.Errorf("failed to create ConsoleNotification: %w", err) + } + klog.Infof("Created console notification: %s", text) + return nil + } + if err != nil { + return fmt.Errorf("failed to get ConsoleNotification: %w", err) + } + + existingText, _, _ := unstructured.NestedString(existing.Object, "spec", "text") + if existingText == text { + klog.V(4).Infof("Console notification already up to date") + return nil + } + + u.SetResourceVersion(existing.GetResourceVersion()) + if _, err := c.dynamicClient.Resource(consoleNotificationGVR).Update(ctx, u, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("failed to update ConsoleNotification: %w", err) + } + klog.Infof("Updated console notification: %s", text) + return nil +} + +func (c *consoleNotificationController) deleteNotification(ctx context.Context) error { + err := c.dynamicClient.Resource(consoleNotificationGVR).Delete(ctx, consoleNotificationName, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to delete ConsoleNotification: %w", err) + } + if err == nil { + klog.Infof("Deleted console notification: pacemaker health restored") + } + return nil +} diff --git a/pkg/tnf/pkg/pacemaker/consolenotification_test.go b/pkg/tnf/pkg/pacemaker/consolenotification_test.go new file mode 100644 index 000000000..0afd3fee8 --- /dev/null +++ b/pkg/tnf/pkg/pacemaker/consolenotification_test.go @@ -0,0 +1,196 @@ +package pacemaker + +import ( + "strings" + "testing" + "time" + + pacmkrv1 "github.com/openshift/api/etcd/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// withNodeCondition returns a copy of conditions with the given type overridden to the given status. +func withNodeCondition(conditions []metav1.Condition, condType string, status metav1.ConditionStatus) []metav1.Condition { + out := make([]metav1.Condition, len(conditions)) + copy(out, conditions) + for i := range out { + if out[i].Type == condType { + out[i].Status = status + out[i].LastTransitionTime = metav1.Now() + } + } + return out +} + +// makeNodeWithConditions creates a node status with custom conditions and healthy resources. +func makeNodeWithConditions(name, ip string, conditions []metav1.Condition) pacmkrv1.PacemakerClusterNodeStatus { + return pacmkrv1.PacemakerClusterNodeStatus{ + Conditions: conditions, + NodeName: name, + Addresses: []pacmkrv1.PacemakerNodeAddress{{Type: pacmkrv1.PacemakerNodeInternalIP, Address: ip}}, + Resources: []pacmkrv1.PacemakerClusterResourceStatus{ + {Conditions: createHealthyResourceConditions(), Name: pacmkrv1.PacemakerClusterResourceNameKubelet}, + {Conditions: createHealthyResourceConditions(), Name: pacmkrv1.PacemakerClusterResourceNameEtcd}, + }, + } +} + +func TestEvaluateHealth(t *testing.T) { + tests := []struct { + name string + cr *pacmkrv1.PacemakerCluster + wantCount int + wantSubstrings []string + }{ + { + name: "healthy cluster returns no problems", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createHealthyClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + createHealthyNodeStatus("master-0", []string{"192.168.111.20"}), + createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), + }, + }, + }, + wantCount: 0, + }, + { + name: "stale status detected", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.NewTime(time.Now().Add(-10 * time.Minute)), + Conditions: createHealthyClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + createHealthyNodeStatus("master-0", []string{"192.168.111.20"}), + }, + }, + }, + wantCount: 1, + wantSubstrings: []string{"not been updated recently"}, + }, + { + name: "maintenance mode detected", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createMaintenanceModeClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + createHealthyNodeStatus("master-0", []string{"192.168.111.20"}), + }, + }, + }, + wantCount: 1, + wantSubstrings: []string{"maintenance mode"}, + }, + { + name: "offline node detected", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createHealthyClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + makeNodeWithConditions("master-0", "192.168.111.20", + withNodeCondition(createHealthyNodeConditions(), pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse)), + createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), + }, + }, + }, + wantCount: 1, + wantSubstrings: []string{"master-0 is offline"}, + }, + { + name: "offline node skips resource and fencing checks", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createHealthyClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + // Node is offline AND has unhealthy etcd AND no fencing — should only report offline. + makeNodeWithConditions("master-0", "192.168.111.20", + withNodeCondition( + withNodeCondition(createHealthyNodeConditions(), + pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse), + pacmkrv1.NodeFencingAvailableConditionType, metav1.ConditionFalse)), + }, + }, + }, + wantCount: 1, + wantSubstrings: []string{"master-0 is offline"}, + }, + { + name: "unhealthy etcd resource detected", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createHealthyClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + createUnhealthyNodeStatus("master-0", []string{"192.168.111.20"}, pacmkrv1.PacemakerClusterResourceNameEtcd), + createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), + }, + }, + }, + wantCount: 1, + wantSubstrings: []string{"Etcd resource is unhealthy on node master-0"}, + }, + { + name: "fencing unavailable detected", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createHealthyClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + makeNodeWithConditions("master-0", "192.168.111.20", + withNodeCondition(createHealthyNodeConditions(), pacmkrv1.NodeFencingAvailableConditionType, metav1.ConditionFalse)), + createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), + }, + }, + }, + wantCount: 1, + wantSubstrings: []string{"Fencing is unavailable on node master-0"}, + }, + { + name: "multiple problems aggregated", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createMaintenanceModeClusterConditions(), + Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ + makeNodeWithConditions("master-0", "192.168.111.20", + withNodeCondition(createHealthyNodeConditions(), pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse)), + createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), + }, + }, + }, + wantCount: 2, + wantSubstrings: []string{"maintenance mode", "master-0 is offline"}, + }, + { + name: "nil nodes returns no node-level problems", + cr: &pacmkrv1.PacemakerCluster{ + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createHealthyClusterConditions(), + Nodes: nil, + }, + }, + wantCount: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + problems := evaluateHealth(tt.cr) + if len(problems) != tt.wantCount { + t.Fatalf("expected %d problems, got %d: %v", tt.wantCount, len(problems), problems) + } + joined := strings.Join(problems, " | ") + for _, sub := range tt.wantSubstrings { + if !strings.Contains(joined, sub) { + t.Errorf("expected problems to contain %q, got: %v", sub, problems) + } + } + }) + } +} From d59084d7c8553b19e90c128b69886906e8faf0d5 Mon Sep 17 00:00:00 2001 From: Luca Consalvi Date: Fri, 17 Jul 2026 13:50:41 +0200 Subject: [PATCH 2/5] Use periods instead of semicolons in console notification banner Improves readability of multi-problem notifications by joining problems with ". " instead of "; ". Co-Authored-By: Claude Opus 4.6 --- pkg/tnf/pkg/pacemaker/consolenotification.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/tnf/pkg/pacemaker/consolenotification.go b/pkg/tnf/pkg/pacemaker/consolenotification.go index 0fac49a83..5dc2fe801 100644 --- a/pkg/tnf/pkg/pacemaker/consolenotification.go +++ b/pkg/tnf/pkg/pacemaker/consolenotification.go @@ -139,7 +139,7 @@ func evaluateHealth(cr *pacmkrv1.PacemakerCluster) []string { } func (c *consoleNotificationController) ensureNotification(ctx context.Context, problems []string) error { - text := strings.Join(problems, "; ") + ". Check pacemaker status for details." + text := strings.Join(problems, ". ") + ". Check pacemaker status for details." notification := &consolev1.ConsoleNotification{ TypeMeta: metav1.TypeMeta{ From 77ae31dd88accf6d194b3cafd70766bf9912c2e0 Mon Sep 17 00:00:00 2001 From: Luca Consalvi Date: Fri, 17 Jul 2026 13:56:30 +0200 Subject: [PATCH 3/5] Fix test to actually set unhealthy etcd on offline node The "offline node skips resource and fencing checks" test claimed etcd was unhealthy but makeNodeWithConditions creates healthy resources. Add withUnhealthyResource helper and use it so the test genuinely verifies that offline nodes skip resource checks. Co-Authored-By: Claude Opus 4.6 --- .../pkg/pacemaker/consolenotification_test.go | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/pkg/tnf/pkg/pacemaker/consolenotification_test.go b/pkg/tnf/pkg/pacemaker/consolenotification_test.go index 0afd3fee8..91472cfe0 100644 --- a/pkg/tnf/pkg/pacemaker/consolenotification_test.go +++ b/pkg/tnf/pkg/pacemaker/consolenotification_test.go @@ -22,6 +22,20 @@ func withNodeCondition(conditions []metav1.Condition, condType string, status me return out } +// withUnhealthyResource returns a copy of node with the given resource's Healthy condition set to False. +func withUnhealthyResource(node pacmkrv1.PacemakerClusterNodeStatus, resourceName pacmkrv1.PacemakerClusterResourceName) pacmkrv1.PacemakerClusterNodeStatus { + for i := range node.Resources { + if node.Resources[i].Name == resourceName { + for j := range node.Resources[i].Conditions { + if node.Resources[i].Conditions[j].Type == pacmkrv1.ResourceHealthyConditionType { + node.Resources[i].Conditions[j].Status = metav1.ConditionFalse + } + } + } + } + return node +} + // makeNodeWithConditions creates a node status with custom conditions and healthy resources. func makeNodeWithConditions(name, ip string, conditions []metav1.Condition) pacmkrv1.PacemakerClusterNodeStatus { return pacmkrv1.PacemakerClusterNodeStatus{ @@ -108,11 +122,13 @@ func TestEvaluateHealth(t *testing.T) { Conditions: createHealthyClusterConditions(), Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ // Node is offline AND has unhealthy etcd AND no fencing — should only report offline. - makeNodeWithConditions("master-0", "192.168.111.20", - withNodeCondition( - withNodeCondition(createHealthyNodeConditions(), - pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse), - pacmkrv1.NodeFencingAvailableConditionType, metav1.ConditionFalse)), + withUnhealthyResource( + makeNodeWithConditions("master-0", "192.168.111.20", + withNodeCondition( + withNodeCondition(createHealthyNodeConditions(), + pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse), + pacmkrv1.NodeFencingAvailableConditionType, metav1.ConditionFalse)), + pacmkrv1.PacemakerClusterResourceNameEtcd), }, }, }, From 6b1fc2ca98167e140a6c422d518f129dab88d38e Mon Sep 17 00:00:00 2001 From: Luca Consalvi Date: Mon, 20 Jul 2026 17:06:39 +0200 Subject: [PATCH 4/5] Split console notifications per problem category with correct doc links Separate pacemaker health problems into two notification categories (degraded-operations vs troubleshooting) so each banner links to the relevant documentation section. Consolidate shared logic, add test helpers, and point doc URLs to the published Red Hat docs. Co-Authored-By: Claude Opus 4.6 --- pkg/tnf/pkg/pacemaker/consolenotification.go | 227 ++++---- .../pkg/pacemaker/consolenotification_test.go | 509 ++++++++++++------ pkg/tnf/pkg/pacemaker/healthcheck.go | 259 +++------ pkg/tnf/pkg/pacemaker/healthcheck_test.go | 130 ++--- 4 files changed, 624 insertions(+), 501 deletions(-) diff --git a/pkg/tnf/pkg/pacemaker/consolenotification.go b/pkg/tnf/pkg/pacemaker/consolenotification.go index 5dc2fe801..ace89589e 100644 --- a/pkg/tnf/pkg/pacemaker/consolenotification.go +++ b/pkg/tnf/pkg/pacemaker/consolenotification.go @@ -3,14 +3,15 @@ package pacemaker import ( "context" "fmt" + "slices" "strings" - "time" consolev1 "github.com/openshift/api/console/v1" pacmkrv1 "github.com/openshift/api/etcd/v1" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" - apierrors "k8s.io/apimachinery/pkg/api/errors" + "github.com/openshift/library-go/pkg/operator/resource/resourceapply" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -21,36 +22,73 @@ import ( ) const ( - consoleNotificationName = "pacemaker-etcd-failure" - - // PatternFly danger palette for the banner. notificationTextColor = "#fff" notificationBackgroundColor = "#c9190b" - troubleshootingURL = "https://docs.openshift.com/container-platform/latest/edge_computing/two-node-fencing/tnf-troubleshooting.html" - troubleshootingText = "Troubleshooting guide" + docsBasePath = "https://docs.redhat.com/en/documentation/openshift_container_platform/4.22/html/installing_a_two_node_openshift_cluster/two-node-with-fencing" +) + +type notificationCategory struct { + name string + linkHref string + linkText string +} + +var ( + categoryDegraded = notificationCategory{ + name: "pacemaker-cluster-degraded", + linkHref: docsBasePath + "#operating-a-degraded-tnf", + linkText: "Recovery guide", + } + + categoryTroubleshooting = notificationCategory{ + name: "pacemaker-troubleshooting", + linkHref: docsBasePath + "#installing-post-tnf", + linkText: "Troubleshooting guide", + } + + allCategories = []notificationCategory{categoryDegraded, categoryTroubleshooting} + + // degradedPatterns match problems routed to the degraded-operations notification. + degradedPatterns = []string{"offline", "unhealthy", "fencing", "maintenance", "insufficient", "excessive"} + + consoleNotificationGVR = schema.GroupVersionResource{ + Group: "console.openshift.io", + Version: "v1", + Resource: "consolenotifications", + } ) -var consoleNotificationGVR = schema.GroupVersionResource{ - Group: "console.openshift.io", - Version: "v1", - Resource: "consolenotifications", +// classifyProblems splits errors and warnings into the two notification categories. +// Problems not matching any degraded pattern are routed to troubleshooting. +func classifyProblems(status *HealthStatus) (degraded, troubleshooting []string) { + for _, msg := range slices.Concat(status.Errors, status.Warnings) { + if isDegradedProblem(msg) { + degraded = append(degraded, msg) + } else { + troubleshooting = append(troubleshooting, msg) + } + } + return +} + +func isDegradedProblem(msg string) bool { + lower := strings.ToLower(msg) + for _, p := range degradedPatterns { + if strings.Contains(lower, p) { + return true + } + } + return false } -// consoleNotificationController manages a ConsoleNotification banner that surfaces -// pacemaker health problems directly in the OpenShift web console. It watches the -// PacemakerCluster CR (via a shared informer) and creates or removes the banner -// based on the health of etcd resources and fencing agents. -// -// Uses the dynamic client because the console client-go package is not vendored. type consoleNotificationController struct { - dynamicClient dynamic.Interface - pacemakerInformer cache.SharedIndexInformer + dynamicClient dynamic.Interface + recorder events.Recorder + pacemakerInformer cache.SharedIndexInformer + consoleUnavailable bool } -// NewConsoleNotificationController creates a controller that manages a ConsoleNotification -// banner for pacemaker podman-etcd failures. The controller shares the PacemakerCluster -// informer with the healthcheck and metrics controllers. func NewConsoleNotificationController( pacemakerInformer cache.SharedIndexInformer, dynamicClient dynamic.Interface, @@ -58,6 +96,7 @@ func NewConsoleNotificationController( ) factory.Controller { c := &consoleNotificationController{ dynamicClient: dynamicClient, + recorder: eventRecorder, pacemakerInformer: pacemakerInformer, } @@ -69,15 +108,16 @@ func NewConsoleNotificationController( } func (c *consoleNotificationController) sync(ctx context.Context, _ factory.SyncContext) error { - klog.V(4).Infof("syncing console notification for pacemaker health") + if c.consoleUnavailable { + return nil + } item, exists, err := c.pacemakerInformer.GetStore().GetByKey(PacemakerClusterResourceName) if err != nil { return err } - if !exists { - return c.deleteNotification(ctx) + return c.deleteAllNotifications(ctx) } cr, ok := item.(*pacmkrv1.PacemakerCluster) @@ -85,69 +125,78 @@ func (c *consoleNotificationController) sync(ctx context.Context, _ factory.Sync return fmt.Errorf("unexpected object type in informer store: %T", item) } - problems := evaluateHealth(cr) - if len(problems) == 0 { - return c.deleteNotification(ctx) - } + status := BuildHealthStatusFromCR(cr) + degradedProblems, troubleshootingProblems := classifyProblems(status) - return c.ensureNotification(ctx, problems) + if err := c.manageNotification(ctx, categoryDegraded, degradedProblems); err != nil { + return err + } + return c.manageNotification(ctx, categoryTroubleshooting, troubleshootingProblems) } -// evaluateHealth inspects the PacemakerCluster CR for conditions that warrant a -// console notification. Returns a list of human-readable problem descriptions, -// or nil when the cluster is healthy. -func evaluateHealth(cr *pacmkrv1.PacemakerCluster) []string { - var problems []string - - if !cr.Status.LastUpdated.IsZero() && time.Since(cr.Status.LastUpdated.Time) > StatusStalenessThreshold { - problems = append(problems, "Pacemaker status has not been updated recently — health monitoring may be offline") +func (c *consoleNotificationController) manageNotification(ctx context.Context, cat notificationCategory, problems []string) error { + if len(problems) == 0 { + return c.deleteNotification(ctx, cat) } + return c.ensureNotification(ctx, cat, problems) +} - if getConditionStatus(cr.Status.Conditions, pacmkrv1.ClusterInServiceConditionType) == metav1.ConditionFalse { - problems = append(problems, "Pacemaker cluster is in maintenance mode") - } +func (c *consoleNotificationController) ensureNotification(ctx context.Context, cat notificationCategory, problems []string) error { + text := strings.Join(problems, ". ") + ". Check pacemaker status for details." - if cr.Status.Nodes == nil { - return problems + u, err := buildNotificationUnstructured(cat, text) + if err != nil { + return err } - for i := range *cr.Status.Nodes { - node := &(*cr.Status.Nodes)[i] - name := node.NodeName - - if getConditionStatus(node.Conditions, pacmkrv1.NodeOnlineConditionType) == metav1.ConditionFalse { - problems = append(problems, fmt.Sprintf("Node %s is offline", name)) - continue - } + _, _, err = resourceapply.ApplyUnstructuredResourceImproved( + ctx, c.dynamicClient, c.recorder, u, nil, consoleNotificationGVR, nil, nil) + return c.filterConsoleError(err) +} - for j := range node.Resources { - res := &node.Resources[j] - if res.Name != pacmkrv1.PacemakerClusterResourceNameEtcd { - continue - } - if getConditionStatus(res.Conditions, pacmkrv1.ResourceHealthyConditionType) == metav1.ConditionFalse { - problems = append(problems, fmt.Sprintf("Etcd resource is unhealthy on node %s", name)) - } - } +func (c *consoleNotificationController) deleteNotification(ctx context.Context, cat notificationCategory) error { + u := &unstructured.Unstructured{} + u.SetName(cat.name) + u.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "console.openshift.io", + Version: "v1", + Kind: "ConsoleNotification", + }) + + _, _, err := resourceapply.DeleteUnstructuredResource( + ctx, c.dynamicClient, c.recorder, u, consoleNotificationGVR) + return c.filterConsoleError(err) +} - if getConditionStatus(node.Conditions, pacmkrv1.NodeFencingAvailableConditionType) == metav1.ConditionFalse { - problems = append(problems, fmt.Sprintf("Fencing is unavailable on node %s — automatic recovery is not possible", name)) +func (c *consoleNotificationController) deleteAllNotifications(ctx context.Context) error { + for _, cat := range allCategories { + if err := c.deleteNotification(ctx, cat); err != nil { + return err } } - - return problems + return nil } -func (c *consoleNotificationController) ensureNotification(ctx context.Context, problems []string) error { - text := strings.Join(problems, ". ") + ". Check pacemaker status for details." +func (c *consoleNotificationController) filterConsoleError(err error) error { + if err == nil { + return nil + } + if meta.IsNoMatchError(err) { + klog.Infof("Console API not available on this cluster, disabling notification management") + c.consoleUnavailable = true + return nil + } + return err +} +func buildNotificationUnstructured(cat notificationCategory, text string) (*unstructured.Unstructured, error) { notification := &consolev1.ConsoleNotification{ TypeMeta: metav1.TypeMeta{ APIVersion: "console.openshift.io/v1", Kind: "ConsoleNotification", }, ObjectMeta: metav1.ObjectMeta{ - Name: consoleNotificationName, + Name: cat.name, Labels: map[string]string{ "app": "cluster-etcd-operator", "app.kubernetes.io/part-of": "cluster-etcd-operator", @@ -160,51 +209,15 @@ func (c *consoleNotificationController) ensureNotification(ctx context.Context, Color: notificationTextColor, BackgroundColor: notificationBackgroundColor, Link: &consolev1.Link{ - Href: troubleshootingURL, - Text: troubleshootingText, + Href: cat.linkHref, + Text: cat.linkText, }, }, } obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(notification) if err != nil { - return fmt.Errorf("failed to convert ConsoleNotification to unstructured: %w", err) - } - u := &unstructured.Unstructured{Object: obj} - - existing, err := c.dynamicClient.Resource(consoleNotificationGVR).Get(ctx, consoleNotificationName, metav1.GetOptions{}) - if apierrors.IsNotFound(err) { - if _, err := c.dynamicClient.Resource(consoleNotificationGVR).Create(ctx, u, metav1.CreateOptions{}); err != nil { - return fmt.Errorf("failed to create ConsoleNotification: %w", err) - } - klog.Infof("Created console notification: %s", text) - return nil - } - if err != nil { - return fmt.Errorf("failed to get ConsoleNotification: %w", err) - } - - existingText, _, _ := unstructured.NestedString(existing.Object, "spec", "text") - if existingText == text { - klog.V(4).Infof("Console notification already up to date") - return nil - } - - u.SetResourceVersion(existing.GetResourceVersion()) - if _, err := c.dynamicClient.Resource(consoleNotificationGVR).Update(ctx, u, metav1.UpdateOptions{}); err != nil { - return fmt.Errorf("failed to update ConsoleNotification: %w", err) - } - klog.Infof("Updated console notification: %s", text) - return nil -} - -func (c *consoleNotificationController) deleteNotification(ctx context.Context) error { - err := c.dynamicClient.Resource(consoleNotificationGVR).Delete(ctx, consoleNotificationName, metav1.DeleteOptions{}) - if err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to delete ConsoleNotification: %w", err) - } - if err == nil { - klog.Infof("Deleted console notification: pacemaker health restored") + return nil, fmt.Errorf("failed to convert ConsoleNotification to unstructured: %w", err) } - return nil + return &unstructured.Unstructured{Object: obj}, nil } diff --git a/pkg/tnf/pkg/pacemaker/consolenotification_test.go b/pkg/tnf/pkg/pacemaker/consolenotification_test.go index 91472cfe0..9867af23f 100644 --- a/pkg/tnf/pkg/pacemaker/consolenotification_test.go +++ b/pkg/tnf/pkg/pacemaker/consolenotification_test.go @@ -1,212 +1,417 @@ package pacemaker import ( + "context" + "slices" "strings" "testing" "time" pacmkrv1 "github.com/openshift/api/etcd/v1" + "github.com/openshift/cluster-etcd-operator/pkg/tnf/internal/testutil" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + fakedynamic "k8s.io/client-go/dynamic/fake" + clocktesting "k8s.io/utils/clock/testing" ) -// withNodeCondition returns a copy of conditions with the given type overridden to the given status. -func withNodeCondition(conditions []metav1.Condition, condType string, status metav1.ConditionStatus) []metav1.Condition { - out := make([]metav1.Condition, len(conditions)) - copy(out, conditions) - for i := range out { - if out[i].Type == condType { - out[i].Status = status - out[i].LastTransitionTime = metav1.Now() - } +// --------------------------------------------------------------------------- +// Functional-option builders for test data +// --------------------------------------------------------------------------- + +type nodeOpt func(*pacmkrv1.PacemakerClusterNodeStatus) +type clusterOpt func(*pacmkrv1.PacemakerCluster) + +func offline() nodeOpt { + return setNodeCondition(pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse) +} + +func fencingUnavailable() nodeOpt { + return func(n *pacmkrv1.PacemakerClusterNodeStatus) { + setNodeCondition(pacmkrv1.NodeHealthyConditionType, metav1.ConditionFalse)(n) + setNodeCondition(pacmkrv1.NodeFencingAvailableConditionType, metav1.ConditionFalse)(n) } - return out } -// withUnhealthyResource returns a copy of node with the given resource's Healthy condition set to False. -func withUnhealthyResource(node pacmkrv1.PacemakerClusterNodeStatus, resourceName pacmkrv1.PacemakerClusterResourceName) pacmkrv1.PacemakerClusterNodeStatus { - for i := range node.Resources { - if node.Resources[i].Name == resourceName { - for j := range node.Resources[i].Conditions { - if node.Resources[i].Conditions[j].Type == pacmkrv1.ResourceHealthyConditionType { - node.Resources[i].Conditions[j].Status = metav1.ConditionFalse +func fencingDegraded() nodeOpt { + return setNodeCondition(pacmkrv1.NodeFencingHealthyConditionType, metav1.ConditionFalse) +} + +func unhealthyEtcd() nodeOpt { + return func(n *pacmkrv1.PacemakerClusterNodeStatus) { + setNodeCondition(pacmkrv1.NodeHealthyConditionType, metav1.ConditionFalse)(n) + for i := range n.Resources { + if n.Resources[i].Name == pacmkrv1.PacemakerClusterResourceNameEtcd { + for j := range n.Resources[i].Conditions { + if n.Resources[i].Conditions[j].Type == pacmkrv1.ResourceHealthyConditionType { + n.Resources[i].Conditions[j].Status = metav1.ConditionFalse + } } + break } } } +} + +func setNodeCondition(condType string, status metav1.ConditionStatus) nodeOpt { + return func(n *pacmkrv1.PacemakerClusterNodeStatus) { + for i := range n.Conditions { + if n.Conditions[i].Type == condType { + n.Conditions[i].Status = status + } + } + } +} + +func staleBy(d time.Duration) clusterOpt { + return func(c *pacmkrv1.PacemakerCluster) { + c.Status.LastUpdated = metav1.NewTime(time.Now().Add(-d)) + } +} + +func inMaintenanceMode() clusterOpt { + return func(c *pacmkrv1.PacemakerCluster) { + c.Status.Conditions = createMaintenanceModeClusterConditions() + } +} + +func withNodes(nodes ...pacmkrv1.PacemakerClusterNodeStatus) clusterOpt { + return func(c *pacmkrv1.PacemakerCluster) { + ns := make([]pacmkrv1.PacemakerClusterNodeStatus, len(nodes)) + copy(ns, nodes) + c.Status.Nodes = &ns + } +} + +func testNode(name, ip string, opts ...nodeOpt) pacmkrv1.PacemakerClusterNodeStatus { + node := createHealthyNodeStatus(name, []string{ip}) + for _, opt := range opts { + opt(&node) + } return node } -// makeNodeWithConditions creates a node status with custom conditions and healthy resources. -func makeNodeWithConditions(name, ip string, conditions []metav1.Condition) pacmkrv1.PacemakerClusterNodeStatus { - return pacmkrv1.PacemakerClusterNodeStatus{ - Conditions: conditions, - NodeName: name, - Addresses: []pacmkrv1.PacemakerNodeAddress{{Type: pacmkrv1.PacemakerNodeInternalIP, Address: ip}}, - Resources: []pacmkrv1.PacemakerClusterResourceStatus{ - {Conditions: createHealthyResourceConditions(), Name: pacmkrv1.PacemakerClusterResourceNameKubelet}, - {Conditions: createHealthyResourceConditions(), Name: pacmkrv1.PacemakerClusterResourceNameEtcd}, +func testCluster(opts ...clusterOpt) *pacmkrv1.PacemakerCluster { + cr := &pacmkrv1.PacemakerCluster{ + ObjectMeta: metav1.ObjectMeta{Name: PacemakerClusterResourceName}, + Status: pacmkrv1.PacemakerClusterStatus{ + LastUpdated: metav1.Now(), + Conditions: createHealthyClusterConditions(), }, } + for _, opt := range opts { + opt(cr) + } + return cr } -func TestEvaluateHealth(t *testing.T) { +// --------------------------------------------------------------------------- +// BuildHealthStatusFromCR tests (validates shared health evaluation) +// --------------------------------------------------------------------------- + +func TestBuildHealthStatusFromCR_ConsoleScenarios(t *testing.T) { tests := []struct { name string cr *pacmkrv1.PacemakerCluster - wantCount int + wantProblems int wantSubstrings []string }{ { - name: "healthy cluster returns no problems", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createHealthyClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - createHealthyNodeStatus("master-0", []string{"192.168.111.20"}), - createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), - }, - }, - }, - wantCount: 0, - }, - { - name: "stale status detected", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.NewTime(time.Now().Add(-10 * time.Minute)), - Conditions: createHealthyClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - createHealthyNodeStatus("master-0", []string{"192.168.111.20"}), - }, - }, - }, - wantCount: 1, - wantSubstrings: []string{"not been updated recently"}, - }, - { - name: "maintenance mode detected", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createMaintenanceModeClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - createHealthyNodeStatus("master-0", []string{"192.168.111.20"}), - }, - }, - }, - wantCount: 1, + name: "healthy cluster returns no problems", + cr: testCluster(withNodes(testNode("master-0", "192.168.111.20"), testNode("master-1", "192.168.111.21"))), + wantProblems: 0, + }, + { + name: "stale status detected", + cr: testCluster(staleBy(10*time.Minute), withNodes(testNode("master-0", "192.168.111.20"))), + wantProblems: 1, + wantSubstrings: []string{"stale"}, + }, + { + name: "maintenance mode detected", + cr: testCluster(inMaintenanceMode(), withNodes(testNode("master-0", "192.168.111.20"))), + wantProblems: 1, wantSubstrings: []string{"maintenance mode"}, }, { name: "offline node detected", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createHealthyClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - makeNodeWithConditions("master-0", "192.168.111.20", - withNodeCondition(createHealthyNodeConditions(), pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse)), - createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), - }, - }, - }, - wantCount: 1, - wantSubstrings: []string{"master-0 is offline"}, + cr: testCluster(withNodes( + testNode("master-0", "192.168.111.20", offline()), + testNode("master-1", "192.168.111.21"), + )), + wantProblems: 1, + wantSubstrings: []string{"offline"}, }, { name: "offline node skips resource and fencing checks", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createHealthyClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - // Node is offline AND has unhealthy etcd AND no fencing — should only report offline. - withUnhealthyResource( - makeNodeWithConditions("master-0", "192.168.111.20", - withNodeCondition( - withNodeCondition(createHealthyNodeConditions(), - pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse), - pacmkrv1.NodeFencingAvailableConditionType, metav1.ConditionFalse)), - pacmkrv1.PacemakerClusterResourceNameEtcd), - }, - }, - }, - wantCount: 1, - wantSubstrings: []string{"master-0 is offline"}, + cr: testCluster(withNodes( + testNode("master-0", "192.168.111.20", offline(), fencingUnavailable(), unhealthyEtcd()), + )), + wantProblems: 1, + wantSubstrings: []string{"offline"}, }, { name: "unhealthy etcd resource detected", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createHealthyClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - createUnhealthyNodeStatus("master-0", []string{"192.168.111.20"}, pacmkrv1.PacemakerClusterResourceNameEtcd), - createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), - }, - }, - }, - wantCount: 1, - wantSubstrings: []string{"Etcd resource is unhealthy on node master-0"}, + cr: testCluster(withNodes( + testNode("master-0", "192.168.111.20", unhealthyEtcd()), + testNode("master-1", "192.168.111.21"), + )), + wantProblems: 1, + wantSubstrings: []string{"Etcd"}, }, { name: "fencing unavailable detected", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createHealthyClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - makeNodeWithConditions("master-0", "192.168.111.20", - withNodeCondition(createHealthyNodeConditions(), pacmkrv1.NodeFencingAvailableConditionType, metav1.ConditionFalse)), - createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), - }, - }, - }, - wantCount: 1, - wantSubstrings: []string{"Fencing is unavailable on node master-0"}, + cr: testCluster(withNodes( + testNode("master-0", "192.168.111.20", fencingUnavailable()), + testNode("master-1", "192.168.111.21"), + )), + wantProblems: 1, + wantSubstrings: []string{"fencing unavailable"}, + }, + { + name: "fencing degraded detected as warning", + cr: testCluster(withNodes( + testNode("master-0", "192.168.111.20", fencingDegraded()), + testNode("master-1", "192.168.111.21"), + )), + wantProblems: 1, + wantSubstrings: []string{"fencing at risk"}, }, { name: "multiple problems aggregated", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createMaintenanceModeClusterConditions(), - Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{ - makeNodeWithConditions("master-0", "192.168.111.20", - withNodeCondition(createHealthyNodeConditions(), pacmkrv1.NodeOnlineConditionType, metav1.ConditionFalse)), - createHealthyNodeStatus("master-1", []string{"192.168.111.21"}), - }, - }, - }, - wantCount: 2, - wantSubstrings: []string{"maintenance mode", "master-0 is offline"}, + cr: testCluster(inMaintenanceMode(), withNodes( + testNode("master-0", "192.168.111.20", offline()), + testNode("master-1", "192.168.111.21"), + )), + wantProblems: 2, + wantSubstrings: []string{"maintenance mode", "offline"}, }, { - name: "nil nodes returns no node-level problems", - cr: &pacmkrv1.PacemakerCluster{ - Status: pacmkrv1.PacemakerClusterStatus{ - LastUpdated: metav1.Now(), - Conditions: createHealthyClusterConditions(), - Nodes: nil, - }, - }, - wantCount: 0, + name: "nil nodes reports error", + cr: testCluster(), + wantProblems: 1, + wantSubstrings: []string{"No nodes found"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - problems := evaluateHealth(tt.cr) - if len(problems) != tt.wantCount { - t.Fatalf("expected %d problems, got %d: %v", tt.wantCount, len(problems), problems) - } + status := BuildHealthStatusFromCR(tt.cr) + problems := slices.Concat(status.Errors, status.Warnings) + require.Equal(t, tt.wantProblems, len(problems), "problems: %v", problems) joined := strings.Join(problems, " | ") for _, sub := range tt.wantSubstrings { - if !strings.Contains(joined, sub) { - t.Errorf("expected problems to contain %q, got: %v", sub, problems) - } + require.Contains(t, joined, sub) } }) } } + +// --------------------------------------------------------------------------- +// Problem classification tests +// --------------------------------------------------------------------------- + +func TestClassifyProblems(t *testing.T) { + tests := []struct { + name string + status *HealthStatus + wantDegraded int + wantTroubleshooting int + }{ + { + name: "node offline routes to degraded", + status: &HealthStatus{Errors: []string{"Node master-0 is offline"}}, + wantDegraded: 1, + wantTroubleshooting: 0, + }, + { + name: "node unhealthy routes to degraded", + status: &HealthStatus{Errors: []string{"Node master-0 node is unhealthy: Etcd resource is unhealthy"}}, + wantDegraded: 1, + wantTroubleshooting: 0, + }, + { + name: "fencing warning routes to degraded", + status: &HealthStatus{Warnings: []string{"master-0: fencing at risk (agent running but not managed for recovery)"}}, + wantDegraded: 1, + wantTroubleshooting: 0, + }, + { + name: "maintenance mode routes to degraded", + status: &HealthStatus{Errors: []string{"Cluster is in maintenance mode"}}, + wantDegraded: 1, + wantTroubleshooting: 0, + }, + { + name: "stale status routes to troubleshooting", + status: &HealthStatus{Errors: []string{"Pacemaker status is stale (last updated: 2024-01-01T00:00:00Z)"}}, + wantDegraded: 0, + wantTroubleshooting: 1, + }, + { + name: "no status routes to troubleshooting", + status: &HealthStatus{Errors: []string{"PacemakerCluster CR has no status populated"}}, + wantDegraded: 0, + wantTroubleshooting: 1, + }, + { + name: "mixed problems split correctly", + status: &HealthStatus{ + Errors: []string{"Node master-0 is offline", "Pacemaker status is stale (last updated: 2024-01-01T00:00:00Z)"}, + Warnings: []string{"master-1: fencing at risk (agent running but not managed for recovery)"}, + }, + wantDegraded: 2, + wantTroubleshooting: 1, + }, + { + name: "no problems produces empty lists", + status: &HealthStatus{}, + wantDegraded: 0, + wantTroubleshooting: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + degraded, troubleshooting := classifyProblems(tt.status) + require.Len(t, degraded, tt.wantDegraded, "degraded: %v", degraded) + require.Len(t, troubleshooting, tt.wantTroubleshooting, "troubleshooting: %v", troubleshooting) + }) + } +} + +// --------------------------------------------------------------------------- +// Sync-level tests +// --------------------------------------------------------------------------- + +func newTestController(cr *pacmkrv1.PacemakerCluster) (*consoleNotificationController, *fakedynamic.FakeDynamicClient) { + scheme := runtime.NewScheme() + dynClient := fakedynamic.NewSimpleDynamicClient(scheme) + + return &consoleNotificationController{ + dynamicClient: dynClient, + recorder: events.NewInMemoryRecorder("test", clocktesting.NewFakeClock(time.Now())), + pacemakerInformer: testutil.CreateFakeInformer(cr), + }, dynClient +} + +func getCreatedName(t *testing.T, dynClient *fakedynamic.FakeDynamicClient) string { + t.Helper() + for _, a := range dynClient.Actions() { + if a.GetVerb() == "create" { + obj := a.(interface{ GetObject() runtime.Object }).GetObject() + return obj.(*unstructured.Unstructured).GetName() + } + } + t.Fatal("no create action found") + return "" +} + +func countActions(dynClient *fakedynamic.FakeDynamicClient, verb string) int { + count := 0 + for _, a := range dynClient.Actions() { + if a.GetVerb() == verb { + count++ + } + } + return count +} + +func TestSync_HealthyCluster_DeletesBothNotifications(t *testing.T) { + cr := testCluster(withNodes(testNode("master-0", "192.168.111.20"), testNode("master-1", "192.168.111.21"))) + ctrl, dynClient := newTestController(cr) + + err := ctrl.sync(context.Background(), nil) + require.NoError(t, err) + require.Equal(t, 2, countActions(dynClient, "delete"), "expected delete actions for both notification categories") +} + +func TestSync_NodeOffline_CreatesDegradedNotification(t *testing.T) { + cr := testCluster(withNodes( + testNode("master-0", "192.168.111.20", offline()), + testNode("master-1", "192.168.111.21"), + )) + ctrl, dynClient := newTestController(cr) + + err := ctrl.sync(context.Background(), nil) + require.NoError(t, err) + require.Equal(t, categoryDegraded.name, getCreatedName(t, dynClient)) +} + +func TestSync_StaleStatus_CreatesTroubleshootingNotification(t *testing.T) { + cr := testCluster(staleBy(10*time.Minute), withNodes(testNode("master-0", "192.168.111.20"))) + ctrl, dynClient := newTestController(cr) + + err := ctrl.sync(context.Background(), nil) + require.NoError(t, err) + require.Equal(t, categoryTroubleshooting.name, getCreatedName(t, dynClient)) +} + +func TestSync_FencingDegraded_CreatesDegradedNotification(t *testing.T) { + cr := testCluster(withNodes( + testNode("master-0", "192.168.111.20", fencingDegraded()), + testNode("master-1", "192.168.111.21"), + )) + ctrl, dynClient := newTestController(cr) + + err := ctrl.sync(context.Background(), nil) + require.NoError(t, err) + require.Equal(t, categoryDegraded.name, getCreatedName(t, dynClient)) +} + +func TestSync_CRNotFound_DeletesBothNotifications(t *testing.T) { + ctrl, dynClient := newTestController(nil) + + err := ctrl.sync(context.Background(), nil) + require.NoError(t, err) + require.Equal(t, 2, countActions(dynClient, "delete"), "expected delete actions for both categories") +} + +func TestSync_ConsoleUnavailable_SkipsSilently(t *testing.T) { + cr := testCluster(withNodes( + testNode("master-0", "192.168.111.20", unhealthyEtcd()), + testNode("master-1", "192.168.111.21"), + )) + ctrl, _ := newTestController(cr) + ctrl.consoleUnavailable = true + + err := ctrl.sync(context.Background(), nil) + require.NoError(t, err) +} + +// --------------------------------------------------------------------------- +// buildNotificationUnstructured tests +// --------------------------------------------------------------------------- + +func TestBuildNotificationUnstructured_DegradedCategory(t *testing.T) { + u, err := buildNotificationUnstructured(categoryDegraded, "Node master-0 is offline. Check pacemaker status for details.") + require.NoError(t, err) + require.Equal(t, categoryDegraded.name, u.GetName()) + + text, _, _ := unstructured.NestedString(u.Object, "spec", "text") + require.Contains(t, text, "offline") + + bg, _, _ := unstructured.NestedString(u.Object, "spec", "backgroundColor") + require.Equal(t, notificationBackgroundColor, bg) + + href, _, _ := unstructured.NestedString(u.Object, "spec", "link", "href") + require.Equal(t, categoryDegraded.linkHref, href) + + linkText, _, _ := unstructured.NestedString(u.Object, "spec", "link", "text") + require.Equal(t, categoryDegraded.linkText, linkText) +} + +func TestBuildNotificationUnstructured_TroubleshootingCategory(t *testing.T) { + u, err := buildNotificationUnstructured(categoryTroubleshooting, "Pacemaker status is stale. Check pacemaker status for details.") + require.NoError(t, err) + require.Equal(t, categoryTroubleshooting.name, u.GetName()) + + href, _, _ := unstructured.NestedString(u.Object, "spec", "link", "href") + require.Equal(t, categoryTroubleshooting.linkHref, href) + + linkText, _, _ := unstructured.NestedString(u.Object, "spec", "link", "text") + require.Equal(t, categoryTroubleshooting.linkText, linkText) +} diff --git a/pkg/tnf/pkg/pacemaker/healthcheck.go b/pkg/tnf/pkg/pacemaker/healthcheck.go index 684f9ee54..cd2bbff91 100644 --- a/pkg/tnf/pkg/pacemaker/healthcheck.go +++ b/pkg/tnf/pkg/pacemaker/healthcheck.go @@ -3,6 +3,7 @@ package pacemaker import ( "context" "fmt" + "slices" "strings" "sync" "time" @@ -28,17 +29,14 @@ type HealthStatusValue string // Local constants for healthcheck controller const ( // Status values for health assessment - statusHealthy HealthStatusValue = "Healthy" - statusWarning HealthStatusValue = "Warning" - statusError HealthStatusValue = "Error" - statusUnknown HealthStatusValue = "Unknown" + StatusHealthy HealthStatusValue = "Healthy" + StatusWarning HealthStatusValue = "Warning" + StatusError HealthStatusValue = "Error" + StatusUnknown HealthStatusValue = "Unknown" // Degraded condition reason reasonPacemakerUnhealthy = "PacemakerUnhealthy" - // Warning message prefixes for categorizing events. - // Note: warningPrefixFailedAction was removed - failed action events are recorded - // by the status collector directly from pacemaker XML, not the healthcheck controller. warningPrefixFencingEvent = "Recent fencing event:" // Operator condition types @@ -255,77 +253,36 @@ func (c *HealthCheck) sync(ctx context.Context, syncCtx factory.SyncContext) err func (c *HealthCheck) getPacemakerStatus(ctx context.Context) (*HealthStatus, *HealthStatus, error) { klog.V(4).Infof("Retrieving pacemaker status from CR...") - // Read previous status c.previousMu.Lock() previous := c.previous c.previousMu.Unlock() - // Get the PacemakerCluster CR from the informer cache item, exists, err := c.pacemakerInformer.GetStore().GetByKey(PacemakerClusterResourceName) if err != nil { - // Unknown status - don't update previous (preserves last valid for grace period) - return &HealthStatus{ - OverallStatus: statusUnknown, - Warnings: []string{}, - Errors: []string{fmt.Sprintf("Failed to get PacemakerCluster CR from cache: %v", err)}, - }, previous, nil + return unknownHealthStatus(fmt.Sprintf("Failed to get PacemakerCluster CR from cache: %v", err)), previous, nil } if !exists { - // Unknown status - CR not found in cache - return &HealthStatus{ - OverallStatus: statusUnknown, - Warnings: []string{}, - Errors: []string{"PacemakerCluster CR not found in cache"}, - }, previous, nil + return unknownHealthStatus("PacemakerCluster CR not found in cache"), previous, nil } pacemakerCR, ok := item.(*pacmkrv1.PacemakerCluster) if !ok { - return &HealthStatus{ - OverallStatus: statusUnknown, - Warnings: []string{}, - Errors: []string{"Failed to convert cached item to PacemakerCluster"}, - }, previous, nil - } - - // Check if status is populated (LastUpdated is zero means status was never set) - if pacemakerCR.Status.LastUpdated.IsZero() { - // Unknown status - don't update previous - return &HealthStatus{ - OverallStatus: statusUnknown, - Warnings: []string{}, - Errors: []string{"PacemakerCluster CR has no status populated"}, - }, previous, nil - } - - crLastUpdated := pacemakerCR.Status.LastUpdated.Time - - // Check staleness first - any update (even with errors) clears staleness. - timeSinceUpdate := time.Since(crLastUpdated) - if timeSinceUpdate > StatusStalenessThreshold { - // Unknown status - don't update previous - // Use absolute timestamp (stable) for event deduplication. - return &HealthStatus{ - OverallStatus: statusUnknown, - Warnings: []string{}, - Errors: []string{fmt.Sprintf("Pacemaker status is stale (last updated: %s)", crLastUpdated.Format(time.RFC3339))}, - }, previous, nil - } - - // Check if lastUpdated timestamp has changed since last sync. - // If unchanged, skip processing to avoid redundant work on timer-triggered syncs. - // Use previous.CRLastUpdated for comparison (only set for non-Unknown status). - if previous != nil && !previous.CRLastUpdated.IsZero() && crLastUpdated.Equal(previous.CRLastUpdated) { - klog.V(4).Infof("Skipping sync: lastUpdated timestamp unchanged (%v)", crLastUpdated) + return unknownHealthStatus("Failed to convert cached item to PacemakerCluster"), previous, nil + } + + // Staleness grows over time and must bypass the timestamp-unchanged optimization. + isStale := pacemakerCR.Status.LastUpdated.IsZero() || + time.Since(pacemakerCR.Status.LastUpdated.Time) > StatusStalenessThreshold + + if !isStale && previous != nil && !previous.CRLastUpdated.IsZero() && + pacemakerCR.Status.LastUpdated.Time.Equal(previous.CRLastUpdated) { + klog.V(4).Infof("Skipping sync: lastUpdated timestamp unchanged (%v)", pacemakerCR.Status.LastUpdated.Time) return nil, nil, nil } - // Build health status from the CRD status fields - currentStatus := c.buildHealthStatusFromCR(pacemakerCR) - currentStatus.CRLastUpdated = crLastUpdated + currentStatus := BuildHealthStatusFromCR(pacemakerCR) - // Only update previous for non-Unknown status (preserves last valid for grace period) - if currentStatus.OverallStatus != statusUnknown { + if currentStatus.OverallStatus != StatusUnknown { c.previousMu.Lock() c.previous = currentStatus c.previousMu.Unlock() @@ -334,36 +291,44 @@ func (c *HealthCheck) getPacemakerStatus(ctx context.Context) (*HealthStatus, *H return currentStatus, previous, nil } -// buildHealthStatusFromCR builds a HealthStatus from the PacemakerCluster CR status fields -// Note: This function assumes Status is not nil (checked by caller in getPacemakerStatus) -func (c *HealthCheck) buildHealthStatusFromCR(pacemakerStatus *pacmkrv1.PacemakerCluster) *HealthStatus { +func unknownHealthStatus(msg string) *HealthStatus { + return &HealthStatus{ + OverallStatus: StatusUnknown, + Warnings: []string{}, + Errors: []string{msg}, + } +} + +// BuildHealthStatusFromCR evaluates a PacemakerCluster CR and returns its complete health status. +func BuildHealthStatusFromCR(cr *pacmkrv1.PacemakerCluster) *HealthStatus { status := &HealthStatus{ - OverallStatus: statusUnknown, + OverallStatus: StatusUnknown, Warnings: []string{}, Errors: []string{}, } - // Defensive check: this should not happen as getPacemakerStatus checks for unpopulated Status - if pacemakerStatus == nil || pacemakerStatus.Status.LastUpdated.IsZero() { - klog.Errorf("buildHealthStatusFromCR called with nil PacemakerCluster or unpopulated Status") - status.Errors = append(status.Errors, "Internal error: unpopulated Status in buildHealthStatusFromCR") + if cr == nil || cr.Status.LastUpdated.IsZero() { + status.Errors = append(status.Errors, "PacemakerCluster CR has no status populated") return status } - // Check cluster-level configuration issues FIRST (node count, maintenance mode) - // These are often root causes (e.g., "excessive nodes" causes resource failures on extra node) - c.checkClusterConditions(pacemakerStatus, status) + status.CRLastUpdated = cr.Status.LastUpdated.Time - // Check node statuses (node-level conditions and resource health) - c.checkNodeStatuses(pacemakerStatus, status) + if time.Since(cr.Status.LastUpdated.Time) > StatusStalenessThreshold { + status.Errors = append(status.Errors, fmt.Sprintf("Pacemaker status is stale (last updated: %s)", + cr.Status.LastUpdated.Time.Format(time.RFC3339))) + return status + } + + checkClusterConditions(cr, status) + checkNodeStatuses(cr, status) - // Determine overall status: errors > warnings > healthy if len(status.Errors) > 0 { - status.OverallStatus = statusError + status.OverallStatus = StatusError } else if len(status.Warnings) > 0 { - status.OverallStatus = statusWarning + status.OverallStatus = StatusWarning } else { - status.OverallStatus = statusHealthy + status.OverallStatus = StatusHealthy } return status @@ -372,34 +337,29 @@ func (c *HealthCheck) buildHealthStatusFromCR(pacemakerStatus *pacmkrv1.Pacemake // checkClusterConditions checks cluster-level configuration issues (node count, maintenance mode). // These are ALWAYS reported regardless of node-level errors, as they often represent root causes. // For example, "excessive nodes" causes resource failures on the extra node. -func (c *HealthCheck) checkClusterConditions(pacemakerStatus *pacmkrv1.PacemakerCluster, status *HealthStatus) { - conditions := pacemakerStatus.Status.Conditions +func checkClusterConditions(cr *pacmkrv1.PacemakerCluster, status *HealthStatus) { + conditions := cr.Status.Conditions if len(conditions) == 0 { - // Missing cluster conditions means we can't verify cluster health configuration klog.V(2).Infof("No cluster conditions present in status") status.Errors = append(status.Errors, "No cluster conditions available") return } - // Always check cluster-level configuration issues - these are root causes - clusterIssues := c.getClusterConditionIssues(conditions, pacemakerStatus) - - // Add cluster-level error if there are configuration issues - if len(clusterIssues) > 0 { - status.Errors = append(status.Errors, fmt.Sprintf(msgClusterUnhealthy, strings.Join(clusterIssues, ", "))) + if issues := getClusterConditionIssues(conditions, cr); len(issues) > 0 { + status.Errors = append(status.Errors, fmt.Sprintf(msgClusterUnhealthy, strings.Join(issues, ", "))) } } // getClusterConditionIssues returns specific issues from cluster-level conditions (non-summary conditions) -func (c *HealthCheck) getClusterConditionIssues(conditions []metav1.Condition, pacemakerStatus *pacmkrv1.PacemakerCluster) []string { +func getClusterConditionIssues(conditions []metav1.Condition, cr *pacmkrv1.PacemakerCluster) []string { var issues []string // Check NodeCountAsExpected condition nodeCountCondition := FindCondition(conditions, pacmkrv1.ClusterNodeCountAsExpectedConditionType) if nodeCountCondition != nil && nodeCountCondition.Status != metav1.ConditionTrue { nodeCount := 0 - if pacemakerStatus.Status.Nodes != nil { - nodeCount = len(*pacemakerStatus.Status.Nodes) + if cr.Status.Nodes != nil { + nodeCount = len(*cr.Status.Nodes) } switch nodeCountCondition.Reason { case pacmkrv1.ClusterNodeCountAsExpectedReasonInsufficientNodes: @@ -419,15 +379,15 @@ func (c *HealthCheck) getClusterConditionIssues(conditions []metav1.Condition, p } // checkNodeStatuses checks if all nodes have healthy conditions and resources -func (c *HealthCheck) checkNodeStatuses(pacemakerStatus *pacmkrv1.PacemakerCluster, status *HealthStatus) { +func checkNodeStatuses(cr *pacmkrv1.PacemakerCluster, status *HealthStatus) { // Nil-guard for Nodes field - missing node data is an error (cannot verify cluster health) - if pacemakerStatus.Status.Nodes == nil { + if cr.Status.Nodes == nil { klog.V(2).Infof("Pacemaker.Status.Nodes is nil, cannot determine node status") status.Errors = append(status.Errors, msgNoNodesFound) return } - nodes := *pacemakerStatus.Status.Nodes + nodes := *cr.Status.Nodes // Empty nodes list is also an error - cannot verify cluster health without node data if len(nodes) == 0 { @@ -438,58 +398,39 @@ func (c *HealthCheck) checkNodeStatuses(pacemakerStatus *pacmkrv1.PacemakerClust // Check each node's conditions and resource health (consolidated into single error per node) for _, node := range nodes { - c.checkNodeConditions(node, status) + checkNodeConditions(node, status) } } // checkNodeConditions checks the conditions of a single node and its resources, // routing issues to errors or warnings based on severity. // Errors degrade the operator; warnings are informational (e.g., fencing redundancy lost). -func (c *HealthCheck) checkNodeConditions(node pacmkrv1.PacemakerClusterNodeStatus, status *HealthStatus) { +func checkNodeConditions(node pacmkrv1.PacemakerClusterNodeStatus, status *HealthStatus) { conditions := node.Conditions if len(conditions) == 0 { klog.V(2).Infof("Node %s has no conditions", node.NodeName) return } - // Check Online condition - this is critical for degraded status - onlineCondition := FindCondition(conditions, pacmkrv1.NodeOnlineConditionType) - if onlineCondition != nil && onlineCondition.Status != metav1.ConditionTrue { + if cond := FindCondition(conditions, pacmkrv1.NodeOnlineConditionType); cond != nil && cond.Status != metav1.ConditionTrue { status.Errors = append(status.Errors, fmt.Sprintf(msgNodeOffline, node.NodeName)) - return // If node is offline, other conditions don't matter + return } - // Always check for fencing warnings - these are independent of overall node health. - // Fencing redundancy degraded (FencingHealthy=False but FencingAvailable=True) is a warning - // that should be reported even when the node is otherwise healthy. - fencingWarnings := c.getFencingWarnings(conditions) + fencingWarnings := getFencingWarnings(conditions) for _, warning := range fencingWarnings { status.Warnings = append(status.Warnings, fmt.Sprintf("%s: %s", node.NodeName, warning)) } - // Check overall node Healthy condition healthyCondition := FindCondition(conditions, pacmkrv1.NodeHealthyConditionType) if healthyCondition == nil || healthyCondition.Status == metav1.ConditionTrue { - return // Node is healthy (except for warnings already captured above) + return } - // Node is unhealthy - collect errors (warnings already captured above) - nodeErrors := c.getNodeConditionErrors(conditions) - - // Collect resource errors (all resource issues are currently errors) - resourceErrors := c.getNodeResourceSummaries(node) - - // Combine all errors - allErrors := append(nodeErrors, resourceErrors...) - - // If there are errors, build consolidated error message + allErrors := slices.Concat(getNodeConditionErrors(conditions), getNodeResourceSummaries(node)) if len(allErrors) > 0 { status.Errors = append(status.Errors, fmt.Sprintf(msgNodeUnhealthy, node.NodeName, strings.Join(allErrors, ", "))) - } - - // If no specific issues found but node is unhealthy, use generic message. - // Skip if we already captured fencing warnings - those explain the unhealthy state. - if len(allErrors) == 0 && len(fencingWarnings) == 0 { + } else if len(fencingWarnings) == 0 { status.Errors = append(status.Errors, fmt.Sprintf(msgNodeUnhealthy, node.NodeName, healthyCondition.Message)) } } @@ -497,11 +438,9 @@ func (c *HealthCheck) checkNodeConditions(node pacmkrv1.PacemakerClusterNodeStat // getFencingWarnings returns warnings about degraded fencing redundancy. // This is checked independently of overall node health because fencing redundancy // degradation should be reported even when the node is otherwise healthy. -func (c *HealthCheck) getFencingWarnings(conditions []metav1.Condition) []string { +func getFencingWarnings(conditions []metav1.Condition) []string { var warnings []string - // Check FencingHealthy - if false but FencingAvailable is true, fencing redundancy is degraded (warning) - // This is a warning because the node CAN still be fenced, just with reduced redundancy. fencingAvailableCondition := FindCondition(conditions, pacmkrv1.NodeFencingAvailableConditionType) fencingHealthyCondition := FindCondition(conditions, pacmkrv1.NodeFencingHealthyConditionType) @@ -538,7 +477,7 @@ var nodeConditionChecks = []nodeConditionCheck{ // getNodeConditionErrors returns errors from node-level conditions. // Errors require immediate action and cause the operator to degrade. -func (c *HealthCheck) getNodeConditionErrors(conditions []metav1.Condition) []string { +func getNodeConditionErrors(conditions []metav1.Condition) []string { var errors []string for _, check := range nodeConditionChecks { @@ -557,50 +496,34 @@ func (c *HealthCheck) getNodeConditionErrors(conditions []metav1.Condition) []st // getNodeResourceSummaries returns summaries of unhealthy resources on a node. // Each summary includes the resource name and its specific issue. -func (c *HealthCheck) getNodeResourceSummaries(node pacmkrv1.PacemakerClusterNodeStatus) []string { +func getNodeResourceSummaries(node pacmkrv1.PacemakerClusterNodeStatus) []string { var summaries []string for _, resource := range node.Resources { - healthyCondition := FindCondition(resource.Conditions, pacmkrv1.ResourceHealthyConditionType) - if healthyCondition != nil && healthyCondition.Status != metav1.ConditionTrue { - // Get specific reason for this resource - reason := c.getResourceIssue(resource.Conditions) - summaries = append(summaries, fmt.Sprintf("%s %s", resource.Name, reason)) + if cond := FindCondition(resource.Conditions, pacmkrv1.ResourceHealthyConditionType); cond != nil && cond.Status != metav1.ConditionTrue { + summaries = append(summaries, fmt.Sprintf("%s %s", resource.Name, getResourceIssue(resource.Conditions))) } } return summaries } -// getResourceIssue returns a specific issue description for an unhealthy resource. -// -// All resource-level issues are treated as errors. The Active=True with Started=False state -// (anomalous transitional state) could theoretically be a warning since it might self-resolve, -// but routing resource issues to warnings vs errors would require significant refactoring. -// Additionally: (1) this state is rare and brief and (2) etcd not running causes API server -// degradation anyway, so treating it as an error is appropriate. -func (c *HealthCheck) getResourceIssue(conditions []metav1.Condition) string { - // Check for specific failure conditions (prioritized by severity) - operationalCondition := FindCondition(conditions, pacmkrv1.ResourceOperationalConditionType) - if operationalCondition != nil && operationalCondition.Status != metav1.ConditionTrue { - return "has failed" - } - - startedCondition := FindCondition(conditions, pacmkrv1.ResourceStartedConditionType) - if startedCondition != nil && startedCondition.Status != metav1.ConditionTrue { - return "is stopped" - } - - activeCondition := FindCondition(conditions, pacmkrv1.ResourceActiveConditionType) - if activeCondition != nil && activeCondition.Status != metav1.ConditionTrue { - return "is not active" - } - - managedCondition := FindCondition(conditions, pacmkrv1.ResourceManagedConditionType) - if managedCondition != nil && managedCondition.Status != metav1.ConditionTrue { - return "is unmanaged" +// getResourceIssue returns a human-readable issue for an unhealthy resource, prioritized by severity. +func getResourceIssue(conditions []metav1.Condition) string { + checks := []struct { + condType string + msg string + }{ + {pacmkrv1.ResourceOperationalConditionType, "has failed"}, + {pacmkrv1.ResourceStartedConditionType, "is stopped"}, + {pacmkrv1.ResourceActiveConditionType, "is not active"}, + {pacmkrv1.ResourceManagedConditionType, "is unmanaged"}, + } + for _, check := range checks { + if cond := FindCondition(conditions, check.condType); cond != nil && cond.Status != metav1.ConditionTrue { + return check.msg + } } - return "is unhealthy" } @@ -619,16 +542,16 @@ func (c *HealthCheck) updateOperatorStatus(ctx context.Context, status *HealthSt // Update operator conditions based on pacemaker status switch status.OverallStatus { - case statusError: + case StatusError: return c.setPacemakerDegradedCondition(ctx, status) - case statusHealthy, statusWarning: + case StatusHealthy, StatusWarning: // Both healthy and warning states should clear degraded condition // Warnings are informational (e.g. recent fencing, node count mismatch) and don't indicate degradation - if status.OverallStatus == statusWarning { + if status.OverallStatus == StatusWarning { klog.V(2).Infof("Pacemaker health check has warnings but cluster is operational: %v", status.Warnings) } return c.clearPacemakerDegradedCondition(ctx, status) - case statusUnknown: + case StatusUnknown: // Unknown status means we cannot determine pacemaker health (CR not found, stale, no status, etc.) // Only mark as degraded if we haven't received a valid status in a while (grace period). // Use previous.CRLastUpdated which reflects when we last had valid cluster data. @@ -831,12 +754,12 @@ func (c *HealthCheck) recordHealthCheckEvents(current *HealthStatus, previous *H func (c *HealthCheck) recordHealthTransitionEvents(current *HealthStatus, previous *HealthStatus) { // Determine previous state from the previous HealthStatus we computed. // This is derived from the previous PacemakerCluster CR we processed. - previousWasUnknown := previous == nil || previous.OverallStatus == statusUnknown - previousWasDegraded := previous != nil && previous.OverallStatus == statusError + previousWasUnknown := previous == nil || previous.OverallStatus == StatusUnknown + previousWasDegraded := previous != nil && previous.OverallStatus == StatusError previousHadWarnings := previous != nil && len(previous.Warnings) > 0 // Determine current state - operationallyHealthy := current.OverallStatus == statusHealthy || current.OverallStatus == statusWarning + operationallyHealthy := current.OverallStatus == StatusHealthy || current.OverallStatus == StatusWarning currentHasNoWarnings := len(current.Warnings) == 0 // Record PacemakerHealthy when transitioning to operationally healthy from: @@ -862,10 +785,6 @@ func (c *HealthCheck) recordHealthTransitionEvents(current *HealthStatus, previo } } -// recordWarningEvent records appropriate warning events based on warning type. -// Note: Failed action events (warningPrefixFailedAction) are recorded by the status collector -// directly from pacemaker XML, not by the healthcheck controller. The PacemakerCluster CR -// only contains conditions, not raw failed action history. func (c *HealthCheck) recordWarningEvent(warning string) { switch { case strings.Contains(warning, warningPrefixFencingEvent): diff --git a/pkg/tnf/pkg/pacemaker/healthcheck_test.go b/pkg/tnf/pkg/pacemaker/healthcheck_test.go index c9f2af19f..ff57c1133 100644 --- a/pkg/tnf/pkg/pacemaker/healthcheck_test.go +++ b/pkg/tnf/pkg/pacemaker/healthcheck_test.go @@ -105,7 +105,7 @@ func TestHealthCheck_getPacemakerStatus(t *testing.T) { LastUpdated: metav1.Now(), }, }, - expectedStatus: statusHealthy, + expectedStatus: StatusHealthy, expectErrors: false, expectWarnings: false, }, @@ -128,7 +128,7 @@ func TestHealthCheck_getPacemakerStatus(t *testing.T) { LastUpdated: metav1.Time{Time: time.Now().Add(-10 * time.Minute)}, // > 5 min threshold }, }, - expectedStatus: statusUnknown, + expectedStatus: StatusUnknown, expectErrors: true, expectWarnings: false, }, @@ -144,7 +144,7 @@ func TestHealthCheck_getPacemakerStatus(t *testing.T) { }, Status: pacmkrv1.PacemakerClusterStatus{}, // Status not populated yet (zero LastUpdated) }, - expectedStatus: statusUnknown, + expectedStatus: StatusUnknown, expectErrors: true, expectWarnings: false, }, @@ -166,7 +166,7 @@ func TestHealthCheck_getPacemakerStatus(t *testing.T) { LastUpdated: metav1.Now(), }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, expectWarnings: false, }, @@ -189,7 +189,7 @@ func TestHealthCheck_getPacemakerStatus(t *testing.T) { LastUpdated: metav1.Now(), }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, expectWarnings: false, }, @@ -212,7 +212,7 @@ func TestHealthCheck_getPacemakerStatus(t *testing.T) { LastUpdated: metav1.Now(), }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, expectWarnings: false, }, @@ -235,7 +235,7 @@ func TestHealthCheck_getPacemakerStatus(t *testing.T) { LastUpdated: metav1.Now(), }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, expectWarnings: false, }, @@ -296,7 +296,7 @@ func TestHealthCheck_getPacemakerStatus_UnchangedTimestamp(t *testing.T) { current1, _, err1 := controller.getPacemakerStatus(ctx) require.NoError(t, err1, "First getPacemakerStatus should not return an error") require.NotNil(t, current1, "First call should return a status") - require.Equal(t, statusHealthy, current1.OverallStatus, "First call should return healthy status") + require.Equal(t, StatusHealthy, current1.OverallStatus, "First call should return healthy status") // Second call with same timestamp should return nil (no change) current2, _, err2 := controller.getPacemakerStatus(ctx) @@ -313,36 +313,36 @@ func TestHealthCheck_updateOperatorStatus(t *testing.T) { { name: "error_status_sets_degraded", status: &HealthStatus{ - OverallStatus: statusError, + OverallStatus: StatusError, Warnings: []string{"Test warning"}, Errors: []string{"Test error"}, }, previous: &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, CRLastUpdated: time.Now().Add(-1 * time.Minute), }, }, { name: "healthy_status_clears_degraded", status: &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}, }, previous: &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, CRLastUpdated: time.Now().Add(-1 * time.Minute), }, }, { name: "warning_status_clears_degraded", status: &HealthStatus{ - OverallStatus: statusWarning, + OverallStatus: StatusWarning, Warnings: []string{"Test warning"}, Errors: []string{}, }, previous: &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, CRLastUpdated: time.Now().Add(-1 * time.Minute), }, }, @@ -363,9 +363,9 @@ func TestHealthCheck_recordHealthCheckEvents(t *testing.T) { controller := createTestHealthCheck() // Test with warnings and errors - previous was healthy (not degraded, no warnings, known status) - previousHealthy := &HealthStatus{OverallStatus: statusHealthy, Warnings: []string{}, Errors: []string{}} + previousHealthy := &HealthStatus{OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}} currentWithWarnings := &HealthStatus{ - OverallStatus: statusWarning, + OverallStatus: StatusWarning, Warnings: []string{ "Recent failed resource action: kubelet monitor on master-0 failed", "Recent fencing event: reboot of master-1 success", @@ -378,7 +378,7 @@ func TestHealthCheck_recordHealthCheckEvents(t *testing.T) { // Test with healthy status - previous had warnings currentHealthy := &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}, } @@ -396,7 +396,7 @@ func TestHealthCheck_eventDeduplication(t *testing.T) { // First recording - Error state (start in Error to test transitions properly) // previousStatus=nil because this is first sync errorStatus := &HealthStatus{ - OverallStatus: statusError, + OverallStatus: StatusError, Warnings: []string{}, Errors: []string{"Test error"}, } @@ -416,7 +416,7 @@ func TestHealthCheck_eventDeduplication(t *testing.T) { // Transition to Warning (operationally healthy) - should record PacemakerHealthy event // previousStatus is errorStatus (degraded), so healthy event should fire warningStatus := &HealthStatus{ - OverallStatus: statusWarning, + OverallStatus: StatusWarning, Warnings: []string{ "Recent failed resource action: kubelet monitor on master-0 failed", }, @@ -434,7 +434,7 @@ func TestHealthCheck_eventDeduplication(t *testing.T) { // Test fencing event deduplication (longer window) fencingStatus := &HealthStatus{ - OverallStatus: statusWarning, + OverallStatus: StatusWarning, Warnings: []string{ "Recent fencing event: reboot of master-1 success", }, @@ -453,7 +453,7 @@ func TestHealthCheck_eventDeduplication(t *testing.T) { // Transition from Warning to Healthy - fires PacemakerWarningsCleared event // previousStatus has warnings, so WarningsCleared should fire healthyStatus := &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}, } @@ -479,7 +479,7 @@ func TestHealthCheck_eventDeduplication(t *testing.T) { require.Greater(t, afterRecovery, beforeRecovery, "PacemakerHealthy event should be recorded on transition from Error to Healthy") } -func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { +func TestBuildHealthStatusFromCR(t *testing.T) { tests := []struct { name string cr *pacmkrv1.PacemakerCluster @@ -499,7 +499,7 @@ func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { }, }, }, - expectedStatus: statusHealthy, + expectedStatus: StatusHealthy, expectErrors: false, }, { @@ -514,7 +514,7 @@ func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { }, }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, errorContains: string(pacmkrv1.PacemakerClusterResourceNameKubelet), }, @@ -530,7 +530,7 @@ func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { }, }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, errorContains: string(pacmkrv1.PacemakerClusterResourceNameEtcd), }, @@ -545,7 +545,7 @@ func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { }, }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, errorContains: "Insufficient nodes", }, @@ -558,7 +558,7 @@ func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { Nodes: &[]pacmkrv1.PacemakerClusterNodeStatus{}, }, }, - expectedStatus: statusError, + expectedStatus: StatusError, expectErrors: true, errorContains: "No nodes found", }, @@ -567,16 +567,15 @@ func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { cr: &pacmkrv1.PacemakerCluster{ Status: pacmkrv1.PacemakerClusterStatus{}, // Zero LastUpdated }, - expectedStatus: statusUnknown, + expectedStatus: StatusUnknown, expectErrors: true, - errorContains: "Internal error", + errorContains: "no status populated", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := &HealthCheck{} - status := controller.buildHealthStatusFromCR(tt.cr) + status := BuildHealthStatusFromCR(tt.cr) require.NotNil(t, status, "HealthStatus should not be nil") require.Equal(t, tt.expectedStatus, status.OverallStatus) @@ -601,7 +600,7 @@ func TestHealthCheck_buildHealthStatusFromCR(t *testing.T) { } } -func TestHealthCheck_checkClusterConditions(t *testing.T) { +func TestCheckClusterConditions(t *testing.T) { tests := []struct { name string conditions []metav1.Condition @@ -661,8 +660,7 @@ func TestHealthCheck_checkClusterConditions(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := &HealthCheck{} - pacemakerStatus := &pacmkrv1.PacemakerCluster{ + cr := &pacmkrv1.PacemakerCluster{ Status: pacmkrv1.PacemakerClusterStatus{ Conditions: tt.conditions, Nodes: tt.nodes, @@ -670,7 +668,7 @@ func TestHealthCheck_checkClusterConditions(t *testing.T) { } status := &HealthStatus{Warnings: []string{}, Errors: []string{}} - controller.checkClusterConditions(pacemakerStatus, status) + checkClusterConditions(cr, status) if tt.expectErrors { require.NotEmpty(t, status.Errors, "Should have errors") @@ -684,7 +682,7 @@ func TestHealthCheck_checkClusterConditions(t *testing.T) { } } -func TestHealthCheck_checkNodeStatuses(t *testing.T) { +func TestCheckNodeStatuses(t *testing.T) { tests := []struct { name string nodes *[]pacmkrv1.PacemakerClusterNodeStatus @@ -727,8 +725,7 @@ func TestHealthCheck_checkNodeStatuses(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := &HealthCheck{} - pacemakerStatus := &pacmkrv1.PacemakerCluster{ + cr := &pacmkrv1.PacemakerCluster{ Status: pacmkrv1.PacemakerClusterStatus{ Conditions: createHealthyClusterConditions(), Nodes: tt.nodes, @@ -736,7 +733,7 @@ func TestHealthCheck_checkNodeStatuses(t *testing.T) { } status := &HealthStatus{Warnings: []string{}, Errors: []string{}} - controller.checkNodeStatuses(pacemakerStatus, status) + checkNodeStatuses(cr, status) if tt.expectErrors { require.NotEmpty(t, status.Errors, "Should have errors") @@ -757,19 +754,14 @@ func TestHealthCheck_checkNodeStatuses(t *testing.T) { } } -func TestHealthCheck_checkNodeStatuses_MultipleUnhealthyResources(t *testing.T) { - controller := &HealthCheck{} - - // Create a node with multiple unhealthy resources +func TestCheckNodeStatuses_MultipleUnhealthyResources(t *testing.T) { node := createHealthyNodeStatus("master-0", []string{"192.168.1.10"}) - // Mark node as unhealthy for i := range node.Conditions { if node.Conditions[i].Type == pacmkrv1.NodeHealthyConditionType { node.Conditions[i].Status = metav1.ConditionFalse node.Conditions[i].Reason = pacmkrv1.NodeHealthyReasonUnhealthy } } - // Mark kubelet and etcd as unhealthy for i := range node.Resources { if node.Resources[i].Name == pacmkrv1.PacemakerClusterResourceNameKubelet || node.Resources[i].Name == pacmkrv1.PacemakerClusterResourceNameEtcd { @@ -787,9 +779,7 @@ func TestHealthCheck_checkNodeStatuses_MultipleUnhealthyResources(t *testing.T) } status := &HealthStatus{Warnings: []string{}, Errors: []string{}} - controller.checkNodeStatuses(pacemakerStatus, status) - - // Should have 1 consolidated error for the unhealthy node (includes all resource issues) + checkNodeStatuses(pacemakerStatus, status) require.Len(t, status.Errors, 1, "Should have 1 consolidated error for unhealthy node") // Verify the single error mentions both resources @@ -800,9 +790,7 @@ func TestHealthCheck_checkNodeStatuses_MultipleUnhealthyResources(t *testing.T) require.Contains(t, nodeError, string(pacmkrv1.PacemakerClusterResourceNameEtcd), "Error should mention Etcd") } -func TestHealthCheck_getResourceIssue(t *testing.T) { - controller := &HealthCheck{} - +func TestGetResourceIssue(t *testing.T) { tests := []struct { name string conditions []metav1.Condition @@ -837,7 +825,7 @@ func TestHealthCheck_getResourceIssue(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := controller.getResourceIssue(tt.conditions) + got := getResourceIssue(tt.conditions) require.Equal(t, tt.wantIssue, got) }) } @@ -936,7 +924,7 @@ func TestHealthCheck_eventDeduplication_StatusTransitions(t *testing.T) { // Start with healthy status from Unknown state (first sync or recovering from stale) // previousStatus=nil: PacemakerHealthy event fires on initial healthy status healthyStatus := &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}, } @@ -951,7 +939,7 @@ func TestHealthCheck_eventDeduplication_StatusTransitions(t *testing.T) { // Transition to Warning - should record warning but not healthy event (already in healthy state) warningStatus := &HealthStatus{ - OverallStatus: statusWarning, + OverallStatus: StatusWarning, Warnings: []string{"Some warning"}, Errors: []string{}, } @@ -969,7 +957,7 @@ func TestHealthCheck_eventDeduplication_StatusTransitions(t *testing.T) { // Transition to Error errorStatus := &HealthStatus{ - OverallStatus: statusError, + OverallStatus: StatusError, Warnings: []string{}, Errors: []string{"Critical error"}, } @@ -995,9 +983,9 @@ func TestHealthCheck_eventDeduplication_CleanupOldEntries(t *testing.T) { controller.recordedEventsMu.Unlock() // Trigger cleanup by recording a new event - previousStatus := &HealthStatus{OverallStatus: statusHealthy, Warnings: []string{}, Errors: []string{}} + previousStatus := &HealthStatus{OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}} currentStatus := &HealthStatus{ - OverallStatus: statusWarning, + OverallStatus: StatusWarning, Warnings: []string{"New warning"}, Errors: []string{}, } @@ -1010,8 +998,7 @@ func TestHealthCheck_eventDeduplication_CleanupOldEntries(t *testing.T) { controller.recordedEventsMu.Unlock() } -func TestHealthCheck_getNodeConditionErrorsAndWarnings(t *testing.T) { - controller := &HealthCheck{} +func TestGetNodeConditionErrorsAndWarnings(t *testing.T) { now := metav1.Now() tests := []struct { @@ -1102,8 +1089,8 @@ func TestHealthCheck_getNodeConditionErrorsAndWarnings(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Check errors and warnings using the separate functions - errors := controller.getNodeConditionErrors(tt.conditions) - warnings := controller.getFencingWarnings(tt.conditions) + errors := getNodeConditionErrors(tt.conditions) + warnings := getFencingWarnings(tt.conditions) // Check errors require.Equal(t, len(tt.expectedErrors), len(errors), "Number of errors should match") @@ -1137,7 +1124,7 @@ func TestHealthCheck_getNodeConditionErrorsAndWarnings(t *testing.T) { // TestHealthCheck_FencingRedundancyWarning tests fencing redundancy warnings in various scenarios. // Fencing redundancy degraded (FencingHealthy=False but FencingAvailable=True) should be // reported as a warning, not an error, and should be captured regardless of overall node health. -func TestHealthCheck_FencingRedundancyWarning(t *testing.T) { +func TestCheckNodeConditions_FencingRedundancyWarning(t *testing.T) { tests := []struct { name string nodeHealthy bool // Overall node healthy condition @@ -1176,7 +1163,6 @@ func TestHealthCheck_FencingRedundancyWarning(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - controller := &HealthCheck{} now := metav1.Now() // Build conditions based on test case @@ -1219,7 +1205,7 @@ func TestHealthCheck_FencingRedundancyWarning(t *testing.T) { } status := &HealthStatus{Warnings: []string{}, Errors: []string{}} - controller.checkNodeConditions(node, status) + checkNodeConditions(node, status) // Check warnings if tt.expectWarnings { @@ -1254,12 +1240,12 @@ func TestHealthCheck_WarningsClearedEvent(t *testing.T) { { name: "warning_to_healthy", previousStatus: &HealthStatus{ - OverallStatus: statusWarning, + OverallStatus: StatusWarning, Warnings: []string{"master-0: " + msgFencingRedundancyLost}, Errors: []string{}, }, currentStatus: &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}, }, @@ -1269,12 +1255,12 @@ func TestHealthCheck_WarningsClearedEvent(t *testing.T) { { name: "error_without_warnings_to_healthy", previousStatus: &HealthStatus{ - OverallStatus: statusError, + OverallStatus: StatusError, Warnings: []string{}, Errors: []string{"Critical error"}, }, currentStatus: &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}, }, @@ -1284,12 +1270,12 @@ func TestHealthCheck_WarningsClearedEvent(t *testing.T) { { name: "error_with_warnings_to_error_without_warnings", previousStatus: &HealthStatus{ - OverallStatus: statusError, + OverallStatus: StatusError, Warnings: []string{"master-0: " + msgFencingRedundancyLost}, Errors: []string{"Critical error"}, }, currentStatus: &HealthStatus{ - OverallStatus: statusError, + OverallStatus: StatusError, Warnings: []string{}, Errors: []string{"Critical error"}, }, @@ -1299,12 +1285,12 @@ func TestHealthCheck_WarningsClearedEvent(t *testing.T) { { name: "error_with_warnings_to_healthy", previousStatus: &HealthStatus{ - OverallStatus: statusError, + OverallStatus: StatusError, Warnings: []string{"master-0: " + msgFencingRedundancyLost}, Errors: []string{"Critical error"}, }, currentStatus: &HealthStatus{ - OverallStatus: statusHealthy, + OverallStatus: StatusHealthy, Warnings: []string{}, Errors: []string{}, }, From b5e1f48843a41de6c903f4e3d21f97e2ffc52ddb Mon Sep 17 00:00:00 2001 From: Luca Consalvi Date: Mon, 20 Jul 2026 17:30:09 +0200 Subject: [PATCH 5/5] Skip console notifications for uninitialized PacemakerCluster CR When the CR exists but the status collector CronJob hasn't run yet (LastUpdated is zero), treat it the same as a missing CR to avoid a false-positive troubleshooting banner at startup. Co-Authored-By: Claude Opus 4.6 --- pkg/tnf/pkg/pacemaker/consolenotification.go | 4 ++++ pkg/tnf/pkg/pacemaker/consolenotification_test.go | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/pkg/tnf/pkg/pacemaker/consolenotification.go b/pkg/tnf/pkg/pacemaker/consolenotification.go index ace89589e..57a2d9e52 100644 --- a/pkg/tnf/pkg/pacemaker/consolenotification.go +++ b/pkg/tnf/pkg/pacemaker/consolenotification.go @@ -125,6 +125,10 @@ func (c *consoleNotificationController) sync(ctx context.Context, _ factory.Sync return fmt.Errorf("unexpected object type in informer store: %T", item) } + if cr.Status.LastUpdated.IsZero() { + return c.deleteAllNotifications(ctx) + } + status := BuildHealthStatusFromCR(cr) degradedProblems, troubleshootingProblems := classifyProblems(status) diff --git a/pkg/tnf/pkg/pacemaker/consolenotification_test.go b/pkg/tnf/pkg/pacemaker/consolenotification_test.go index 9867af23f..1fe72418e 100644 --- a/pkg/tnf/pkg/pacemaker/consolenotification_test.go +++ b/pkg/tnf/pkg/pacemaker/consolenotification_test.go @@ -362,6 +362,18 @@ func TestSync_FencingDegraded_CreatesDegradedNotification(t *testing.T) { require.Equal(t, categoryDegraded.name, getCreatedName(t, dynClient)) } +func TestSync_UninitializedCR_DeletesBothNotifications(t *testing.T) { + cr := &pacmkrv1.PacemakerCluster{ + ObjectMeta: metav1.ObjectMeta{Name: PacemakerClusterResourceName}, + } + ctrl, dynClient := newTestController(cr) + + err := ctrl.sync(context.Background(), nil) + require.NoError(t, err) + require.Equal(t, 2, countActions(dynClient, "delete"), "expected delete actions for both categories") + require.Equal(t, 0, countActions(dynClient, "create"), "should not create any notification") +} + func TestSync_CRNotFound_DeletesBothNotifications(t *testing.T) { ctrl, dynClient := newTestController(nil)