MGMT-24176: add BMH-based agent import playbook with AAP config-as-code - #279
Conversation
|
@danmanor: This pull request references MGMT-24176 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor 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 |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughThis PR introduces a complete bare metal server import workflow for OpenShift Assisted Installer Agent resources. It adds a new Ansible variable to enable the feature via environment variable, extends AAP controller configuration with a new job template, scheduled periodic reconciliation job, and required volume mounts for configuration data. A detailed documentation file explains the workflow, prerequisites, and operational procedures. The core playbook loads server inventory from a ConfigMap, reconciles bare metal host and agent resources by creating missing entries, deleting stale entries, waiting for agent registration, and updating agent metadata with hostname and resource labels. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 32 minutes and 49 seconds.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: 1
🧹 Nitpick comments (1)
playbook_osac_import_agents.yml (1)
112-128: 💤 Low valueRedundant
whenclause on stale agent deletion.The
when: import_agents_stale_agents.results | length > 0check on line 128 is redundant because the loop on lines 119-124 already produces an empty list when there are no stale agents, causing the task to be skipped naturally. This is harmless but could be removed for clarity.♻️ Optional: Remove redundant when clause
loop_control: loop_var: agent label: "{{ agent.metadata.name }}" - when: import_agents_stale_agents.results | length > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbook_osac_import_agents.yml` around lines 112 - 128, Remove the redundant "when: import_agents_stale_agents.results | length > 0" from the "Delete stale Agent CRs" task; locate the task with name "Delete stale Agent CRs" that loops over import_agents_stale_agents.results (loop_var: agent) and delete the when clause so the task relies on the loop producing an empty list to skip execution naturally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@samples/import_agents_extra_vars.yml`:
- Around line 1-5: Remove the trailing whitespace at the end of the first line
that starts with "# To run:" (the comment block containing the
KUBECONFIG/ansible-playbook example) so the file no longer has trailing spaces;
commit the change so the pre-commit hook / CI no longer flags the file.
---
Nitpick comments:
In `@playbook_osac_import_agents.yml`:
- Around line 112-128: Remove the redundant "when:
import_agents_stale_agents.results | length > 0" from the "Delete stale Agent
CRs" task; locate the task with name "Delete stale Agent CRs" that loops over
import_agents_stale_agents.results (loop_var: agent) and delete the when clause
so the task relies on the loop producing an empty list to skip execution
naturally.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5147c15-80e7-4f34-81c7-d31f36d3664a
📒 Files selected for processing (6)
collections/ansible_collections/osac/config_as_code/playbooks/vars/config.ymlcollections/ansible_collections/osac/config_as_code/roles/aap/vars/controller.ymldocs/import-agents.mdplaybook_osac_import_agents.ymlsamples/import_agents_bmc_secrets.ymlsamples/import_agents_extra_vars.yml
ba34734 to
5eecb74
Compare
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
PR #279: MGMT-24176: add BMH-based agent import playbook with AAP config-as-code
This PR adds a new single-file playbook that reconciles a file-based server inventory against BareMetalHost and Agent CRs — creating missing BMHs, deleting stale ones, waiting for agent registration, and labeling agents. It includes AAP config-as-code for a job template with a 10-minute schedule, documentation, and sample files. The overall structure is sound and the reconciliation logic is well thought out (delete-agent-before-BMH ordering, managed-by label scoping, MAC-based agent matching).
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 3 |
| 💡 Suggestion | 2 |
🟡 BMH updates silently skipped for existing servers
File: playbook_osac_import_agents.yml:67-90
The when: server.name not in import_agents_existing_bmh_names guard skips the kubernetes.core.k8s task entirely for any server whose BMH already exists. This means if an operator changes a server's bmc_url, bmc_secret, or boot_mac in the inventory, the update will never be applied — the playbook will silently skip it.
Since kubernetes.core.k8s with state: present already does an apply (create-or-patch), removing the when clause would make the task idempotent for both creation and updates. The only cost is extra API calls on each run, but these are lightweight patch operations.
Recommendation: Remove the when guard and let the k8s module handle idempotency:
- name: Reconcile BareMetalHost for each server
kubernetes.core.k8s:
state: present
definition:
# ... same definition ...
loop: "{{ import_agents_servers }}"
loop_control:
loop_var: server
label: "{{ server.name }}"If the extra API calls are a concern (many servers on a 10-minute schedule), consider a changed_when annotation or a separate "diff" task — but don't silently skip updates.
🟡 Job template missing allow_simultaneous and ask_variables_on_launch
File: collections/ansible_collections/osac/config_as_code/roles/aap/vars/controller.yml:225-234
Every other job template in this file sets allow_simultaneous: true and ask_variables_on_launch: true. The new import-agents template omits both.
For allow_simultaneous: omitting it is arguably correct for a reconciliation playbook (you don't want concurrent runs contending on the same BMH/Agent CRs). But this relies on the AAP default rather than being explicit, and breaks the pattern established by every other template. If it's intentional, make it explicit.
For ask_variables_on_launch: without this, operators can't override variables (e.g., import_agents_namespace, import_agents_inventory_path) when manually triggering the job template. This is particularly useful for debugging or testing against a different namespace.
Recommendation:
- name: "{{ aap_prefix }}-import-agents"
# ...existing fields...
allow_simultaneous: false
ask_variables_on_launch: true
verbosity: 0🟡 Local testing command in docs will fail — missing required variables
File: docs/import-agents.md:160-162 and samples/import_agents_extra_vars.yml
The documented local testing command is:
ansible-playbook playbook_osac_import_agents.yml \
-e import_agents_inventory_path=samples/import_agents_extra_vars.ymlBut the playbook vars reference default_agent_namespace and hosted_cluster_default_infraenv, which are defined in group_vars/all/infrastructure.yaml. When running locally without the AAP inventory, these are undefined and the playbook will fail with an "undefined variable" error.
Recommendation: Either add these to the sample extra vars file:
# samples/import_agents_extra_vars.yml
default_agent_namespace: "hardware-inventory"
hosted_cluster_default_infraenv: "infraenv"
servers:
- name: ostest-extraworker-0
# ...Or use literal defaults directly in the playbook vars (like other playbooks in this repo do):
vars:
import_agents_namespace: "hardware-inventory"
import_agents_infraenv_name: "infraenv"💡 ConfigMap mounted at /var/secrets/ path
File: collections/ansible_collections/osac/config_as_code/roles/aap/vars/controller.yml:455-461
The import-agents-inventory ConfigMap is mounted at /var/secrets/import-agents/. Using /var/secrets/ for a ConfigMap is misleading — it suggests sensitive data when the content is a server inventory (names, MACs, resource classes). Consider /var/config/import-agents/ to match the data source type.
💡 Schedule frequency vs. wait timeout
File: collections/ansible_collections/osac/config_as_code/roles/aap/vars/controller.yml:402 and playbook_osac_import_agents.yml:143-155
The schedule runs every 10 minutes, but the wait task allows up to 15 minutes per server (90 retries * 10s delay). With multiple new servers, a single playbook run can exceed 10 minutes, potentially queuing scheduled runs. This is handled correctly because allow_simultaneous defaults to false, so AAP will skip/queue. But worth documenting this behavior — operators should know that adding many servers at once will naturally take longer than the schedule interval.
Overall: Well-structured playbook with good reconciliation logic and thorough documentation. The three important items are straightforward to address. Approve with requested changes.
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
Nice work — the reconciliation logic is solid. tzvatot already covered the main items so I'm only adding what wasn't mentioned yet.
Address review feedback: - Add pre-flight check for missing inventory ConfigMap - Remove when guard on BMH creation for idempotent updates - Make disableCertificateVerification configurable per-server - Remove redundant when clause on stale agent deletion - Add failed_when guard against None agent name race condition - Add allow_simultaneous and ask_variables_on_launch to job template - Add missing vars to sample extra vars for local testing - Change ConfigMap mount path from /var/secrets/ to /var/config/
5eecb74 to
7f5111e
Compare
|
/lgtm |
|
Just a note that we currently have an equivalent role for OpenStack at https://github.com/osac-project/osac-aap/blob/main/collections/ansible_collections/osac/service/roles/manage_agents/tasks/import_agents.yaml; do we want to make this parallel to that? |
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Re-review
All findings from the previous round were addressed in the merged commit: pre-flight inventory check, configurable per-server BMC TLS verification, explicit allow_simultaneous: false / ask_variables_on_launch: true, failed_when guard on agent labeling, ConfigMap mount at /var/config/ instead of /var/secrets/, and no redundant when clauses. Solid turnaround.
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 1 |
| 💡 Suggestion | 1 |
🟡 Local testing command in docs still won't work
File: docs/import-agents.md:160-162 and samples/import_agents_extra_vars.yml
The documented local testing command is:
ansible-playbook playbook_osac_import_agents.yml \
-e @samples/import_agents_extra_vars.ymlThe extra vars file now correctly defines default_agent_namespace and hosted_cluster_default_infraenv (addressing the original feedback), but it does not override import_agents_inventory_path. The playbook will still try to load the server inventory from the default path /var/config/import-agents/servers.yml, hit the pre-flight stat check, and fail.
The PR body references a different file (samples/import_agents_servers.yml) that doesn't exist in the repo, which suggests this was the intended inventory file but got consolidated into the extra vars file.
Recommendation: Split samples/import_agents_extra_vars.yml into two files — one for the variables (import_agents_extra_vars.yml) and one for the inventory (import_agents_servers.yml):
# samples/import_agents_extra_vars.yml
default_agent_namespace: "hardware-inventory"
hosted_cluster_default_infraenv: "infraenv"
import_agents_inventory_path: "samples/import_agents_servers.yml"# samples/import_agents_servers.yml
servers:
- name: ostest-extraworker-0
bmc_url: "redfish-virtualmedia+https://..."
bmc_secret: "ostest-extraworker-0-bmc-secret"
boot_mac: "00:8f:09:3e:24:af"
netris_server_name: "extraworker-0"
resource_class: "x86_64"
# ...This also makes the sample structure match the production layout (extra vars separate from the inventory ConfigMap content).
💡 failed_when on agent labeling fires after the API call, not before
File: playbook_osac_import_agents.yml:190-194
The failed_when guard correctly catches the case where mac_to_agent_name returns None, but Ansible evaluates failed_when after the task executes. So the kubernetes.core.k8s module will first attempt to apply a definition with name: "None" (the stringified Jinja2 result), get a 404 or validation error from the API server, and then failed_when marks it failed.
In practice this is harmless — the wait task at line 143 ensures all agents exist before we reach this point, so this is a defense-in-depth guard that should rarely fire. But if it does fire, the user will see two errors (the API error and the failed_when), which can be confusing.
A cleaner alternative is a when guard that skips + warns instead of failing after-the-fact:
when: >-
[server.boot_mac] |
osac.service.mac_to_agent_name(import_agents_final_agents.resources) is not none
register: import_agents_label_result
- name: Warn about unlabeled agents
ansible.builtin.debug:
msg: "Agent not found for server {{ server.name }} (MAC {{ server.boot_mac }}) — skipping label"
loop: "{{ import_agents_servers }}"
loop_control:
loop_var: server
label: "{{ server.name }}"
when: >-
[server.boot_mac] |
osac.service.mac_to_agent_name(import_agents_final_agents.resources) is noneBut the current approach is functional. Up to you.
Overall: Previous review feedback was fully addressed. The reconciliation logic, stale-resource cleanup ordering, managed-by label scoping, and AAP integration are all clean. The local testing docs are the only actionable item remaining.
5 participants
Single-file playbook that imports bare metal servers as Assisted Installer agents using BareMetalHost CRs via Ironic. Reads server inventory from a mounted ConfigMap, reconciles BMH CRs (create missing, delete stale), waits for Agent registration, and labels agents with resource class and Netris server name metadata.
Config-as-code changes add a job template, a 10-minute periodic schedule, and mount the import-agents-inventory ConfigMap into the cluster-fulfillment-ig pod spec.
Includes a sample inventory file for local testing:
Summary by CodeRabbit
New Features
Documentation
Chores