pkg/tnf/pacemaker/healthcheck.go (3)

71-75: Avoid event spam: emit “Healthy” only on transitions.

At 30s resync, always emitting a Healthy event will generate noisy duplicates. Track last status and only emit on transition to Healthy.

 type HealthCheck struct {
   operatorClient v1helpers.StaticPodOperatorClient
   kubeClient     kubernetes.Interface
   eventRecorder  events.Recorder
+  lastOverallStatus string
 }
@@
-  // Record a normal event for healthy status
-  if status.OverallStatus == StatusHealthy {
-    c.eventRecorder.Eventf(EventReasonHealthy, "Pacemaker cluster is healthy - all nodes online and critical resources started")
-  }
+  // Record a normal event for healthy status only on transition
+  if status.OverallStatus == StatusHealthy && c.lastOverallStatus != StatusHealthy {
+    c.eventRecorder.Eventf(EventReasonHealthy, "Pacemaker cluster is healthy - all nodes online and critical resources started")
+  }
+  c.lastOverallStatus = status.OverallStatus

Also applies to: 446-450

---

347-356: Micro: precompile timestamp regex in isWithinTimeWindow.

Avoid recompiling on each iteration.

 func (c *HealthCheck) isWithinTimeWindow(line string, cutoffTime time.Time) bool {
   timeFormats := []string{
     "Mon Jan 2 15:04:05 2006",
     "2006-01-02 15:04:05.000000Z",
     "2006-01-02 15:04:05Z",
   }
 
-  for _, format := range timeFormats {
-    re := regexp.MustCompile(`'([^']+)'`)
-    matches := re.FindStringSubmatch(line)
+  tsRe := regexp.MustCompile(`'([^']+)'`)
+  for _, format := range timeFormats {
+    matches := tsRe.FindStringSubmatch(line)
     if len(matches) > 1 {
       if t, err := time.Parse(format, matches[1]); err == nil {
         return t.After(cutoffTime)
       }
     }
   }

---

71-75: Unused dependency: kubeClient is stored but never used.

Drop it from the struct/constructor or put it to use (e.g., namespacing events, future lookups). Keeping it increases surface without value.

 type HealthCheck struct {
   operatorClient v1helpers.StaticPodOperatorClient
-  kubeClient     kubernetes.Interface
   eventRecorder  events.Recorder
   lastOverallStatus string
 }
@@
 func NewHealthCheck(
   livenessChecker *health.MultiAlivenessChecker,
   operatorClient v1helpers.StaticPodOperatorClient,
-  kubeClient kubernetes.Interface,
   eventRecorder events.Recorder,
 ) factory.Controller {
   c := &HealthCheck{
     operatorClient: operatorClient,
-    kubeClient:     kubeClient,
     eventRecorder:  eventRecorder,
   }
@@
-    operatorClient,
-    kubeClient,
+    operatorClient,
     eventRecorder,
 )

If this is intentionally reserved for future use, ignore and mark with a TODO comment to avoid churn.

Also applies to: 80-84

pkg/tnf/pacemaker/healthcheck_test.go (4)

106-137: Assert condition transitions, not just error absence.

Validate that degraded/healthy actually update the operator condition in the fake client.

   err := controller.updateOperatorAvailability(ctx, status)
   require.NoError(t, err, "updateOperatorAvailability should not return an error")
+  // Verify degraded condition set
+  _, st, _, _ := operatorClient.GetStaticPodOperatorState()
+  require.Condition(t, func() (success bool) {
+    for _, c := range st.Conditions {
+      if c.Type == ConditionTypeDegraded && c.Status == operatorv1.ConditionTrue {
+        return true
+      }
+    }
+    return false
+  }, "expected degraded condition to be true")
@@
   err = controller.updateOperatorAvailability(ctx, status)
   require.NoError(t, err, "updateOperatorAvailability should not return an error")
+  // Verify degraded condition cleared
+  _, st, _, _ = operatorClient.GetStaticPodOperatorState()
+  require.Condition(t, func() (success bool) {
+    for _, c := range st.Conditions {
+      if c.Type == ConditionTypeDegraded && c.Status == operatorv1.ConditionFalse {
+        return true
+      }
+    }
+    return false
+  }, "expected degraded condition to be false")

---

163-184: Validate emitted events.

Use the in-memory recorder to assert reasons were produced (failed action, fencing, generic warning, error, healthy transition).

   controller.recordHealthCheckEvents(status)
-  
+  // Collect and assert events for warnings and error
+  evts := eventRecorder.Events()
+  require.NotEmpty(t, evts)
+  // Sanity: at least one of each warning type and an error
+  require.Contains(t, reasons(evts), EventReasonFailedAction)
+  require.Contains(t, reasons(evts), EventReasonFencingEvent)
+  require.Contains(t, reasons(evts), EventReasonWarning)
+  require.Contains(t, reasons(evts), EventReasonError)
@@
   status = &HealthStatus{
     OverallStatus: StatusHealthy,
     Warnings:      []string{},
     Errors:        []string{},
   }
 
   controller.recordHealthCheckEvents(status)
+  // After transition to healthy, expect a healthy event
+  evts = eventRecorder.Events()
+  require.Contains(t, reasons(evts), EventReasonHealthy)
+
+}
+
+// helper
+func reasons(evts []events.Event) []string {
+  out := make([]string, 0, len(evts))
+  for _, e := range evts {
+    out = append(out, e.Reason)
+  }
+  return out
 }

---

261-291: Add a test for “standby” node to ensure it’s a Warning, not Degraded.

Guards the updated parsing logic and aligns with the stated degradation policy.

   // Test with offline node
@@
   require.NotEmpty(t, status.Errors, "Should have errors for offline nodes")
+
+  // Test with standby node -> warning, but still considered online
+  lines = []string{
+    SectionNodeList,
+    "  * Node master-0 (1): online, feature set 3.19.6",
+    "  * Node master-1 (2): standby, feature set 3.19.6",
+    SectionFullListResources,
+  }
+  status = &HealthStatus{Warnings: []string{}, Errors: []string{}}
+  allOnline = controller.parseNodeStatus(lines, status)
+  require.True(t, allOnline, "Standby should not be treated as offline")
+  require.NotEmpty(t, status.Warnings, "Standby should surface as a warning")

---

296-313: Cover dynamic node-count logic.

Add a case where only one node has resources started while two nodes are listed; expect an error message reflecting 1/2.

   // Test with all resources started
@@
   require.Empty(t, status.Errors, "Should have no errors for started resources")
+
+  // Test with partial placement (only one node started)
+  lines = []string{
+    SectionNodeList,
+    "  * Node master-0 (1): online, feature set 3.19.6",
+    "  * Node master-1 (2): online, feature set 3.19.6",
+    SectionFullListResources,
+    "  * Clone Set: kubelet-clone [kubelet]:",
+    "    * kubelet   (systemd:kubelet):   Started master-0",
+    "  * Clone Set: etcd-clone [etcd]:",
+    "    * etcd (ocf:heartbeat:podman-etcd):     Started master-0",
+    SectionNodeAttributes,
+  }
+  status = &HealthStatus{Warnings: []string{}, Errors: []string{}}
+  allStarted = controller.parseResourceStatus(lines, status)
+  require.False(t, allStarted, "Resources not started on all nodes should be false")
+  require.NotEmpty(t, status.Errors)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (1)

502-510: Degrade on StatusError as well.

If we fail to collect status (OverallStatus == StatusError), the operator condition remains untouched, so outages never surface. Treat StatusError the same as StatusDegraded when updating conditions.

Suggested adjustment:

-	if status.OverallStatus == StatusDegraded {
-		return c.setDegradedCondition(ctx, status)
-	} else if status.OverallStatus == StatusHealthy {
+	switch status.OverallStatus {
+	case StatusDegraded, StatusError:
+		return c.setDegradedCondition(ctx, status)
+	case StatusHealthy:
 		return c.clearDegradedCondition(ctx)
 	}
-
-	// For Warning or Error status, we don't update the degraded condition
-	return nil
+	// Warning / Unknown: leave condition unchanged
+	return nil

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (3)

3-9: Prevent hangs; add exec timeout and non-interactive sudo (-n). Also introduce StatusError for collection failures.

pcs can block (sudo prompt/hung pcs). Use a short timeout context and sudo -n. While here, add StatusError and treat exec failures accordingly.

Apply:

--- a/pkg/tnf/pacemaker/healthcheck.go
+++ b/pkg/tnf/pacemaker/healthcheck.go
@@
 import (
+	"errors"
 	"context"
 	"encoding/xml"
 	"fmt"
 	"strings"
 	"time"
@@
 const (
   // Time windows for detecting recent events
   FailedActionTimeWindow = 5 * time.Minute
   FencingEventTimeWindow = 24 * time.Hour
+
+  // Upper bound for executing pcs commands
+  ExecTimeout = 10 * time.Second
@@
   // Status strings
   StatusHealthy  = "Healthy"
   StatusWarning  = "Warning"
   StatusDegraded = "Degraded"
   StatusUnknown  = "Unknown"
+  StatusError    = "Error"
@@
   // Pacemaker commands
-  PcsStatusXMLCommand = "sudo pcs status xml"
+  PcsStatusXMLCommand = "sudo -n pcs status xml"
@@
-  // Execute the pcs status xml command
-  stdout, stderr, err := exec.Execute(ctx, PcsStatusXMLCommand)
+  // Execute the pcs status xml command with timeout and non-interactive sudo
+  ctxExec, cancel := context.WithTimeout(ctx, ExecTimeout)
+  defer cancel()
+  stdout, stderr, err := exec.Execute(ctxExec, PcsStatusXMLCommand)
   if err != nil {
-    return newErrorHealthStatus(fmt.Sprintf("Failed to execute pcs status xml command: %v", err)), nil
+    if errors.Is(err, context.DeadlineExceeded) {
+      err = fmt.Errorf("pcs status xml command timed out: %w", err)
+    }
+    return newErrorHealthStatus(fmt.Sprintf("Failed to execute pcs status xml command: %v", err)), nil
   }

Also applies to: 21-84, 336-345

---

379-383: Set OverallStatus when pacemaker isn’t running (avoid returning Unknown).

Without setting OverallStatus here, outages can be masked and the degraded condition won’t be set.

   if result.Summary.Stack.PacemakerdState != PacemakerStateRunning {
     status.Errors = append(status.Errors, "Pacemaker is not running")
-    return status, nil
+    status.OverallStatus = StatusDegraded
+    return status, nil
   }

---

568-576: Degrade on StatusError as well (not only Degraded).

Collection failures should surface as degraded to operators/users.

 switch status.OverallStatus {
- case StatusDegraded:
+ case StatusDegraded, StatusError:
   return c.setDegradedCondition(ctx, status)
 case StatusHealthy:
   return c.clearDegradedCondition(ctx)
 default:
   // For Warning or Unknown status, we don't update the degraded condition
   return nil
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (3)

386-390: Set OverallStatus to StatusDegraded before returning.

When Pacemaker isn't running, the code appends an error but returns with OverallStatus still set to StatusUnknown (from initialization at line 370). This prevents the degraded condition from being set downstream, masking the outage.

Apply this diff:

 	// Check if pacemaker is running
 	if result.Summary.Stack.PacemakerdState != PacemakerStateRunning {
 		status.Errors = append(status.Errors, "Pacemaker is not running")
+		status.OverallStatus = StatusDegraded
 		return status, nil
 	}

---

567-575: Handle StatusError in the switch statement.

If StatusError is added (per earlier comment), the updateOperatorStatus switch should treat it like StatusDegraded and set the degraded condition.

Apply this diff:

 	// Update operator conditions based on pacemaker status
 	switch status.OverallStatus {
-	case StatusDegraded:
+	case StatusDegraded, StatusError:
 		return c.setDegradedCondition(ctx, status)
 	case StatusHealthy:
 		return c.clearDegradedCondition(ctx)
 	default:
-		// For Warning or Unknown status, we don't update the degraded condition
+		// For Warning or Unknown status, we don't update the degraded condition
 		return nil
 	}

---

267-274: Rename to newErrorHealthStatus and use StatusError instead of StatusUnknown.

Collection failures (exec timeout, XML parse errors, etc.) should be marked as StatusError rather than StatusUnknown so they're surfaced as actionable failures in the operator condition. This aligns with previous feedback.

Apply this diff:

-// newUnknownHealthStatus creates a HealthStatus with Unknown status and an error message
-func newUnknownHealthStatus(errMsg string) *HealthStatus {
+// newErrorHealthStatus creates a HealthStatus with Error status and an error message
+func newErrorHealthStatus(errMsg string) *HealthStatus {
 	return &HealthStatus{
-		OverallStatus: StatusUnknown,
+		OverallStatus: StatusError,
 		Warnings:      []string{},
 		Errors:        []string{errMsg},
 	}
 }

Then update all call sites (lines 343, 346, 355, 361) to use newErrorHealthStatus instead of newUnknownHealthStatus.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (8)

267-274: Duplicate: Mark collection failures as StatusError.

Per past review, this function should return StatusError instead of StatusUnknown to properly distinguish collection failures from unknown states. Collection failures are actionable and should be surfaced as errors.

Apply this diff:

 func newUnknownHealthStatus(errMsg string) *HealthStatus {
   return &HealthStatus{
-    OverallStatus: StatusUnknown,
+    OverallStatus: StatusError,
     Warnings:      []string{},
     Errors:        []string{errMsg},
   }
 }

Note: This requires adding the StatusError constant to the status constants section (lines 34-38).

---

34-38: Add StatusError constant for collection failures.

To properly handle collection/communication failures distinct from operational issues, add a StatusError constant alongside the existing status constants.

Apply this diff:

   // Status strings
   StatusHealthy  = "Healthy"
   StatusWarning  = "Warning"
   StatusDegraded = "Degraded"
+  StatusError    = "Error"
   StatusUnknown  = "Unknown"

---

429-433: Distinguish offline from standby nodes.

The current check treats any non-online node as an error, including legitimate standby states. Per the PR objectives, only offline nodes should trigger degradation.

Apply this diff:

   for _, node := range result.Nodes.Node {
-    if node.Online != NodeStatusOnline {
-      status.Errors = append(status.Errors, fmt.Sprintf("Node %s is not online", node.Name))
+    if node.Online == "false" {
+      status.Errors = append(status.Errors, fmt.Sprintf("Node %s is offline", node.Name))
+    } else if node.Standby == BooleanValueTrue {
+      status.Warnings = append(status.Warnings, fmt.Sprintf("Node %s is in standby", node.Name))
     }
   }

---

387-390: Set OverallStatus when pacemaker isn't running.

When pacemaker is down, the function returns with OverallStatus still set to Unknown. This prevents the degraded condition from being set, masking the outage.

Apply this diff:

   if result.Summary.Stack.PacemakerdState != PacemakerStateRunning {
     status.Errors = append(status.Errors, "Pacemaker is not running")
+    status.OverallStatus = StatusDegraded
     return status, nil
   }

---

497-509: Fix failure detection logic: use OR instead of AND.

The condition requires both RC and RCText to indicate failure, which misses cases where only one field is non-success. Use OR logic to catch all failure cases.

Apply this diff:

         for _, operation := range resourceHistory.OperationHistory {
           // Check for failed operations (rc != "0")
-          if operation.RC != OperationRCSuccess && operation.RCText != OperationRCTextSuccess {
+          if operation.RC != OperationRCSuccess || operation.RCText != OperationRCTextSuccess {
             // Parse the timestamp
             t, err := time.Parse(PacemakerTimeFormat, operation.LastRCChange)

---

567-575: Handle StatusError in operator status update.

When status collection fails (StatusError), the operator should surface a degraded condition to avoid masking outages. Currently only StatusDegraded triggers the degraded condition.

Apply this diff:

   // Update operator conditions based on pacemaker status
   switch status.OverallStatus {
-  case StatusDegraded:
+  case StatusDegraded, StatusError:
     return c.setDegradedCondition(ctx, status)
   case StatusHealthy:
     return c.clearDegradedCondition(ctx)
   default:
-    // For Warning or Unknown status, we don't update the degraded condition
+    // For Warning or Unknown status, we don't update the degraded condition
     return nil
   }

---

469-487: Derive expected node count dynamically from parsed XML.

Replace the hard-coded ExpectedNodeCount constant with a dynamic count derived from the parsed node list. This makes the validation flexible for clusters of varying sizes.

Apply this diff:

   // Check if we have both resources started on all nodes
+  expectedCount := len(result.Nodes.Node)
   kubeletCount := len(resourcesStarted[ResourceKubelet])
   etcdCount := len(resourcesStarted[ResourceEtcd])
 
-  kubeletOK := c.validateResourceCount(ResourceKubelet, kubeletCount, status)
-  etcdOK := c.validateResourceCount(ResourceEtcd, etcdCount, status)
+  kubeletOK := c.validateResourceCount(ResourceKubelet, kubeletCount, expectedCount, status)
+  etcdOK := c.validateResourceCount(ResourceEtcd, etcdCount, expectedCount, status)
 
   return kubeletOK && etcdOK
 }
 
 // validateResourceCount validates that a resource is started on the expected number of nodes
-func (c *HealthCheck) validateResourceCount(resourceName string, actualCount int, status *HealthStatus) bool {
-  if actualCount < ExpectedNodeCount {
+func (c *HealthCheck) validateResourceCount(resourceName string, actualCount, expectedCount int, status *HealthStatus) bool {
+  if actualCount < expectedCount {
     status.Errors = append(status.Errors, fmt.Sprintf("%s resource not started on all nodes (started on %d/%d nodes)",
-      resourceName, actualCount, ExpectedNodeCount))
+      resourceName, actualCount, expectedCount))
     return false
   }
   return true

---

423-427: Remove hard-coded node count check.

The hard-coded ExpectedNodeCount = 2 check is inflexible for clusters that may scale or undergo node replacement. Since the subsequent validation already uses the actual node count from the XML, this strict equality check can be removed.

Apply this diff:

-  // Check if we have the expected number of nodes.
-  // This should almost always be 2, but 1 node is possible during a control-plane node replacement event.
-  if len(result.Nodes.Node) != ExpectedNodeCount {
-    status.Warnings = append(status.Warnings, fmt.Sprintf("Expected %d nodes, found %d", ExpectedNodeCount, len(result.Nodes.Node)))
-  }
-

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (1)

386-390: Set OverallStatus before returning when pacemaker is not running.

When pacemaker is not running, the function appends an error and returns immediately (line 389), but OverallStatus remains Unknown (initialized at line 370). This prevents the degraded condition from being set properly since updateOperatorStatus (line 567-575) only sets degraded for StatusError, not StatusUnknown.

Apply this diff:

 	if result.Summary.Stack.PacemakerdState != PacemakerStateRunning {
 		status.Errors = append(status.Errors, "Pacemaker is not running")
+		status.OverallStatus = c.determineOverallStatus(status)
 		return status, nil
 	}

pkg/tnf/pacemaker/healthcheck_test.go (1)

102-113: Clarify test expectation: exec failure should not return an error.

The comment states "exec.Execute will fail due to permissions" but the test expects NoError. This appears correct (the function returns Unknown status without propagating the error), but the comment could be clearer about this design choice.

Consider updating the comment to:

-  // In test environment, exec.Execute will fail due to permissions
-  require.NoError(t, err, "getPacemakerStatus should not return an error")
+  // In test environment, exec.Execute will fail due to permissions.
+  // getPacemakerStatus handles this gracefully by returning Unknown status without propagating the error.
+  require.NoError(t, err, "getPacemakerStatus should not return an error even when exec fails")

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (1)

267-274: Collection failures return Unknown; confirm policy.

Exec/parse failures set StatusUnknown, so no degraded condition is set. If outages should surface, consider marking as StatusError to degrade. If Unknown is intentional, ignore.

Also applies to: 341-347

pkg/tnf/pacemaker/testdata/healthy_cluster.xml (1)

79-82: Remove duplicated operation_history entry.

Line 80 duplicates line 79 for the kubelet probe. Drop one to avoid confusion in tests and future parsing.

-        <operation_history call="330" task="probe" rc="0" rc_text="ok" last-rc-change="Thu Sep 18 12:40:35 2025" exec-time="4ms" queue-time="0ms"/>

pkg/tnf/pacemaker/healthcheck_test.go (1)

115-149: Optional: rename test for consistency with impl name.

The function under test is updateOperatorStatus; consider renaming the test to match for easier discovery.

-func TestHealthCheck_updateOperatorAvailability(t *testing.T) {
+func TestHealthCheck_updateOperatorStatus(t *testing.T) {

pkg/tnf/pacemaker/healthcheck.go (4)

338-341: Extract exec timeout into a constant.

Avoid magic number; define a reusable ExecTimeout alongside other constants and use it here.

@@
- // Execute the pcs status xml command with a timeout and non-interactive sudo
- ctxExec, cancel := context.WithTimeout(ctx, 10*time.Second)
+ // Execute the pcs status xml command with a timeout and non-interactive sudo
+ ctxExec, cancel := context.WithTimeout(ctx, ExecTimeout)

Add near other constants:

+ // Upper bound for "pcs status" execution
+ ExecTimeout = 10 * time.Second

---

430-434: If standby is considered “offline”, handle it explicitly.

Currently only Online != "true" is flagged. To align with the stated intent to treat standby as offline, also error on Standby == "true".

 for _, node := range result.Nodes.Node {
-    if node.Online != NodeStatusOnline {
-        status.Errors = append(status.Errors, fmt.Sprintf("Node %s is not online", node.Name))
-    }
+    if node.Online != NodeStatusOnline {
+        status.Errors = append(status.Errors, fmt.Sprintf("Node %s is not online", node.Name))
+    } else if node.Standby == BooleanValueTrue {
+        status.Errors = append(status.Errors, fmt.Sprintf("Node %s is in standby (treated as offline)", node.Name))
+    }
 }

---

449-453: Match resources by agent, not ID substring.

ID-based substring matching is brittle. Use ResourceAgent to detect kubelet/etcd reliably.

-            // Check if this is a kubelet or etcd resource
-            if strings.Contains(resource.ID, ResourceKubelet) {
-                resourcesStarted[ResourceKubelet][nodeName] = true
-            } else if strings.Contains(resource.ID, ResourceEtcd) {
-                resourcesStarted[ResourceEtcd][nodeName] = true
-            }
+            // Identify kubelet/etcd resources by their agents
+            switch {
+            case strings.HasPrefix(resource.ResourceAgent, "systemd:kubelet"):
+                resourcesStarted[ResourceKubelet][nodeName] = true
+            case strings.HasPrefix(resource.ResourceAgent, "ocf:heartbeat:podman-etcd"):
+                resourcesStarted[ResourceEtcd][nodeName] = true
+            }

---

629-643: Optional: gate repeated warning events to reduce noise.

At 30s resync, warnings within the 5m/24h windows will re-emit every cycle. Consider deduplication (e.g., last-emitted cache, once-per-window) to avoid event spam.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (1)

388-401: Optional: size guard exists only in caller, not in parser.

parsePacemakerStatusXML trusts input size; only getPacemakerStatus enforces maxXMLSize. If parse is used elsewhere, consider early-checking length or using an xml.Decoder with a bounded reader to keep invariants local to the parser. Low priority.

pkg/tnf/pacemaker/healthcheck_test.go (3)

50-55: Use require.NoErrorf for formatted failure message.

Minor test ergonomics: this call passes a format string and arg; use the “f” variant.

-	data, err := os.ReadFile(path)
-	require.NoError(t, err, "Failed to read test XML file: %s", filename)
+	data, err := os.ReadFile(path)
+	require.NoErrorf(t, err, "Failed to read test XML file: %s", filename)

Based on learnings

---

642-659: Test name/comment mismatch with behavior.

The test name/comment suggest exercising “large XML” behavior, but the parser test simply validates invalid XML. Either update the comment/name to reflect invalid XML, or drive the size check via getPacemakerStatus (which enforces maxXMLSize). Optional.

---

484-498: Add a test to assert standby is treated as offline.

To lock in the intended behavior, add a case where node.Online="true" and Standby="true" and assert an error is recorded.

Example:

func TestHealthCheck_collectNodeStatus_StandbyTreatedAsOffline(t *testing.T) {
	controller := &HealthCheck{}
	result := &PacemakerResult{
		Nodes: Nodes{
			Node: []Node{
				{Name: "master-0", Online: booleanValueTrue, Standby: booleanValueTrue},
				{Name: "master-1", Online: booleanValueTrue, Standby: "false"},
			},
		},
	}
	status := &HealthStatus{Warnings: []string{}, Errors: []string{}}
	controller.collectNodeStatus(result, status)
	require.NotEmpty(t, status.Errors)
	require.Contains(t, strings.Join(status.Errors, " "), "standby")
}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pacemaker/healthcheck.go (3)

524-528: Timestamp parsing errors are logged but not surfaced.

When time.Parse fails, the error is logged at V(4) level and the operation is skipped with continue. If all timestamps consistently fail to parse (e.g., due to Pacemaker version change in timestamp format), recent failures would be silently ignored.

Consider accumulating a count of parse errors and emitting a warning if a threshold is exceeded, to detect systematic parsing issues.

---

640-665: Move cleanup logic outside the event-checking loop for efficiency.

The cleanup of old deduplication entries (lines 647-653) currently runs on every call to shouldRecordEvent, which means it executes once per warning/error. This is inefficient when processing multiple events.

Based on learnings

Apply this refactor to clean up once per sync:

 // recordHealthCheckEvents records events for pacemaker warnings and fencing history
 func (c *HealthCheck) recordHealthCheckEvents(status *HealthStatus) {
+	// Clean up old entries once at the start
+	c.cleanupOldRecordedEvents()
+
 	// Record events for warnings with appropriate deduplication window
 	for _, warning := range status.Warnings {
 		eventKey := fmt.Sprintf("warning:%s", warning)
@@ -696,6 +700,19 @@
 	}
 }
 
+// cleanupOldRecordedEvents removes entries older than the longest deduplication window
+func (c *HealthCheck) cleanupOldRecordedEvents() {
+	c.recordedEventsMu.Lock()
+	defer c.recordedEventsMu.Unlock()
+	
+	now := time.Now()
+	maxWindow := eventDeduplicationWindowFencing
+	for key, timestamp := range c.recordedEvents {
+		if now.Sub(timestamp) > maxWindow {
+			delete(c.recordedEvents, key)
+		}
+	}
+}
+
 // shouldRecordEvent checks if an event should be recorded based on deduplication logic
 func (c *HealthCheck) shouldRecordEvent(eventKey string, deduplicationWindow time.Duration) bool {
 	c.recordedEventsMu.Lock()
 	defer c.recordedEventsMu.Unlock()
 
 	now := time.Now()
-
-	// Clean up old entries that are outside the longest deduplication window
-	maxWindow := eventDeduplicationWindowFencing
-	for key, timestamp := range c.recordedEvents {
-		if now.Sub(timestamp) > maxWindow {
-			delete(c.recordedEvents, key)
-		}
-	}
 
 	// Check if this event was recently recorded
 	if lastRecorded, exists := c.recordedEvents[eventKey]; exists {

---

619-626: Verify degraded condition ownership before clearing.

The clearDegradedCondition function clears any degraded condition of type PacemakerHealthCheckDegraded without verifying it was set by this controller. If multiple components manage similar conditions, this could inadvertently clear a condition set by another component.

Consider checking the condition's reason or message to confirm it was set by this health check before clearing it.

Based on learnings

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

pkg/tnf/pkg/jobs/cronjob_controller.go (1)

167-175: Prefer structured mutation over text templating (reduces brittleness).

Parse the manifest once, then set fields on the CronJob object (name, namespace, schedule, image, command) instead of line‑based replacements. This avoids YAML formatting pitfalls and future template drift.

Example:

cronJob := ReadCronJobV1OrDie(c.manifest)
cronJob.Name = c.name
cronJob.Namespace = c.namespace
cronJob.Spec.Schedule = c.schedule
if len(cronJob.Spec.JobTemplate.Spec.Template.Spec.Containers) > 0 {
  ct := &cronJob.Spec.JobTemplate.Spec.Template.Spec.Containers[0]
  ct.Image = c.image
  ct.Command = append([]string(nil), c.command...)
}
return cronJob

pkg/tnf/pacemaker/healthcheck.go (2)

306-309: Remove unused lastProcessedStatus or use it meaningfully.

State is written but never read; drop it or leverage it to suppress duplicate event processing based on CR changes.

Also applies to: 433-436

---

515-523: Optional: clarify standby-as-offline in a code comment.

Behavior is intentional; a brief inline comment above the loop will aid future readers.

pkg/cmd/pacemakerstatuscollector/collector.go (1)

165-177: Remove unused Kubernetes client wiring to simplify.

You construct a client but don’t use it; updatePacemakerStatusCR rebuilds REST config anyway. Simplify by dropping the client parameter and the extra creation.

Apply:

@@
-  // Create Kubernetes client
-  client, err := createKubernetesClient()
-  if err != nil {
-    return fmt.Errorf("failed to create kubernetes client: %w", err)
-  }
-
   // Update PacemakerStatus CR
-  if err := updatePacemakerStatusCR(ctx, client, rawXML, summary, collectionError); err != nil {
+  if err := updatePacemakerStatusCR(ctx, rawXML, summary, collectionError); err != nil {
     return fmt.Errorf("failed to update PacemakerStatus CR: %w", err)
   }

And change the function signature:

-func updatePacemakerStatusCR(ctx context.Context, client *kubernetes.Clientset, rawXML string, summary v1alpha1.PacemakerSummary, collectionError string) error {
+func updatePacemakerStatusCR(ctx context.Context, rawXML string, summary v1alpha1.PacemakerSummary, collectionError string) error {

Optionally remove createKubernetesClient if unused elsewhere.

Also applies to: 321-321

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting