Add baremetal deploy via Ansible playbook#89
Merged
openshift-merge-bot[bot] merged 35 commits intoJul 6, 2026
Merged
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds baremetal adoption tooling for TNF fencing clusters: new make targets, inventory capture, adoption artifact generation, and deployment validation plus orchestration. ChangesBaremetal Adoption Flow
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
the
ready-for-human-review
![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: 3
🤖 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/inventory_baremetal.ini.sample`:
- Around line 24-25: The sample inventory entries still use a literal bmc_pass
value, which should be replaced with a clearly non-usable placeholder. Update
the bmc_pass fields in the master-0 and master-1 example entries to an obvious
placeholder or commented/blank value, keeping the sample format intact and
avoiding any real-looking credential in inventory_baremetal.ini.sample.
In `@deploy/openshift-clusters/scripts/baremetal-adopt.sh`:
- Around line 190-193: The inventory validation in parse_inventory() is only
warning on unsupported node counts instead of stopping the TNF-only flow. Change
the NODE_NAMES count check so it fails fast when the inventory is anything other
than exactly 2 nodes, using die with a clear unsupported-topology message after
the existing zero-nodes guard.
In `@deploy/openshift-clusters/scripts/deploy-baremetal.sh`:
- Around line 190-199: The setup_dev_scripts flow currently clones
DEV_SCRIPTS_REPO at runtime, which leaves stacked-dev-scripts unpinned and can
silently pick up upstream HEAD changes. Update setup_dev_scripts to use a fixed
branch/tag/commit for the git clone, or require an already checked-out
DEV_SCRIPTS_PATH and fail if the checkout is not the expected revision. Apply
the same pinning approach anywhere the deployment relies on the remote
provisioning-host path so the behavior stays stable across runs.
🪄 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: d6acc4b1-e856-4b79-95e5-ac473a54f6df
📒 Files selected for processing (7)
deploy/Makefiledeploy/openshift-clusters/.gitignoredeploy/openshift-clusters/inventory_baremetal.ini.sampledeploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_fencing_example.shdeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/baremetal-wizard.shdeploy/openshift-clusters/scripts/deploy-baremetal.sh
coderabbitai
Bot
removed
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deploy/openshift-clusters/scripts/deploy-baremetal.sh (2)
45-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate option values before reading
$2.With
set -u,--dev-scripts-repoor--dev-scripts-branchwithout a value aborts with an unbound-variable error instead of a clear usage failure.Proposed fix
parse_args() { while [[ $# -gt 0 ]]; do case $1 in --cluster-name) + [[ $# -ge 2 && -n "${2:-}" ]] || die "--cluster-name requires a value" CLUSTER_NAME="$2" shift 2 ;; --dev-scripts-path) + [[ $# -ge 2 && -n "${2:-}" ]] || die "--dev-scripts-path requires a value" DEV_SCRIPTS_PATH="$2" shift 2 ;; --dev-scripts-repo) + [[ $# -ge 2 && -n "${2:-}" ]] || die "--dev-scripts-repo requires a value" DEV_SCRIPTS_REPO="$2" shift 2 ;; --dev-scripts-branch) + [[ $# -ge 2 && -n "${2:-}" ]] || die "--dev-scripts-branch requires a value" DEV_SCRIPTS_BRANCH="$2" shift 2 ;;🤖 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/deploy-baremetal.sh` around lines 45 - 63, The parse_args() option handling reads $2 for --dev-scripts-repo and --dev-scripts-branch before confirming a value exists, which can trigger an unbound-variable failure under set -u. Update parse_args() in deploy-baremetal.sh to validate that each option has a following argument before assigning from $2, and fail with a clear usage/error message when the value is missing. Keep the checks in the same case branches for --dev-scripts-repo and --dev-scripts-branch so the script exits cleanly instead of aborting unexpectedly.
351-371: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winQuote remote args and forward the effective repo/branch overrides.
In provisioning-host mode, local
--dev-scripts-repo/--dev-scripts-branchvalues are dropped unless duplicated ininventory_baremetal.ini, and the string-built SSH command shell-splits unquoted repo/path/branch values. Build an argv array, shell-quote it for SSH, and merge the effective local/inventory overrides. As per coding guidelines, shell scripts should quote variables properly.Proposed fix
exec_on_remote() { - # shellcheck disable=SC2088 # tilde expands on the remote shell via SSH - local remote_script="~/${PROV_WORKING_DIR}/scripts/deploy-baremetal.sh" - local remote_args="--cluster-name ${CLUSTER_NAME}" - if [[ -n "${PROV_DEV_SCRIPTS_PATH:-}" ]]; then - remote_args+=" --dev-scripts-path ${PROV_DEV_SCRIPTS_PATH}" + local remote_script="${PROV_WORKING_DIR}/scripts/deploy-baremetal.sh" + local remote_args=(--cluster-name "${CLUSTER_NAME}") + local remote_dev_scripts_repo="${PROV_DEV_SCRIPTS_REPO:-${DEV_SCRIPTS_REPO}}" + local remote_dev_scripts_branch="${PROV_DEV_SCRIPTS_BRANCH:-${DEV_SCRIPTS_BRANCH}}" + + if [[ -n "${PROV_DEV_SCRIPTS_PATH:-}" ]]; then + remote_args+=(--dev-scripts-path "${PROV_DEV_SCRIPTS_PATH}") fi - if [[ -n "${PROV_DEV_SCRIPTS_REPO:-}" ]]; then - remote_args+=" --dev-scripts-repo ${PROV_DEV_SCRIPTS_REPO}" + if [[ -n "${remote_dev_scripts_repo}" ]]; then + remote_args+=(--dev-scripts-repo "${remote_dev_scripts_repo}") fi - if [[ -n "${PROV_DEV_SCRIPTS_BRANCH:-}" ]]; then - remote_args+=" --dev-scripts-branch ${PROV_DEV_SCRIPTS_BRANCH}" + if [[ -n "${remote_dev_scripts_branch}" ]]; then + remote_args+=(--dev-scripts-branch "${remote_dev_scripts_branch}") fi + + local remote_cmd="cd && TNT_REMOTE_EXEC=1 bash" + printf -v remote_cmd '%s %q' "${remote_cmd}" "${remote_script}" + local arg + for arg in "${remote_args[@]}"; do + printf -v remote_cmd '%s %q' "${remote_cmd}" "${arg}" + done info "Executing deploy on ${PROV_SSH_TARGET}..." info "Remote output follows:" echo "==========================================" # shellcheck disable=SC2029 ssh -tt "${SSH_OPTS[@]}" "${PROV_SSH_TARGET}" \ - "TNT_REMOTE_EXEC=1 bash ${remote_script} ${remote_args}" + "${remote_cmd}" }🤖 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/deploy-baremetal.sh` around lines 351 - 371, The exec_on_remote function is dropping effective dev-scripts overrides in provisioning-host mode and is also building the SSH command from unquoted strings, which can split repo/path/branch values incorrectly. Update exec_on_remote to collect the final dev-scripts repo/branch values from the local environment or inventory defaults, then build the remote invocation as an argv-style array instead of concatenating remote_args. Ensure the ssh call passes a properly shell-quoted command using the exec_on_remote and remote_args logic, and quote all variable expansions used in the command construction.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.
Outside diff comments:
In `@deploy/openshift-clusters/scripts/deploy-baremetal.sh`:
- Around line 45-63: The parse_args() option handling reads $2 for
--dev-scripts-repo and --dev-scripts-branch before confirming a value exists,
which can trigger an unbound-variable failure under set -u. Update parse_args()
in deploy-baremetal.sh to validate that each option has a following argument
before assigning from $2, and fail with a clear usage/error message when the
value is missing. Keep the checks in the same case branches for
--dev-scripts-repo and --dev-scripts-branch so the script exits cleanly instead
of aborting unexpectedly.
- Around line 351-371: The exec_on_remote function is dropping effective
dev-scripts overrides in provisioning-host mode and is also building the SSH
command from unquoted strings, which can split repo/path/branch values
incorrectly. Update exec_on_remote to collect the final dev-scripts repo/branch
values from the local environment or inventory defaults, then build the remote
invocation as an argv-style array instead of concatenating remote_args. Ensure
the ssh call passes a properly shell-quoted command using the exec_on_remote and
remote_args logic, and quote all variable expansions used in the command
construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: beb072fd-30ee-453b-b4c7-452f2890b2cf
📒 Files selected for processing (2)
deploy/openshift-clusters/inventory_baremetal.ini.sampledeploy/openshift-clusters/scripts/deploy-baremetal.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/openshift-clusters/inventory_baremetal.ini.sample
openshift-ci
Bot
added
the
needs-rebase
fonta-rh
force-pushed
the
ocpedge-2775-baremetal-deploy
branch
from
5a87a3f to
21dd305
Compare
fonta-rh
changed the title
Introduce adopt-baremetal.sh to onboard existing baremetal nodes into the dev-scripts deployment workflow. Parses inventory_baremetal.ini (template or wizard-generated), validates BMC credentials via Redfish, and generates ironic_nodes.json + config_baremetal_fencing.sh artifacts for NODES_PLATFORM=baremetal deployments. OCPEDGE-2774 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split the interactive wizard out of adopt-baremetal.sh into its own baremetal-wizard.sh script. Add input validation (IPv4, MAC format, hostname), re-prompt on invalid input instead of dying, a summary table with masked passwords before confirmation, and Y/n/q flow. Rename all baremetal scripts and Make targets to use a baremetal-* prefix for consistent grouping (baremetal-adopt, baremetal-verify, baremetal-wizard). OCPEDGE-2774 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The wizard only accepted IPv4 addresses for BMC endpoints, but real environments (e.g., HPE iLO) commonly use FQDNs. Accept both IPv4 and hostnames, and update prompts/labels accordingly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bmc_address accepts both IPs and hostnames (matching wizard change). boot_mac is now optional — when omitted, the adopt script queries Redfish EthernetInterfaces for an enabled NIC's MAC. Falls back to a clear warning if discovery fails (e.g., firmware doesn't expose MACs). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the EthernetInterfaces-based discovery (which returned the wrong NIC) with BootOptions-based discovery. Walks the BIOS boot order, finds the first PXE IPv4 entry, and extracts the MAC from the UEFI device path. Tested against HPE iLO 5 (Edgeline e920t). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix jq null handling in Redfish system ID discovery (// empty) - Fix script names in usage text and generated comments - Fix missing / separator in BootOptions URL construction - Add --inventory flag and info messages to wizard trigger - Add warning when inventory has != 2 nodes - Honor bmc_verify_ca in curl calls via bmc_curl wrapper - Restrict permissions on generated credential artifacts (umask 077) Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace heredoc JSON interpolation with jq -n --arg to prevent malformed output from passwords containing quotes or backslashes - Fail hard on missing boot MAC instead of writing DISCOVERY_FAILED placeholder that silently breaks dev-scripts downstream - Gate all BMC contact (discovery + verification) behind --skip-verify so offline users don't get silent failures baked into artifacts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Redfish @odata.id values include a trailing slash (/redfish/v1/Systems/1/), which produced a double-slash URL (.../Systems/1//BootOptions/) that iLO returns null content for. Strip the trailing slash after the leading one. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add declarative [baremetal_network] INI section for cluster-wide network vars (machine_network, gateway, api_vip, ingress_vip) and per-node node_ip field. The adoption script translates these into dev-scripts exports (EXTERNAL_SUBNET_V4, BAREMETAL_GATEWAY, BAREMETAL_API_VIP, BAREMETAL_INGRESS_VIP, BAREMETAL_IPS) and always emits bridge overrides (MANAGE_BR_BRIDGE=n, MANAGE_PRO_BRIDGE=n, MANAGE_INT_BRIDGE=n). All new fields are optional for backward compatibility. The wizard emits skipped fields as commented placeholders so users can fill them later without referencing the sample file. BAREMETAL_IPS is only emitted when ALL nodes have node_ip set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The wizard now collects optional provisioning host config (ssh_target, ssh_key, dev_scripts_path, working_dir) and writes a [provisioning_host] section to inventory_baremetal.ini. Skipped fields are commented out. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Without an explicit port in the Redfish URL, CEO's fence_redfish agent fails to connect to the BMC during TNF auth jobs. The port (default 443) now flows through verification, discovery, and ironic_nodes.json generation as redfish://host:port/redfish/v1/Systems/1. Adds bmc_port as a per-node field with a group default in [baremetal_nodes:vars], matching the existing inheritance pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Place ironic_nodes.json and config_baremetal_fencing.sh in roles/dev-scripts/install-dev/files/ instead of clusters/<name>/ so existing Ansible config.yml tasks can reference them by basename. Remove --cluster-name flag (only used for the clusters/ subdirectory). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
deploy/openshift-clusters/scripts/deploy-baremetal.sh (1)
36-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a comment explaining the section-parsing logic.
The
sed/greppipeline that checks for configured hosts under[provisioning_host]is non-trivial (range match to next section header, strip header line, filter comments/blanks). As per coding guidelines,**/*.shfiles should "add comments for complex logic".📝 Proposed comment addition
+# Extract lines between [provisioning_host] and the next section header, +# drop the header line itself, and check for at least one non-comment/blank entry. if ! grep -qvE '^\s*(#|$|\[)' <(sed -n '/\[provisioning_host\]/,/^\[/p' "${INVENTORY}" | tail -n +2); then🤖 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/deploy-baremetal.sh` around lines 36 - 41, The host-check logic in deploy-baremetal.sh is complex and needs an inline comment. Add a brief comment above the sed/grep pipeline in the provisioning_host validation block explaining that it extracts the [provisioning_host] section, skips the header, and filters out comments/blank lines to detect whether any hosts are configured.Source: Coding guidelines
deploy/openshift-clusters/deploy-baremetal.yml (1)
65-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
ansible.builtin.fileover shell for directory creation.Using
ansible.builtin.shell: mkdir -p ...withchanged_when: falsesacrifices idempotent change reporting for no real benefit;ansible.builtin.filewithstate: directoryachieves the same result natively.♻️ Proposed refactor
- - name: Create working directory - ansible.builtin.shell: mkdir -p "${HOME}/dev-scripts-workdir" - changed_when: false + - name: Create working directory + ansible.builtin.file: + path: "{{ ansible_env.HOME }}/dev-scripts-workdir" + state: directory + mode: "0755"🤖 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/deploy-baremetal.yml` around lines 65 - 67, The “Create working directory” task is using a shell command just to make a directory, which should be replaced with Ansible’s native file handling. Update that task in the playbook to use ansible.builtin.file with state set to directory for the same "${HOME}/dev-scripts-workdir" path, and remove the shell-based mkdir and changed_when suppression. Keep the task name and behavior aligned with the existing deploy-baremetal workflow.
🤖 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/deploy-baremetal.yml`:
- Around line 82-94: The ABI pipeline task uses the short name make, which may
not resolve because this play does not declare a collections scope. Update the
task in the “Run dev-scripts ABI pipeline” block to use the fully qualified
community.general.make module, or alternatively add community.general to the
play’s collections so the “dev-scripts: {{ item }}” task resolves reliably.
---
Nitpick comments:
In `@deploy/openshift-clusters/deploy-baremetal.yml`:
- Around line 65-67: The “Create working directory” task is using a shell
command just to make a directory, which should be replaced with Ansible’s native
file handling. Update that task in the playbook to use ansible.builtin.file with
state set to directory for the same "${HOME}/dev-scripts-workdir" path, and
remove the shell-based mkdir and changed_when suppression. Keep the task name
and behavior aligned with the existing deploy-baremetal workflow.
In `@deploy/openshift-clusters/scripts/deploy-baremetal.sh`:
- Around line 36-41: The host-check logic in deploy-baremetal.sh is complex and
needs an inline comment. Add a brief comment above the sed/grep pipeline in the
provisioning_host validation block explaining that it extracts the
[provisioning_host] section, skips the header, and filters out comments/blank
lines to detect whether any hosts are configured.
🪄 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: 1a36f074-0c93-4030-9375-a952efb2ffb5
📒 Files selected for processing (6)
deploy/Makefiledeploy/openshift-clusters/deploy-baremetal.ymldeploy/openshift-clusters/inventory_baremetal.ini.sampledeploy/openshift-clusters/roles/dev-scripts/install-dev/files/.gitignoredeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/deploy-baremetal.sh
✅ Files skipped from review due to trivial changes (2)
- deploy/openshift-clusters/roles/dev-scripts/install-dev/files/.gitignore
- deploy/openshift-clusters/inventory_baremetal.ini.sample
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/Makefile
- deploy/openshift-clusters/scripts/baremetal-adopt.sh
fonta-rh
force-pushed
the
ocpedge-2775-baremetal-deploy
branch
from
21dd305 to
9f45936
Compare
openshift-ci
Bot
removed
the
needs-rebase
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
coderabbitai
Bot
added
the
ready-for-human-review
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deploy/openshift-clusters/scripts/baremetal-wizard.sh (1)
462-482: 🔒 Security & Privacy | 🔵 TrivialConsider vault-encrypting BMC credentials before persisting to disk.
bmc_passvalues are written in plaintext intoinventory_baremetal.ini.mktempdefaults to mode 600 so the file itself isn't world-readable, but plaintext BMC passwords on disk are still a risk if the file is copied, backed up, or committed accidentally. Consider prompting to encrypt the password field withansible-vaultor documenting that the generated inventory must be protected/gitignored.🤖 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/baremetal-wizard.sh` around lines 462 - 482, The inventory generation in baremetal-wizard.sh writes BMC credentials directly into the temporary inventory file via the loop that builds each node’s line, including bmc_pass in plaintext. Update this flow so WIZ_PASSES is not persisted unencrypted: either vault-encrypt the password before writing the inventory or replace the plaintext field with guidance/documentation that the generated inventory must be protected and gitignored. Keep the change localized to the inventory write logic around the mktemp/tmp_inventory generation and the per-node line assembly.
🤖 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/baremetal-wizard.sh`:
- Around line 484-491: The baremetal inventory defaults to bmc_verify_ca=False
in the inventory block written by the wizard, which disables Redfish TLS
verification for everyone. Update the baremetal-wizard.sh flow that builds the
[baremetal_nodes:vars] section to make this setting configurable through the
wizard (or otherwise preserve the secure default), and ensure the generated
tmp_inventory reflects the user’s choice instead of hardcoding the insecure
value.
- Around line 389-409: The wizard reset logic is leaving WIZ_PORTS stale, which
can misalign bmc_port data after a restart. Update the array reinitialization
block in baremetal-wizard.sh to clear WIZ_PORTS along with WIZ_NAMES, WIZ_IPS,
WIZ_USERS, WIZ_PASSES, WIZ_MACS, and WIZ_NODE_IPS, so the node data collected in
the loop around prompt_bmc_port stays in sync when show_summary and
write_inventory are reached again after a continue.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/baremetal-wizard.sh`:
- Around line 462-482: The inventory generation in baremetal-wizard.sh writes
BMC credentials directly into the temporary inventory file via the loop that
builds each node’s line, including bmc_pass in plaintext. Update this flow so
WIZ_PASSES is not persisted unencrypted: either vault-encrypt the password
before writing the inventory or replace the plaintext field with
guidance/documentation that the generated inventory must be protected and
gitignored. Keep the change localized to the inventory write logic around the
mktemp/tmp_inventory generation and the per-node line assembly.
🪄 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: d3a43c7f-14e6-4d69-bf28-70f47cf41181
📒 Files selected for processing (9)
deploy/Makefiledeploy/openshift-clusters/.gitignoredeploy/openshift-clusters/deploy-baremetal.ymldeploy/openshift-clusters/inventory_baremetal.ini.sampledeploy/openshift-clusters/roles/dev-scripts/install-dev/files/.gitignoredeploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_fencing_example.shdeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/baremetal-wizard.shdeploy/openshift-clusters/scripts/deploy-baremetal.sh
✅ Files skipped from review due to trivial changes (4)
- deploy/openshift-clusters/.gitignore
- deploy/openshift-clusters/roles/dev-scripts/install-dev/files/config_fencing_example.sh
- deploy/openshift-clusters/roles/dev-scripts/install-dev/files/.gitignore
- deploy/openshift-clusters/inventory_baremetal.ini.sample
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/openshift-clusters/scripts/deploy-baremetal.sh
- deploy/Makefile
- deploy/openshift-clusters/deploy-baremetal.yml
- deploy/openshift-clusters/scripts/baremetal-adopt.sh
On multi-NIC servers the PXE boot NIC may differ from the data NIC. Add a per-node data_mac inventory field that emits BAREMETAL_MACS in config_baremetal_fencing.sh so dev-scripts uses the correct MAC for agent-config hostname mapping. Also add the missing [provisioning_host] section to the inventory sample. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ve GATEWAY Three vars are required for the baremetal DHCP deploy path but were silently optional: BAREMETAL_IPS, BAREMETAL_API_VIP, and AGENT_E2E_TEST_SCENARIO. Fail early at adoption time instead of crashing deep in dev-scripts. Remove GATEWAY (nothing reads it) and add a post-generation reminder to verify CI_TOKEN and release image. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DHCP is the only supported network mode and agent-config always needs the data NIC MAC for hostname mapping. Error early if data_mac is missing rather than silently falling back to the boot MAC. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BMCs need the full agent ISO URL to mount via Redfish VirtualMedia. Add iso_url as a required field in [baremetal_network], parsed and emitted as BAREMETAL_ISO_SERVER in config_baremetal_fencing.sh. Also make data_mac mandatory since DHCP is the only supported mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Guard --inventory, --config-base, and --output against missing values so the scripts emit a clear error instead of a cryptic bash unbound variable message under set -o nounset. Co-Authored-By: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bug fix: - Reset WIZ_PORTS and WIZ_DATA_MACS arrays on wizard retry — stale values from the first round accumulated across rounds Poka-yoke improvements: - Reject contradictory --skip-verify + --verify-only flags - Log a warning when Redfish system discovery fails and falls back to /redfish/v1/Systems/1 - Validate data_mac and node_ip immediately after parsing inventory, before expensive BMC round-trips - Re-prompt on invalid confirmation input instead of restarting the entire wizard from scratch Documentation: - Note in sample that bmc_pass must not contain spaces or # Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add deploy-baremetal.sh — a standalone shell script that deploys a TNF fencing cluster on adopted baremetal nodes using dev-scripts' agent pipeline. Skips libvirt/qemu setup and drives Redfish VirtualMedia for ISO delivery. Supports remote execution: if [provisioning_host] is configured in inventory_baremetal.ini, syncs artifacts via rsync and runs the deploy on the remote host via SSH. Falls back to local execution if absent. Changes: - deploy-baremetal.sh: pre-flight validation, dev-scripts setup, remote execution via SSH with credential retrieval - inventory_baremetal.ini.sample: add [provisioning_host] section - baremetal-adopt.sh: append AGENT_E2E_TEST_SCENARIO to generated config - Makefile: add baremetal-fencing-agent target Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add --dev-scripts-repo and --dev-scripts-branch CLI args so the deploy script can clone from a fork or switch an existing checkout to a specific branch (needed for dev-scripts PRs not yet merged upstream) - Add dev_scripts_repo and dev_scripts_branch to [provisioning_host] inventory section, forwarded via SSH remote execution - Remove validate_tools() — dev-scripts' make requirements now runs as part of the pipeline, handling all dependency installation - Remove python wrapper and PYTHONPATH workarounds (Fedora laptop hacks, not needed on RHEL provisioning host) - Normalize [[ ]] && to if/then blocks for readability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
With DHCP networking mode, nodes get DNS from the lab DHCP server instead of static nmstate config. The PROVISIONING_HOST_EXTERNAL_IP variable is no longer needed for node configuration — dev-scripts provides a harmless default via network.sh. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the 448-line shell script with a 90-line Ansible playbook (deploy-baremetal.yml) that reuses the install-dev role's config validation via include_role tasks_from: config. The shell script is gutted to a thin wrapper (~45 lines) that validates inventory_baremetal.ini has a configured [provisioning_host] and calls ansible-playbook, matching the deploy-cluster.sh pattern. - Playbook targets [provisioning_host] group from inventory_baremetal.ini - SSH/rsync remote execution replaced by Ansible's native connection layer - Config/pull-secret validation reused from install-dev role (zero duplication) - Dev-scripts ABI pipeline via make module loop (6 targets) - Credentials fetched back to controller via fetch module - Error recovery block with cleanup instructions Also updates inventory_baremetal.ini.sample to use standard Ansible inventory format for [provisioning_host] section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Generate SSH keypair on provisioning host if missing, inject SSH_PUB_KEY into dev-scripts config so cluster nodes accept SSH - Clean stale installer state (state.json, ISO, manifests) before running ABI pipeline to prevent release image SHA mismatches - Skip host dependency install (runc, containernetworking-plugins) for baremetal path — dev-scripts make requirements handles its own - Add test_cluster_name to play-level vars (include_role defaults don't persist to play scope with dynamic includes) - Fetch SSH private key alongside kubeconfig for node access - Pass method as task-level var on include_role (role vars/main.yml overrides play-level vars but not task-level vars) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reuse the existing proxy-setup role (Squid forward proxy) to provide cluster access from the developer laptop through the provisioning host. Fix inventory_hostname extraction to support both user@host and plain host formats. Replace manual credential fetch in deploy-baremetal.yml with proxy-setup role inclusion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Match the messaging style used by kcli-install.yml and the hypervisor deploy scripts — numbered steps, full playbook_dir path for sourcing proxy.env from anywhere. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After deploy, propagates the laptop user's SSH key through the provisioning host to RHCOS nodes and writes a [cluster_nodes] section into inventory_baremetal.ini with ProxyJump configuration. Mirrors the existing update-cluster-inventory.yml pattern for libvirt VMs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reuses the common cluster-state role to write cluster-state.json (deploying → deployed) into clusters/<cluster_name>/, alongside the existing adoption artifacts and credentials. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fonta-rh
force-pushed
the
ocpedge-2775-baremetal-deploy
branch
from
12b26b8 to
4bf3655
Compare
The wizard wrote ssh_target= and ssh_key= as bare key=value lines under [provisioning_host], which Ansible parsed as hostnames instead of variables. Emit a proper host entry line with ansible_user and ansible_ssh_private_key_file, and put optional overrides in a [provisioning_host:vars] section. Also apply defaults for dev-scripts path and working directory prompts so hitting Enter writes the displayed default instead of an empty string. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The baremetal playbook includes only tasks_from: config from the install-dev role, skipping the git clone that normally creates the directory. The copy task then fails because the destination doesn't exist yet. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Role defaults are only visible inside include_role scope. The mkdir task added in the previous commit runs before the role and needs the variable at play level. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Role defaults aren't visible to tasks outside include_role. Move the git clone ahead of the config role so the directory exists when the pull secret is copied, and define dev_scripts vars at play level (matching role defaults) so all tasks can resolve them. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The VM flow sets up git identity via the config role, but the baremetal playbook bypasses it. Without .gitconfig on the provisioning host, dev-scripts' sync_repo_and_patch prints noisy "Committer identity unknown" errors on every git am --abort. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Play-level vars override inventory group vars in Ansible, so hardcoded dev_scripts_* values in the playbook prevented [provisioning_host:vars] from taking effect. Move them to inline defaults on the git task (the only pre-role consumer); subsequent tasks pick them up from role defaults or inventory. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Play vars override inventory; role defaults are scoped to the role. set_fact with |default() defers to inventory values when present and falls back to hardcoded defaults otherwise, and the resulting facts persist for the entire play. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
/lgtm |
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
2 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
deploy-baremetal.yml— Ansible playbook that deploys a TNF fencing cluster on adopted baremetal nodes using dev-scripts' agent pipelineinstall-devrole'sconfig.ymlfor all validation (CI token, pull-secret, scenario prefix) viainclude_role tasks_from: config— zero duplicationdeploy-baremetal.shas a thin wrapper (~45 lines) that validates inventory and callsansible-playbook, matching thedeploy-cluster.shpattern[provisioning_host]inventory section to standard Ansible formatDeploy flow
The playbook targets
[provisioning_host]frominventory_baremetal.ini:config.yml(CI token, pull-secret, scenario check)For local deployment, add
localhost ansible_connection=localto[provisioning_host].What changed from the shell script approach
The original
deploy-baremetal.sh(448 lines) reimplemented config validation and had ~140 lines of SSH/rsync remote execution code. The Ansible playbook eliminates both:include_role(not reimplemented)fetchmodule instead of rsyncTest plan
ansible-playbook deploy-baremetal.yml --syntax-checkpassesshellcheck deploy-baremetal.shpasses cleanmake helpshows updated descriptionmake baremetal-fencing-agentwrapper validates inventory correctly🤖 Generated with Claude Code
Summary by CodeRabbit
Summary
makeworkflow commands:baremetal-adopt,baremetal-verify,baremetal-fencing-agent, andbaremetal-wizard.