This repository was archived by the owner on Sep 9, 2026. It is now read-only.
Repository navigation
OSAC-1558: add bm_host_metal3_provisioning template role - #358
Merged
openshift-merge-bot[bot] merged 3 commits intoJun 19, 2026
Merged
Conversation
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Nick Carboni <ncarboni@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Nick Carboni <ncarboni@redhat.com>
…role The role now reads sshKey and userDataSecret from templateParameters, builds cloud-init userData, creates a Secret in the BMH namespace, and patches the BareMetalHost with spec.userData. Supports SSH key only, user data only, or merged SSH key into existing cloud-config. Cleanup on deprovision clears spec.userData and deletes the userData Secret. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Nick Carboni <ncarboni@redhat.com>
|
@carbonin: This pull request references OSAC-1558 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughTwo new Ansible task files are added to the ChangesMetal3 BareMetalHost Provisioning Role
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml`:
- Around line 63-139: The tasks that handle sensitive user data and credentials
need to suppress logging to prevent exposure in failures or verbose output. Add
`no_log: true` to the following tasks: "Extract user data content from Secret",
"Generate cloud-init with SSH key only", "Use existing user data as-is (no SSH
key to merge)", "Merge SSH key into existing cloud-init user data" block, and
"Create userData Secret in BMH namespace". This will prevent the
`existing_user_data`, `final_user_data`, and Secret `stringData` containing
credentials and bootstrap material from being logged or displayed in Ansible
output.
- Around line 26-32: Add the kubeconfig parameter to all Kubernetes module calls
in this file to ensure they target the remote cluster instead of the default
Kubernetes context. For every kubernetes.core.k8s_info and kubernetes.core.k8s
task (including the "Get BareMetalHost" task, and those at the other mentioned
line ranges), add the argument kubeconfig: "{{ kubeconfig | default(omit) }}" to
the module parameters. This follows the coding guideline to always include
osac.service.common to get remote_cluster_kubeconfig before creating Kubernetes
resources on remote clusters.
- Around line 84-86: The "Extract user data content from Secret" task at line 86
reads from `.data.userdata` (lowercase u), but the role creates the Secret with
the key `userData` (capital U) at line 139 in the stringData section. These key
names must match for the role to work correctly with user-provided Secrets.
Change the reference in the set_fact task from `.data.userdata` to
`.data.userData` to be consistent with the key that is actually written when the
Secret is created.
- Around line 16-19: The task "Parse externalHostID into namespace and name"
splits the externalHostID without validation, which causes cryptic Jinja index
errors on malformed input instead of clear validation failures. Add a validation
task before this task to ensure bare_metal_instance.spec.externalHostID exists
and contains exactly one forward slash to split into two parts. If validation
fails, use the fail module to provide a clear error message explaining the
expected format. Only proceed to the set_fact task with the split operation
after validation succeeds.
- Around line 49-53: The already_provisioned fact in the "Check if BMH is
already provisioned with the correct image" task only compares the image URL but
ignores checksum validation. Extend the comparison logic to also verify that
both the imageChecksum and checksumType from template_params match the
corresponding values in bmh_info.status.provisioning.image. Add these additional
conditions to the already_provisioned fact so that a provisioning task is
triggered when either the URL or checksum fields differ, preventing incomplete
provisioning when only the checksum changes.
- Around line 88-96: The merge path (around lines 119-127) uses the folded
scalar indicator `>-` for the final_user_data block when it should use the
literal scalar indicator `|` to preserve line breaks and the cloud-config header
format, matching the approach used in the SSH-only path. Change the `>-` to `|`
in the merge path's final_user_data block to ensure the cloud-config YAML
content maintains proper formatting with the header and YAML structure on
separate lines instead of being folded into a single line.
In
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/delete.yaml`:
- Around line 7-13: The kubernetes.core.k8s_info module call is missing the
kubeconfig parameter, which can cause the task to target the wrong cluster
context when deprovisioning resources. Add the kubeconfig parameter `kubeconfig:
"{{ kubeconfig | default(omit) }}"` to the kubernetes.core.k8s_info task shown
in the diff, and apply the same kubeconfig parameter addition to all other
Kubernetes module calls in this file including the k8s module invocations at the
line ranges mentioned (39-50, 52-61, 63-69, 71-77) to ensure consistent cluster
targeting across all resource operations.
- Around line 27-37: The task "Remove image and networkData from BMH" is
currently gated only on the bmh_has_image condition being true, but this leaves
stale references to Secrets when spec.userData or spec.networkData are present
without an image URL being set. Modify the logic to ensure that spec.userData
and spec.networkData are cleared from the BMH regardless of whether
bmh_has_image is true, so that dangling Secret references are not left behind
after Secret cleanup operations.
- Around line 2-5: The task that sets bmh_namespace and bmh_name by splitting
bare_metal_instance.spec.externalHostID assumes the value contains exactly one
slash without any validation. If the externalHostID is malformed or empty, the
split operation will fail with an unhelpful Jinja index error. Add a validation
step before the split that checks whether externalHostID is in the correct
format (contains exactly one slash resulting in two parts when split), and if
validation fails, use a task to fail with a clear error message that explains
the expected format of externalHostID.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a4221858-87cf-4daa-9560-20652a7e8115
📒 Files selected for processing (2)
collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yamlcollections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/delete.yaml
Comment on lines
+16
to
+19
| - name: Parse externalHostID into namespace and name | ||
| ansible.builtin.set_fact: | ||
| bmh_namespace: "{{ bare_metal_instance.spec.externalHostID.split('/')[0] }}" | ||
| bmh_name: "{{ bare_metal_instance.spec.externalHostID.split('/')[1] }}" |
There was a problem hiding this comment.
Validate externalHostID before splitting it.
Line 19 assumes spec.externalHostID is exactly namespace/name; malformed input fails with a Jinja index error before the role can produce an actionable validation failure.
Proposed fix
+- name: Validate externalHostID format
+ ansible.builtin.assert:
+ that:
+ - (bare_metal_instance.spec.externalHostID | default('')) is match('^[^/]+/[^/]+$')
+ fail_msg: "spec.externalHostID must be '<namespace>/<name>'"
+
- name: Parse externalHostID into namespace and name
ansible.builtin.set_fact:
bmh_namespace: "{{ bare_metal_instance.spec.externalHostID.split('/')[0] }}"
bmh_name: "{{ bare_metal_instance.spec.externalHostID.split('/')[1] }}"🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml`
around lines 16 - 19, The task "Parse externalHostID into namespace and name"
splits the externalHostID without validation, which causes cryptic Jinja index
errors on malformed input instead of clear validation failures. Add a validation
task before this task to ensure bare_metal_instance.spec.externalHostID exists
and contains exactly one forward slash to split into two parts. If validation
fails, use the fail module to provide a clear error message explaining the
expected format. Only proceed to the set_fact task with the split operation
after validation succeeds.
Comment on lines
+26
to
+32
| - name: Get BareMetalHost | ||
| kubernetes.core.k8s_info: | ||
| api_version: metal3.io/v1alpha1 | ||
| kind: BareMetalHost | ||
| namespace: "{{ bmh_namespace }}" | ||
| name: "{{ bmh_name }}" | ||
| register: bmh_result |
There was a problem hiding this comment.
Pass the remote kubeconfig to every Kubernetes module call.
These kubernetes.core.k8s_info / kubernetes.core.k8s tasks omit kubeconfig, so the role can act against the controller/default Kubernetes context instead of the remote cluster prepared for OSAC provisioning.
Proposed fix pattern
kubernetes.core.k8s_info:
+ kubeconfig: "{{ kubeconfig | default(omit) }}"
api_version: metal3.io/v1alpha1
kind: BareMetalHost kubernetes.core.k8s:
+ kubeconfig: "{{ kubeconfig | default(omit) }}"
state: present
definition:Apply the same module argument to every Kubernetes task in this file.
As per coding guidelines, “Always include osac.service.common to get remote_cluster_kubeconfig before creating K8s resources on remote clusters.” Based on learnings, use kubeconfig: "{{ kubeconfig | default(omit) }}" for this collection’s role task YAMLs.
Also applies to: 69-75, 129-139, 141-152, 190-197, 199-208
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml`
around lines 26 - 32, Add the kubeconfig parameter to all Kubernetes module
calls in this file to ensure they target the remote cluster instead of the
default Kubernetes context. For every kubernetes.core.k8s_info and
kubernetes.core.k8s task (including the "Get BareMetalHost" task, and those at
the other mentioned line ranges), add the argument kubeconfig: "{{ kubeconfig |
default(omit) }}" to the module parameters. This follows the coding guideline to
always include osac.service.common to get remote_cluster_kubeconfig before
creating Kubernetes resources on remote clusters.
Sources: Coding guidelines, Learnings
Comment on lines
+49
to
+53
| - name: Check if BMH is already provisioned with the correct image | ||
| ansible.builtin.set_fact: | ||
| already_provisioned: >- | ||
| {{ (bmh_info.status.provisioning.state | default('') == 'provisioned') and | ||
| (bmh_info.status.provisioning.image.url | default('') == template_params.imageURL) }} |
There was a problem hiding this comment.
Include checksum fields in the idempotency guard.
The skip condition only compares the image URL, so a reused URL with a changed imageChecksum or checksumType reports success while leaving the host on the previous image content.
Proposed fix
already_provisioned: >-
{{ (bmh_info.status.provisioning.state | default('') == 'provisioned') and
- (bmh_info.status.provisioning.image.url | default('') == template_params.imageURL) }}
+ (bmh_info.spec.image.url | default('') == template_params.imageURL) and
+ (bmh_info.spec.image.checksum | default('') == (template_params.imageChecksum | default('')) ) and
+ (bmh_info.spec.image.checksumType | default('auto') == (template_params.checksumType | default('auto')) ) }}🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml`
around lines 49 - 53, The already_provisioned fact in the "Check if BMH is
already provisioned with the correct image" task only compares the image URL but
ignores checksum validation. Extend the comparison logic to also verify that
both the imageChecksum and checksumType from template_params match the
corresponding values in bmh_info.status.provisioning.image. Add these additional
conditions to the already_provisioned fact so that a provisioning task is
triggered when either the URL or checksum fields differ, preventing incomplete
provisioning when only the checksum changes.
Comment on lines
+63
to
+139
| - name: Build userData content | ||
| when: ssh_key | length > 0 or user_data_secret_ref | length > 0 | ||
| block: | ||
| - name: Read userDataSecret from BareMetalInstance namespace | ||
| when: user_data_secret_ref | length > 0 | ||
| block: | ||
| - name: Fetch userDataSecret | ||
| kubernetes.core.k8s_info: | ||
| api_version: v1 | ||
| kind: Secret | ||
| namespace: "{{ bare_metal_instance.metadata.namespace }}" | ||
| name: "{{ user_data_secret_ref }}" | ||
| register: user_data_secret_result | ||
|
|
||
| - name: Fail if userDataSecret not found | ||
| ansible.builtin.fail: | ||
| msg: >- | ||
| Secret '{{ user_data_secret_ref }}' not found in namespace | ||
| '{{ bare_metal_instance.metadata.namespace }}' | ||
| when: user_data_secret_result.resources | length == 0 | ||
|
|
||
| - name: Extract user data content from Secret | ||
| ansible.builtin.set_fact: | ||
| existing_user_data: "{{ user_data_secret_result.resources[0].data.userdata | b64decode }}" | ||
|
|
||
| - name: Generate cloud-init with SSH key only | ||
| when: | ||
| - ssh_key | length > 0 | ||
| - user_data_secret_ref | length == 0 | ||
| ansible.builtin.set_fact: | ||
| final_user_data: | | ||
| #cloud-config | ||
| ssh_authorized_keys: | ||
| - {{ ssh_key }} | ||
|
|
||
| - name: Use existing user data as-is (no SSH key to merge) | ||
| when: | ||
| - ssh_key | length == 0 | ||
| - user_data_secret_ref | length > 0 | ||
| ansible.builtin.set_fact: | ||
| final_user_data: "{{ existing_user_data }}" | ||
|
|
||
| - name: Merge SSH key into existing cloud-init user data | ||
| when: | ||
| - ssh_key | length > 0 | ||
| - user_data_secret_ref | length > 0 | ||
| block: | ||
| - name: Fail if user data is not cloud-config format | ||
| ansible.builtin.fail: | ||
| msg: >- | ||
| Cannot inject SSH key into user data that is not cloud-config format. | ||
| User data must start with '#cloud-config' to support SSH key injection. | ||
| Either remove the ssh_key parameter or provide user data in cloud-config | ||
| format with ssh_authorized_keys included. | ||
| when: not (existing_user_data | trim is match('^#cloud-config')) | ||
|
|
||
| - name: Parse existing cloud-init and merge SSH key | ||
| ansible.builtin.set_fact: | ||
| final_user_data: >- | ||
| #cloud-config | ||
| {{ (existing_user_data | regex_replace('^#cloud-config\s*\n?', '') | from_yaml | default({})) | ||
| | combine({'ssh_authorized_keys': | ||
| ((existing_user_data | regex_replace('^#cloud-config\s*\n?', '') | from_yaml | default({})).ssh_authorized_keys | default([])) | ||
| + [ssh_key]}) | ||
| | to_nice_yaml }} | ||
|
|
||
| - name: Create userData Secret in BMH namespace | ||
| kubernetes.core.k8s: | ||
| state: present | ||
| definition: | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: "{{ bmh_name }}-user-data" | ||
| namespace: "{{ bmh_namespace }}" | ||
| stringData: | ||
| userData: "{{ final_user_data }}" |
There was a problem hiding this comment.
Suppress Ansible logging for cloud-init and Secret payloads.
existing_user_data, final_user_data, and Secret stringData can contain credentials or bootstrap material; without no_log: true, failures or verbose AAP output can expose them.
Proposed fix pattern
- name: Build userData content
+ no_log: true
when: ssh_key | length > 0 or user_data_secret_ref | length > 0
block: - name: Create networkData Secret
+ no_log: true
when: template_params.networkData | default('') | length > 0
kubernetes.core.k8s:Also applies to: 141-152
🧰 Tools
🪛 Checkov (3.3.1)
[low] 63-142: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
[low] 66-89: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
[low] 105-130: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml`
around lines 63 - 139, The tasks that handle sensitive user data and credentials
need to suppress logging to prevent exposure in failures or verbose output. Add
`no_log: true` to the following tasks: "Extract user data content from Secret",
"Generate cloud-init with SSH key only", "Use existing user data as-is (no SSH
key to merge)", "Merge SSH key into existing cloud-init user data" block, and
"Create userData Secret in BMH namespace". This will prevent the
`existing_user_data`, `final_user_data`, and Secret `stringData` containing
credentials and bootstrap material from being logged or displayed in Ansible
output.
Comment on lines
+84
to
+86
| - name: Extract user data content from Secret | ||
| ansible.builtin.set_fact: | ||
| existing_user_data: "{{ user_data_secret_result.resources[0].data.userdata | b64decode }}" |
There was a problem hiding this comment.
Read the same userData Secret key that this role writes.
Line 86 reads .data.userdata, but Line 139 creates stringData.userData; a user-provided Secret following this role’s own key contract will fail or decode the wrong field.
Proposed fix
+ - name: Fail if userDataSecret is missing userData key
+ ansible.builtin.fail:
+ msg: "Secret '{{ user_data_secret_ref }}' must contain data.userData"
+ when: user_data_secret_result.resources[0].data.userData is not defined
+
- name: Extract user data content from Secret
ansible.builtin.set_fact:
- existing_user_data: "{{ user_data_secret_result.resources[0].data.userdata | b64decode }}"
+ existing_user_data: "{{ user_data_secret_result.resources[0].data.userData | b64decode }}"Also applies to: 129-139
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml`
around lines 84 - 86, The "Extract user data content from Secret" task at line
86 reads from `.data.userdata` (lowercase u), but the role creates the Secret
with the key `userData` (capital U) at line 139 in the stringData section. These
key names must match for the role to work correctly with user-provided Secrets.
Change the reference in the set_fact task from `.data.userdata` to
`.data.userData` to be consistent with the key that is actually written when the
Secret is created.
Comment on lines
+88
to
+96
| - name: Generate cloud-init with SSH key only | ||
| when: | ||
| - ssh_key | length > 0 | ||
| - user_data_secret_ref | length == 0 | ||
| ansible.builtin.set_fact: | ||
| final_user_data: | | ||
| #cloud-config | ||
| ssh_authorized_keys: | ||
| - {{ ssh_key }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine the file
fd -t f "create.yaml" | grep "bm_host_metal3_provisioning"Repository: osac-project/osac-aap
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Get file size and examine the specific lines
if [ -f "collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml" ]; then
wc -l "collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml"
echo "=== Lines 80-100 ==="
sed -n '80,100p' "collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml"
echo "=== Lines 110-135 ==="
sed -n '110,135p' "collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml"
fiRepository: osac-project/osac-aap
Length of output: 2386
Change the merge path from >- (folded) to | (literal) to preserve the #cloud-config header line.
The SSH-only path (lines 88–96) correctly uses | to preserve literal YAML formatting. However, the merge path (lines 119–127) uses >-, which folds newlines into spaces, causing #cloud-config and the YAML content to render on a single line. This breaks cloud-init parsing.
Proposed fix
- name: Parse existing cloud-init and merge SSH key
ansible.builtin.set_fact:
final_user_data: |
`#cloud-config`
{{ (existing_user_data | regex_replace('^`#cloud-config`\s*\n?', '') | from_yaml | default({}))
| combine({'ssh_authorized_keys':
((existing_user_data | regex_replace('^`#cloud-config`\s*\n?', '') | from_yaml | default({})).ssh_authorized_keys | default([]))
+ [ssh_key]})
| to_nice_yaml }}Consider extracting the regex/from_yaml operation into a separate task to avoid duplication:
+ - name: Parse existing cloud-init content
+ ansible.builtin.set_fact:
+ parsed_user_data: "{{ existing_user_data | regex_replace('^`#cloud-config`\\s*\\n?', '') | from_yaml | default({}, true) }}"
+
- name: Parse existing cloud-init and merge SSH key
ansible.builtin.set_fact:
final_user_data: |
`#cloud-config`
- {{ (existing_user_data | regex_replace('^`#cloud-config`\s*\n?', '') | from_yaml | default({}))
- | combine({'ssh_authorized_keys':
- ((existing_user_data | regex_replace('^`#cloud-config`\s*\n?', '') | from_yaml | default({})).ssh_authorized_keys | default([]))
+ {{ parsed_user_data
+ | combine({'ssh_authorized_keys':
+ (parsed_user_data.ssh_authorized_keys | default([]))
+ [ssh_key]})
| to_nice_yaml }}🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/create.yaml`
around lines 88 - 96, The merge path (around lines 119-127) uses the folded
scalar indicator `>-` for the final_user_data block when it should use the
literal scalar indicator `|` to preserve line breaks and the cloud-config header
format, matching the approach used in the SSH-only path. Change the `>-` to `|`
in the merge path's final_user_data block to ensure the cloud-config YAML
content maintains proper formatting with the header and YAML structure on
separate lines instead of being folded into a single line.
Comment on lines
+2
to
+5
| - name: Parse externalHostID into namespace and name | ||
| ansible.builtin.set_fact: | ||
| bmh_namespace: "{{ bare_metal_instance.spec.externalHostID.split('/')[0] }}" | ||
| bmh_name: "{{ bare_metal_instance.spec.externalHostID.split('/')[1] }}" |
There was a problem hiding this comment.
Validate externalHostID before splitting it.
Line 5 assumes spec.externalHostID contains exactly one slash; malformed input fails with an unhelpful Jinja index error before the delete flow can decide whether to skip or deprovision.
Proposed fix
+- name: Validate externalHostID format
+ ansible.builtin.assert:
+ that:
+ - (bare_metal_instance.spec.externalHostID | default('')) is match('^[^/]+/[^/]+$')
+ fail_msg: "spec.externalHostID must be '<namespace>/<name>'"
+
- name: Parse externalHostID into namespace and name
ansible.builtin.set_fact:
bmh_namespace: "{{ bare_metal_instance.spec.externalHostID.split('/')[0] }}"
bmh_name: "{{ bare_metal_instance.spec.externalHostID.split('/')[1] }}"🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/delete.yaml`
around lines 2 - 5, The task that sets bmh_namespace and bmh_name by splitting
bare_metal_instance.spec.externalHostID assumes the value contains exactly one
slash without any validation. If the externalHostID is malformed or empty, the
split operation will fail with an unhelpful Jinja index error. Add a validation
step before the split that checks whether externalHostID is in the correct
format (contains exactly one slash resulting in two parts when split), and if
validation fails, use a task to fail with a clear error message that explains
the expected format of externalHostID.
Comment on lines
+7
to
+13
| - name: Get BareMetalHost | ||
| kubernetes.core.k8s_info: | ||
| api_version: metal3.io/v1alpha1 | ||
| kind: BareMetalHost | ||
| namespace: "{{ bmh_namespace }}" | ||
| name: "{{ bmh_name }}" | ||
| register: bmh_result |
There was a problem hiding this comment.
Pass the remote kubeconfig to every Kubernetes module call.
These Kubernetes tasks also omit kubeconfig, so deprovisioning and Secret deletion can target the wrong cluster context.
Proposed fix pattern
kubernetes.core.k8s_info:
+ kubeconfig: "{{ kubeconfig | default(omit) }}"
api_version: metal3.io/v1alpha1
kind: BareMetalHost kubernetes.core.k8s:
+ kubeconfig: "{{ kubeconfig | default(omit) }}"
api_version: v1
kind: SecretApply the same module argument to every Kubernetes task in this file.
As per coding guidelines, “Always include osac.service.common to get remote_cluster_kubeconfig before creating K8s resources on remote clusters.” Based on learnings, use kubeconfig: "{{ kubeconfig | default(omit) }}" for this collection’s role task YAMLs.
Also applies to: 39-50, 52-61, 63-69, 71-77
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/delete.yaml`
around lines 7 - 13, The kubernetes.core.k8s_info module call is missing the
kubeconfig parameter, which can cause the task to target the wrong cluster
context when deprovisioning resources. Add the kubeconfig parameter `kubeconfig:
"{{ kubeconfig | default(omit) }}"` to the kubernetes.core.k8s_info task shown
in the diff, and apply the same kubeconfig parameter addition to all other
Kubernetes module calls in this file including the k8s module invocations at the
line ranges mentioned (39-50, 52-61, 63-69, 71-77) to ensure consistent cluster
targeting across all resource operations.
Sources: Coding guidelines, Learnings
Comment on lines
+27
to
+37
| - name: Check if BMH has an image set | ||
| ansible.builtin.set_fact: | ||
| bmh_has_image: "{{ bmh_info.spec.image is defined and bmh_info.spec.image.url is defined and bmh_info.spec.image.url | length > 0 }}" | ||
|
|
||
| - name: Skip deprovisioning when no image is set | ||
| when: not (bmh_has_image | bool) | ||
| ansible.builtin.debug: | ||
| msg: "BMH {{ bmh_namespace }}/{{ bmh_name }} has no image set, skipping deprovisioning" | ||
|
|
||
| - name: Remove image and networkData from BMH | ||
| when: bmh_has_image | bool |
There was a problem hiding this comment.
Clear stale BMH data refs even when no image URL is set.
The patch is gated only on bmh_has_image, but the generated Secrets are deleted afterward. If spec.userData or spec.networkData remains while spec.image.url is absent, this leaves the BMH pointing at deleted Secrets.
Proposed fix
- - name: Check if BMH has an image set
+ - name: Check if BMH has provisioning fields set
ansible.builtin.set_fact:
- bmh_has_image: "{{ bmh_info.spec.image is defined and bmh_info.spec.image.url is defined and bmh_info.spec.image.url | length > 0 }}"
+ bmh_has_provisioning_fields: >-
+ {{ (bmh_info.spec.image is defined and bmh_info.spec.image.url is defined and bmh_info.spec.image.url | length > 0) or
+ (bmh_info.spec.networkData is defined) or
+ (bmh_info.spec.userData is defined) }}
- name: Skip deprovisioning when no image is set
- when: not (bmh_has_image | bool)
+ when: not (bmh_has_provisioning_fields | bool)
ansible.builtin.debug:
- msg: "BMH {{ bmh_namespace }}/{{ bmh_name }} has no image set, skipping deprovisioning"
+ msg: "BMH {{ bmh_namespace }}/{{ bmh_name }} has no provisioning fields set, skipping deprovisioning"
- name: Remove image and networkData from BMH
- when: bmh_has_image | bool
+ when: bmh_has_provisioning_fields | bool
block:Also applies to: 63-77
🤖 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
`@collections/ansible_collections/osac/templates/roles/bm_host_metal3_provisioning/tasks/delete.yaml`
around lines 27 - 37, The task "Remove image and networkData from BMH" is
currently gated only on the bmh_has_image condition being true, but this leaves
stale references to Secrets when spec.userData or spec.networkData are present
without an image URL being set. Modify the logic to ensure that spec.userData
and spec.networkData are cleared from the BMH regardless of whether
bmh_has_image is true, so that dangling Secret references are not left behind
after Secret cleanup operations.
Contributor
|
/retest |
|
|
||
| - name: Parse existing cloud-init and merge SSH key | ||
| ansible.builtin.set_fact: | ||
| final_user_data: >- |
Contributor
There was a problem hiding this comment.
if spec.metaData maps to cloud init metadata, then the ssh keys can be injected there: https://docs.cloud-init.io/en/24.1/explanation/instancedata.html#v1-public-ssh-keys.
It's then it's the image that will configure the default account with these keys (fedora user for Fedora, core user for RHCOS, ...). From what I've seen, it's a common pattern in cloud providers, and even in kubevirt.
adriengentil
approved these changes
Jun 19, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, carbonin 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 |
This was referenced Jun 19, 2026
2 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 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
Adds the
bm_host_metal3_provisioningtemplate role for provisioning and deprovisioning bare metal hosts via the Metal3/BMO backend.create.yaml:
hostClass == "metal3", parsestemplateParametersforimageURL,imageChecksum,checksumType,networkData,sshKey, anduserDataSecretnetworkDataanduserDataSecrets in the BMH namespace as neededspec.image,spec.networkData, andspec.userData, then waits forprovisionedstatessh_authorized_keys, or merges into existing cloud-config user datadelete.yaml:
spec.image,spec.networkData, andspec.userDataavailable/readystatenetworkDataanduserDataSecretsTest plan
Validated end-to-end on ostest cluster:
Ready, BMH reachesprovisionedavailable, image cleared, Secrets cleaned upsshKey— userData Secret created with cloud-initssh_authorized_keys, BMHspec.userDatasetssh fedora@<host>using injected key)spec.userDataclearedansible-lintpassesAssisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit