Add baremetal node adoption automation#88
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[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
|
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: a wizard writes ChangesBaremetal adoption flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 8-9: The sample command references the wrong Make target name, so
update the documented invocation to use the actual target added by this PR. Fix
the guidance in the inventory_baremetal.ini.sample comment to match the Makefile
target name baremetal-adopt, keeping the rest of the setup instructions
unchanged.
In `@deploy/openshift-clusters/scripts/baremetal-adopt.sh`:
- Around line 57-71: The option parsing in baremetal-adopt.sh assigns values for
CLUSTER_NAME and CONFIG_BASE from $2 without checking that an argument exists,
so update the case handling for --cluster-name and --config-base to validate the
next token before shifting. In the argument parsing loop, add a guard that emits
a usage/error message and exits cleanly when either flag is provided without a
value, and only then perform the shift 2.
- Around line 157-159: The inventory parser in baremetal-adopt.sh currently
accepts any non-empty NODE_NAMES list, but this workflow must only support
exactly two nodes. Update the validation logic around the NODE_NAMES parsing
step to reject inventories unless ${`#NODE_NAMES`[@]} is exactly 2, and keep the
existing die/info flow in the same parsing function so unsupported topologies
fail before artifacts are generated.
- Around line 253-270: The nodes_json construction in baremetal-adopt.sh is
manually concatenating JSON, so embedded quotes or backslashes in fields like
name, bmc_user, or bmc_pass can break ironic_nodes.json. Replace the
string-building block with a real JSON encoder path, such as generating the node
object via jq -n with --arg inputs (or equivalent), and keep the existing fields
redfish address, ports, and properties mapped through the encoder so the output
is always valid JSON.
- Around line 276-277: The JSON artifacts written by the adoption script are
inheriting the caller’s umask, so sensitive BMC credentials can end up
world-readable. Update the write paths in the `baremetal-adopt.sh` flow around
the `nodes_json`/`output_file` generation (including the later similar block
noted in the comment) to explicitly create these files with restrictive
permissions before or during the write, rather than relying on `echo | jq`. Keep
the existing `info` logging, but ensure the generated credential files are only
accessible to the owning user.
- Around line 94-143: The inventory parsing loop in baremetal-adopt.sh currently
strips inline content after # and tokenizes node fields on whitespace, so BMC
credentials with spaces or # are corrupted before verification. Update the
parsing logic around the in_nodes/in_vars handling to preserve quoted values for
bmc_user, bmc_pass, and related fields, or validate and reject unsupported
characters consistently with the wizard input. Make sure the node pair parsing
no longer loses data when reading the rest of each baremetal_nodes entry.
- Around line 169-171: The Redfish curl calls in the baremetal adoption script
are always using insecure TLS, which ignores the bmc_verify_ca setting. Update
the Redfish request paths that fetch systems and any other BMC endpoints to
conditionally use certificate verification based on bmc_verify_ca, instead of
hardcoding -k. Make the curl options in baremetal-adopt.sh honor the existing
bmc_verify_ca variable consistently in both call sites so TLS verification is
only disabled when explicitly requested.
- Around line 173-174: The Redfish system ID extraction is returning the literal
string null instead of an empty value, which prevents the fallback logic from
running. Update the jq-based lookup used for the system ID in baremetal-adopt.sh
so that missing Members[0]."`@odata.id`" is normalized to empty before assignment,
and apply the same handling in the other matching block mentioned in the comment
so the redfish:// address never ends up with /null.
In `@deploy/openshift-clusters/scripts/baremetal-wizard.sh`:
- Around line 39-41: The --output option handling in baremetal-wizard.sh reads
OUTPUT from $2 without first checking that an argument exists, which can trigger
an unbound-variable failure instead of usage handling. Update the case branch
for --output to validate that the next positional argument is present before
assigning OUTPUT and shifting, and route the missing-argument path through the
script’s existing usage/error flow so baremetal-wizard.sh --output reports a
proper message.
- Around line 8-13: The help/usage text in baremetal-wizard.sh still refers to
the old command name and should be updated to match the actual script. Change
the usage banner and any generated inventory comment in the script’s help/output
logic so they consistently use baremetal-wizard.sh instead of
wizard-baremetal.sh, including the relevant strings near the top-of-file help
block and the inventory generation path.
🪄 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: 62d66789-5fa1-4fce-8065-c0760040c417
📒 Files selected for processing (5)
deploy/Makefiledeploy/openshift-clusters/.gitignoredeploy/openshift-clusters/inventory_baremetal.ini.sampledeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/baremetal-wizard.sh
| --cluster-name) | ||
| CLUSTER_NAME="$2" | ||
| shift 2 | ||
| ;; | ||
| --skip-verify) | ||
| SKIP_VERIFY=true | ||
| shift | ||
| ;; | ||
| --verify-only) | ||
| VERIFY_ONLY=true | ||
| shift | ||
| ;; | ||
| --config-base) | ||
| CONFIG_BASE="$2" | ||
| shift 2 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate option arguments before shifting.
--cluster-name and --config-base read $2 unconditionally. If either flag is passed without a value, the script aborts under set -u instead of returning a clean usage error.
🤖 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-adopt.sh` around lines 57 - 71,
The option parsing in baremetal-adopt.sh assigns values for CLUSTER_NAME and
CONFIG_BASE from $2 without checking that an argument exists, so update the case
handling for --cluster-name and --config-base to validate the next token before
shifting. In the argument parsing loop, add a guard that emits a usage/error
message and exits cleanly when either flag is provided without a value, and only
then perform the shift 2.
| [[ ${#NODE_NAMES[@]} -eq 0 ]] && die "No nodes found in inventory" | ||
| info "Parsed ${#NODE_NAMES[@]} node(s) from inventory" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject inventories that are not exactly two nodes.
This workflow is for TNF two-node adoption, but the parser accepts any non-empty inventory. A 3+ node file will still generate artifacts even though it does not match the supported topology.
🤖 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-adopt.sh` around lines 157 - 159,
The inventory parser in baremetal-adopt.sh currently accepts any non-empty
NODE_NAMES list, but this workflow must only support exactly two nodes. Update
the validation logic around the NODE_NAMES parsing step to reject inventories
unless ${`#NODE_NAMES`[@]} is exactly 2, and keep the existing die/info flow in
the same parsing function so unsupported topologies fail before artifacts are
generated.
| --output) | ||
| OUTPUT="$2" | ||
| shift 2 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate option arguments before reading $2.
--output consumes $2 unconditionally. Invoking the script as baremetal-wizard.sh --output exits with Bash's unbound-variable error instead of the intended usage message.
🤖 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 39 - 41,
The --output option handling in baremetal-wizard.sh reads OUTPUT from $2 without
first checking that an argument exists, which can trigger an unbound-variable
failure instead of usage handling. Update the case branch for --output to
validate that the next positional argument is present before assigning OUTPUT
and shifting, and route the missing-argument path through the script’s existing
usage/error flow so baremetal-wizard.sh --output reports a proper message.
fonta-rh
force-pushed
the
ocpedge-2774-baremetal-adopt
branch
from
daac159 to
2f8c4ae
Compare
openshift-ci
Bot
removed
the
needs-rebase
coderabbitai
Bot
added
the
ready-for-human-review
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deploy/openshift-clusters/scripts/baremetal-adopt.sh (1)
91-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign shell variable casing with the repository standard.
This new script uses lower-case local variables throughout. Please rename shell variables consistently to
UPPER_CASE, or document an explicit local-variable exception. As per coding guidelines, “Use meaningful variable names in UPPER_CASE.”🤖 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-adopt.sh` around lines 91 - 143, The new parsing logic in baremetal-adopt.sh uses lower-case local variables, which is inconsistent with the repository’s shell naming standard. Update the variables used in the main loop and parsing blocks (such as in_nodes, in_vars, key, val, name, rest, and the BMC-related locals) to UPPER_CASE, or add a clear explicit exception if local lowercase variables are intended. Keep the naming consistent throughout the script, including the variable assignments and condition checks in the same section.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/openshift-clusters/scripts/baremetal-adopt.sh`:
- Around line 8-16: The help text and generated comments in baremetal-adopt.sh
use the wrong script name, which can mislead users. Update the usage line and
any related generated output strings in the script to reference
baremetal-adopt.sh consistently, and check the help-printing logic or comment
block that emits these labels so the displayed command name matches the actual
script.
- Around line 169-187: The Redfish curl calls in the BMC helper functions are
exposing `${bmc_pass}` on the command line, so update the request flow in the
systems lookup and `verify_bmc` logic to avoid argv-based credentials. Use a
temporary restrictive curl config or netrc-based approach for authentication,
reference the existing `systems_json` fetch and `http_code` check, and ensure
any temporary secret file is removed immediately after the request completes.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/baremetal-adopt.sh`:
- Around line 91-143: The new parsing logic in baremetal-adopt.sh uses
lower-case local variables, which is inconsistent with the repository’s shell
naming standard. Update the variables used in the main loop and parsing blocks
(such as in_nodes, in_vars, key, val, name, rest, and the BMC-related locals) to
UPPER_CASE, or add a clear explicit exception if local lowercase variables are
intended. Keep the naming consistent throughout the script, including the
variable assignments and condition checks in the same section.
🪄 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: 77e14be3-4ba0-4be6-a3b0-3da7d3f23182
📒 Files selected for processing (5)
deploy/Makefiledeploy/openshift-clusters/.gitignoredeploy/openshift-clusters/inventory_baremetal.ini.sampledeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/baremetal-wizard.sh
✅ Files skipped from review due to trivial changes (2)
- deploy/openshift-clusters/.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-wizard.sh
| systems_json=$(curl -sk --connect-timeout 5 --max-time 10 \ | ||
| -u "${bmc_user}:${bmc_pass}" \ | ||
| "https://${bmc_ip}/redfish/v1/Systems/" 2>/dev/null) || return 1 | ||
|
|
||
| echo "${systems_json}" | jq -r '.Members[0]."@odata.id"' 2>/dev/null | ||
| } | ||
|
|
||
| verify_bmc() { | ||
| local name="$1" bmc_ip="$2" bmc_user="$3" bmc_pass="$4" | ||
| local rc=0 | ||
|
|
||
| printf " %-12s %-20s " "${name}" "${bmc_ip}" | ||
|
|
||
| # Verify Redfish root is reachable and credentials work | ||
| local http_code | ||
| http_code=$(curl -sk --connect-timeout 5 --max-time 10 \ | ||
| -o /dev/null -w '%{http_code}' \ | ||
| -u "${bmc_user}:${bmc_pass}" \ | ||
| "https://${bmc_ip}/redfish/v1/" 2>/dev/null) || http_code="000" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid passing BMC passwords in curl argv.
Both Redfish calls put ${bmc_pass} in the curl command line, which can leak through process listings while the request is running. Use a restrictive temporary curl config/netrc file and delete it after the request. As per coding guidelines, “Handle secrets and credentials securely.”
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 183-186: curl is invoked with -k/--insecure, which disables TLS certificate verification and exposes the connection to man-in-the-middle attacks. Remove the insecure flag and let curl validate the server certificate; if you need to trust a private CA, pin it with --cacert instead.
Context: curl -sk --connect-timeout 5 --max-time 10
-o /dev/null -w '%{http_code}'
-u "${bmc_user}:${bmc_pass}"
"https://${bmc_ip}/redfish/v1/"
Note: [CWE-295] Improper Certificate Validation.
(curl-insecure-tls-bash)
🤖 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-adopt.sh` around lines 169 - 187,
The Redfish curl calls in the BMC helper functions are exposing `${bmc_pass}` on
the command line, so update the request flow in the systems lookup and
`verify_bmc` logic to avoid argv-based credentials. Use a temporary restrictive
curl config or netrc-based approach for authentication, reference the existing
`systems_json` fetch and `http_code` check, and ensure any temporary secret file
is removed immediately after the request completes.
Source: Coding guidelines
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/baremetal-adopt.sh`:
- Around line 179-181: The Redfish boot MAC discovery in baremetal-adopt.sh is
using a malformed EthernetInterfaces URL built from system_id, so fix the
request path in the ifaces_json curl call to target the proper
EthernetInterfaces collection. If no MAC can be discovered after that, do not
write DISCOVERY_FAILED into ironic_nodes.json; instead make the script abort
immediately and stop generating a placeholder port entry. Use the existing
discovery flow around ifaces_json and the ironic_nodes.json write path to locate
the change.
🪄 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: 0eacb9ad-52cd-4098-b98e-98bd9394bb01
📒 Files selected for processing (3)
deploy/openshift-clusters/inventory_baremetal.ini.sampledeploy/openshift-clusters/scripts/baremetal-adopt.shdeploy/openshift-clusters/scripts/baremetal-wizard.sh
✅ Files skipped from review due to trivial changes (1)
- deploy/openshift-clusters/inventory_baremetal.ini.sample
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/openshift-clusters/scripts/baremetal-wizard.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
deploy/openshift-clusters/scripts/baremetal-adopt.sh (1)
179-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse uppercase names for the new shell locals.
The new helper introduces lowercase variables such as
boot_order,options_json,option_paths, andraw_mac. As per coding guidelines, shell scripts should useUPPER_CASEfor variable names.🤖 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-adopt.sh` around lines 179 - 216, The new helper in baremetal-adopt.sh uses lowercase shell locals like boot_order, options_json, option_paths, and raw_mac, which violates the shell variable naming convention. Update the local variable names in the boot-order parsing logic and the PXE IPv4/MAC matching block to UPPER_CASE, and make sure every reference inside the same section is renamed consistently so the flow in the boot order/BootOptions handling still works.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/openshift-clusters/scripts/baremetal-adopt.sh`:
- Around line 187-189: The curl URL in the BootOptions lookup is concatenating
system_id directly with BootOptions, which breaks Redfish paths like
/redfish/v1/Systems/1. Update the URL construction in the baremetal-adopt.sh
logic that sets options_json so it inserts the missing separator between the
existing system_id value and BootOptions, preserving the expected
/.../Systems/1/BootOptions/ path.
---
Nitpick comments:
In `@deploy/openshift-clusters/scripts/baremetal-adopt.sh`:
- Around line 179-216: The new helper in baremetal-adopt.sh uses lowercase shell
locals like boot_order, options_json, option_paths, and raw_mac, which violates
the shell variable naming convention. Update the local variable names in the
boot-order parsing logic and the PXE IPv4/MAC matching block to UPPER_CASE, and
make sure every reference inside the same section is renamed consistently so the
flow in the boot order/BootOptions handling still works.
🪄 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: 405def63-c36a-4905-8be9-78cc95bbd331
📒 Files selected for processing (1)
deploy/openshift-clusters/scripts/baremetal-adopt.sh
| # | ||
| # NOTE: This is separate from inventory.ini, which targets the hypervisor host. | ||
| # This file describes the physical baremetal nodes to be adopted as OpenShift nodes. | ||
| # inventory.ini → hypervisor (where dev-scripts runs) |
There was a problem hiding this comment.
Is a hypervisor necessary to deploy here ? Seems like overhead. I guess dev-scripts need to run somewhere ?
There was a problem hiding this comment.
No, there is no hypervisor deploy. These 2 comment lines are to describe the difference between inventory.ini and inventory_baremetal.ini
There was a problem hiding this comment.
ah, ok. Sorry this makes sense now that I have context of the existing inventory.ini
dhensel-rh
left a comment
dhensel-rh
left a comment
There was a problem hiding this comment.
Poka-yoke review — silent-wrong-output risks
Three places where the script produces output that looks correct but causes failures downstream:
1. JSON injection via string interpolation (baremetal-adopt.sh:290-315)
generate_ironic_nodes_json() interpolates BMC credentials directly into a heredoc. A password like P@ss"word or admin\n produces malformed JSON. The trailing jq . catches it, but the error is a cryptic parse failure with no hint which field broke.
Fix: Build the JSON with jq instead of string interpolation:
jq -n --arg addr "redfish://${bmc_address}/${system_id}" \
--arg user "${bmc_user}" \
--arg pass "${bmc_pass}" \
--arg mac "${boot_mac}" \
'{name: $name, driver: "redfish", driver_info: {address: $addr, username: $user, password: $pass}, ports: [{address: $mac}]}'2. DISCOVERY_FAILED written as a MAC address (baremetal-adopt.sh:278-281)
When Redfish MAC discovery fails, the script sets boot_mac="DISCOVERY_FAILED" and writes it into ironic_nodes.json as "address": "DISCOVERY_FAILED". This is syntactically valid JSON, so it passes any schema check and reaches dev-scripts, which fails later with an unrelated error.
Fix: Fail hard or track failures and exit non-zero after generation:
echo " ERROR: ${name}: could not discover boot MAC — set boot_mac in inventory" >&2
INCOMPLETE=trueThen after the loop: ${INCOMPLETE} && die "Incomplete artifacts — set missing boot_mac values in inventory".
3. --skip-verify still contacts BMCs for discovery (baremetal-adopt.sh:268-283)
The flag skips credential verification but Redfish discovery (system ID + boot MAC) still runs. A user passing --skip-verify because they're on a different network gets silent DISCOVERY_FAILED values baked into the artifact.
Fix: Either gate all BMC contact behind --skip-verify (requiring boot_mac to be set in inventory), or rename the flag to --skip-credential-check so the scope is clear.
- 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>
|
/retest-required |
openshift-ci
Bot
added
needs-rebase
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>
fonta-rh
force-pushed
the
ocpedge-2774-baremetal-adopt
branch
from
73d00d9 to
8d9c936
Compare
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>
| WIZ_PASSES=() | ||
| WIZ_MACS=() | ||
| WIZ_NODE_IPS=() | ||
|
|
There was a problem hiding this comment.
Do you need these two ?
| WIZ_PORTS=() | |
| WIZ_DATA_MACS=() |
dhensel-rh
left a comment
dhensel-rh
left a comment
There was a problem hiding this comment.
Poka-yoke review — 4 mistake-proofing findings (see inline comments).
| ############################################################################## | ||
|
|
||
| main() { | ||
| parse_args "$@" |
There was a problem hiding this comment.
Poka-yoke: --skip-verify + --verify-only are contradictory but accepted
With both flags set, the script skips verification, then prints "Verification complete" and exits — telling the user verification passed when nothing was verified.
Suggestion — add a mutual-exclusion check right after parse_args:
if ${SKIP_VERIFY} && ${VERIFY_ONLY}; then
die "--skip-verify and --verify-only are mutually exclusive"
fi| system_id="redfish/v1/Systems/1" | ||
| else | ||
| system_id=$(discover_redfish_system_id "${bmc_address}" "${bmc_user}" "${bmc_pass}" "${bmc_port}" 2>/dev/null) || true | ||
| system_id="${system_id:-/redfish/v1/Systems/1}" |
There was a problem hiding this comment.
Poka-yoke: silent fallback produces plausible-but-wrong artifacts
When discover_redfish_system_id fails, this silently falls back to /redfish/v1/Systems/1. The generated ironic_nodes.json looks correct but may point to the wrong system on a multi-system BMC (blade chassis). The failure only surfaces much later during deployment.
Suggestion — log when falling back so the user knows discovery failed:
if [[ -z "${system_id}" ]]; then
echo " WARNING: ${name}: Redfish system discovery failed, using default /redfish/v1/Systems/1" >&2
system_id="/redfish/v1/Systems/1"
fi| "${SCRIPT_DIR}/baremetal-wizard.sh" --output "${INVENTORY}" | ||
| fi | ||
|
|
||
| parse_inventory |
There was a problem hiding this comment.
Poka-yoke: validate required fields before expensive BMC calls
data_mac and node_ip are stored as empty strings here, then BMC verification runs (multiple Redfish round-trips per node). Only during artifact generation does the script die with "data_mac is required". All that network wait was wasted on something that could have been caught instantly.
Suggestion — validate right after parse_inventory:
parse_inventory
for ((i = 0; i < ${#NODE_NAMES[@]}; i++)); do
[[ -z "${NODE_DATA_MACS[$i]}" ]] && die "Node '${NODE_NAMES[$i]}': data_mac is required"
[[ -z "${NODE_IPS[$i]}" ]] && die "Node '${NODE_NAMES[$i]}': node_ip is required"
done| ;; | ||
| *) | ||
| echo "" | ||
| info "Starting over — re-enter node information" |
There was a problem hiding this comment.
Poka-yoke: typo in confirmation restarts the entire wizard
The *) catch-all restarts data entry from scratch. Typing "ye" instead of "yes", or any accidental keypress, discards all node data the user just entered — potentially several minutes of BMC details.
Suggestion — reject unknown input and re-prompt instead of restarting:
while true; do
read -rp "Proceed with this configuration? [Y/n/q]: " confirm
confirm="${confirm:-Y}"
case "${confirm}" in
[Yy]|[Yy]es) break 2 ;; # break outer loop
[Nn]|[Nn]o) info "Starting over"; break ;; # restart
[Qq]|[Qq]uit) die "Wizard cancelled by user" ;;
*) echo " Please enter Y, n, or q" >&2 ;;
esac
done
dhensel-rh
left a comment
dhensel-rh
left a comment
There was a problem hiding this comment.
Bug: wizard retry loop corrupts data (see inline comment).
| WIZ_NAMES=() | ||
| WIZ_IPS=() | ||
| WIZ_USERS=() | ||
| WIZ_PASSES=() | ||
| WIZ_MACS=() | ||
| WIZ_NODE_IPS=() |
There was a problem hiding this comment.
Bug: WIZ_PORTS and WIZ_DATA_MACS not reset on retry
This block resets 6 of the 8 arrays but misses WIZ_PORTS and WIZ_DATA_MACS. If the user answers "n" at the summary and re-enters data, ports and data MACs from the first round accumulate. On the second pass with 2 nodes, WIZ_PORTS has 4 entries — the inventory writer then indexes past the intended range and writes stale values from round 1.
Add the missing two:
WIZ_NAMES=()
WIZ_IPS=()
WIZ_USERS=()
WIZ_PASSES=()
WIZ_PORTS=()
WIZ_MACS=()
WIZ_DATA_MACS=()
WIZ_NODE_IPS=()
dhensel-rh
left a comment
dhensel-rh
left a comment
There was a problem hiding this comment.
Limitation: INI parser silently truncates passwords with spaces (see inline comments).
| rest="${line#* }" | ||
|
|
||
| local bmc_address="" bmc_user="" bmc_pass="" bmc_port="" boot_mac="" node_ip="" data_mac="" | ||
| for pair in ${rest}; do |
There was a problem hiding this comment.
Limitation: word-splitting silently truncates values containing spaces
for pair in ${rest} splits on whitespace. A password like bmc_pass=my secret parses as bmc_pass=my (truncated) and secret (dropped silently — no case branch matches bare words). The user gets no error; the password just doesn't work at deploy time.
Not necessarily worth fixing (the INI format inherently doesn't support unquoted spaces), but worth documenting the constraint in the sample file so users know to avoid spaces in passwords.
| # bmc_address - BMC/iDRAC/iLO management address (IP or hostname) | ||
| # bmc_user - BMC login username | ||
| # bmc_pass - BMC login password | ||
| # bmc_port - (optional) BMC Redfish port (default: 443) |
There was a problem hiding this comment.
Consider adding a note that values must not contain spaces (the parser uses whitespace as the field delimiter). e.g.:
# bmc_pass - BMC login password (must not contain spaces)
2 participants
Summary
baremetal-adopt.shto onboard existing baremetal nodes into the dev-scripts deployment workflow — parsesinventory_baremetal.ini, validates BMC credentials via Redfish, and generatesironic_nodes.json+config_baremetal_fencing.shbaremetal-wizard.shas a standalone interactive wizard with input validation (IPv4, MAC format), re-prompt on error, summary table with masked passwords, and Y/n/q confirmation flowinventory_baremetal.ini.sampletemplate andbaremetal-adopt,baremetal-verify,baremetal-wizardMake targetsJira: OCPEDGE-2774
Related: OCPEDGE-2775 (baremetal install via dev-scripts — consumes adoption artifacts)
Test plan
make baremetal-wizard— verify input validation, re-prompts, summary, and confirmationmake baremetal-adoptwith existinginventory_baremetal.ini— skips wizard, proceeds directlymake baremetal-verify— verify-only mode, no artifacts generatedironic_nodes.jsonschema matches dev-scripts expectationsconfig_baremetal_fencing.shcorrectly sources base config + adds baremetal overridesmake shellcheck— no new warnings🤖 Generated with Claude Code
Summary by CodeRabbit
inventory_baremetal.iniwith validated hostnames, BMC details, and optional PXE boot MAC (auto-discovery when omitted).--verify-onlyand--skip-verifysupport for safer credential checks.make helpwith a new “Baremetal Adoption” section describing the new commands.