feat: add runlibvirt to create multiple VMs that can network to each other#244
Merged
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds libvirt-based multi-VM testing support, including new tox environments, VM provisioning and teardown scripts, CLI orchestration, and updated README guidance. ChangesLibvirt testing support
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
richm
force-pushed
the
runlibvirt
branch
3 times, most recently
from
July 2, 2026 19:39
2b2e3c4 to
6bde645
Compare
richm
force-pushed
the
runlibvirt
branch
2 times, most recently
from
July 2, 2026 20:26
1522c89 to
c1478fc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/tox_lsr/test_scripts/runlibvirt.py (1)
562-600: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPeer
/etc/hostsentries omit the FQDN and may be clobbered by cloud-init.The published entries map IP → short hostname only. For the static network the libvirt DNS block advertises both the short name and
hostname.dns_domain, but for an externally supplied--libvirt-network(no injected DNS records) peers can only be resolved via this/etc/hostsblock — so multihost playbooks that reference peers by FQDN will fail to resolve them. Additionally, cloud-initmanage_etc_hosts: true(set in_make_cloud_init) rewrites/etc/hostsfrom a template on boot and can drop this block on a reboot. Consider appending the FQDN alongside the short name here.♻️ Suggested change
- entries = "\n".join( - "{} {}".format(vm["ipaddr"], vm["hostname"]) for vm in self.vms - ) + entries = "\n".join( + "{} {} {}.{}".format( + vm["ipaddr"], vm["hostname"], vm["hostname"], self.dns_domain + ) + for vm in self.vms + )🤖 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 `@src/tox_lsr/test_scripts/runlibvirt.py` around lines 562 - 600, The peer host entries written by _configure_peer_hostnames() only publish the short hostname, so add the FQDN alongside it when building the /etc/hosts block (using the VM’s hostname plus dns_domain where available) to match the static-network DNS behavior. Also make the injected block resilient to cloud-init rewrites by ensuring the HOSTS_MARKER_BEGIN/HOSTS_MARKER_END section is appended in a way that survives _make_cloud_init’s manage_etc_hosts behavior, or by reinserting it on boot if needed.
🤖 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 `@README.md`:
- Around line 891-916: The libvirt setup note is using an incorrect placeholder
for qemu.conf’s user setting. Update the README section about libvirt testing so
the example in the qemu.conf edit step uses the libvirt account name expected by
libvirt, not a numeric UID, and make it clear in that paragraph that the `user`
field should be set to an account name; keep the guidance anchored around the
libvirt testing instructions and the `user =` example.
- Around line 917-965: The example in README.md uses an incorrect FQCN for the
client role; update the `include_role` entry in the `Set up client and run
tests` play to use the same namespace pattern as
`fedora.linux_system_roles.server_role`. Keep the server and client examples
consistent by replacing `linux-system-roles.client_role` with the correct fully
qualified role name for the client role.
In `@src/tox_lsr/test_scripts/runlibvirt.py`:
- Around line 1237-1278: The provisioner reuse block in runlibvirt.py is
comparing LibvirtProvisioner.image_path against a raw current_image value, so
normalize current_image the same way as in LibvirtProvisioner.__init__ before
comparing to avoid recreating the provisioner on every batch. Also, when a new
provisioner is created in this branch, reset vms_started so
internal_run_ansible_playbooks_libvirt can start the new VM and refresh the
inventory_path instead of reusing stale state.
- Around line 649-677: The boot wait logic in the domain state loop only treats
VIR_DOMAIN_SHUTDOWN and VIR_DOMAIN_CRASHED as fatal, so a guest that powers off
into VIR_DOMAIN_SHUTOFF is misreported as an IP timeout. Update the state checks
in the boot-wait code around the dom.state() handling to treat
VIR_DOMAIN_SHUTOFF as a boot-failure condition as well, alongside the existing
shutdown/crashed cases, and raise an immediate RuntimeError with an appropriate
message before the SSH/IP wait continues.
---
Nitpick comments:
In `@src/tox_lsr/test_scripts/runlibvirt.py`:
- Around line 562-600: The peer host entries written by
_configure_peer_hostnames() only publish the short hostname, so add the FQDN
alongside it when building the /etc/hosts block (using the VM’s hostname plus
dns_domain where available) to match the static-network DNS behavior. Also make
the injected block resilient to cloud-init rewrites by ensuring the
HOSTS_MARKER_BEGIN/HOSTS_MARKER_END section is appended in a way that survives
_make_cloud_init’s manage_etc_hosts behavior, or by reinserting it on boot if
needed.
🪄 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 Plus
Run ID: 4fbf6b43-db4c-4203-9df7-2d3667291fe9
📒 Files selected for processing (6)
README.mdsrc/tox_lsr/config_files/tox-default.inisrc/tox_lsr/test_scripts/libvirt-cleanup.shsrc/tox_lsr/test_scripts/runlibvirt.pytests/fixtures/test_tox_merge_ini/result.initox.ini
💤 Files with no reviewable changes (1)
- tox.ini
richm
force-pushed
the
runlibvirt
branch
3 times, most recently
from
July 2, 2026 20:41
963395f to
7ceb155
Compare
spetrosi
previously approved these changes
Jul 3, 2026
Contributor
Author
|
Latest version has full support for tests/provision.fmf - memory, disks, network interfaces |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/tox_lsr/test_scripts/runlibvirt.py`:
- Around line 1193-1208: `resolve_hostnames` currently passes user-supplied
`--hostnames` through without validation, which can later break `vm_dir`,
libvirt XML/cloud-init, `/etc/hosts`, and inventory naming. Add validation and
normalization inside `resolve_hostnames` (or immediately after it) to reject
unsafe names such as path separators, `..`, quotes, whitespace, and empty
entries, and to deduplicate repeated hostnames before returning the list. Keep
the checks close to the `resolve_hostnames` logic so all downstream uses inherit
the sanitized hostname set.
- Around line 1004-1007: The inventory assembly in runlibvirt.py is silently
overwriting VMs that share the same inventory_name, so update the logic that
builds hosts from self.vms to detect duplicates and fail fast instead of
replacing earlier entries. Add the same duplicate-name validation in both
inventory construction paths referenced by the vm iteration block and the later
inventory-building block, using inventory_name as the key and rejecting repeated
values with a clear error. Keep the fix localized around the inventory/hosts
mapping code so every VM must have a unique inventory_name before the inventory
is emitted.
- Around line 915-965: The domain created by self.conn.createXML() is not
tracked soon enough, so failures in the IP/SSH wait path can leave a running VM
that destroy() never cleans up. Update the VM startup flow around createXML(),
the wait loop, and _wait_for_ssh() so the new domain is either appended to
self.vms immediately after successful creation or explicitly destroyed in every
exception/timeout path before re-raising. Also make the same
cleanup/registration change in the other startup blocks referenced by the
related sections so all created domains are always visible to destroy().
- Around line 825-831: The SSH readiness wait in _wait_for_ssh currently uses a
fixed iteration count, which can stretch far beyond the intended timeout when
_ssh_run is slow; change it to use a wall-clock deadline instead. Update
_wait_for_ssh to record a start time or deadline and keep retrying until that
absolute timeout is reached, while still calling _ssh_run(ipaddr, "/bin/true",
check=False) and sleeping between attempts. Preserve the existing RuntimeError
path in _wait_for_ssh, but trigger it based on elapsed time rather than
SSH_WAIT_TIMEOUT loop iterations.
- Around line 1514-1552: The provisioner reuse check in runlibvirt should also
account for batch-specific provisioning inputs, not just the image path. Update
the reuse key logic around the LibvirtProvisioner creation/reuse path so changes
in tests_dir and the effective provision.fmf content/config force a fresh
provisioner instead of reusing one with stale VM settings. Use the existing
provisioner_kwargs, tests_dir assignment, and the provisioner.image_path
comparison as the place to incorporate these values before deciding whether to
stop_libvirt and instantiate a new LibvirtProvisioner.
- Around line 1316-1367: The snapshot setup flow can leave behind a partially
configured snapfile if internal_run_ansible_playbooks_libvirt fails after the
qemu-img create step. Update this runlibvirt path so the temporary snapshot is
cleaned up on setup failure, not just the provisioner and inventory_path, by
removing snapfile in the exception/cleanup path when the playbook run does not
complete successfully. Keep the fix local to the snapshot creation and
internal_run_ansible_playbooks_libvirt call sequence so later runs do not
incorrectly reuse an incomplete snapshot.
🪄 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 Plus
Run ID: d40c1846-39db-468f-b0fb-5d4918feddc0
📒 Files selected for processing (6)
README.mdsrc/tox_lsr/config_files/tox-default.inisrc/tox_lsr/test_scripts/libvirt-cleanup.shsrc/tox_lsr/test_scripts/runlibvirt.pytests/fixtures/test_tox_merge_ini/result.initox.ini
💤 Files with no reviewable changes (1)
- tox.ini
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tox_lsr/test_scripts/libvirt-cleanup.sh
- tests/fixtures/test_tox_merge_ini/result.ini
- src/tox_lsr/config_files/tox-default.ini
Comment on lines
+915
to
+965
| dom = self.conn.createXML(xml, 0) | ||
| if dom is None: | ||
| raise RuntimeError( | ||
| "Failed to create libvirt domain {}".format( | ||
| domain_name | ||
| ) | ||
| ) | ||
| logging.info( | ||
| "Started libvirt domain %s for hostname %s (mac %s)", | ||
| domain_name, | ||
| hostname, | ||
| plan["mac"], | ||
| ) | ||
| ipaddr = None | ||
| expected_ip = plan.get("ip") | ||
| for _ in range(SSH_WAIT_TIMEOUT): | ||
| state, _ = dom.state() | ||
| if state == libvirt.VIR_DOMAIN_SHUTDOWN: | ||
| raise RuntimeError( | ||
| "Domain {} shut down during boot".format( | ||
| domain_name | ||
| ) | ||
| ) | ||
| if state == libvirt.VIR_DOMAIN_SHUTOFF: | ||
| raise RuntimeError( | ||
| "Domain {} shut off during boot".format( | ||
| domain_name | ||
| ) | ||
| ) | ||
| if state == libvirt.VIR_DOMAIN_CRASHED: | ||
| raise RuntimeError( | ||
| "Domain {} crashed during boot".format(domain_name) | ||
| ) | ||
| ipaddr = self._get_domain_ip(dom) | ||
| if ipaddr: | ||
| if expected_ip and ipaddr != expected_ip: | ||
| logging.warning( | ||
| "VM %s got IP %s, expected %s", | ||
| hostname, | ||
| ipaddr, | ||
| expected_ip, | ||
| ) | ||
| break | ||
| time.sleep(1) | ||
| else: | ||
| raise RuntimeError( | ||
| "Timed out waiting for IP address on domain {}".format( | ||
| domain_name | ||
| ) | ||
| ) | ||
| self._wait_for_ssh(ipaddr) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Track or destroy domains immediately after createXML().
If a VM gets created but fails before Line 973 appends it to self.vms—for example IP/SSH timeout—destroy() never sees that domain, so the failed VM can be left running and block network cleanup.
Proposed fix
dom = self.conn.createXML(xml, 0)
if dom is None:
raise RuntimeError(
"Failed to create libvirt domain {}".format(
domain_name
)
)
+ domain_recorded = False
+ try:
+ # existing boot/IP/SSH wait logic remains here
+ ...
+ self.vms.append({...})
+ domain_recorded = True
+ except Exception:
+ if not domain_recorded:
+ try:
+ if dom.isActive():
+ dom.destroy()
+ except libvirt.libvirtError as err:
+ logging.warning(
+ "Error destroying failed domain %s: %s",
+ domain_name,
+ err,
+ )
+ raiseAlso applies to: 997-999, 1020-1059
🤖 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 `@src/tox_lsr/test_scripts/runlibvirt.py` around lines 915 - 965, The domain
created by self.conn.createXML() is not tracked soon enough, so failures in the
IP/SSH wait path can leave a running VM that destroy() never cleans up. Update
the VM startup flow around createXML(), the wait loop, and _wait_for_ssh() so
the new domain is either appended to self.vms immediately after successful
creation or explicitly destroyed in every exception/timeout path before
re-raising. Also make the same cleanup/registration change in the other startup
blocks referenced by the related sections so all created domains are always
visible to destroy().
Comment on lines
+1316
to
+1367
| with rq.file_or_stdout(log_file) as (stdout, stderr): | ||
| subprocess.check_call( # nosec | ||
| [ | ||
| "qemu-img", | ||
| "create", | ||
| "-f", | ||
| "qcow2", | ||
| "-b", | ||
| image_file, | ||
| "-F", | ||
| "qcow2", | ||
| snapfile, | ||
| ], | ||
| stdout=stdout, | ||
| stderr=stderr, | ||
| ) | ||
| snap_kwargs = dict(provisioner_kwargs) | ||
| snap_kwargs["image_path"] = snapfile | ||
| snap_kwargs["hostnames"] = [snap_kwargs["hostnames"][0]] | ||
| snap_kwargs["write_to_image"] = True | ||
| snap_kwargs["debug"] = False | ||
| provisioner = LibvirtProvisioner(**snap_kwargs) | ||
| inventory_path = tempfile.NamedTemporaryFile( | ||
| suffix=".yml", delete=False | ||
| ).name | ||
| test_env_setup = {} | ||
| test_env_setup.update(test_env) | ||
| if "TEST_DEBUG" in test_env_setup: | ||
| del test_env_setup["TEST_DEBUG"] | ||
| if "TEST_ARTIFACTS" in test_env_setup: | ||
| test_env_setup["TEST_ARTIFACTS"] = ( | ||
| test_env_setup["TEST_ARTIFACTS"] + ".snap" | ||
| ) | ||
| if "LOCK_ON_FILE" in test_env_setup: | ||
| del test_env_setup["LOCK_ON_FILE"] | ||
| try: | ||
| internal_run_ansible_playbooks_libvirt( | ||
| provisioner, | ||
| inventory_path, | ||
| test_env_setup, | ||
| ansible_args, | ||
| setup_yml, | ||
| cwd, | ||
| ansible_container, | ||
| wait_on_vm=True, | ||
| log_file=log_file, | ||
| start_vms=True, | ||
| ) | ||
| finally: | ||
| provisioner.destroy() | ||
| if os.path.exists(inventory_path): | ||
| os.unlink(inventory_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid reusing a partially configured snapshot after setup failure.
snapfile is created before the setup playbooks run. If setup fails after creation, the incomplete snapshot remains and the next run can skip refresh because the file exists and is fresh.
Proposed fix
+ tmp_snapfile = "{}.tmp-{}".format(snapfile, uuid.uuid4().hex)
with rq.file_or_stdout(log_file) as (stdout, stderr):
subprocess.check_call( # nosec
[
"qemu-img",
"create",
@@
- snapfile,
+ tmp_snapfile,
],
stdout=stdout,
stderr=stderr,
)
@@
- snap_kwargs["image_path"] = snapfile
+ snap_kwargs["image_path"] = tmp_snapfile
@@
try:
internal_run_ansible_playbooks_libvirt(
@@
)
+ os.replace(tmp_snapfile, snapfile)
finally:
provisioner.destroy()
+ if os.path.exists(tmp_snapfile):
+ os.unlink(tmp_snapfile)
if os.path.exists(inventory_path):
os.unlink(inventory_path)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with rq.file_or_stdout(log_file) as (stdout, stderr): | |
| subprocess.check_call( # nosec | |
| [ | |
| "qemu-img", | |
| "create", | |
| "-f", | |
| "qcow2", | |
| "-b", | |
| image_file, | |
| "-F", | |
| "qcow2", | |
| snapfile, | |
| ], | |
| stdout=stdout, | |
| stderr=stderr, | |
| ) | |
| snap_kwargs = dict(provisioner_kwargs) | |
| snap_kwargs["image_path"] = snapfile | |
| snap_kwargs["hostnames"] = [snap_kwargs["hostnames"][0]] | |
| snap_kwargs["write_to_image"] = True | |
| snap_kwargs["debug"] = False | |
| provisioner = LibvirtProvisioner(**snap_kwargs) | |
| inventory_path = tempfile.NamedTemporaryFile( | |
| suffix=".yml", delete=False | |
| ).name | |
| test_env_setup = {} | |
| test_env_setup.update(test_env) | |
| if "TEST_DEBUG" in test_env_setup: | |
| del test_env_setup["TEST_DEBUG"] | |
| if "TEST_ARTIFACTS" in test_env_setup: | |
| test_env_setup["TEST_ARTIFACTS"] = ( | |
| test_env_setup["TEST_ARTIFACTS"] + ".snap" | |
| ) | |
| if "LOCK_ON_FILE" in test_env_setup: | |
| del test_env_setup["LOCK_ON_FILE"] | |
| try: | |
| internal_run_ansible_playbooks_libvirt( | |
| provisioner, | |
| inventory_path, | |
| test_env_setup, | |
| ansible_args, | |
| setup_yml, | |
| cwd, | |
| ansible_container, | |
| wait_on_vm=True, | |
| log_file=log_file, | |
| start_vms=True, | |
| ) | |
| finally: | |
| provisioner.destroy() | |
| if os.path.exists(inventory_path): | |
| os.unlink(inventory_path) | |
| tmp_snapfile = "{}.tmp-{}".format(snapfile, uuid.uuid4().hex) | |
| with rq.file_or_stdout(log_file) as (stdout, stderr): | |
| subprocess.check_call( # nosec | |
| [ | |
| "qemu-img", | |
| "create", | |
| "-f", | |
| "qcow2", | |
| "-b", | |
| image_file, | |
| "-F", | |
| "qcow2", | |
| tmp_snapfile, | |
| ], | |
| stdout=stdout, | |
| stderr=stderr, | |
| ) | |
| snap_kwargs = dict(provisioner_kwargs) | |
| snap_kwargs["image_path"] = tmp_snapfile | |
| snap_kwargs["hostnames"] = [snap_kwargs["hostnames"][0]] | |
| snap_kwargs["write_to_image"] = True | |
| snap_kwargs["debug"] = False | |
| provisioner = LibvirtProvisioner(**snap_kwargs) | |
| inventory_path = tempfile.NamedTemporaryFile( | |
| suffix=".yml", delete=False | |
| ).name | |
| test_env_setup = {} | |
| test_env_setup.update(test_env) | |
| if "TEST_DEBUG" in test_env_setup: | |
| del test_env_setup["TEST_DEBUG"] | |
| if "TEST_ARTIFACTS" in test_env_setup: | |
| test_env_setup["TEST_ARTIFACTS"] = ( | |
| test_env_setup["TEST_ARTIFACTS"] + ".snap" | |
| ) | |
| if "LOCK_ON_FILE" in test_env_setup: | |
| del test_env_setup["LOCK_ON_FILE"] | |
| try: | |
| internal_run_ansible_playbooks_libvirt( | |
| provisioner, | |
| inventory_path, | |
| test_env_setup, | |
| ansible_args, | |
| setup_yml, | |
| cwd, | |
| ansible_container, | |
| wait_on_vm=True, | |
| log_file=log_file, | |
| start_vms=True, | |
| ) | |
| os.replace(tmp_snapfile, snapfile) | |
| finally: | |
| provisioner.destroy() | |
| if os.path.exists(tmp_snapfile): | |
| os.unlink(tmp_snapfile) | |
| if os.path.exists(inventory_path): | |
| os.unlink(inventory_path) |
🤖 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 `@src/tox_lsr/test_scripts/runlibvirt.py` around lines 1316 - 1367, The
snapshot setup flow can leave behind a partially configured snapfile if
internal_run_ansible_playbooks_libvirt fails after the qemu-img create step.
Update this runlibvirt path so the temporary snapshot is cleaned up on setup
failure, not just the provisioner and inventory_path, by removing snapfile in
the exception/cleanup path when the playbook run does not complete successfully.
Keep the fix local to the snapshot creation and
internal_run_ansible_playbooks_libvirt call sequence so later runs do not
incorrectly reuse an incomplete snapshot.
Comment on lines
+1514
to
+1552
| provisioner_kwargs["tests_dir"] = cwd | ||
|
|
||
| if ( | ||
| (batch.args and batch.args.erase_old_snapshot) | ||
| or erase_old_snapshot | ||
| ) and os.path.exists(snapfile): | ||
| os.unlink(snapfile) | ||
|
|
||
| current_image = image_file | ||
| if (batch.args and batch.args.use_snapshot) or use_snapshot: | ||
| current_image = snapfile | ||
| refresh_snapshot_libvirt( | ||
| image_file, | ||
| snapfile, | ||
| provisioner_kwargs, | ||
| test_env, | ||
| batch.ansible_args, | ||
| batch.setup_playbooks, | ||
| cwd, | ||
| post_snap_sleep_time, | ||
| local_log_file, | ||
| ansible_container, | ||
| ) | ||
| playbooks_to_run = batch.playbooks | ||
| else: | ||
| playbooks_to_run = batch.setup_playbooks + batch.playbooks | ||
|
|
||
| if ( | ||
| provisioner is None | ||
| or provisioner.image_path != os.path.abspath(current_image) | ||
| ): | ||
| if provisioner is not None: | ||
| stop_libvirt(provisioner, lock_on_file) | ||
| vms_started = False | ||
| pk = dict(provisioner_kwargs) | ||
| pk["image_path"] = current_image | ||
| pk["artifacts"] = test_env["TEST_ARTIFACTS"] | ||
| pk["debug"] = bool(test_env.get("TEST_DEBUG")) | ||
| provisioner = LibvirtProvisioner(**pk) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include tests_dir/provision.fmf in the provisioner reuse key.
provisioner_kwargs["tests_dir"] is updated per batch, but an existing provisioner is reused when only the image path matches. Batches with different provision.fmf settings can therefore run against VMs provisioned with stale memory, disk, or NIC configuration.
Proposed fix
if (
provisioner is None
or provisioner.image_path != os.path.abspath(current_image)
+ or provisioner.tests_dir != cwd
):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| provisioner_kwargs["tests_dir"] = cwd | |
| if ( | |
| (batch.args and batch.args.erase_old_snapshot) | |
| or erase_old_snapshot | |
| ) and os.path.exists(snapfile): | |
| os.unlink(snapfile) | |
| current_image = image_file | |
| if (batch.args and batch.args.use_snapshot) or use_snapshot: | |
| current_image = snapfile | |
| refresh_snapshot_libvirt( | |
| image_file, | |
| snapfile, | |
| provisioner_kwargs, | |
| test_env, | |
| batch.ansible_args, | |
| batch.setup_playbooks, | |
| cwd, | |
| post_snap_sleep_time, | |
| local_log_file, | |
| ansible_container, | |
| ) | |
| playbooks_to_run = batch.playbooks | |
| else: | |
| playbooks_to_run = batch.setup_playbooks + batch.playbooks | |
| if ( | |
| provisioner is None | |
| or provisioner.image_path != os.path.abspath(current_image) | |
| ): | |
| if provisioner is not None: | |
| stop_libvirt(provisioner, lock_on_file) | |
| vms_started = False | |
| pk = dict(provisioner_kwargs) | |
| pk["image_path"] = current_image | |
| pk["artifacts"] = test_env["TEST_ARTIFACTS"] | |
| pk["debug"] = bool(test_env.get("TEST_DEBUG")) | |
| provisioner = LibvirtProvisioner(**pk) | |
| provisioner_kwargs["tests_dir"] = cwd | |
| if ( | |
| (batch.args and batch.args.erase_old_snapshot) | |
| or erase_old_snapshot | |
| ) and os.path.exists(snapfile): | |
| os.unlink(snapfile) | |
| current_image = image_file | |
| if (batch.args and batch.args.use_snapshot) or use_snapshot: | |
| current_image = snapfile | |
| refresh_snapshot_libvirt( | |
| image_file, | |
| snapfile, | |
| provisioner_kwargs, | |
| test_env, | |
| batch.ansible_args, | |
| batch.setup_playbooks, | |
| cwd, | |
| post_snap_sleep_time, | |
| local_log_file, | |
| ansible_container, | |
| ) | |
| playbooks_to_run = batch.playbooks | |
| else: | |
| playbooks_to_run = batch.setup_playbooks + batch.playbooks | |
| if ( | |
| provisioner is None | |
| or provisioner.image_path != os.path.abspath(current_image) | |
| or provisioner.tests_dir != cwd | |
| ): | |
| if provisioner is not None: | |
| stop_libvirt(provisioner, lock_on_file) | |
| vms_started = False | |
| pk = dict(provisioner_kwargs) | |
| pk["image_path"] = current_image | |
| pk["artifacts"] = test_env["TEST_ARTIFACTS"] | |
| pk["debug"] = bool(test_env.get("TEST_DEBUG")) | |
| provisioner = LibvirtProvisioner(**pk) |
🤖 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 `@src/tox_lsr/test_scripts/runlibvirt.py` around lines 1514 - 1552, The
provisioner reuse check in runlibvirt should also account for batch-specific
provisioning inputs, not just the image path. Update the reuse key logic around
the LibvirtProvisioner creation/reuse path so changes in tests_dir and the
effective provision.fmf content/config force a fresh provisioner instead of
reusing one with stale VM settings. Use the existing provisioner_kwargs,
tests_dir assignment, and the provisioner.image_path comparison as the place to
incorporate these values before deciding whether to stop_libvirt and instantiate
a new LibvirtProvisioner.
richm
force-pushed
the
runlibvirt
branch
3 times, most recently
from
July 6, 2026 22:44
68fe147 to
e210ee2
Compare
richm
added a commit
to richm/linux-system-roles-nbde_client
that referenced
this pull request
Jul 7, 2026
This relies on linux-system-roles/tox-lsr#244 or some other system by which you can create two machines with hostnames nbde-server and nbde-client that can network with each other. Run it like this: ```bash tox -e libvirt-ansible-core-2-X -- --image-name centos-Y --log-level debug --hostnames nbde-client --hostnames nbde-server -- \ tests/tests_simple_bind_multihost.yml 2>&1 | tee output ``` Signed-off-by: Rich Megginson <rmeggins@redhat.com>
This was referenced Jul 7, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/tox_lsr/test_scripts/runqemu.py`:
- Around line 88-94: The ansible-config probe in runqemu’s import-time fallback
only catches subprocess.CalledProcessError, so missing or non-executable
binaries can still crash initialization. Update the exception handling around
the subprocess.check_output call in the ansible-config check to also cover
OSError/FileNotFoundError, or guard the call with shutil.which before invoking
it. Keep the existing logging and False fallback behavior in the same helper so
COLL_PATH_ENV_VAR initialization can proceed cleanly.
🪄 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 Plus
Run ID: 2300cc86-9601-4834-a884-9965afd2f371
📒 Files selected for processing (7)
README.mdsrc/tox_lsr/config_files/tox-default.inisrc/tox_lsr/test_scripts/libvirt-cleanup.shsrc/tox_lsr/test_scripts/runlibvirt.pysrc/tox_lsr/test_scripts/runqemu.pytests/fixtures/test_tox_merge_ini/result.initox.ini
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- tox.ini
- src/tox_lsr/test_scripts/libvirt-cleanup.sh
Comment on lines
+88
to
+94
| try: | ||
| result = subprocess.check_output( # nosec | ||
| ["ansible-config", "list"], stderr=subprocess.STDOUT, encoding="utf-8" | ||
| ) | ||
| except subprocess.CalledProcessError: | ||
| logging.error("Failed to run ansible-config list: %s", traceback.format_exc()) | ||
| return False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols first.
ast-grep outline src/tox_lsr/test_scripts/runqemu.py --view expanded || true
# Read the relevant section with line numbers.
sed -n '1,180p' src/tox_lsr/test_scripts/runqemu.py | cat -n
# Find uses of the helper and the environment variable initialization path.
rg -n "is_ansible_env_var_supported|COLL_PATH_ENV_VAR|ansible-config list|subprocess\.check_output|subprocess\.CalledProcessError" src/tox_lsr/test_scripts/runqemu.pyRepository: linux-system-roles/tox-lsr
Length of output: 9609
Broaden the exception guard. subprocess.check_output(["ansible-config", "list"], ...) can raise OSError/FileNotFoundError when ansible-config is missing or not executable, and this runs at import time before COLL_PATH_ENV_VAR is set. Catch OSError too (or gate with shutil.which) so the fallback still initializes cleanly.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 88-90: Avoid command injection
Context: subprocess.check_output( # nosec
["ansible-config", "list"], stderr=subprocess.STDOUT, encoding="utf-8"
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-python)
[error] 88-90: Command coming from incoming request
Context: subprocess.check_output( # nosec
["ansible-config", "list"], stderr=subprocess.STDOUT, encoding="utf-8"
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
[error] 90-90: Starting a process with a partial executable path
(S607)
🤖 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 `@src/tox_lsr/test_scripts/runqemu.py` around lines 88 - 94, The ansible-config
probe in runqemu’s import-time fallback only catches
subprocess.CalledProcessError, so missing or non-executable binaries can still
crash initialization. Update the exception handling around the
subprocess.check_output call in the ansible-config check to also cover
OSError/FileNotFoundError, or guard the call with shutil.which before invoking
it. Keep the existing logging and False fallback behavior in the same helper so
COLL_PATH_ENV_VAR initialization can proceed cleanly.
…other For example: ```bash tox -e libvirt-ansible-core-2-21 -- --image-name centos-10 --hostnames my_server \ --hostnames my_client -- tests/tests_multihost.yml ``` This will create two centos-10 VMs, one with hostname `my_server` and one with hostname `my_client`. From `my_client` you can do `curl http://my_server` (assuming there is an HTTP server running on my_server). Assisted-by: Cursor using models Codex 5.3, Sonnet 5, Sonnet 4.6, Composer 2.5 Signed-off-by: Rich Megginson <rmeggins@redhat.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
For example:
This will create two centos-10 VMs, one with hostname
my_serverand one withhostname
my_client. Frommy_clientyou can docurl http://my_server(assuming there is an HTTP server running on my_server).
Assisted-by: Cursor using models Codex 5.3, Sonnet 5, Sonnet 4.6, Composer 2.5
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by CodeRabbit
toxusage for multi-host playbooks.