Add ability to use hv with sno deployments

[agurenko]

Add ability to use hypervisors with sno deployments for the SNO HUB + Virtual Spoke deployment use case

Summary by CodeRabbit

• New Features

  • Added support for SNO hub deployments with virtual spoke cluster configurations.
  • Enhanced inventory generation for hypervisors and virtual machines, including networking, hardware, storage, sizing, and additional disks.
  • Added configurable hypervisor inventory output, with clear placeholders when the feature is disabled.

• Bug Fixes

  • Added validation to ensure adequate node allocation and defined worker counts for hub and spoke deployments.
  • Improved authentication and inventory configuration handling for more reliable setup.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[coderabbitai]

Warning

Review limit reached

@agurenko, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 49 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 81e538dc-eb85-4757-ac2e-a19fc85884c1

📥 Commits

Reviewing files that changed from the base of the PR and between 0a64385 and 0e8ea5e.

📒 Files selected for processing (2)

• ansible/roles/create-inventory/tasks/main.yml
• ansible/roles/create-inventory/templates/inventory-sno.j2

📝 Walkthrough

Walkthrough

Adds hv_inventory support to the SNO create-inventory role. The tasks assign hypervisor and SNO nodes conditionally, guard worker slicing, qualify modules, and update facts. The template renders hypervisor and VM inventory sections with computed settings when enabled.

Changes

SNO Virtual Spoke Cluster (hv_inventory)

Layer / File(s)	Summary
SNO node allocation and task processing ansible/roles/create-inventory/tasks/main.yml	Assigns ocpinventory_hv_nodes from nodes[2:]. Limits ocpinventory_sno_nodes to one node when hv_inventory is enabled. Guards worker slicing with defined and non-None worker_node_count checks. Qualifies modules and updates bond and fact handling.
Dynamic hypervisor and VM inventory rendering ansible/roles/create-inventory/templates/inventory-sno.j2	Generates hypervisor and VM inventory sections with computed addresses, hardware metadata, credentials, sizing, networking, MAC addresses, UUIDs, bandwidth, and disk settings. Emits comment-only placeholders when hv_inventory is disabled.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: mcornea, josecastillolema

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: adding hypervisor support for SNO deployments.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[akrzos]

Thanks Alex, I looked at this PR with claude code to help find what things we should improve with the new deployment topology (SNO + Virtual Spoke Clusters) that this is adding.

hv_count usage: For MNO with spoke virtual clusters, all leftover nodes after bastion + controlplane + workers become hypervisors — no limit. The SNO+HV topology is the same shape (bastion, SNO hub, then everything else), so I'd suggest dropping the hv_count gating here and simplifying to ocpinventory.json.nodes[2:]. This also removes the need for the validation assert and keeps things consistent with MNO. The original commented-out block at line 431 had this same approach.

Node overlap: ocpinventory_sno_nodes is set to nodes[1:] (all nodes after bastion), but the new hv block also assigns nodes starting at index 2. The same physical machines end up in both [sno] (commented out) and [hv] (active) inventory groups. Similarly, if worker_node_count is set alongside hv_inventory, both workers and hv nodes slice from index 2 and would overlap. Compare with MNO where workers get nodes[4:max_nodes] and HV gets nodes[max_nodes:] — properly partitioned. Consider trimming ocpinventory_sno_nodes to exclude the hv nodes when hv_inventory is true, and starting the hv slice after any workers.

HV IP addressing: The [hv] IP formula in the template uses mno_worker_node_offset (default 7), which is MNO-specific. SNO nodes use sno_controlplane_ip_offset (default 3). With enough SNO nodes this could produce IP collisions — unlikely in the typical single-hub case but worth guarding against.

Small stuff:

• Missing newline at end of inventory-sno.j2
• Typo on line 224: "populates" → "populate"
• The old commented-out # SNO - Set ocpinventory hv nodes block (lines 427-431) can be removed now that it's implemented
• New validation assert is missing success_msg (every other assert in the file has one)
• hv_count default comment in defaults/main/main.yml still says "For VMNO cluster type" — needs updating if it stays

For the current work, I don't think we need to support SNO + Workers + HVs, and we can always extend that if it is ever needed, so I suggest just having it mimic the MNO pattern for now.

[agurenko]

Thanks Alex, I looked at this PR with claude code to help find what things we should improve with the new deployment topology (SNO + Virtual Spoke Clusters) that this is adding.

hv_count usage: For MNO with spoke virtual clusters, all leftover nodes after bastion + controlplane + workers become hypervisors — no limit. The SNO+HV topology is the same shape (bastion, SNO hub, then everything else), so I'd suggest dropping the hv_count gating here and simplifying to ocpinventory.json.nodes[2:]. This also removes the need for the validation assert and keeps things consistent with MNO. The original commented-out block at line 431 had this same approach.
Node overlap: ocpinventory_sno_nodes is set to nodes[1:] (all nodes after bastion), but the new hv block also assigns nodes starting at index 2. The same physical machines end up in both [sno] (commented out) and [hv] (active) inventory groups. Similarly, if worker_node_count is set alongside hv_inventory, both workers and hv nodes slice from index 2 and would overlap. Compare with MNO where workers get nodes[4:max_nodes] and HV gets nodes[max_nodes:] — properly partitioned. Consider trimming ocpinventory_sno_nodes to exclude the hv nodes when hv_inventory is true, and starting the hv slice after any workers.
HV IP addressing: The [hv] IP formula in the template uses mno_worker_node_offset (default 7), which is MNO-specific. SNO nodes use sno_controlplane_ip_offset (default 3). With enough SNO nodes this could produce IP collisions — unlikely in the typical single-hub case but worth guarding against.
Small stuff:

