OCPEDGE-2094: Add console notification for pacemaker podman-etcd failures#1654
OCPEDGE-2094: Add console notification for pacemaker podman-etcd failures#1654lucaconsalvi wants to merge 3 commits into
Conversation
…ures Surface pacemaker health problems directly in the OpenShift web console via a ConsoleNotification banner. When etcd resources fail, fencing becomes unavailable, or pacemaker status goes stale, a red banner appears linking to the troubleshooting guide. The banner is removed automatically when health is restored. New controller shares the existing PacemakerCluster informer with the healthcheck and metrics controllers, following the same registration pattern in runPacemakerControllers(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@lucaconsalvi: This pull request references OCPEDGE-2094 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughChangesPacemaker controller startup now includes a console notification controller. It evaluates Pacemaker console notification flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PacemakerInformer
participant ConsoleNotificationController
participant DynamicClient
PacemakerInformer->>ConsoleNotificationController: Observe PacemakerCluster
ConsoleNotificationController->>ConsoleNotificationController: Evaluate health
ConsoleNotificationController->>DynamicClient: Create or update ConsoleNotification
ConsoleNotificationController->>DynamicClient: Delete notification when healthy
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/tnf/pkg/pacemaker/consolenotification_test.go (1)
38-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd tests for the ConsoleNotification lifecycle.
Health evaluation is covered, but creation, deletion, update drift, conflict/error handling, and preservation of unmanaged metadata are not. Exercise
syncwith a fake dynamic client before relying on this controller in production.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/tnf/pkg/pacemaker/consolenotification_test.go` around lines 38 - 196, Add lifecycle-focused tests for the ConsoleNotification controller’s sync flow, using a fake dynamic client to cover creation, deletion, update drift, resource conflicts/errors, and preservation of unmanaged metadata. Keep the existing evaluateHealth tests unchanged, and assert the resulting resource state and propagated handling through the relevant sync/controller methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/tnf/pkg/pacemaker/consolenotification_test.go`:
- Around line 104-120: Update the “offline node skips resource and fencing
checks” test setup around makeNodeWithConditions to explicitly set the node’s
Etcd Healthy condition to False before evaluation, while preserving the offline
and fencing-unavailable conditions and the expected offline-only notification.
In `@pkg/tnf/pkg/pacemaker/consolenotification.go`:
- Line 180: Update the logging statements in the console notification creation
and related paths around klog.Infof so they do not include the notification
text; instead log the operation and notification problem count, avoiding
exposure of node hostnames while retaining useful summary information.
- Around line 102-112: Update the Pacemaker status checks around
getConditionStatus and the cr.Status.Nodes guard so uninitialized status is
reported as unhealthy: flag zero LastUpdated, treat missing or
metav1.ConditionUnknown conditions as problems, and report nil or empty Nodes
according to the CRD’s catastrophic-state guidance. Preserve existing stale,
maintenance, and healthy-status behavior, and add coverage for uninitialized and
Unknown statuses.
- Around line 187-195: Update the reconciliation logic around existingText and
the ConsoleNotification Update call to compare and reconcile every
controller-owned field, including backgroundColor, color, link, location, and
text. Mutate a copy of existing rather than updating u directly, preserve its
annotations, finalizers, labels, and other metadata, set the desired owned
fields on the copy, retain the existing resource version, and submit that copy
through the dynamic client.
---
Nitpick comments:
In `@pkg/tnf/pkg/pacemaker/consolenotification_test.go`:
- Around line 38-196: Add lifecycle-focused tests for the ConsoleNotification
controller’s sync flow, using a fake dynamic client to cover creation, deletion,
update drift, resource conflicts/errors, and preservation of unmanaged metadata.
Keep the existing evaluateHealth tests unchanged, and assert the resulting
resource state and propagated handling through the relevant sync/controller
methods.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d68afcfd-0760-4e1e-83e7-699379c44f13
📒 Files selected for processing (3)
pkg/tnf/operator/starter.gopkg/tnf/pkg/pacemaker/consolenotification.gopkg/tnf/pkg/pacemaker/consolenotification_test.go
Improves readability of multi-problem notifications by joining problems with ". " instead of "; ". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The "offline node skips resource and fencing checks" test claimed etcd was unhealthy but makeNodeWithConditions creates healthy resources. Add withUnhealthyResource helper and use it so the test genuinely verifies that offline nodes skip resource checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Manual Testing on TNF ClusterTested the console notification on a live TNF baremetal cluster deployed via cluster-bot with a custom CEO image built from this PR. Cluster: Test 1 — Banner appears on pacemaker failureTriggered maintenance mode via Red banner visible at the top of the OpenShift web console with the "Troubleshooting guide" link. Test 2 — Banner disappears when health is restoredDisabled maintenance mode via PacemakerCluster CR returned to ResultPASS — Full lifecycle verified: banner creation, content accuracy, troubleshooting link, and automatic cleanup on health restoration.
|
|
@lucaconsalvi: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |


Summary
ConsoleNotificationControllerthat watches thePacemakerClusterCR and manages aConsoleNotificationbanner in the OpenShift web console when pacemaker health problems are detectedPacemakerClusterinformer with healthcheck and metrics controllers, registered alongside them inrunPacemakerControllers()Files Changed
pkg/tnf/pkg/pacemaker/consolenotification.gopkg/tnf/pkg/pacemaker/consolenotification_test.gopkg/tnf/operator/starter.gorunPacemakerControllers()Test plan
Jira: https://issues.redhat.com/browse/OCPEDGE-2094
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests