OCPEDGE-2934: feat: add macAddress as alternative identifier for fencing credentials - #1916
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @fracappa. Thanks for your PR. I'm waiting for a openshift-metal3 member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
a0ee3a7 to
b1b2692
Compare
0c17b82 to
b1b2692
Compare
aaa2b9a to
8eb12dc
Compare
fracappa
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
/retest-required |
|
/retest |
|
Looks alright to me :) |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtantsur 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
lucaconsalvi
left a comment
lucaconsalvi
left a comment
There was a problem hiding this comment.
Thanks for this PR, Francesco! Clean approach to supporting macAddress-based fencing credentials — the identifier_key/identifier_value pattern in utils.sh is nicely done.
A few findings below — the first one (missing AGENT_MASTER_MACS in the baremetal code path) is a deployment blocker that affects all baremetal deployments, not just macAddress mode.
Minor: The PR description references FENCING_CREDENTIALS_IDENTIFIER (plural) but the code consistently uses FENCING_CREDENTIAL_IDENTIFIER (singular). Worth fixing for searchability.
| fi | ||
| AGENT_NODES_MACS+=("$node_mac") | ||
| if [[ "$node_type" == "master" ]]; then | ||
| AGENT_MASTER_MACS+=("$node_mac") |
There was a problem hiding this comment.
Bug: AGENT_MASTER_MACS is populated here in configure_node() (called by get_static_ips_and_macs()), but the alternative code path — get_baremetal_ips_and_macs() — never declares or populates this array.
Since generate_cluster_manifests() unconditionally serializes it ("${AGENT_MASTER_MACS[@]}"), and the script runs with set -euxo pipefail, this will crash with unbound variable on every NODES_PLATFORM=baremetal deployment — even with the default hostname mode.
Suggested fix — mirror the AGENT_MASTER_HOSTNAMES pattern in get_baremetal_ips_and_macs():
- Add
AGENT_MASTER_MACS=()to the initialization block - Add
AGENT_MASTER_MACS+=("$mac")inside theif (( i < NUM_MASTERS ))block
Also worth adding AGENT_MASTER_MACS=() to the init block in get_static_ips_and_macs() to match the established pattern.
| fi | ||
|
|
||
| # Controls whether fencing credentials use "hostname" or "macAddress" to identify nodes | ||
| export FENCING_CREDENTIAL_IDENTIFIER=${FENCING_CREDENTIAL_IDENTIFIER:-hostname} |
There was a problem hiding this comment.
Suggestion: Any value other than the exact string macAddress silently falls through to hostname mode in both code paths. A typo like macaddress or mac would be silently ignored.
The BMC_DRIVER validation just below establishes a precedent for early validation — worth adding a similar guard:
|
|
||
| if [[ "${FENCING_CREDENTIAL_IDENTIFIER}" == "macAddress" ]]; then | ||
| identifier_key="macAddress" | ||
| identifier_value=$(node_val ${idx} "ports[0].address") |
There was a problem hiding this comment.
Suggestion: node_val uses jq -r, which returns the literal string "null" with exit code 0 when a JSON path doesn't exist. If the node JSON lacks ports[0].address, the generated YAML would contain - macAddress: null — which YAML parsers interpret as a null value.
Consider adding a guard:
Also — the original code had a helpful comment explaining why the FQDN append is needed for IPv6/DualStack. That "why" was lost in the refactor. Worth restoring above the if [[ $IP_STACK != 'v4' ]] check.
| address: {{ master_bmc_addresses[i] }} | ||
| username: {{ master_bmc_usernames[i] }} | ||
| password: {{ master_bmc_passwords[i] }} | ||
| certificateVerification: {{ 'Disabled' if bmc_verify_cas[i] == "False" else 'Enabled' }} |
There was a problem hiding this comment.
Nit (pre-existing): This uses bmc_verify_cas[i] (the all-nodes array from line 6) instead of master_bmc_verify_cas[i] (the master-specific array set on line 44). Invisible in 2-master/0-worker TNF but technically wrong. Since the PR touches this line, good opportunity to fix.
40ff347 to
7353cda
Compare
Introduce FENCING_CREDENTIALS_IDENTIFIER env var to control whether fencing credentials identify nodes by hostname (default) or macAddress.
075bfb5 to
7613ce0
Compare
|
/lgtm |
|
@lucaconsalvi: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@fracappa: 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. |
5 participants
Introduce FENCING_CREDENTIALS_IDENTIFIER env var to control whether fencing credentials identify nodes by hostname (default) or macAddress.