Skc baremetal environment - #2023
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Failure
|
|
||
| - name: Undeploy baremetals in 'deploy failed' or 'error' state | ||
| ansible.builtin.command: | ||
| cmd: "{{ venv }}/bin/openstack baremetal node undeploy {{ inventory_hostname }}" |
There was a problem hiding this comment.
JohnG "Maybe lets move this into ./baremetal-4-clean.yml, at least for deploy failed."
|
@claudia-lola @assumptionsandg could you outline the next steps for this change? It's a massive PR at the moment and I think it would make sense to break it down into some smaller steps. |
We're dropping the Sushy environment for now... and will consider merging that later since it's a bit out-of-scope for this PR. I've rebased this on Jack's work and have started the initial work needed to merge the two. We still have some outstanding complications... such as ensuring the 'redfish' groups work with all possible boot interfaces and resolving documentation conflicts. I propose limiting our new scripts to running specifically on the 'redfish' groups since they have only been tested in this configuration and have BMC checks which specifically use the Redfish API. In addition to this... I will ensure the release note for this change mentions that existing users of this mixin will need to make sure nodes currently under 'baremetal-redfish' will need to be moved to 'baremetal-compute-redfish' due to overcloud baremetal support being merged in this change. |
There was a problem hiding this comment.
What is wrong with kayobe baremetal compute register?
There was a problem hiding this comment.
'kayobe baremetal compute register' is limited to only baremetal compute nodes... rather than including the overcloud. It's probably wise to keep a distinction between the two... while supporting both.
In addition to that... it's missing fields we need to configure interfaces individually on baremetal nodes compared to just relying on global Ironic configuration which hasn't been ideal at 6G... for instance when needing to set RAID interfaces etc.
dougszumski
left a comment
dougszumski
left a comment
There was a problem hiding this comment.
Thanks @assumptionsandg for trimming this back
I think it largely looks good
| --- | ||
|
|
||
| - name: Register baremetal compute nodes |
There was a problem hiding this comment.
nit: Change to "Install OpenStack client"
| {% endfor %} | ||
| --resource-class {{ ironic_resource_class }} \ | ||
| {% if ironic_boot_interface %} | ||
| --boot-interface {{ ironic_boot_interface }} \ |
There was a problem hiding this comment.
Please could we support --deploy-interface? We are going to need to start setting it to autodetect at some point for diskless. We can continue to set it to direct by default
| {% for key, value in ironic_properties.items() %} | ||
| --property {{ key }}={{ value }} \ | ||
| {% endfor %} | ||
| --resource-class {{ ironic_resource_class }} \ |
There was a problem hiding this comment.
At this point - please can we use {{ ironic_resource_class }}_enroll for the resource class, and then change it back at the end. This is to make it clear that the node is not ready for scheduling.
| controller_host: "{{ groups['controllers'][0] }}" | ||
|
|
||
| tasks: | ||
| - name: Check Ironic variables are defined |
There was a problem hiding this comment.
nit:L This check task could be split out into a separate playbook and included here and in other playbooks via an include task to avoid repeating it.
| url: "{{ ironic_redfish_address + '/redfish/v1' }}" | ||
| method: GET | ||
| status_code: 200 | ||
| validate_certs: false |
There was a problem hiding this comment.
validate_certs should be {{ redfish_verify_ca | bool }} given we define it above?
|
|
||
| Baremetal nodes are defined in the inventory located ``stackhpc-baremetal/inventory/hosts`` file. | ||
| This inventory can be hand-written or generated (e.g. from a Python script). | ||
| Each node must have the required Ironic and Redfish variables. |
There was a problem hiding this comment.
We now need to consider the various other hardware types - even if we simply tell people not to use the groups - they are there for legacy reasons.
| dependencies: | ||
| - ci-aio | ||
|
|
||
| Activate the environment using ``source kayobe-config/kayobe-env --environment stackhpc-baremetal`` |
There was a problem hiding this comment.
s/stackhpc-baremetal/baremetal
| Run the full baremetal workflow using:: | ||
|
|
||
| kayobe playbook run \ | ||
| etc/kayobe/environments/stackhpc-baremetal/ansible/baremetal-all.yml |
| - stackhpc_radosgw_usage_exporter_frontend_port | ||
| - stackhpc_radosgw_usage_exporter_backend_port | ||
| - internal_net_name | ||
| - "{{ internal_net_name }}_ips" |
There was a problem hiding this comment.
I think we need- "{{ oob_oc_net_name }}_ips" here too
| - gpu_group_map | ||
| - stackhpc_radosgw_usage_exporter_frontend_port | ||
| - stackhpc_radosgw_usage_exporter_backend_port | ||
| - internal_net_name |
There was a problem hiding this comment.
where is internal_net_name used?
|
|
||
| [baremetal-overcloud-idrac] | ||
| [baremetal-overcloud-ipmi] | ||
| [baremetal-overcloud-redfish] |
There was a problem hiding this comment.
should we create baremetal-overcloud-redfish-vmedia] and `baremetal-overcloud-redfish-ipxe] groups to support both mechanisms?
| # Default is 60 seconds | ||
| heartbeat_timeout = 360 | ||
| sync_local_state_interval = 360 | ||
| # Neccesary for virtual media boot |
There was a problem hiding this comment.
This comes via Kolla now, which is neat
| - node_show.rc != 0 | ||
| changed_when: false | ||
|
|
||
| # NOTE: The openstack.cloud.baremetal_node module cannot be used in this |
There was a problem hiding this comment.
We shifted chunks of this into the Kayobe node commands:
https://github.com/openstack/kayobe/blob/master/ansible/baremetal-compute-register.yml#L52
I think we should switch over to using those and extend wherever needed.
We can use groups to make a distinction between hypervisors and user facing Ironic nodes.
The end goal should be that this all lives in the Kayobe upstream code base, and we can test it there.
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
| @@ -1,2 +1,4 @@ | |||
| --- | |||
| kolla_enable_ironic: true | |||
| kolla_enable_ironic_dnsmasq: false # NOTE(hollie): need to double check this since it could break iPXE. | |||
There was a problem hiding this comment.
This is a little controversial in 2025.1 as people could be using inspection rules, but is the default in 2026.1 with kolla_inspector_enable_discovery: false.
If no one is using the environment, then it doesn't matter. Perhaps we could do a company survery.
There was a problem hiding this comment.
I do think we should make this environment enforce our current best practice, I just don't want to break anyone!
There was a problem hiding this comment.
Looks good. Seems like this config options have no effect on ironic-api.
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
10 similar comments
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
|
Happy Friday @claudia-lola, this is a friendly reminder that this PR is waiting for your changes or response. Please take a look when you have a moment! Note: Once your changes are ready, remove the |
📝 WalkthroughAdds a StackHPC baremetal environment with Kayobe documentation, Ansible workflows for enrolment, inspection, cleaning, recovery and provisioning, and Redfish diagnostics. Updates inventory groups, Ironic interfaces, Kolla settings and Ironic HTTP configuration. Removes obsolete inspector configuration and supports separate compute and overcloud baremetal groups. WalkthroughAdds a baremetal environment with Redfish-based Ironic enrolment, inspection, cleaning, recovery, image handling, and instance provisioning workflows. It also adds inventory groups, Ironic and Kolla configuration, progress metadata, diagnostics, recovery commands, and documentation. ChangesBaremetal environment
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to This PR adds bare-metal enrollment, inspection, cleaning, recovery, and provisioning, but the current head contains multiple concrete merge blockers: provisioning can reference a different image than the one uploaded, forced deletion can fail before deleting the server, host-group targeting can skip or misconfigure nodes, and virtual-media and Redfish traffic disable peer authentication. These issues can cause failed or misdirected lifecycle operations and permit interception or impersonation on management paths, so the PR is not merge-ready until they are corrected or explicitly accepted by owners. Sequence Diagram(s)sequenceDiagram
participant Kayobe
participant Controller as Controller host
participant Ironic
participant Redfish as Redfish BMC
Kayobe->>Controller: Run baremetal-all.yml
Controller->>Ironic: Enrol baremetal node
Controller->>Redfish: GET /redfish/v1
Redfish-->>Controller: HTTP 200
Controller->>Ironic: Run Redfish inspection
Controller->>Ironic: Run agent inspection
Ironic-->>Controller: Store inspection timestamps in extra
Controller->>Ironic: Clean node
Ironic-->>Controller: Node available
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (18 skipped: 18 unsupported.) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
etc/kayobe/environments/baremetal/ansible/provision-nova.yml (1)
14-116: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffConsider adding a rescue block for partial-resource cleanup.
Static analysis flags this block for missing error handling. If
Deploy Server(lines 91-104) fails afterCreate port(lines 75-83) succeeds, the created port is left behind with no cleanup path. Arescuesection that removes the port on failure would prevent orphaned resources.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 95390deb-a305-416f-8fe3-58f18c446ac9
📒 Files selected for processing (20)
etc/kayobe/environments/baremetal/README-fix-merge.rstetc/kayobe/environments/baremetal/ansible/baremetal-0-enroll-baremetal.ymletc/kayobe/environments/baremetal/ansible/baremetal-1-check-bmc-up.ymletc/kayobe/environments/baremetal/ansible/baremetal-2-ensure-redfish-inspect.ymletc/kayobe/environments/baremetal/ansible/baremetal-3-ensure-agent-inspect.ymletc/kayobe/environments/baremetal/ansible/baremetal-4-clean.ymletc/kayobe/environments/baremetal/ansible/baremetal-all.ymletc/kayobe/environments/baremetal/ansible/diagnose-baremetal.ymletc/kayobe/environments/baremetal/ansible/download-host-image.ymletc/kayobe/environments/baremetal/ansible/provision-nova.ymletc/kayobe/environments/baremetal/ansible/recover-baremetal.ymletc/kayobe/environments/baremetal/inventory/group_vars/baremetal-redfish/ironic.ymletc/kayobe/environments/baremetal/inventory/groupsetc/kayobe/environments/baremetal/ironic.ymletc/kayobe/environments/baremetal/kolla.ymletc/kayobe/environments/baremetal/kolla/config/ironic-inspector.confetc/kayobe/environments/baremetal/kolla/config/ironic.confetc/kayobe/environments/baremetal/kolla/config/ironic/ironic-api.confetc/kayobe/environments/baremetal/kolla/config/ironic/ironic-http-httpd.confetc/kayobe/kolla.yml
💤 Files with no reviewable changes (2)
- etc/kayobe/environments/baremetal/kolla/config/ironic-inspector.conf
- etc/kayobe/environments/baremetal/kolla/config/ironic/ironic-api.conf
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Ansible 2.17 lint with Python 3.10
- GitHub Check: Ansible 2.18 lint with Python 3.12
🧰 Additional context used
🪛 Checkov (3.3.10)
etc/kayobe/environments/baremetal/ansible/baremetal-2-ensure-redfish-inspect.yml
[medium] 35-44: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[low] 17-88: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
etc/kayobe/environments/baremetal/ansible/baremetal-4-clean.yml
[medium] 35-44: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[low] 18-150: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
etc/kayobe/environments/baremetal/ansible/diagnose-baremetal.yml
[low] 68-85: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
etc/kayobe/environments/baremetal/ansible/provision-nova.yml
[low] 14-116: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
etc/kayobe/environments/baremetal/ansible/baremetal-1-check-bmc-up.yml
[medium] 53-73: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[medium] 72-85: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[low] 26-105: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
etc/kayobe/environments/baremetal/ansible/baremetal-3-ensure-agent-inspect.yml
[medium] 35-44: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[low] 17-117: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
etc/kayobe/environments/baremetal/ansible/baremetal-0-enroll-baremetal.yml
[low] 42-107: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
etc/kayobe/environments/baremetal/ansible/recover-baremetal.yml
[medium] 37-49: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[low] 19-51: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
🔇 Additional comments (9)
etc/kayobe/environments/baremetal/ansible/download-host-image.yml (1)
28-33: 🔒 Security & PrivacyThe Pulp content URL uses HTTPS
etc/kayobe/pulp.ymlsetsstackhpc_release_pulp_schemeto"https". No code change is required for this concern.etc/kayobe/environments/baremetal/ironic.yml (1)
51-54: LGTM!Also applies to: 129-129
etc/kayobe/environments/baremetal/ansible/baremetal-all.yml (1)
1-11: LGTM!etc/kayobe/kolla.yml (1)
727-728: Pass the OOB network variables through to Kolla.The baremetal Ironic templates read
oob_net_ips, but this list forwards only the internal network mapping. Pass the configured OOB network name and its IP mapping, then resolve both mappings from their configured names.etc/kayobe/environments/baremetal/kolla/config/ironic.conf (1)
77-81: 🔒 Security & PrivacyUse trusted TLS certificates for IPA HTTPS endpoints.
ipa-insecure=1disables certificate validation. Remove it when IPA uses HTTPS. If private certificates are required, include the trusted CA in the IPA ramdisk.etc/kayobe/environments/baremetal/ansible/baremetal-4-clean.yml (2)
2-2: 📐 Maintainability & Code QualityUpdate the play title to match its purpose.
The title "Check baremetal node bmc is up" does not describe cleaning. This was already raised in a previous review.
3-3: 🗄️ Data Integrity & IntegrationConfirm the target group matches the group used by
recover-baremetal.yml.This play targets
baremetal-redfish, butrecover-baremetal.ymltargetsbaremetal-compute-redfishand imports this file. A previous reviewer already asked why this play is scoped tobaremetal-redfishinstead ofbaremetal. See the consolidated comment on this cross-file group mismatch.etc/kayobe/environments/baremetal/ansible/recover-baremetal.yml (2)
50-52: Omit thecmd:key, per a previous review comment.This is the same style point raised previously for this file.
60-61: 🗄️ Data Integrity & IntegrationNo issue:
baremetal-redfishincludesbaremetal-compute-redfishas a child group, so the imported playbook covers these hosts.
| extra_args: "{% if pip_upper_constraints_file %}-c {{ pip_upper_constraints_file }}{% endif %}" | ||
|
|
||
| - name: Ensure baremetal nodes are registered in ironic | ||
| hosts: baremetal |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a '^groups$' etc/kayobe/environments/baremetal/inventory \
-x sh -c 'echo "===== $1 ====="; sed -n "1,240p" "$1"' _ {}
printf '\n===== Redfish enrolment targets =====\n'
rg -n -C 4 'hosts: baremetal|hosts: baremetal-redfish|ironic_(driver|boot_interface|inspect_interface|management_interface|raid_interface)' \
etc/kayobe/environments/baremetalRepository: stackhpc/stackhpc-kayobe-config
Length of output: 17653
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '===== Applicable repository conventions ====='
head -5 /tmp/coderabbit-repo-knowledge/stackhpc-stackhpc-kayobe-config-578eaa86/*/*.md 2>/dev/null || true
printf '%s\n' '===== Enrolment playbook ====='
cat -n etc/kayobe/environments/baremetal/ansible/baremetal-0-enroll-baremetal.yml | sed -n '1,110p'
printf '%s\n' '===== Relevant inventory group variables ====='
for f in $(fd -a -t f . etc/kayobe/environments/baremetal/inventory/group_vars | rg '/(baremetal|baremetal-(idrac|ipmi|redfish))/(ironic|.*)\.yml$'); do
echo "===== $f ====="
cat -n "$f"
doneRepository: stackhpc/stackhpc-kayobe-config
Length of output: 9014
Target the interface-specific groups for enrolment.
baremetal includes IDRAC, IPMI, and Redfish hosts. This play requires Redfish variables and always passes Redfish driver information. IDRAC and IPMI hosts can therefore fail the assertions or receive incorrect settings. Target baremetal-redfish, or split enrolment by interface group.
| ansible.builtin.shell: | ||
| cmd: | | ||
| {{ venv }}/bin/openstack baremetal node create \ | ||
| --name {{ inventory_hostname }} \ | ||
| --driver {{ ironic_driver }} \ | ||
| --driver-info redfish_system_id={{ ironic_redfish_system_id }} \ | ||
| --driver-info redfish_address={{ ironic_redfish_address }} \ | ||
| {% if ironic_redfish_username %} | ||
| --driver-info redfish_username={{ ironic_redfish_username }} \ | ||
| {% endif %} | ||
| {% if ironic_redfish_password %} | ||
| --driver-info redfish_password={{ ironic_redfish_password }} \ | ||
| {% endif %} | ||
| --driver-info redfish_verify_ca={{ ironic_redfish_verify_ca }} \ | ||
| {% for key, value in ironic_properties.items() %} | ||
| --property {{ key }}={{ value }} \ | ||
| {% endfor %} | ||
| --resource-class {{ ironic_resource_class }} \ | ||
| {% if ironic_boot_interface %} | ||
| --boot-interface {{ ironic_boot_interface }} \ | ||
| {% endif %} | ||
| {% if ironic_inspect_interface %} | ||
| --inspect-interface {{ ironic_inspect_interface }} \ | ||
| {% endif %} | ||
| {% if ironic_management_interface %} | ||
| --management-interface {{ ironic_management_interface }} \ | ||
| {% endif %} | ||
| {% if ironic_network_interface %} | ||
| --network-interface {{ ironic_network_interface }} \ | ||
| {% endif %} | ||
| {% if ironic_raid_interface %} | ||
| --raid-interface {{ ironic_raid_interface }} \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository conventions ---'
head -5 /tmp/coderabbit-repo-knowledge/stackhpc-stackhpc-kayobe-config-578eaa86/*/*.md 2>/dev/null || true
printf '%s\n' '--- changed file ---'
sed -n '1,140p' etc/kayobe/environments/baremetal/ansible/baremetal-0-enroll-baremetal.yml
printf '%s\n' '--- relevant variable definitions and command usage ---'
rg -n -C 3 'ironic_(redfish_(username|password)|properties)|baremetal-0-enroll|openstack baremetal node create|ansible\.builtin\.(shell|command)' etc/kayobe/environments/baremetalRepository: stackhpc/stackhpc-kayobe-config
Length of output: 44663
Quote each generated OpenStack CLI value.
Because ansible.builtin.shell parses this command through a shell, whitespace in a Redfish credential or ironic_properties value creates extra arguments. The node-create command can then fail before enrolment. Use ansible.builtin.command with argv, or apply quote to each complete KEY=VALUE argument.
🧰 Tools
🪛 Checkov (3.3.10)
[low] 42-107: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
| - name: Check if bmc is up | ||
| ansible.builtin.set_fact: | ||
| kayobe_bmc_up: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_bmc_up') }}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat a missing BMC timestamp as empty.
Newly created nodes do not set extra.kayobe_bmc_up. dict.get('kayobe_bmc_up') therefore returns None, so Line 92 is false and the workflow never records the first successful BMC check. Default the missing key to ''.
Proposed fix
- kayobe_bmc_up: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_bmc_up') }}"
+ kayobe_bmc_up: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_bmc_up', '') }}"Also applies to: 84-92
🧰 Tools
🪛 Checkov (3.3.10)
[low] 26-105: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
| - name: Check BMC is up | ||
| ansible.builtin.uri: | ||
| url: "{{ ironic_redfish_address + '/redfish/v1' }}" | ||
| method: GET | ||
| status_code: 200 | ||
| validate_certs: false | ||
| timeout: 10 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use the configured Redfish certificate policy consistently across lifecycle checks.
The enrollment configuration exposes ironic_redfish_verify_ca, but the direct Redfish checks in baremetal-2-ensure-redfish-inspect.yml, baremetal-3-ensure-agent-inspect.yml, baremetal-4-clean.yml, and recover-baremetal.yml hardcode validate_certs: false. This allows controller-to-BMC traffic to accept an impersonated peer and can influence inspection, cleaning, and recovery decisions. Pass ironic_redfish_verify_ca to each check, or document and enforce an intentional equivalent trust boundary.
📍 Affects 3 files
etc/kayobe/environments/baremetal/ansible/baremetal-2-ensure-redfish-inspect.yml#L35-L41(this comment)etc/kayobe/environments/baremetal/ansible/baremetal-3-ensure-agent-inspect.yml#L36-L41etc/kayobe/environments/baremetal/ansible/baremetal-4-clean.yml#L35-L41
| - name: Check for agent inspection details | ||
| ansible.builtin.set_fact: | ||
| kayobe_agent_inspect_done: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_agent_inspect_done') }}" | ||
| kayobe_clean_done: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_clean_done') }}" | ||
| network_interface: "{{ (node_show.stdout | from_json)['network_interface'] }}" | ||
| provision_state: "{{ (node_show.stdout | from_json)['provision_state'] }}" | ||
| node_maintenance: "{{ (node_show.stdout | from_json)['maintenance'] }}" | ||
|
|
||
| - name: Fail if not agent inspection done | ||
| ansible.builtin.fail: | ||
| msg: "{{ inventory_hostname }} has not been inspected" | ||
| when: | ||
| - kayobe_agent_inspect_done == "" | ||
|
|
||
| - name: Fail if not in manageable or clean failed state | ||
| ansible.builtin.fail: | ||
| msg: "{{ inventory_hostname }} has the wrong provision_state: {{ provision_state }}" | ||
| when: | ||
| - provision_state not in ["manageable", "clean failed"] | ||
| - kayobe_clean_done == "" | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the .get() default so "not done" checks work correctly.
Line 45, line 46, and their equivalents in diagnose-baremetal.yml all call .get('key') without a default. If the extra dict does not contain the key at all (the normal case before the first inspection or clean), .get() returns None. None is never equal to "", so every downstream check that tests kayobe_agent_inspect_done == "" or kayobe_clean_done == "" (lines 55, 62, 91, 103, 115, 127, 138, 148) fails to detect the "not yet done" state. This can let a node bypass the "Fail if not agent inspection done" gate and the RAID/network-interface configuration steps that are meant to run only before the first clean.
Set an explicit empty-string default on both .get() calls.
🐛 Proposed fix
- name: Check for agent inspection details
ansible.builtin.set_fact:
- kayobe_agent_inspect_done: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_agent_inspect_done') }}"
- kayobe_clean_done: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_clean_done') }}"
+ kayobe_agent_inspect_done: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_agent_inspect_done', '') }}"
+ kayobe_clean_done: "{{ (node_show.stdout | from_json)['extra'].get('kayobe_clean_done', '') }}"Also applies to: 91-91, 103-103, 115-115, 127-127, 138-138, 148-148
🧰 Tools
🪛 Checkov (3.3.10)
[medium] 35-44: Ensure that certificate validation isn't disabled with uri
(CKV_ANSIBLE_1)
[low] 18-150: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
| ironic_redfish_address: | ||
| ironic_redfish_username: | ||
| ironic_redfish_password: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty Redfish credentials are not caught until later in the workflow.
ironic_redfish_username and ironic_redfish_password default to empty here, instead of resolving from secrets_redfish_baremetal_username/secrets_redfish_baremetal_password. In baremetal-0-enroll-baremetal.yml, the assert checks only that ironic_redfish_address is defined, and the --driver-info redfish_username=.../redfish_password=... arguments are only added when the variables are truthy. A host that does not override these variables in host_vars enrols without any BMC credentials, and the failure only appears later, at the first Redfish call that needs authentication.
Document that every host must set these variables, or add a check that fails enrolment when they are empty (if credentials are required for the target BMC).
| [baremetal-compute:children] | ||
| baremetal-idrac | ||
| baremetal-ipmi | ||
| baremetal-redfish | ||
| baremetal-compute-idrac | ||
| baremetal-compute-ipmi | ||
| baremetal-compute-redfish | ||
|
|
||
| [baremetal-overcloud:children] | ||
| baremetal-overcloud-idrac | ||
| baremetal-overcloud-ipmi | ||
| baremetal-overcloud-redfish | ||
|
|
||
| [baremetal-idrac:children] | ||
| baremetal-compute-idrac | ||
| baremetal-overcloud-idrac | ||
|
|
||
| [baremetal-ipmi:children] | ||
| baremetal-compute-ipmi | ||
| baremetal-overcloud-ipmi | ||
|
|
||
| [baremetal-redfish:children] | ||
| baremetal-compute-redfish | ||
| baremetal-overcloud-redfish | ||
|
|
||
| [baremetal-compute-is-bonded] | ||
| [baremetal:children] | ||
| baremetal-compute | ||
| baremetal-overcloud |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Search for lingering references to the removed is-bonded group.
set -euo pipefail
rg -n -C3 'baremetal-compute-is-bonded|is-bonded' --hidden -g '!.git'Repository: stackhpc/stackhpc-kayobe-config
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository guidance ---'
find /tmp/coderabbit-repo-knowledge/stackhpc-stackhpc-kayobe-config-578eaa86 -type f -name '*.md' -print \
-exec sh -c 'printf "\n### %s\n" "$1"; head -80 "$1"' sh {} \;
printf '%s\n' '--- relevant inventory and change context ---'
cat -n etc/kayobe/environments/baremetal/inventory/groups
git diff -- etc/kayobe/environments/baremetal/inventory/groups
printf '%s\n' '--- exact group references in configuration files ---'
rg -n -C3 --hidden -g '!.git' -g '!*.pyc' \
'baremetal-compute-is-bonded|baremetal-compute|is-bonded' \
etc/kayobe environments 2>/dev/null || trueRepository: stackhpc/stackhpc-kayobe-config
Length of output: 8384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- add-port-groups playbook ---'
cat -n etc/kayobe/environments/baremetal/ansible/add-port-groups.yml
printf '%s\n' '--- base baremetal inventory groups ---'
sed -n '108,125p' etc/kayobe/inventory/groups
printf '%s\n' '--- current group declarations ---'
rg -n '^\[(baremetal-compute-is-bonded|baremetal-compute|baremetal-overcloud|baremetal)\]($|:children)' \
etc/kayobe/environments/baremetal/inventory/groups etc/kayobe/inventory/groupsRepository: stackhpc/stackhpc-kayobe-config
Length of output: 7238
Restore baremetal-compute-is-bonded or update add-port-groups.yml.
add-port-groups.yml targets the removed group. The play will not process baremetal compute hosts, so it will not create or associate their Ironic port groups.
| # boot iso needs to be reachable from OOB network | ||
| external_http_url = http://{% if oob_ip %}{{ oob_ip }}{% endif %}:8089 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- repository conventions ---'
find /tmp/coderabbit-repo-knowledge/stackhpc-stackhpc-kayobe-config-578eaa86 -maxdepth 2 -type f -name '*.md' -print
printf '%s\n' '--- convention headers ---'
for f in /tmp/coderabbit-repo-knowledge/stackhpc-stackhpc-kayobe-config-578eaa86/*/*.md; do
[ -f "$f" ] || continue
printf '\n### %s\n' "$f"
head -40 "$f"
done
printf '%s\n' '--- referenced files ---'
cat -n etc/kayobe/environments/baremetal/kolla/config/ironic.conf | sed -n '35,70p'
cat -n etc/kayobe/environments/baremetal/kolla/config/ironic/ironic-http-httpd.conf | sed -n '1,45p'
printf '%s\n' '--- directly related definitions/usages ---'
rg -n -S --glob '!*.md' 'external_http_url|ironic_http_port|httpboot|virtual.media|virtual_media|boot_iso|boot-from|https' etc kayobe playbooks roles 2>/dev/null | head -240Repository: stackhpc/stackhpc-kayobe-config
Length of output: 25446
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Redfish and deployment configuration ---'
cat -n etc/kayobe/environments/baremetal/inventory/group_vars/baremetal-redfish/ironic.yml | sed -n '1,80p'
cat -n etc/kayobe/environments/baremetal/ironic.yml | sed -n '1,50p'
printf '%s\n' '--- related local configuration ---'
rg -n -S 'ironic_http|kolla_enable_tls|tls|ssl|certificate|redfish-virtual-media|external_http_url' etc/kayobe/environments/baremetal etc/kayobe/inventory/group_vars 2>/dev/null | head -160Repository: stackhpc/stackhpc-kayobe-config
Length of output: 6128
🌐 Web query:
site:docs.openstack.org/ironic external_http_url redfish-virtual-media HTTPS
💡 Result:
In the context of OpenStack Ironic's Redfish driver, the settings deploy.http_url and external_http_url are used to control the base URL provided to a Baseboard Management Controller (BMC) when performing virtual media operations [1][2]. The external_http_url parameter is specifically intended to provide a routable URL that the remote BMC can use to access deployment artifacts (such as ISO images) when the Ironic conductor is operating in an environment where the internal provisioning network is not directly accessible to the BMC [1]. Key points regarding these settings: 1. Functionality: When the Ironic driver manages a node, it generates a URL pointing the BMC to the deployment files. If external_http_url is configured (either globally in the Ironic configuration or as a specific node property), it overrides the default deploy.http_url [1]. 2. HTTPS Support: The Redfish virtual media interface generally supports both HTTP and HTTPS protocols for image retrieval [3][4]. The specific protocol used depends on the URL provided to the BMC; if an HTTPS URL is passed (e.g., via external_http_url or a Swift temporary URL), the BMC must be capable of establishing an HTTPS connection to that location [3][2][4]. 3. Node-Specific Overrides: While these can be set globally, they can also be defined on a per-node basis using driver_info[external_http_url], which takes precedence over global configuration settings [1]. This mechanism allows operators to deploy nodes across different network segments (Layer 3 deployments) by ensuring the BMC receives a reachable path to the necessary boot media [1].
Citations:
- 1: https://docs.openstack.org/ironic/latest/admin/dhcp-less.html
- 2: https://docs.openstack.org/ironic/2026.1/admin/drivers/redfish.html
- 3: https://docs.openstack.org/ironic/latest/contributor/api/ironic.drivers.modules.redfish.management.html
- 4: https://docs.openstack.org/ironic/latest/admin/ramdisk-boot.html
Other (CWE-494): Download of Code Without Integrity Check
Reachability: External · Exploitability: Difficult
Serve virtual-media boot artefacts through authenticated HTTPS.
external_http_url advertises http://...:8089, and Apache serves /var/lib/ironic/httpboot without TLS. The redfish-virtual-media interface gives the BMC this URL. An attacker who intercepts OOB traffic can replace the ISO before the BMC mounts it. Configure TLS with a certificate trusted by the BMC, and change external_http_url to https://...:8089.
etc/kayobe/environments/baremetal/kolla/config/ironic.conf#L53-L54etc/kayobe/environments/baremetal/kolla/config/ironic/ironic-http-httpd.conf#L5-L24
📍 Affects 2 files
etc/kayobe/environments/baremetal/kolla/config/ironic.conf#L53-L54(this comment)etc/kayobe/environments/baremetal/kolla/config/ironic/ironic-http-httpd.conf#L5-L24
| @@ -0,0 +1,69 @@ | |||
| Baremetal Environment | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider merging this file into the main README.
A previous reviewer suggested merging this content into README.rst rather than keeping it as a separate README-fix-merge.rst file. The file name still indicates a temporary merge artifact.
| Inventory | ||
| --------- | ||
|
|
||
| Baremetal nodes are defined in the inventory located ``stackhpc-baremetal/inventory/hosts`` file. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the environment name used in the setup and run instructions.
The instructions use the path and environment name stackhpc-baremetal. The actual environment directory in this PR is baremetal (etc/kayobe/environments/baremetal/). A user who copies these commands cannot activate the environment or run the playbook. This was already raised in a previous review round and is still unresolved.
📝 Proposed fix
-Baremetal nodes are defined in the inventory located ``stackhpc-baremetal/inventory/hosts`` file.
+Baremetal nodes are defined in the inventory located ``baremetal/inventory/hosts`` file.-Create a ``.kayobe-environment`` file in the base of stackhpc-baremetal environment and add your
+Create a ``.kayobe-environment`` file in the base of the baremetal environment and add your-Activate the environment using ``source kayobe-config/kayobe-env --environment stackhpc-baremetal``
+Activate the environment using ``source kayobe-config/kayobe-env --environment baremetal`` kayobe playbook run \
- etc/kayobe/environments/stackhpc-baremetal/ansible/baremetal-all.yml
+ etc/kayobe/environments/baremetal/ansible/baremetal-all.ymlAlso applies to: 26-27, 33-33, 40-41
10 participants
No description provided.