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..57a2d9e52 --- /dev/null +++ b/pkg/tnf/pkg/pacemaker/consolenotification.go @@ -0,0 +1,227 @@ +package pacemaker + +import ( + "context" + "fmt" + "slices" + "strings" + + 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" + "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" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/tools/cache" + "k8s.io/klog/v2" +) + +const ( + notificationTextColor = "#fff" + notificationBackgroundColor = "#c9190b" + + 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", + } +) + +// 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 +} + +type consoleNotificationController struct { + dynamicClient dynamic.Interface + recorder events.Recorder + pacemakerInformer cache.SharedIndexInformer + consoleUnavailable bool +} + +func NewConsoleNotificationController( + pacemakerInformer cache.SharedIndexInformer, + dynamicClient dynamic.Interface, + eventRecorder events.Recorder, +) factory.Controller { + c := &consoleNotificationController{ + dynamicClient: dynamicClient, + recorder: eventRecorder, + 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 { + if c.consoleUnavailable { + return nil + } + + item, exists, err := c.pacemakerInformer.GetStore().GetByKey(PacemakerClusterResourceName) + if err != nil { + return err + } + if !exists { + return c.deleteAllNotifications(ctx) + } + + cr, ok := item.(*pacmkrv1.PacemakerCluster) + if !ok { + 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) + + if err := c.manageNotification(ctx, categoryDegraded, degradedProblems); err != nil { + return err + } + return c.manageNotification(ctx, categoryTroubleshooting, troubleshootingProblems) +} + +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) +} + +func (c *consoleNotificationController) ensureNotification(ctx context.Context, cat notificationCategory, problems []string) error { + text := strings.Join(problems, ". ") + ". Check pacemaker status for details." + + u, err := buildNotificationUnstructured(cat, text) + if err != nil { + return err + } + + _, _, err = resourceapply.ApplyUnstructuredResourceImproved( + ctx, c.dynamicClient, c.recorder, u, nil, consoleNotificationGVR, nil, nil) + return c.filterConsoleError(err) +} + +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) +} + +func (c *consoleNotificationController) deleteAllNotifications(ctx context.Context) error { + for _, cat := range allCategories { + if err := c.deleteNotification(ctx, cat); err != nil { + return err + } + } + return nil +} + +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: cat.name, + 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: cat.linkHref, + Text: cat.linkText, + }, + }, + } + + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(notification) + if err != nil { + return nil, fmt.Errorf("failed to convert ConsoleNotification to unstructured: %w", err) + } + return &unstructured.Unstructured{Object: obj}, 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..1fe72418e --- /dev/null +++ b/pkg/tnf/pkg/pacemaker/consolenotification_test.go @@ -0,0 +1,429 @@ +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" +) + +// --------------------------------------------------------------------------- +// 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) + } +} + +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 +} + +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 +} + +// --------------------------------------------------------------------------- +// BuildHealthStatusFromCR tests (validates shared health evaluation) +// --------------------------------------------------------------------------- + +func TestBuildHealthStatusFromCR_ConsoleScenarios(t *testing.T) { + tests := []struct { + name string + cr *pacmkrv1.PacemakerCluster + wantProblems int + wantSubstrings []string + }{ + { + 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: 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: testCluster(withNodes( + testNode("master-0", "192.168.111.20", offline(), fencingUnavailable(), unhealthyEtcd()), + )), + wantProblems: 1, + wantSubstrings: []string{"offline"}, + }, + { + name: "unhealthy etcd resource detected", + 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: 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: 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 reports error", + cr: testCluster(), + wantProblems: 1, + wantSubstrings: []string{"No nodes found"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + 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 { + 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_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) + + 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 ca78e097d..13422d40a 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 @@ -275,78 +273,37 @@ func (c *HealthCheck) getPacemakerStatus(ctx context.Context) (*HealthStatus, *H c.crNodes = nil 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 + return unknownHealthStatus("Failed to convert cached item to PacemakerCluster"), previous, nil } c.crNodes = pacemakerCR.Status.Nodes - // 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) + // 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() @@ -355,36 +312,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 @@ -393,34 +358,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: @@ -440,15 +400,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 { @@ -459,58 +419,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)) } } @@ -518,11 +459,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) @@ -559,7 +498,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 { @@ -578,50 +517,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" } @@ -640,16 +563,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. @@ -852,12 +775,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: @@ -883,10 +806,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{}, },