Remove manual Redfish fencing roles for 4.22 GA#87
Open
fonta-rh wants to merge 1 commit into
Open
Conversation
The CEO auto-configures STONITH fencing from BMH resources since 4.19, making the manual bare-metal redfish role and post-install stonith configuration dead code. Strip these and bump stale version defaults. Removed: - roles/redfish/ and redfish.yml (bare-metal manual stonith) - kcli-redfish.yml playbook and stonith task files (kcli manual stonith) - setup.yml post_tasks block (is_ocp_4_19 version gate + redfish prompt) Kept: - roles/kcli/kcli-redfish/ with only ksushy startup (BMC simulator still needed for virtual deployments) Updated: - Default OCP version 4.20 → 4.22 - Removed DevPreviewNoUpgrade/TechPreview feature gates from examples - Updated release image references to 4.22 - Cleaned up docs (CLAUDE.md, READMEs) 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
needs-rebase
|
PR needs rebase. 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. |
openshift-ci
Bot
added
the
approved
|
@fonta-rh: The following tests failed, say
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. |
Contributor
WalkthroughRemoves manual Redfish/PCS STONITH playbooks ( ChangesRedfish STONITH Removal and kcli-redfish Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/main.yml (1)
26-42: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate before starting ksushy.
include_tasks: start_ksushy.ymlruns before theassert, so a bad config can still start or modify the simulator service before the role fails. Move validation ahead of any side effects.🔧 Suggested reorder
+- name: Validate configuration + assert: + that: + - test_cluster_name != "" + - ksushy_ip != "" + - bmc_user != "" + - bmc_password != "" + delegate_to: localhost + - name: Start ksushy BMC simulator include_tasks: start_ksushy.yml🤖 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/roles/kcli/kcli-redfish/tasks/main.yml` around lines 26 - 42, The validation in kcli-redfish currently happens after start_ksushy.yml, so bad input can trigger side effects before the role fails. Move the assert block in tasks/main.yml ahead of include_tasks: start_ksushy.yml so test_cluster_name, ksushy_ip, bmc_user, and bmc_password are checked before any simulator startup or modification occurs.
🤖 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/README.md`:
- Around line 184-186: Remove the stale OpenShift 4.19.x version qualifier from
the automatic Redfish STONITH description in the fencing topology section.
Update the wording around the automatic fencing behavior for
cluster-etcd-operator (CEO) and BareMetalHost resources so it no longer reads as
version-locked, and ensure the README text reflects the current 4.22 flow
instead of referencing only 4.19.x.
In
`@deploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_arbiter_example.sh`:
- Line 36: The OPENSHIFT_RELEASE_IMAGE example in config_arbiter_example.sh is
x86_64-only and currently lacks the ARM64/aarch64 guidance that
config_fencing_example.sh includes. Update this example to explicitly document
the aarch64/Graviton override variables next to OPENSHIFT_RELEASE_IMAGE,
including the required Metal3 container image overrides, so users copying the
script to ARM64 hosts can switch to the correct payload architecture instead of
the default x86_64 image.
---
Outside diff comments:
In `@deploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/main.yml`:
- Around line 26-42: The validation in kcli-redfish currently happens after
start_ksushy.yml, so bad input can trigger side effects before the role fails.
Move the assert block in tasks/main.yml ahead of include_tasks: start_ksushy.yml
so test_cluster_name, ksushy_ip, bmc_user, and bmc_password are checked before
any simulator startup or modification occurs.
🪄 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: 051753f2-f9ef-488b-8bc9-f2e0c5ca2305
📒 Files selected for processing (20)
CLAUDE.mddeploy/aws-hypervisor/scripts/configure.shdeploy/openshift-clusters/README-kcli.mddeploy/openshift-clusters/README.mddeploy/openshift-clusters/kcli-install.ymldeploy/openshift-clusters/kcli-redfish.ymldeploy/openshift-clusters/redfish.ymldeploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_arbiter_example.shdeploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_fencing_example.shdeploy/openshift-clusters/roles/kcli/kcli-redfish/README.mddeploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/configure_stonith.ymldeploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/main.ymldeploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/post-installation.ymldeploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/verify_stonith.ymldeploy/openshift-clusters/roles/redfish/README.mddeploy/openshift-clusters/roles/redfish/defaults/main.ymldeploy/openshift-clusters/roles/redfish/tasks/main.ymldeploy/openshift-clusters/roles/redfish/tasks/process_bmh.ymldeploy/openshift-clusters/setup.ymldeploy/openshift-clusters/vars/init-host.yml
💤 Files with no reviewable changes (10)
- deploy/openshift-clusters/roles/redfish/README.md
- deploy/openshift-clusters/roles/redfish/tasks/process_bmh.yml
- deploy/openshift-clusters/roles/redfish/tasks/main.yml
- deploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/post-installation.yml
- deploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/configure_stonith.yml
- deploy/openshift-clusters/kcli-redfish.yml
- deploy/openshift-clusters/redfish.yml
- deploy/openshift-clusters/roles/redfish/defaults/main.yml
- deploy/openshift-clusters/roles/kcli/kcli-redfish/tasks/verify_stonith.yml
- deploy/openshift-clusters/setup.yml
Comment on lines
184
to
+186
| For clusters using the fencing topology on OpenShift 4.19.x, automatic Redfish stonith configuration is available. This feature configures Pacemaker stonith resources using Redfish fencing for BareMetalHost resources. | ||
|
|
||
| Redfish configuration can be applied in two ways: | ||
|
|
||
| **Integrated Usage:** | ||
| - When running the main deployment playbook in interactive mode with fencing topology, you will be prompted to configure Redfish stonith automatically | ||
| - Redfish configuration runs as part of the main deployment workflow | ||
|
|
||
| **Standalone Usage:** | ||
| - Redfish configuration can be run independently using: `ansible-playbook redfish.yml` | ||
| - This allows for running it separately from the main deployment or re-running it if needed | ||
|
|
||
| For detailed configuration options, verification commands, and requirements, refer to the [Redfish role documentation](roles/redfish/README.md). | ||
| Fencing topology clusters use automatic fencing configuration via the cluster-etcd-operator (CEO). The CEO discovers BareMetalHost resources and configures STONITH automatically during installation. No manual Redfish configuration is required. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale 4.19.x qualifier.
This section still limits automatic Redfish STONITH to OpenShift 4.19.x, which conflicts with the new 4.22 flow and makes the docs read as version-locked to an old release. Please update or drop the version reference.
🤖 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/README.md` around lines 184 - 186, Remove the stale
OpenShift 4.19.x version qualifier from the automatic Redfish STONITH
description in the fencing topology section. Update the wording around the
automatic fencing behavior for cluster-etcd-operator (CEO) and BareMetalHost
resources so it no longer reads as version-locked, and ensure the README text
reflects the current 4.22 flow instead of referencing only 4.19.x.
| # and select your preferred version. Public sources can be found at https://mirror.openshift.com/pub/openshift-v4/ | ||
|
|
||
| export OPENSHIFT_RELEASE_IMAGE=quay.io/openshift-release-dev/ocp-release:4.21.0-x86_64 | ||
| export OPENSHIFT_RELEASE_IMAGE=quay.io/openshift-release-dev/ocp-release:4.22.0-x86_64 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the aarch64 override guidance next to this release image.
This example now hardcodes an x86_64 payload image but still omits the ARM64 notes that config_fencing_example.sh already carries. Copying this file onto Graviton/aarch64 hypervisors will still point users at the wrong payload architecture and miss the required Metal3 container overrides.
Suggested update
export OPENSHIFT_RELEASE_IMAGE=quay.io/openshift-release-dev/ocp-release:4.22.0-x86_64
+# aarch64 (Graviton): switch the payload image to the matching architecture and
+# override Metal3 infrastructure images with arm64 rebuilds.
+# if [ "$(uname -m)" = "aarch64" ]; then
+# export OPENSHIFT_RELEASE_IMAGE=quay.io/openshift-release-dev/ocp-release:4.22.0-aarch64
+# export IRONIC_IMAGE=quay.io/rh-edge-enablement/ironic:2026-06
+# export VBMC_IMAGE=quay.io/rh-edge-enablement/vbmc:2026-06
+# export SUSHY_TOOLS_IMAGE=quay.io/rh-edge-enablement/sushy-tools:2026-06
+# fiBased on learnings, "When reviewing dev-scripts config example shell files in two-node-toolbox, ensure that the ARM64 (aarch64/Graviton) image override variables are handled explicitly... Upstream Metal3 images ... are x86_64-only and will fail on aarch64 hosts with Exec format error."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export OPENSHIFT_RELEASE_IMAGE=quay.io/openshift-release-dev/ocp-release:4.22.0-x86_64 | |
| export OPENSHIFT_RELEASE_IMAGE=quay.io/openshift-release-dev/ocp-release:4.22.0-x86_64 | |
| # aarch64 (Graviton): switch the payload image to the matching architecture and | |
| # override Metal3 infrastructure images with arm64 rebuilds. | |
| # if [ "$(uname -m)" = "aarch64" ]; then | |
| # export OPENSHIFT_RELEASE_IMAGE=quay.io/openshift-release-dev/ocp-release:4.22.0-aarch64 | |
| # export IRONIC_IMAGE=quay.io/rh-edge-enablement/ironic:2026-06 | |
| # export VBMC_IMAGE=quay.io/rh-edge-enablement/vbmc:2026-06 | |
| # export SUSHY_TOOLS_IMAGE=quay.io/rh-edge-enablement/sushy-tools:2026-06 | |
| # fi |
🤖 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/roles/dev-scripts/install-dev/files/config_arbiter_example.sh`
at line 36, The OPENSHIFT_RELEASE_IMAGE example in config_arbiter_example.sh is
x86_64-only and currently lacks the ARM64/aarch64 guidance that
config_fencing_example.sh includes. Update this example to explicitly document
the aarch64/Graviton override variables next to OPENSHIFT_RELEASE_IMAGE,
including the required Metal3 container image overrides, so users copying the
script to ARM64 hosts can switch to the correct payload architecture instead of
the default x86_64 image.
Source: Learnings
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
1 participant
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
roles/redfish/role andredfish.ymlplaybook — the CEO auto-configures STONITH from BMH resources since 4.19roles/kcli/kcli-redfish/, keeping only the ksushy BMC simulator startup needed for virtual deploymentsis_ocp_4_19version gate and redfish prompt fromsetup.ymlpost_tasks (157 lines)DevPreviewNoUpgrade/TechPreviewfeature gates from example configsTest plan
make shellcheckpasses (pre-existing SC2034 warning inresolve_vm_ip.shis unrelated)ansible-playbook --syntax-check setup.ymlpassesansible-playbook --syntax-check kcli-install.ymlpassesgrep -r 'redfish' deploy/shows only kcli-redfish/ksushy references — no dangling refs to removed role🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores