feat: add 'make status' unified dashboard#94
Open
fonta-rh wants to merge 7 commits into
Open
Conversation
Implements the plan in docs/plans/02-make-status.md: a single command answering "what do I have running right now", with per-section graceful degradation so it is useful at any deployment stage. - Add deploy/openshift-clusters/scripts/status.sh with five sections: local config, EC2 instance (absorbs make info output plus live state and uptime), cluster state file, hypervisor probes bundled into one SSH round trip (dev-scripts dir, VM states, squid proxy, BMC simulator for fencing), and a cluster API probe via proxy.env. - Every network operation has an explicit timeout; failing probes WARN and the script continues instead of aborting. - Make 'info' an alias of 'status' and remove print_instance_data.sh, whose output is covered by the EC2 instance section. - Update README/CLAUDE.md command references and point the troubleshooting docs at 'make status' first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013T7qRUL2UGSKn5Si9ApYED
common.sh was relocated to deploy/ in an earlier commit; update the source path in status.sh to match. Remove docs/plans/02-make-status.md which is an internal design artifact not intended for upstream. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Track copy errors separately from synced count so the Makefile target exits non-zero on failures instead of printing the misleading "everything up to date" message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fonta-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughA unified ChangesStatus dashboard and deployment workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Makefile
participant status.sh
participant AWS
participant RemoteHost
participant ClusterAPI
Operator->>Makefile: Run make status
Makefile->>status.sh: Execute status dashboard
status.sh->>AWS: Query EC2 instance state
status.sh->>RemoteHost: Check VMs and services over SSH
status.sh->>ClusterAPI: Query node readiness through proxy
status.sh-->>Operator: Display aggregated status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deploy/openshift-clusters/scripts/status.sh (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
#!/usr/bin/bashshebang.The repo shell-script standard mandates the
#!/usr/bin/bashshebang.Proposed change
-#!/bin/bash +#!/usr/bin/bashAs per coding guidelines: "
**/*.sh: Use the#!/usr/bin/bashshebang".🤖 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 `@deploy/openshift-clusters/scripts/status.sh` at line 1, Update the shebang at the beginning of the script to use /usr/bin/bash instead of /bin/bash, matching the repository standard for shell scripts.Source: Coding guidelines
🤖 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 `@deploy/Makefile`:
- Around line 72-76: Add a .PHONY declaration for both status and info near
their targets in the Makefile, ensuring these targets always execute their
associated commands regardless of same-named files or directories.
In `@deploy/README.md`:
- Around line 277-283: Fix the MD014 violation in the status-dashboard example
by removing the `$` shell prompt from the `make status` command, or add
representative command output beneath it; update the documentation block
accordingly.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/status.sh`:
- Line 1: Update the shebang at the beginning of the script to use /usr/bin/bash
instead of /bin/bash, matching the repository standard for shell scripts.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e0b45431-3355-4013-9534-685eb061ab72
📒 Files selected for processing (10)
CLAUDE.mdREADME.mddeploy/Makefiledeploy/README.mddeploy/aws-hypervisor/README.mddeploy/aws-hypervisor/scripts/force-stop.shdeploy/aws-hypervisor/scripts/print_instance_data.shdeploy/common.shdeploy/openshift-clusters/README.mddeploy/openshift-clusters/scripts/status.sh
💤 Files with no reviewable changes (1)
- deploy/aws-hypervisor/scripts/print_instance_data.sh
lucaconsalvi
left a comment
lucaconsalvi
left a comment
Contributor
There was a problem hiding this comment.
Thanks for putting this together, Pablo! The unified dashboard is a great improvement — the graceful degradation design is solid, the single SSH round trip is efficient, and the sync-config error fix is well-constructed. The doc updates are thorough and consistent.
A few findings from the review, mostly around 2>/dev/null patterns being a bit too aggressive in some spots. One critical item, three important ones, and a few suggestions inline.
Critical (1)
source common.sh 2>/dev/null masks real errors and the comment is inaccurate — status.sh:17
Suppressing all stderr from common.sh hides real errors (syntax errors, permission issues, broken instance.env). Combined with set -o nounset on line 20, if common.sh fails to source, the color variables (COLOR_BLUE, etc.) are undefined and the script crashes with a cryptic "unbound variable" error — hiding the actual cause.
The comment on line 14 says common.sh sources instance.env "unconditionally", but it's actually guarded by an -f check with a SKIP_INSTANCE_ENV_WARNING variable (common.sh:67-72).
Fix — use the existing purpose-built mechanism:
# common.sh warns to stderr when instance.env is missing; suppress only
# that warning since the local config section already reports its absence.
SKIP_INSTANCE_ENV_WARNING=1 source "${DEPLOY_DIR}/common.sh"Important (3)
See inline comments below.
Suggestions
oc get nodes 2>/dev/null(status.sh:275) collapses auth failures, DNS issues, and timeouts into one generic "unreachable through the proxy" message. At minimum, exit code 124 fromtimeoutis free info worth distinguishing.source "${PROXY_ENV}" >/dev/null 2>&1(status.sh:274) hides syntax errors inproxy.env, causingocto run without proxy vars and then blaming the proxy container.- Consider a one-line comment before
if [[ "${TOPOLOGY}" == "fencing" ]](status.sh:197) explaining why BMC is only checked for fencing (STONITH uses out-of-band power management; other topologies don't). - The
date -u -duptime calculation (status.sh:137) degrades correctly on macOS but a brief inline note would save future maintainers from debugging why uptime never appears.
Strengths
- Graceful degradation at every section — failures produce actionable messages, not crashes
- Single SSH round trip with clear
(single SSH round trip)documentation - sync-config error counter + Makefile propagation is correct end-to-end
"Intentionally no errexit"comment prevents future contributors from breaking the degradation designmake infoalias comment is concise and accurate
Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed CodeRabbit review findings:
|
openshift-ci
Bot
added
the
ready-for-human-review
dhensel-rh
reviewed
Jul 10, 2026
dhensel-rh
left a comment
dhensel-rh
left a comment
Contributor
There was a problem hiding this comment.
Minor: awk '$2 == "Ready"' (line ~282) is an exact match — a cordoned node with status Ready,SchedulingDisabled would be counted as "not Ready" and trigger the warning. Consider $2 ~ /^Ready/ if you want cordoned nodes to count as ready.
Not a blocker — unlikely in a two-node cluster, just a heads-up.
- Replace `source common.sh 2>/dev/null` with SKIP_INSTANCE_ENV_WARNING to avoid masking real source errors (lucaconsalvi) - Add pipefail to match doctor.sh shell options (lucaconsalvi) - Add sudo canary check in SSH probe to surface permission failures instead of silently reporting no VMs (lucaconsalvi) - Wrap SSH probe with `timeout 30` to prevent hang on stuck libvirtd (lucaconsalvi) - Use awk regex match for Ready status to correctly count cordoned nodes as ready (dhensel-rh) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Addressed review findings from lucaconsalvi and dhensel-rh in Applied:
Acknowledged (non-blocking suggestions):
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@deploy/openshift-clusters/scripts/status.sh`:
- Around line 288-296: Align the non-ready detail listing with the
classification used by READY_COUNT and NOT_READY_COUNT. In the node-reporting
awk command within the status-check logic, replace the exact `$2 != "Ready"`
test with the same /^Ready/ criterion so `Ready,SchedulingDisabled` nodes are
not incorrectly listed as not Ready.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ed81ca9e-7b12-43fd-9e74-ec1ecdf029e4
📒 Files selected for processing (10)
CLAUDE.mdREADME.mddeploy/Makefiledeploy/README.mddeploy/aws-hypervisor/README.mddeploy/aws-hypervisor/scripts/force-stop.shdeploy/aws-hypervisor/scripts/print_instance_data.shdeploy/common.shdeploy/openshift-clusters/README.mddeploy/openshift-clusters/scripts/status.sh
💤 Files with no reviewable changes (1)
- deploy/aws-hypervisor/scripts/print_instance_data.sh
GNU date uses -d for parsing; BSD date (macOS) uses -j -f. Try GNU first, fall back to BSD, degrade gracefully if neither works. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lucaconsalvi
left a comment
lucaconsalvi
left a comment
Contributor
There was a problem hiding this comment.
Thanks for addressing all the findings, Pablo! Verified the fixes in the latest commits:
| Finding | Status | Commit |
|---|---|---|
Critical: source common.sh 2>/dev/null + inaccurate comment |
Fixed — SKIP_INSTANCE_ENV_WARNING=1, comment rewritten |
1d2aead |
Important: Missing set -o pipefail |
Fixed — set -uo pipefail |
1d2aead |
| Important: SSH hides sudo failures | Fixed — sudo canary + SUDO_FAILED handler |
1d2aead |
| Important: No SSH timeout | Fixed — timeout 30 ssh ... |
1d2aead |
Suggestion: macOS date -d fallback |
Fixed — added BSD date -j -f chain |
6b0a031 |
| Suggestion: shellcheck directive | Fixed — # shellcheck source=/dev/null |
5f67a7c |
All fixes look correct. No new concerns from the follow-up commits.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
make statusas a unified dashboard that shows instance info, cluster VM state, proxy status, and cluster API health in one viewmake infointomake status(info becomes an alias)print_instance_data.sh— its output is now part of the status dashboardChanges
deploy/openshift-clusters/scripts/status.shdeploy/Makefilestatus/infotargets; fixsync-configerror handlingdeploy/common.shCONFIG_SYNC_ERRORSinsync_config_files()deploy/aws-hypervisor/scripts/print_instance_data.shdeploy/aws-hypervisor/scripts/force-stop.shREADME.md,deploy/README.md,deploy/*/README.mdTest plan
make statusshows instance info, cluster VMs, proxy, and API statusmake infoworks as alias formake statusmake sync-configexits non-zero whencpfailsmake deploystill runs sync-config prerequisite correctlySummary by CodeRabbit
make statusdashboard showing local config, instance state, hypervisor/VM health, proxy health, and cluster API readiness.make infois now an alias formake status.make statusfirst (including when access breaks) and to reference the new dashboard-based instance information.