• Missing newline at end of inventory-sno.j2
• Typo on line 224: "populates" → "populate"
• The old commented-out # SNO - Set ocpinventory hv nodes block (lines 427-431) can be removed now that it's implemented
• New validation assert is missing success_msg (every other assert in the file has one)
• hv_count default comment in defaults/main/main.yml still says "For VMNO cluster type" — needs updating if it stays

For the current work, I don't think we need to support SNO + Workers + HVs, and we can always extend that if it is ever needed, so I suggest just having it mimic the MNO pattern for now.

I've fixed the small stuff and other issues, for the remaining node overlap, I've trimmed the sno list to just one when hv is in use, I don't think I'm gonna look into sno + worker + hv as a part of this PR. Similar to the previous item, current use-case should not see any problem (single sno + hvs), let's revisit it if we get into more complicated setup

[agurenko]

Okay, current version got rid of the hv_count limiter and creates cleaner inventory of 1 bastion, 1 sno (no commented nodes) and rest of the nodes go into the hv section and populated with VMs respectively.

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@ansible/roles/create-inventory/tasks/main.yml`:
- Around line 370-375: The SNO task setting ocpinventory_worker_nodes does not
prevent node overlap with hypervisor assignments. Since line 444 assigns
ocpinventory_hv_nodes from the same node slice (starting at index 2) when
hv_inventory is true, both variables can reference the same hosts. Add a
condition to the when clause of the SNO task that excludes the case when
hv_inventory is true, ensuring ocpinventory_worker_nodes is only set when
hypervisor inventory mode is not enabled.

In `@ansible/roles/create-inventory/templates/inventory-sno.j2`:
- Line 174: The conditional check for hv_inventory on line 174 does not properly
handle string boolean values like "false", which are evaluated as truthy
non-empty strings in Jinja2. Apply the bool filter to coerce the hv_inventory
variable to a proper boolean value by changing the if condition from checking
hv_inventory directly to using the bool filter, so that string values like
"false" are correctly interpreted as false.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4cf35ce1-061c-42cb-a707-26c4b79c0d39

📥 Commits

Reviewing files that changed from the base of the PR and between 629b793 and ac80107.

📒 Files selected for processing (3)

• ansible/roles/create-inventory/defaults/main/main.yml
• ansible/roles/create-inventory/tasks/main.yml
• ansible/roles/create-inventory/templates/inventory-sno.j2

[mcornea]

/test deploy-sno

[mcornea]

/test deploy-sno

[akrzos]

@agurenko Would you be able to close the loop on those last few comments and rebase this so we can get this merged?

[agurenko]

@akrzos yeah, I'm gonna finalize it today

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

ansible/roles/create-inventory/tasks/main.yml (1)

412-414: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use a descriptive task name.

set json query fact does not identify the mac_query fact. Rename the task to state that it sets the MAC-address JSON query fact.

As per path instructions, “All tasks must have clear, descriptive names.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ansible/roles/create-inventory/tasks/main.yml` around lines 412 - 414, Rename
the task currently named “set json query fact” to a clear, descriptive name
stating that it sets the MAC-address JSON query fact. Keep the existing
mac_query set_fact and query value unchanged.

Source: Path instructions

🤖 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 `@ansible/roles/create-inventory/tasks/main.yml`:
- Around line 339-341: In the SNO allocation flow before the “SNO - Set
ocpinventory hv nodes” task, add an assertion requiring at least three inventory
nodes when cluster_type is "sno" and hv_inventory is enabled. Preserve the
existing allocation behavior for other cluster types or when hypervisor
inventory is disabled.
- Around line 396-410: Keep the existing SNO-node enrichment task, and add an
equivalent bond MAC enrichment task for hypervisors that updates
`ocpinventory_hv_nodes` and loops over that collection. Preserve the same
`enable_bond`/`public_vlan` condition and `bond0_macs` construction using
`bond0_port1` and `bond0_port2`.
- Around line 365-384: Enable TLS certificate validation in both Foreman
requests, “SNO - Get lab data for each sno” and “SNO - Get lab mac address for
worker nodes,” by setting their ansible.builtin.uri validate_certs options to
true. Ensure the control host trusts the Foreman CA so both requests continue to
authenticate successfully.

---

Nitpick comments:
In `@ansible/roles/create-inventory/tasks/main.yml`:
- Around line 412-414: Rename the task currently named “set json query fact” to
a clear, descriptive name stating that it sets the MAC-address JSON query fact.
Keep the existing mac_query set_fact and query value unchanged.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c66fff0c-bdb7-40ed-a82a-b56fc4b298dd

📥 Commits

Reviewing files that changed from the base of the PR and between eb46c53 and 0a64385.

📒 Files selected for processing (2)

• ansible/roles/create-inventory/tasks/main.yml
• ansible/roles/create-inventory/templates/inventory-sno.j2

🚧 Files skipped from review as they are similar to previous changes (1)

• ansible/roles/create-inventory/templates/inventory-sno.j2

[agurenko]

/test deploy-sno

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrzos

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [akrzos]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment