Improve virtual_network.qos cases to support new test tool - #6848
Closed
yanglei-rh wants to merge 1 commit into
Closed
yanglei-rh wants to merge 1 commit into
yanglei-rh wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request standardizes VM lifecycle management and network configuration across four libvirt QoS test configurations while refactoring their supporting Python implementations. Configuration files add Linux platform restrictions, libvirt-based VM creation and termination, no-reboot execution mode, and OVMF NVRAM-specific kill options. The source code replaces static bridge creation with dynamic interface/network XML attributes driven by configuration, introduces OVS bridge detection and conditional setup, refactors netperf handling from package installation to source compilation, updates VM console session management, and consolidates bridge teardown logic within a unified cleanup flow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py`:
- Around line 36-37: The code reads params.get('netdst') but the cfg now uses
${bridge_name}, causing bridge_name to be empty; update the parameter reads to
use params.get('bridge_name', params.get('netdst', '')) (or consistently prefer
'bridge_name') where iface_type and bridge_name are set so
setup_ovs_bridge_attrs in provider/virtual_network/network_base.py sees the
correct value; ensure all occurrences (including the block around lines 53–66
that set iface_type and bridge_name) use the same 'bridge_name' key or fall back
to 'netdst' to preserve backward compatibility.
- Line 72: The test captures the initial firewall state in fw_was_running via
firewalld.status() but unconditionally starts the service later, which can
change host state; change the cleanup/teardown logic so any firewalld.start() or
firewalld.stop() calls are conditional based on fw_was_running (i.e., only start
firewalld at the end if fw_was_running was True, and only stop it if it was
running at the start), updating the code paths that call firewalld.start() (and
firewalld.stop()) in this test (referencing fw_was_running and
firewalld.status()) so the host firewall is restored to its original state.
- Around line 158-163: Initialize cleanup handles before the try block to avoid
UnboundLocalError: declare and set host_nserver = None and vm_sess = None (or
appropriate falsy placeholders) prior to network setup/login, then in the
finally block check for truthiness (if host_nserver: ... and if vm_sess:
vm_sess.close()) so early failures won’t raise when cleaning up; update
references to host_nserver and vm_sess in the surrounding function (the
variables used in the finally) accordingly.
🪄 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: d24947dc-8ded-4974-937c-37af5c1aac04
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qos/check_actual_network_throughput.cfglibvirt/tests/cfg/virtual_network/qos/check_bandwidth_by_domiftune.cfglibvirt/tests/cfg/virtual_network/qos/check_qos_floor.cfglibvirt/tests/cfg/virtual_network/qos/test_bandwidth_boundry.cfglibvirt/tests/src/virtual_network/qos/check_actual_network_throughput.pylibvirt/tests/src/virtual_network/qos/check_bandwidth_by_domiftune.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22417
branch
from
March 31, 2026 06:10
a21b311 to
a202a38
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py (1)
36-65:⚠️ Potential issue | 🟠 MajorResolve the bridge from
bridge_namebefore falling back tonetdst.
bridge_nameis now the public knob in the cfg and it is defined with a blank default. Reading onlynetdsthere can overwrite the already-expanded bridge attrs with'', skip thevirbr0guard, and pass an empty bridge intonetwork_base.setup_ovs_bridge_attrs(). Useparams.get("bridge_name") or params.get("netdst", "")in this flow, and make the helper use the same fallback.Possible fix
- bridge_name = params.get('netdst', '') + bridge_name = params.get('bridge_name') or params.get('netdst', '') @@ if iface_type == 'bridge': - iface_attrs['source'] = {'bridge': bridge_name} + if bridge_name: + iface_attrs['source'] = {'bridge': bridge_name} network_base.setup_ovs_bridge_attrs(params, iface_attrs) @@ elif iface_type == 'network' and params.get('net_fwm') == 'bridge': - net_attrs['bridge'] = {'name': bridge_name} + if bridge_name: + net_attrs['bridge'] = {'name': bridge_name} if is_ovs_bridge: net_attrs['virtualport_type'] = 'openvswitch'# provider/virtual_network/network_base.py - netdst = params.get('netdst', '') + netdst = params.get('bridge_name') or params.get('netdst', '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py` around lines 36 - 65, The code currently reads bridge_name from params.get('netdst', '') which can overwrite an already-resolved public knob and lead to blank/incorrect bridge values; change the bridge resolution to use params.get("bridge_name") or params.get("netdst", "") wherever bridge_name is derived so the public param wins, and update the helper call sites (e.g., where network_base.setup_ovs_bridge_attrs is invoked) to use the same fallback; ensure variables iface_attrs and net_attrs continue to be populated based on this resolved bridge_name and preserve the virbr0 guard and the is_ovs_bridge detection that calls utils_net.find_bridge_manager(bridge_name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@provider/virtual_network/netperf_base.py`:
- Around line 166-172: The guest branch currently always sets install_path to
guest_netperf_path which ignores the new client_path; change the logic in the
address-handling block so that when address is a VM you prefer
params.get("client_path") (falling back to guest_netperf_path) to set
install_path (while retaining session = vm.wait_for_login()); update any code
that consumes install_path (e.g., paths used by
check_actual_network_throughput.py) to rely on this chosen install_path. Ensure
the variable names referenced are guest_netperf_path, client_path
(params.get("client_path")), vm.wait_for_login(), session, and install_path so
the correct guest-side install directory is honored.
---
Duplicate comments:
In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py`:
- Around line 36-65: The code currently reads bridge_name from
params.get('netdst', '') which can overwrite an already-resolved public knob and
lead to blank/incorrect bridge values; change the bridge resolution to use
params.get("bridge_name") or params.get("netdst", "") wherever bridge_name is
derived so the public param wins, and update the helper call sites (e.g., where
network_base.setup_ovs_bridge_attrs is invoked) to use the same fallback; ensure
variables iface_attrs and net_attrs continue to be populated based on this
resolved bridge_name and preserve the virbr0 guard and the is_ovs_bridge
detection that calls utils_net.find_bridge_manager(bridge_name).
🪄 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: 7aaf656a-444e-4a20-9466-62fa2d2b8bdf
📒 Files selected for processing (3)
libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.py
yanglei-rh
force-pushed
the
LIBVIRTAT-22417
branch
from
March 31, 2026 06:19
a202a38 to
f1d5812
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py (2)
169-169:⚠️ Potential issue | 🟠 MajorConditionally restore firewall based on its original state.
fw_was_runningis captured at line 72, butfirewalld.start()is called unconditionally here. If the host firewall was intentionally stopped before the test, this leaves the system in a different state than expected, potentially affecting subsequent tests.Proposed fix
- firewalld.start() + if fw_was_running: + firewalld.start()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py` at line 169, The tear-down unconditionally calls firewalld.start() which ignores the recorded fw_was_running state; update the cleanup to check the boolean captured as fw_was_running (from where it was set earlier) and only call firewalld.start() if fw_was_running is True, otherwise leave the firewall stopped (or call firewalld.stop() if you need to enforce the original stopped state); locate the teardown/cleanup block that invokes firewalld.start() and wrap it with a conditional referencing fw_was_running so the host firewall is restored to its original state.
67-73:⚠️ Potential issue | 🔴 CriticalInitialize
vm_sessandhost_nserverbefore thetryblock to preventUnboundLocalError.These variables are assigned at lines 93 and 132 respectively, but the
finallyblock (lines 160-163) checks them. If an exception occurs before their assignment (e.g., duringvm.start()at line 90), thefinallyblock will raiseUnboundLocalError, masking the real failure.Proposed fix
net_xml = NetworkXML.new_from_net_dumpxml('default') bk_net_xml = net_xml.copy() firewalld = service.Factory.create_service("firewalld") fw_was_running = firewalld.status() + vm_sess = None + host_nserver = None try:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py` around lines 67 - 73, The finally block references vm_sess and host_nserver but they are only assigned inside the try block (after vm.start() and later), which can cause UnboundLocalError if an earlier exception occurs; initialize vm_sess and host_nserver to None before the try so the finally can safely check/cleanup them (e.g., set vm_sess = None and host_nserver = None just before the try that contains vm.start(), and keep the existing checks in the finally to only act when they are not None).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libvirt/tests/src/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.py`:
- Around line 41-49: The current sequence batches setup and daemon start into
multi_cmd1/multi_cmd2 so failures are masked and chcon/chown may race virtiofsd
socket creation; instead run and check each preparatory command (mkdir ->
process.run/remote.run_remote_cmd for cmd1), then start virtiofsd separately
(cmd2) without combining it with mkdir, then poll/wait for socket_path to appear
(with a timeout) before running cmd3/cmd4 via process.run so chcon -t
svirt_image_t and chown qemu:qemu are executed only after the socket exists and
their failures are visible; update uses of multi_cmd1/multi_cmd2 to individual
calls and add a small wait-loop for socket_path after invoking virtiofsd.
- Around line 98-109: The cleanup_test function can short-circuit remaining
teardown when migration_obj.cleanup_connection() raises, leaving virtiofsd
remnants; change cleanup_test to collect cleanup actions (e.g., append lambdas
or callables for migration_obj.cleanup_connection, remote.run_remote_cmd('pkill
virtiofsd', ...), remote.run_remote_cmd(f"rm -rf {socket_path}", ...),
process.run("pkill virtiofsd", ...), process.run(f"rm -rf {socket_path}", ...))
into a list as each resource is created/required, then in a finally block
iterate that list and run each cleanup action inside its own try/except so one
failure doesn’t prevent the others from running; ensure you reference
cleanup_test and migration_obj.cleanup_connection when locating the code to
modify.
In `@libvirt/tests/src/virtual_network/qos/check_bandwidth_by_domiftune.py`:
- Line 35: The code uses eval on params.get to parse dict literals (e.g.,
iface_attrs = eval(params.get('iface_attrs', '{}'))), which is unsafe; replace
these eval(...) calls with ast.literal_eval(...) and add an import ast at the
top of the module, ensuring all occurrences that parse parameter dict strings
(the iface_attrs assignment and the other two eval usages in this file) are
converted to use ast.literal_eval to safely parse the literal dictionaries.
In `@v2v/tests/src/nbdkit/nbdkit.py`:
- Around line 746-749: The hard-coded "instances" endpoints (e.g., the instances
list with ports 10809/10810 and hard-coded nbd://localhost and /tmp/test.sock)
cause flakes; change the nbdkit server helper to generate per-test endpoints by
creating unique Unix socket paths inside the test temporary directory (use
tmp_path or tempfile.mkdtemp) or by selecting ephemeral TCP ports (bind a socket
to port 0 to get a free port) and use those values when constructing "instances"
and any nbd:// URLs; update the helper that starts/stops servers (and all places
that build socket paths or URLs — the code around the instances definition and
the other similar blocks called in this file) to accept/use these generated
endpoints and ensure cleanup only removes sockets from the test temp dir.
- Around line 875-890: Tighten the base64 test by verifying exit status and
exact payload: after running process.run(...) in the positive branch (where
test_state is not "negative"), assert result.exit_code == 0 and that
result.stdout (or cmd_output) exactly equals original_text (not just contains
it) before calling test.fail; in the negative branch assert result.exit_code !=
0 and then check that params_get(params, "expected_err_msg") appears in
result.stderr_text (and call test.fail if either condition is not met). Use the
existing variables result, cmd_output/result.stdout, result.stderr_text,
params_get and test.fail to implement these checks.
- Around line 822-856: The test reads the subprocess output before stopping
nbdkit, but the count-filter only emits its aggregate summary on unload; update
the test to call p.stop() (or p.terminate()/p.wait() as appropriate) before
capturing logs via p.get_stdout()/p.get_stderr(), then check for the aggregate
summary emitted on unload (e.g., match the "count bytes: read X, written Y,
zeroed Z, trimmed W" format) instead of per-request "count:
pwrite/pread/zero/trim" lines; adjust the expected_patterns (or replace them
with a single regex/assertion) to look for the aggregate summary after p.stop()
completes.
In `@v2v/tests/src/v2v_options.py`:
- Around line 960-975: The code builds and runs cmd with unchecked vpx_ipv6_addr
and uses process.run(..., shell=True) which risks command injection; instead
validate/sanitize vpx_ipv6_addr (e.g. via the ipaddress.IPv6Address constructor)
before embedding it into cmd, perform the re.sub replacements only after
validation, and avoid shell=True by constructing an argument list for
process.run (use a list of args and pass env={'LIBVIRT_DEBUG': '1'} or merge
into os.environ) when calling process.run in the branch handling checkpoint ==
'vpx_with_invalid_esx_ipv6'; update the process.run call sites (the calls that
set cmd_result) to use shell=False and an args list so the parameter cannot
inject shell metacharacters.
---
Duplicate comments:
In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py`:
- Line 169: The tear-down unconditionally calls firewalld.start() which ignores
the recorded fw_was_running state; update the cleanup to check the boolean
captured as fw_was_running (from where it was set earlier) and only call
firewalld.start() if fw_was_running is True, otherwise leave the firewall
stopped (or call firewalld.stop() if you need to enforce the original stopped
state); locate the teardown/cleanup block that invokes firewalld.start() and
wrap it with a conditional referencing fw_was_running so the host firewall is
restored to its original state.
- Around line 67-73: The finally block references vm_sess and host_nserver but
they are only assigned inside the try block (after vm.start() and later), which
can cause UnboundLocalError if an earlier exception occurs; initialize vm_sess
and host_nserver to None before the try so the finally can safely check/cleanup
them (e.g., set vm_sess = None and host_nserver = None just before the try that
contains vm.start(), and keep the existing checks in the finally to only act
when they are not None).
🪄 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: 6cc06bf2-721c-4107-956f-6724e67a9c04
📒 Files selected for processing (20)
libvirt/tests/cfg/libvirt_mem.cfglibvirt/tests/cfg/memory/memory_discard.cfglibvirt/tests/cfg/memory/memory_misc.cfglibvirt/tests/cfg/memory/nvdimm.cfglibvirt/tests/cfg/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.cfglibvirt/tests/cfg/virtual_network/qos/check_actual_network_throughput.cfglibvirt/tests/cfg/virtual_network/qos/check_bandwidth_by_domiftune.cfglibvirt/tests/cfg/virtual_network/qos/check_qos_floor.cfglibvirt/tests/cfg/virtual_network/qos/test_bandwidth_boundry.cfglibvirt/tests/src/memory/memory_devices/dimm_memory_lifecycle.pylibvirt/tests/src/memory/memory_devices/virtio_mem_device_lifecycle.pylibvirt/tests/src/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.pylibvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.pylibvirt/tests/src/virtual_network/qos/check_actual_network_throughput.pylibvirt/tests/src/virtual_network/qos/check_bandwidth_by_domiftune.pyv2v/tests/cfg/function_test_esx.cfgv2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/cfg/v2v_options.cfgv2v/tests/src/nbdkit/nbdkit.pyv2v/tests/src/v2v_options.py
✅ Files skipped from review due to trivial changes (4)
- libvirt/tests/cfg/libvirt_mem.cfg
- libvirt/tests/cfg/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.cfg
- v2v/tests/cfg/function_test_esx.cfg
- libvirt/tests/cfg/memory/memory_discard.cfg
🚧 Files skipped from review as they are similar to previous changes (3)
- libvirt/tests/cfg/virtual_network/qos/test_bandwidth_boundry.cfg
- libvirt/tests/cfg/virtual_network/qos/check_bandwidth_by_domiftune.cfg
- libvirt/tests/cfg/virtual_network/qos/check_qos_floor.cfg
yanglei-rh
force-pushed
the
LIBVIRTAT-22417
branch
from
March 31, 2026 06:37
f1d5812 to
6b70344
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libvirt/tests/src/virtual_disks/virtual_disks_https.py (1)
139-159:⚠️ Potential issue | 🔴 CriticalCleanup can crash with
UnboundLocalErroron early setup failure.If
setup_test()fails,qemu_configis undefined but still restored infinally.🔧 Suggested fix
- try: + qemu_config = None + try: qemu_config = setup_test() @@ finally: backup_vmxml.sync() - qemu_config.restore() + if qemu_config is not None: + qemu_config.restore() Libvirtd('virtqemud').restart()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_disks/virtual_disks_https.py` around lines 139 - 159, The finally block can raise UnboundLocalError if setup_test() fails because qemu_config (and possibly disk_dev) are not defined; to fix, initialize qemu_config (and any other variables used in finally like disk_dev) to None before the try, and in the finally guard calls with checks (e.g., if qemu_config is not None: qemu_config.restore(); if backup_vmxml is not None: backup_vmxml.sync(); and only call Libvirtd('virtqemud').restart() if appropriate), so replace direct restores with conditional calls around qemu_config.restore() and backup_vmxml.sync() while keeping the same cleanup intent.
♻️ Duplicate comments (7)
libvirt/tests/src/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.py (2)
41-50:⚠️ Potential issue | 🟠 MajorSplit setup/start/relabel steps; current flow still masks failures and races socket relabeling.
Line 44/Line 49 only reflect the last subcommand in each
;chain, andnohup ... &can return before the socket exists, sochcon/chownmay run too early on slower hosts.Suggested fix
- multi_cmd1 = f"{cmd1}; {cmd2}" - process.run(multi_cmd1, shell=True, ignore_status=False, ignore_bg_processes=True) - remote.run_remote_cmd(multi_cmd1, params, ignore_status=False) + process.run(cmd1, shell=True, ignore_status=False) + remote.run_remote_cmd(cmd1, params, ignore_status=False) + process.run(cmd2, shell=True, ignore_status=False, ignore_bg_processes=True) + remote.run_remote_cmd(cmd2, params, ignore_status=False) + wait_cmd = ( + f'for _ in $(seq 1 50); do [ -S "{socket_path}" ] && exit 0; ' + 'sleep 0.2; done; exit 1' + ) + process.run(wait_cmd, shell=True, ignore_status=False) + remote.run_remote_cmd(wait_cmd, params, ignore_status=False) - multi_cmd2 = f"{cmd3}; {cmd4}" - process.run(multi_cmd2, shell=True, ignore_status=False) - remote.run_remote_cmd(multi_cmd2, params, ignore_status=False) + process.run(cmd3, shell=True, ignore_status=False) + process.run(cmd4, shell=True, ignore_status=False) + remote.run_remote_cmd(cmd3, params, ignore_status=False) + remote.run_remote_cmd(cmd4, params, ignore_status=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.py` around lines 41 - 50, The current setup chains commands (multi_cmd1/multi_cmd2) which masks failures and races the socket relabeling; instead split into distinct steps: run the mkdir (cmd1) via process.run/remote.run_remote_cmd, start virtiofsd (cmd2) without chaining and then poll/wait for the socket file to appear (check -S {socket_path} or loop with timeout) before proceeding, and only after the socket exists run the relabel/chown commands (cmd3/cmd4) individually via process.run/remote.run_remote_cmd so each failure is visible and ordering is guaranteed; also avoid relying on immediate success of nohup/& (do not use ignore_bg_processes to hide background startup errors).
105-109:⚠️ Potential issue | 🟠 MajorPrevent cleanup short-circuit; one teardown failure currently skips the rest.
If Line 105 raises, later local/remote
pkilland socket cleanup never run, which can hide the real test failure and leak state.Suggested fix
- migration_obj.cleanup_connection() - remote.run_remote_cmd('pkill virtiofsd', params, ignore_status=True) - remote.run_remote_cmd(f"rm -rf {socket_path}", params, ignore_status=True) - process.run("pkill virtiofsd", shell=True, ignore_status=True) - process.run(f"rm -rf {socket_path}", shell=True, ignore_status=True) + cleanup_steps = [ + lambda: migration_obj.cleanup_connection(), + lambda: remote.run_remote_cmd("pkill virtiofsd", params, ignore_status=True), + lambda: remote.run_remote_cmd(f"rm -rf {socket_path}", params, ignore_status=True), + lambda: process.run("pkill virtiofsd", shell=True, ignore_status=True), + lambda: process.run(f"rm -rf {socket_path}", shell=True, ignore_status=True), + ] + for step in cleanup_steps: + try: + step() + except Exception as err: + test.log.warning("Cleanup step failed: %s", err)Based on learnings, in tp-libvirt tests prefer using a list of cleanup lambda functions appended for performed actions, then executed in finally with per-action error handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.py` around lines 105 - 109, The current teardown calls (migration_obj.cleanup_connection(), remote.run_remote_cmd('pkill virtiofsd', ...), remote.run_remote_cmd(f"rm -rf {socket_path}", ...), process.run("pkill virtiofsd", ...), process.run(f"rm -rf {socket_path}", ...)) are executed sequentially so an exception in migration_obj.cleanup_connection() short-circuits remaining cleanup; change to collect cleanup actions as a list of lambdas/closures when each action is performed (e.g., append a lambda that calls migration_obj.cleanup_connection(), one for remote.run_remote_cmd('pkill virtiofsd', params, ignore_status=True), one for remote.run_remote_cmd(f"rm -rf {socket_path}", ...), and local process.run calls), then in a finally block iterate over that list and invoke each action inside its own try/except to log but not raise on error so all pkill and socket removals always run even if one cleanup fails.v2v/tests/src/v2v_options.py (1)
960-975:⚠️ Potential issue | 🟠 MajorAvoid
shell=Truefor parameter-influenced command execution.The
vpx_ipv6_addrparameter is embedded intocmdviare.suband then executed withshell=True(lines 971-972). While parameters typically come from trusted test configurations, usingshell=Trueis flagged by static analysis (Ruff S604) and is a security anti-pattern. Consider using environment variables instead of string concatenation forLIBVIRT_DEBUG=1.🔧 Suggested improvement
if checkpoint == 'vpx_with_invalid_esx_ipv6': ipv4_pattern = r'\b(?:\d{1,3}\.){3}\d{1,3}\b' vpx_ipv6_addr = params.get('vpx_ipv6_addr') if not vpx_ipv6_addr: test.error("vpx_ipv6_addr parameter is missing") cmd = re.sub(ipv4_pattern, vpx_ipv6_addr, cmd) cmd = re.sub(r'/\[(.*?)\]/', r'/\1/', cmd) - cmd_export_env = 'LIBVIRT_DEBUG=1' - cmd = "%s %s" % (cmd_export_env, cmd) - if checkpoint == 'vpx_with_invalid_esx_ipv6': - cmd_result = process.run(cmd, timeout=v2v_timeout, verbose=True, - ignore_status=True, shell=True) + if checkpoint == 'vpx_with_invalid_esx_ipv6': + env = os.environ.copy() + env['LIBVIRT_DEBUG'] = '1' + cmd_result = process.run(cmd, timeout=v2v_timeout, verbose=True, + ignore_status=True, env=env) else: cmd_result = process.run(cmd, timeout=v2v_timeout, verbose=True, ignore_status=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/src/v2v_options.py` around lines 960 - 975, The code builds a string command (cmd) by inserting the vpx_ipv6_addr and then calls process.run(..., shell=True) when checkpoint == 'vpx_with_invalid_esx_ipv6', which risks shell injection and is flagged; instead construct a safe argument list and pass an environment dict with LIBVIRT_DEBUG=1 to process.run so you can remove shell=True: keep using the same checkpoint check and vpx_ipv6_addr lookup but replace string concatenation of cmd (and the use of cmd_export_env) with a list/sequence of arguments for process.run (or tokenize the existing cmd into args safely) and pass env={**os.environ, "LIBVIRT_DEBUG":"1"} (or equivalent in your test harness) to process.run while calling it without shell=True and still using timeout=v2v_timeout, verbose=True, ignore_status=True.provider/virtual_network/netperf_base.py (1)
166-172:⚠️ Potential issue | 🟡 Minor
client_pathparameter is not honored for VM-side netperf builds.The VM branch always installs under
guest_netperf_path, ignoring anyclient_pathparameter that may be configured in the test cfg. This could cause mismatches between where netperf is installed and where subsequent test code expects to find it.Proposed fix to prefer client_path
- guest_netperf_path = params.get("guest_netperf_path", "/var/tmp/") + guest_netperf_path = ( + params.get("client_path") + or params.get("guest_netperf_path", "/var/tmp/") + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/netperf_base.py` around lines 166 - 172, The VM branch ignores a configured client_path and always sets install_path to guest_netperf_path; update the branch that handles address in params.get('vms', '').split() so it prefers params.get("client_path") (falling back to guest_netperf_path) when assigning install_path for VM builds (the code around guest_netperf_path, the elif branch using env.get_vm(address) and session, install_path = vm.wait_for_login(), guest_netperf_path). Ensure install_path is computed from params.get("client_path", guest_netperf_path) so subsequent code finds netperf in the expected client location.libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py (2)
159-163:⚠️ Potential issue | 🟠 MajorInitialize cleanup handles before entering the
try.
vm_sessandhost_nserverare assigned much later. Any failure during XML setup, VM boot, or netperf compilation will make thisfinallyraiseUnboundLocalErrorand hide the real test failure.🧹 Guard cleanup against early failures
firewalld = service.Factory.create_service("firewalld") fw_was_running = firewalld.status() + vm_sess = None + host_nserver = None try:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py` around lines 159 - 163, Initialize the cleanup handles before entering the try block to avoid UnboundLocalError in the finally: explicitly set vm_sess = None and host_nserver = None (or other appropriate sentinel values) prior to the code that may raise, then in the finally guard the cleanup calls by checking those variables (e.g., if host_nserver is not None: process.run(...); if vm_sess is not None: vm_sess.close()). This ensures the finally block in check_actual_network_throughput.py safely skips cleanup when setup/boot/compilation fails.
72-72:⚠️ Potential issue | 🟠 MajorRestore
firewalldonly if this case stopped it.
fw_was_runningis captured, but cleanup still starts the service unconditionally. If the host firewall was intentionally down before this test, the case leaves the lab in a different state.🔁 Preserve the original firewall state
- firewalld.start() + if fw_was_running: + firewalld.start()Also applies to: 158-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py` at line 72, The test captures the original firewall state in fw_was_running (via firewalld.status()) but the cleanup logic currently starts the firewalld service unconditionally; change the teardown to restore the original state instead: call firewalld.start() only if fw_was_running is True and call firewalld.stop() only if fw_was_running is False (or otherwise ensure the opposite action is taken only when the original state differs). Update the cleanup in check_actual_network_throughput.py (and the similar block around lines 158-169) to reference fw_was_running rather than always starting the service.provider/virtual_network/network_base.py (1)
757-791:⚠️ Potential issue | 🟠 MajorResolve the bridge name consistently in the new OVS helpers.
Several callers already resolve
bridge_namelocally, including a'virbr0'fallback and anetdst or bridge_namefallback, but these helpers re-read onlyparams['netdst']. In those paths,cancel_if_ovs_bridge()can inspect the wrong bridge, andsetup_ovs_bridge_attrs()can raiseBridge '' does not existeven though the caller already has a valid bridge value.🔧 Let callers pass the resolved bridge name
-def cancel_if_ovs_bridge(params, test): +def cancel_if_ovs_bridge(params, test, bridge_name=None): """ Cancel test if host using OVS bridge (for only Linux Bridge tests). @@ - bridge_name = params.get('netdst', '') + bridge_name = bridge_name or params.get('netdst') or params.get('bridge_name', '') @@ -def setup_ovs_bridge_attrs(params, iface_attrs): +def setup_ovs_bridge_attrs(params, iface_attrs, bridge_name=None): """ Setup OVS bridge attributes based on netdst parameter. @@ - netdst = params.get('netdst', '') - br_obj = utils_net.find_bridge_manager(netdst) + bridge_name = bridge_name or params.get('netdst') or params.get('bridge_name', '') + br_obj = utils_net.find_bridge_manager(bridge_name) if br_obj is None: - raise exceptions.TestError(f"Bridge '{netdst}' does not exist") + raise exceptions.TestError(f"Bridge '{bridge_name}' does not exist") if "OpenVSwitch" in br_obj.__class__.__name__: iface_attrs['type_name'] = 'bridge' - iface_attrs['source'] = {'bridge': netdst} + iface_attrs['source'] = {'bridge': bridge_name} iface_attrs['virtualport'] = {'type': 'openvswitch'}You'd then pass each caller's already-resolved
bridge_nameinto these helpers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provider/virtual_network/network_base.py` around lines 757 - 791, The helpers cancel_if_ovs_bridge and setup_ovs_bridge_attrs should use a caller-resolved bridge name instead of always re-reading params['netdst']; change their signatures to accept an optional resolved_bridge (e.g., def cancel_if_ovs_bridge(params, test, resolved_bridge=None) and def setup_ovs_bridge_attrs(params, iface_attrs, resolved_bridge=None)) and then use resolved_bridge if provided (falling back to params.get('netdst', '') only if not), update logic to call utils_net.find_bridge_manager(resolved_bridge) and adjust the TestError message accordingly, and update all callers to pass the previously-resolved bridge_name (or leave omitted when they rely on params).
🧹 Nitpick comments (6)
v2v/tests/cfg/convert_from_file.cfg (1)
267-273: Consider addingversion_requiredfor consistency.The corresponding
char_slashvariant infunction_test_esx.cfgspecifiesversion_required = "[virt-v2v-2.10.0-5,)", but thischaracter_slashvariant does not. If this feature depends on a specific virt-v2v version, adding the version requirement would ensure test consistency and prevent failures on older versions.🔧 Suggested addition
- character_slash: only esx_80 only output_mode.libvirt boottype = 3 + version_required = "[virt-v2v-2.10.0-5,)" v2v_debug = '' checkpoint = 'character_slash' main_vm = VM_NAME_GUEST_DISK_WITH_CHAR_SLASH_V2V_EXAMPLE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2v/tests/cfg/convert_from_file.cfg` around lines 267 - 273, Add a version_required entry to the character_slash variant to match the corresponding char_slash variant in function_test_esx.cfg; specifically add version_required = "[virt-v2v-2.10.0-5,)" under the character_slash block so the test declares the same minimum virt-v2v version dependency and stays consistent with the char_slash variant.virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py (1)
84-85: Consider sharing this staged-bib_refallowlist.This literal list now exists here and in
virttools/tests/src/bootc_image_builder/bootc_disk_image_install.py. The next variant addition will be easy to miss in one path, so a shared constant/helper inprovider.bootc_image_builder.bootc_image_build_utilswould keep build/install behavior aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py` around lines 84 - 85, Extract the literal list of allowed bib_ref values into a shared constant (e.g., STAGED_BIB_REFS) in provider.bootc_image_builder.bootc_image_build_utils, export it, and replace the inline membership checks that reference bib_ref in functions like the one using "if bib_ref in [...]" in bootc_disk_image_build.py and the corresponding check in bootc_disk_image_install.py to use "if bib_ref in STAGED_BIB_REFS" (ensure you import STAGED_BIB_REFS from bootc_image_build_utils); update any tests or callers to import the constant so both build and install paths use the same centralized allowlist.libvirt/tests/src/virtual_network/link_state/link_state_model_type.py (1)
64-67: Slight comment-to-code mismatch.The inline comment says "Configure ovs bridge when host using ovs bridge," but the condition
params.get("network_test") == "yes"is actually triggered by theinterface_type: networkconfig variant. Thesetup_ovs_bridge_attrsfunction itself likely handles detecting whether OVS is in use. Consider clarifying the comment to reflect that this handles OVS-specific attributes when running network interface tests.Suggested comment clarification
- # Configure ovs bridge when host using ovs bridge - if params.get("network_test") == "yes": + # Setup OVS bridge attributes if applicable for network interface tests + if params.get("network_test") == "yes":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/link_state/link_state_model_type.py` around lines 64 - 67, The comment is misleading: replace "Configure ovs bridge when host using ovs bridge" with a clearer note that this block applies OVS-specific attribute adjustments for the network interface test variant (triggered by params.get("network_test") == "yes") because network_base.setup_ovs_bridge_attrs(params, iface_attrs) will itself detect OVS usage; update the inline comment near the params check and the call to network_base.setup_ovs_bridge_attrs to indicate it's applying OVS-related attributes for the network test case rather than unconditionally configuring an OVS bridge.libvirt/tests/cfg/virtual_network/link_state/link_state_model_type.cfg (1)
25-31: Minor indentation inconsistency on line 30.The
kill_vm_libvirt_optionsline has extra leading spaces compared to standard indentation used in other cfg files. While it may still parse correctly, consider aligning with the standard 4-space indent underovmf:.Suggested fix
ovmf: - kill_vm_libvirt_options = --nvram + kill_vm_libvirt_options = --nvram🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/cfg/virtual_network/link_state/link_state_model_type.cfg` around lines 25 - 31, The line with kill_vm_libvirt_options under the ovmf: block has extra leading spaces causing inconsistent indentation; open the cfg and align kill_vm_libvirt_options to use the standard 4-space indent under the ovmf key so it matches other entries (i.e., make the indentation for kill_vm_libvirt_options consistent with the surrounding create_vm_libvirt/use_no_reboot entries).libvirt/tests/src/virtual_network/connectivity/connectivity_check_network_interface.py (1)
69-74: Consider catching a more specific exception.The broad
Exceptioncatch (flagged by Ruff BLE001) works but could mask unexpected errors. Sincerestart_guest_networklikely raises specific exceptions for network failures, consider catching those explicitly, or at minimum useException as ewith the full traceback logged at debug level.That said, the current approach is acceptable here since DHCPv6 may legitimately fail in some test environments, and the warning log provides visibility.
Optional: Log traceback for debugging
for vm_session in [session, ep_session]: try: utils_net.restart_guest_network(vm_session, ip_version='ipv6', timeout=15) LOG.info('DHCPv6 request completed') except Exception as e: - LOG.warning(f'DHCPv6 request failed: {e}') + LOG.warning(f'DHCPv6 request failed: {e}', exc_info=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/connectivity/connectivity_check_network_interface.py` around lines 69 - 74, The loop currently catches a broad Exception for calls to utils_net.restart_guest_network (for vm_session and ep_session); change this to catch more specific exceptions raised by that function (e.g., RuntimeError, OSError, subprocess.CalledProcessError or a custom network exception if one exists) or catch a tuple of those specific types instead of Exception, and keep the LOG.warning message for expected DHCPv6 failures; if you cannot identify a single specific exception, still add a debug-level log of the full traceback (use traceback.format_exc() via LOG.debug) alongside the warning to preserve diagnostic info while avoiding masking unexpected errors.libvirt/tests/src/virtual_network/iface_update.py (1)
273-279: Scope thelibvirtdrestart workaround more narrowly.This now restarts
libvirtdfor every live-update path, while the comment and the follow-up serial-console handling only tie the workaround to the link-state flow. Keeping it behindnew_iface_linkwould avoid extra daemon restarts in unrelated iface-update cases.♻️ Proposed change
- if not cold_update and vm.is_alive() and not libvirtd_restarted: + if new_iface_link and not cold_update and vm.is_alive() and not libvirtd_restarted: logging.info("Restart libvirtd server") libvirtd = utils_libvirtd.Libvirtd() libvirtd.restart() libvirtd_restarted = True🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/iface_update.py` around lines 273 - 279, The libvirtd restart workaround is currently applied for every live-update path; restrict it to only run when the update involves a link-state change by adding the new_iface_link condition to the existing check so the block runs only when not cold_update, vm.is_alive(), not libvirtd_restarted, and new_iface_link is true; keep the existing instantiation (libvirtd = utils_libvirtd.Libvirtd()) and restart() call and set libvirtd_restarted = True as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libvirt/tests/cfg/virtual_network/lifecycle/lifecycle_full_xml_interface.cfg`:
- Around line 65-66: The variant key for s390 is inconsistent: the direct_bridge
block uses "s390_virtio" while other blocks and the global override use
"s390-virtio", preventing s390-specific overrides from applying; update the
variant name used in the direct_bridge configuration to "s390-virtio" (or
alternatively make all occurrences "s390_virtio") so the variant key matches
across the file and the s390 overrides like features_xpath and driver_config
(and network_features) will be applied consistently.
In `@libvirt/tests/cfg/virtual_network/passt/passt_function.cfg`:
- Line 109: The cleanup command stored in kill_backend_cmd currently uses
"disown && killall ..." which prevents killall from running if disown fails;
change kill_backend_cmd so killall is executed unconditionally (e.g., use a
sequence operator or ensure disown errors are ignored) so that "killall tshark
dhcpcd" always runs even if disown fails.
In `@libvirt/tests/src/virtual_disks/virtual_disks_https.py`:
- Around line 70-73: iso_path currently hardcodes the leading
"/fedora/linux/releases/..." and ignores the configured base_url; change the
code that sets iso_path (the iso_path assignment) to build a relative_path =
f"fedora/linux/releases/{version}/{server_type}/{arch}/iso/Fedora-{server_type}-netinst-{arch}-{version}-{build}.iso"
and then combine it with the configured base_url (e.g. base_url) in a safe way
(base_url.rstrip('/') + '/' + relative_path.lstrip('/') or via
urllib.parse.urljoin) so the final URL respects any path component in base_url.
- Around line 17-25: Validate the base_url in get_latest_fedora_version before
calling urlopen: parse base_url (e.g., with urllib.parse.urlparse), ensure
scheme == "https" and that netloc/hostname is present, and raise or return a
clear error if validation fails; apply the same validation before any other
urlopen calls in this module so file://, ftp://, or malformed URLs are rejected
and only HTTPS hostnames are allowed.
- Line 104: Replace unsafe eval() usage with ast.literal_eval for parsing test
parameters: import ast at the top of the module and change the parsing of
disk_dict (currently disk_dict = eval(params.get("disk_dict", "{}") %
(network_device, iso_path))) to use ast.literal_eval on the formatted string,
and likewise replace the eval() that produces expected_xpaths (line ~131) with
ast.literal_eval on its parameter string. Keep the same string formatting with
params.get and ensure you call ast.literal_eval(...) on the resulting string to
safely parse literal data from params.
In `@libvirt/tests/src/virtual_network/driver/rx_tx_queue_size.py`:
- Around line 35-42: The code currently assigns iface_attrs['source'] =
{'bridge': bridge_name} then immediately checks and deletes
iface_attrs['source']['network'], which is dead; either remove that deletion or
merge instead of overwriting: retrieve the original source dict (orig =
iface_attrs.get('source', {}).copy()), pop 'network' from orig if present, set
orig['bridge'] = bridge_name and then assign iface_attrs['source'] = orig so
existing keys are preserved; update the block around the assignment to use this
merge approach (or simply delete the two lines that check/delete 'network' if a
full replacement is intended) and keep the subsequent call to
network_base.setup_ovs_bridge_attrs(params, iface_attrs).
In
`@libvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.py`:
- Around line 76-90: The code currently only returns an address when
address_elem.get('type') == 'pci', which breaks non‑PCI guests; change the logic
in the block that queries dumped_vmxml.devices.by_device_tag('interface') /
iface.xmltreefile.find('address') to read address_elem.get('type') into a
variable (e.g., addr_type) and handle non‑PCI types instead of hardcoding 'pci':
if addr_type == 'pci' keep the existing domain:bus:slot.function formatting,
else for ccw/spapr-vio (or any other addr_type) serialize the address element
into a stable string (for example join its attributes or use its tag+attributes)
and return that along with the addr_type (or a single string prefixed with the
type) so check_pci_address_difference() (or the calling comparison) can either
be gated to only run for pci types or can compare addresses of the same type;
update any callers to expect the new type-aware address format or skip the
comparison for non‑pci types.
In `@libvirt/tests/src/virtual_network/iface_bridge.py`:
- Line 116: The cleanup is unconditionally removing NETWORK_SCRIPT + bridge_name
even when bridge_name was provided via netdst (a pre-existing host bridge);
update the teardown/cleanup logic to detect when bridge_name came from
params.get("netdst") vs when the test created it and skip deleting the host
bridge's ifcfg in netdst cases. Specifically, add a flag or check around the
code that deletes NETWORK_SCRIPT + bridge_name (and the same check used in
bridge teardown) so it only removes files for bridges created by this test
(refer to the bridge_name variable, params.get("netdst"), and NETWORK_SCRIPT)
and apply the same guard at the other occurrence mentioned (lines ~409-410) to
avoid wiping persistent host configs.
- Line 128: The code uses eval() to parse the iface_source test parameter
(iface_source = eval(params.get("iface_source", "{'bridge':'%s'}" %
bridge_name))) which is unsafe; replace eval() with ast.literal_eval() so the
string from params.get(...) is parsed as a literal safe dict (ensure ast is
imported and used to call ast.literal_eval on the same params.get(...)
expression), preserving the default "{'bridge':'%s'}" % bridge_name behavior.
In
`@libvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.py`:
- Around line 84-90: When interface_timing == "hotplug" the code calls
vm.start() then uses virsh.attach_device(..., flagstr="--config") which only
updates persistent XML and does not attach to the running domain; change the
attach call in that branch to use flagstr="--live --config" (or conditionally
include "--live" when vm.is_alive()) so the device is hotplugged into the live
guest and persisted; update the virsh.attach_device invocation that references
vm_name and iface.xml accordingly.
In `@provider/bootc_image_builder/bootc_image_build_utils.py`:
- Line 67: The helper currently mutates os.environ to change Podman logging,
which creates a process-global side effect; instead, stop writing to os.environ
and make the log-level change local by creating a copy of the environment (e.g.,
local_env = os.environ.copy()), set the desired log variable on that copy, and
pass local_env as the env= parameter to the subprocess invocation that runs cmd
(the variable named "cmd") (or simply rely on the existing "--log-level=error"
flag in cmd and remove the os.environ mutation entirely); remove any direct
assignment to os.environ in this helper.
In `@v2v/tests/cfg/specific_kvm.cfg`:
- Around line 330-341: The os_ver_13 variant under the debian_efi test is
missing msg_content and expect_msg entries that os_ver_12 defines; update the
os_ver_13 variant (the block containing main_vm =
VM_NAME_DEBIAN_EFI_OS_VERSION_13_V2V_EXAMPLE) to include msg_content =
'cache=none' and expect_msg = 'no' so both variants assert the same behavior.
In `@v2v/tests/src/function_test_esx.py`:
- Around line 652-655: The truncation of params["main_vm"] inside the
'char_slash' checkpoint is dead because the test sets skip_vm_check = yes and
the function returns before any virsh operations use the modified name; either
remove the replacement/truncation block (the two lines modifying
params["main_vm"]) or instead update the test configuration to clear
skip_vm_check for the 'char_slash' case so the truncated main_vm is actually
exercised; locate the 'char_slash' checkpoint in function_test_esx.py (and the
corresponding entry in function_test_esx.cfg) and choose one of the two fixes:
delete the params["main_vm"] modification if it is unnecessary, or remove/modify
skip_vm_check in the 'char_slash' cfg so the later VM checks use the truncated
name.
In `@virttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfg`:
- Line 54: The Fedora selector rename to fedora_legacy wasn’t fully applied:
update the filter named anaconda-iso..upstream_bib..fedora_40 to use the new
selector name (fedora_legacy) so that the case still gets kickstart = "yes" and
use_toml_config = "yes"; search for the anaconda-iso..upstream_bib..fedora_40
filter and replace the old variant name with fedora_legacy to restore the
intended behavior.
In `@virttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfg`:
- Line 51: The AMI override selector still targets the old variant name so
fedora_legacy cases fall through; update the selector that currently reads
"upstream_bib..fedora.fedora_40" (the AMI override entry) to reference the
renamed variant "fedora_legacy" so the override applies to the fedora_legacy
variant instead of the default AWS config.
---
Outside diff comments:
In `@libvirt/tests/src/virtual_disks/virtual_disks_https.py`:
- Around line 139-159: The finally block can raise UnboundLocalError if
setup_test() fails because qemu_config (and possibly disk_dev) are not defined;
to fix, initialize qemu_config (and any other variables used in finally like
disk_dev) to None before the try, and in the finally guard calls with checks
(e.g., if qemu_config is not None: qemu_config.restore(); if backup_vmxml is not
None: backup_vmxml.sync(); and only call Libvirtd('virtqemud').restart() if
appropriate), so replace direct restores with conditional calls around
qemu_config.restore() and backup_vmxml.sync() while keeping the same cleanup
intent.
---
Duplicate comments:
In
`@libvirt/tests/src/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.py`:
- Around line 41-50: The current setup chains commands (multi_cmd1/multi_cmd2)
which masks failures and races the socket relabeling; instead split into
distinct steps: run the mkdir (cmd1) via process.run/remote.run_remote_cmd,
start virtiofsd (cmd2) without chaining and then poll/wait for the socket file
to appear (check -S {socket_path} or loop with timeout) before proceeding, and
only after the socket exists run the relabel/chown commands (cmd3/cmd4)
individually via process.run/remote.run_remote_cmd so each failure is visible
and ordering is guaranteed; also avoid relying on immediate success of nohup/&
(do not use ignore_bg_processes to hide background startup errors).
- Around line 105-109: The current teardown calls
(migration_obj.cleanup_connection(), remote.run_remote_cmd('pkill virtiofsd',
...), remote.run_remote_cmd(f"rm -rf {socket_path}", ...), process.run("pkill
virtiofsd", ...), process.run(f"rm -rf {socket_path}", ...)) are executed
sequentially so an exception in migration_obj.cleanup_connection()
short-circuits remaining cleanup; change to collect cleanup actions as a list of
lambdas/closures when each action is performed (e.g., append a lambda that calls
migration_obj.cleanup_connection(), one for remote.run_remote_cmd('pkill
virtiofsd', params, ignore_status=True), one for remote.run_remote_cmd(f"rm -rf
{socket_path}", ...), and local process.run calls), then in a finally block
iterate over that list and invoke each action inside its own try/except to log
but not raise on error so all pkill and socket removals always run even if one
cleanup fails.
In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py`:
- Around line 159-163: Initialize the cleanup handles before entering the try
block to avoid UnboundLocalError in the finally: explicitly set vm_sess = None
and host_nserver = None (or other appropriate sentinel values) prior to the code
that may raise, then in the finally guard the cleanup calls by checking those
variables (e.g., if host_nserver is not None: process.run(...); if vm_sess is
not None: vm_sess.close()). This ensures the finally block in
check_actual_network_throughput.py safely skips cleanup when
setup/boot/compilation fails.
- Line 72: The test captures the original firewall state in fw_was_running (via
firewalld.status()) but the cleanup logic currently starts the firewalld service
unconditionally; change the teardown to restore the original state instead: call
firewalld.start() only if fw_was_running is True and call firewalld.stop() only
if fw_was_running is False (or otherwise ensure the opposite action is taken
only when the original state differs). Update the cleanup in
check_actual_network_throughput.py (and the similar block around lines 158-169)
to reference fw_was_running rather than always starting the service.
In `@provider/virtual_network/netperf_base.py`:
- Around line 166-172: The VM branch ignores a configured client_path and always
sets install_path to guest_netperf_path; update the branch that handles address
in params.get('vms', '').split() so it prefers params.get("client_path")
(falling back to guest_netperf_path) when assigning install_path for VM builds
(the code around guest_netperf_path, the elif branch using env.get_vm(address)
and session, install_path = vm.wait_for_login(), guest_netperf_path). Ensure
install_path is computed from params.get("client_path", guest_netperf_path) so
subsequent code finds netperf in the expected client location.
In `@provider/virtual_network/network_base.py`:
- Around line 757-791: The helpers cancel_if_ovs_bridge and
setup_ovs_bridge_attrs should use a caller-resolved bridge name instead of
always re-reading params['netdst']; change their signatures to accept an
optional resolved_bridge (e.g., def cancel_if_ovs_bridge(params, test,
resolved_bridge=None) and def setup_ovs_bridge_attrs(params, iface_attrs,
resolved_bridge=None)) and then use resolved_bridge if provided (falling back to
params.get('netdst', '') only if not), update logic to call
utils_net.find_bridge_manager(resolved_bridge) and adjust the TestError message
accordingly, and update all callers to pass the previously-resolved bridge_name
(or leave omitted when they rely on params).
In `@v2v/tests/src/v2v_options.py`:
- Around line 960-975: The code builds a string command (cmd) by inserting the
vpx_ipv6_addr and then calls process.run(..., shell=True) when checkpoint ==
'vpx_with_invalid_esx_ipv6', which risks shell injection and is flagged; instead
construct a safe argument list and pass an environment dict with LIBVIRT_DEBUG=1
to process.run so you can remove shell=True: keep using the same checkpoint
check and vpx_ipv6_addr lookup but replace string concatenation of cmd (and the
use of cmd_export_env) with a list/sequence of arguments for process.run (or
tokenize the existing cmd into args safely) and pass env={**os.environ,
"LIBVIRT_DEBUG":"1"} (or equivalent in your test harness) to process.run while
calling it without shell=True and still using timeout=v2v_timeout, verbose=True,
ignore_status=True.
---
Nitpick comments:
In `@libvirt/tests/cfg/virtual_network/link_state/link_state_model_type.cfg`:
- Around line 25-31: The line with kill_vm_libvirt_options under the ovmf: block
has extra leading spaces causing inconsistent indentation; open the cfg and
align kill_vm_libvirt_options to use the standard 4-space indent under the ovmf
key so it matches other entries (i.e., make the indentation for
kill_vm_libvirt_options consistent with the surrounding
create_vm_libvirt/use_no_reboot entries).
In
`@libvirt/tests/src/virtual_network/connectivity/connectivity_check_network_interface.py`:
- Around line 69-74: The loop currently catches a broad Exception for calls to
utils_net.restart_guest_network (for vm_session and ep_session); change this to
catch more specific exceptions raised by that function (e.g., RuntimeError,
OSError, subprocess.CalledProcessError or a custom network exception if one
exists) or catch a tuple of those specific types instead of Exception, and keep
the LOG.warning message for expected DHCPv6 failures; if you cannot identify a
single specific exception, still add a debug-level log of the full traceback
(use traceback.format_exc() via LOG.debug) alongside the warning to preserve
diagnostic info while avoiding masking unexpected errors.
In `@libvirt/tests/src/virtual_network/iface_update.py`:
- Around line 273-279: The libvirtd restart workaround is currently applied for
every live-update path; restrict it to only run when the update involves a
link-state change by adding the new_iface_link condition to the existing check
so the block runs only when not cold_update, vm.is_alive(), not
libvirtd_restarted, and new_iface_link is true; keep the existing instantiation
(libvirtd = utils_libvirtd.Libvirtd()) and restart() call and set
libvirtd_restarted = True as before.
In `@libvirt/tests/src/virtual_network/link_state/link_state_model_type.py`:
- Around line 64-67: The comment is misleading: replace "Configure ovs bridge
when host using ovs bridge" with a clearer note that this block applies
OVS-specific attribute adjustments for the network interface test variant
(triggered by params.get("network_test") == "yes") because
network_base.setup_ovs_bridge_attrs(params, iface_attrs) will itself detect OVS
usage; update the inline comment near the params check and the call to
network_base.setup_ovs_bridge_attrs to indicate it's applying OVS-related
attributes for the network test case rather than unconditionally configuring an
OVS bridge.
In `@v2v/tests/cfg/convert_from_file.cfg`:
- Around line 267-273: Add a version_required entry to the character_slash
variant to match the corresponding char_slash variant in function_test_esx.cfg;
specifically add version_required = "[virt-v2v-2.10.0-5,)" under the
character_slash block so the test declares the same minimum virt-v2v version
dependency and stays consistent with the char_slash variant.
In `@virttools/tests/src/bootc_image_builder/bootc_disk_image_build.py`:
- Around line 84-85: Extract the literal list of allowed bib_ref values into a
shared constant (e.g., STAGED_BIB_REFS) in
provider.bootc_image_builder.bootc_image_build_utils, export it, and replace the
inline membership checks that reference bib_ref in functions like the one using
"if bib_ref in [...]" in bootc_disk_image_build.py and the corresponding check
in bootc_disk_image_install.py to use "if bib_ref in STAGED_BIB_REFS" (ensure
you import STAGED_BIB_REFS from bootc_image_build_utils); update any tests or
callers to import the constant so both build and install paths use the same
centralized allowlist.
🪄 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: a010dec3-3510-40e0-962e-83ee2b10cbd9
📒 Files selected for processing (70)
libvirt/tests/cfg/cpu/multi_vms_with_stress.cfglibvirt/tests/cfg/libvirt_mem.cfglibvirt/tests/cfg/memory/memory_discard.cfglibvirt/tests/cfg/memory/memory_misc.cfglibvirt/tests/cfg/memory/nvdimm.cfglibvirt/tests/cfg/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.cfglibvirt/tests/cfg/resource_abnormal.cfglibvirt/tests/cfg/virtual_disks/virtual_disks_https.cfglibvirt/tests/cfg/virtual_disks/virtual_disks_product.cfglibvirt/tests/cfg/virtual_network/attach_detach_device/attach_ethernet_interface.cfglibvirt/tests/cfg/virtual_network/connectivity/connectivity_check_bridge_interface.cfglibvirt/tests/cfg/virtual_network/connectivity/connectivity_check_ethernet_interface.cfglibvirt/tests/cfg/virtual_network/connectivity/connectivity_check_network_interface.cfglibvirt/tests/cfg/virtual_network/driver/check_vhost_cpu_affinity_with_emulatorpin.cfglibvirt/tests/cfg/virtual_network/driver/rx_tx_queue_size.cfglibvirt/tests/cfg/virtual_network/fuzzy_test/virConnectListInterface_test.cfglibvirt/tests/cfg/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.cfglibvirt/tests/cfg/virtual_network/iface_bridge.cfglibvirt/tests/cfg/virtual_network/iface_options.cfglibvirt/tests/cfg/virtual_network/iface_update.cfglibvirt/tests/cfg/virtual_network/lifecycle/lifecycle_full_xml_interface.cfglibvirt/tests/cfg/virtual_network/link_state/link_state_model_type.cfglibvirt/tests/cfg/virtual_network/migrate/migrate_with_bridge_type_interface.cfglibvirt/tests/cfg/virtual_network/migrate/migrate_with_ethernet_interface.cfglibvirt/tests/cfg/virtual_network/mtu.cfglibvirt/tests/cfg/virtual_network/passt/passt_function.cfglibvirt/tests/cfg/virtual_network/qos/check_actual_network_throughput.cfglibvirt/tests/cfg/virtual_network/qos/check_bandwidth_by_domiftune.cfglibvirt/tests/cfg/virtual_network/qos/check_qos_floor.cfglibvirt/tests/cfg/virtual_network/qos/test_bandwidth_boundry.cfglibvirt/tests/cfg/virtual_network/resolve_vm_hostname_by_resolvectl.cfglibvirt/tests/src/guest_os_booting/ovmf_firmware/ovmf_loader.pylibvirt/tests/src/memory/memory_devices/dimm_memory_lifecycle.pylibvirt/tests/src/memory/memory_devices/virtio_mem_device_lifecycle.pylibvirt/tests/src/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.pylibvirt/tests/src/migration/migration_uri/tcp_common.pylibvirt/tests/src/virtual_disks/virtual_disks_https.pylibvirt/tests/src/virtual_network/attach_detach_device/attach_ethernet_interface.pylibvirt/tests/src/virtual_network/connectivity/connectivity_check_bridge_interface.pylibvirt/tests/src/virtual_network/connectivity/connectivity_check_bridge_interface_unprivileged.pylibvirt/tests/src/virtual_network/connectivity/connectivity_check_ethernet_interface.pylibvirt/tests/src/virtual_network/connectivity/connectivity_check_network_interface.pylibvirt/tests/src/virtual_network/driver/rx_tx_queue_size.pylibvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.pylibvirt/tests/src/virtual_network/hotplug/attach_detach_interface/attach_interface_with_options_and_vm_status.pylibvirt/tests/src/virtual_network/iface_bridge.pylibvirt/tests/src/virtual_network/iface_options.pylibvirt/tests/src/virtual_network/iface_update.pylibvirt/tests/src/virtual_network/link_state/link_state_model_type.pylibvirt/tests/src/virtual_network/migrate/migrate_with_bridge_type_interface.pylibvirt/tests/src/virtual_network/migrate/migrate_with_ethernet_interface.pylibvirt/tests/src/virtual_network/mtu.pylibvirt/tests/src/virtual_network/qos/check_actual_network_throughput.pylibvirt/tests/src/virtual_network/qos/check_bandwidth_by_domiftune.pyprovider/bootc_image_builder/bootc_image_build_utils.pyprovider/virtual_network/netperf_base.pyprovider/virtual_network/network_base.pyv2v/tests/cfg/convert_from_file.cfgv2v/tests/cfg/function_test_esx.cfgv2v/tests/cfg/nbdkit/nbdkit.cfgv2v/tests/cfg/specific_kvm.cfgv2v/tests/cfg/v2v_options.cfgv2v/tests/src/convert_from_file.pyv2v/tests/src/function_test_esx.pyv2v/tests/src/nbdkit/nbdkit.pyv2v/tests/src/v2v_options.pyvirttools/tests/cfg/bootc_image_builder/bootc_disk_image_build.cfgvirttools/tests/cfg/bootc_image_builder/bootc_disk_image_install.cfgvirttools/tests/src/bootc_image_builder/bootc_disk_image_build.pyvirttools/tests/src/bootc_image_builder/bootc_disk_image_install.py
💤 Files with no reviewable changes (1)
- libvirt/tests/src/virtual_network/connectivity/connectivity_check_bridge_interface_unprivileged.py
✅ Files skipped from review due to trivial changes (8)
- libvirt/tests/cfg/virtual_disks/virtual_disks_product.cfg
- libvirt/tests/cfg/virtual_network/driver/check_vhost_cpu_affinity_with_emulatorpin.cfg
- libvirt/tests/cfg/cpu/multi_vms_with_stress.cfg
- libvirt/tests/cfg/virtual_network/fuzzy_test/virConnectListInterface_test.cfg
- libvirt/tests/cfg/virtual_network/migrate/migrate_with_ethernet_interface.cfg
- libvirt/tests/src/memory/memory_devices/dimm_memory_lifecycle.py
- libvirt/tests/cfg/memory/memory_discard.cfg
- libvirt/tests/cfg/memory/nvdimm.cfg
🚧 Files skipped from review as they are similar to previous changes (9)
- libvirt/tests/src/memory/memory_devices/virtio_mem_device_lifecycle.py
- libvirt/tests/cfg/migration/destructive_operations_around_live_migration/kill_virtiofsd_during_performphase.cfg
- libvirt/tests/cfg/libvirt_mem.cfg
- libvirt/tests/src/virtual_network/elements_and_attributes/driver_packed.py
- libvirt/tests/cfg/virtual_network/qos/test_bandwidth_boundry.cfg
- libvirt/tests/cfg/virtual_network/qos/check_actual_network_throughput.cfg
- v2v/tests/cfg/v2v_options.cfg
- libvirt/tests/cfg/virtual_network/qos/check_qos_floor.cfg
- libvirt/tests/cfg/virtual_network/qos/check_bandwidth_by_domiftune.cfg
yanglei-rh
force-pushed
the
LIBVIRTAT-22417
branch
from
March 31, 2026 06:49
6b70344 to
f9b9822
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py (2)
72-73:⚠️ Potential issue | 🟠 MajorRestore
firewalldonly if this test changed it.If the service was already stopped before the case, Line 170 turns it back on and leaves the host in a different state for later tests.
Possible fix
- firewalld.stop() + if fw_was_running: + firewalld.stop() disable_firewall = params.get("disable_firewall", "systemctl stop firewalld.service") vm_sess.cmd(disable_firewall) @@ - firewalld.start() + if fw_was_running: + firewalld.start()Also applies to: 123-125, 170-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py` around lines 72 - 73, Record the initial firewalld state in fw_was_running (via service.Factory.create_service("firewalld") and firewalld.status()) and when restoring later (where the test currently unconditionally starts/stops firewalld around line 170 and also at lines 123-125), only change the service if its current state differs from fw_was_running; i.e., check fw_was_running before calling start()/stop() so you restore the service to its original state only if the test actually changed it.
72-73:⚠️ Potential issue | 🟠 MajorInitialize
vm_sessandhost_nserverbefore thetry.A failure before Line 94 or Line 133 leaves these names undefined, and this
finallythen raisesUnboundLocalError, masking the real failure.Possible fix
net_xml = NetworkXML.new_from_net_dumpxml('default') bk_net_xml = net_xml.copy() firewalld = service.Factory.create_service("firewalld") fw_was_running = firewalld.status() + vm_sess = None + host_nserver = None try: @@ - if host_nserver: + if host_nserver: process.run(f"rm -rf {host_nserver.split('/src/')[0]}", shell=True) - if vm_sess: + if vm_sess: vm_sess.close()Also applies to: 159-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py` around lines 72 - 73, The variables vm_sess and host_nserver are sometimes left undefined if an exception occurs before they are assigned; initialize them to None before entering the try block (e.g., vm_sess = None; host_nserver = None) and update the finally block to check "if vm_sess is not None" and "if host_nserver is not None" before using/closing them; apply the same initialization/check pattern for the similar scope around the code referenced at 159-164 so UnboundLocalError is avoided and the original exception is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py`:
- Around line 132-134: The test calls netperf_base.compile_netperf_pkg but that
symbol is not exported from provider/virtual_network/netperf_base.py; fix this
by either adding a compile_netperf_pkg(params, env, target) wrapper in
netperf_base that encapsulates the existing compile/prepare functions and
returns (nserver, nperf), or modify the test to call the existing API provided
by netperf_base (e.g., the existing compile_netperf or prepare_netperf
functions) and adapt the returned values to match how host_nserver, host_nperf,
vm_nserver, vm_nperf are used; reference the netperf_base module and the
compile_netperf_pkg call sites in check_actual_network_throughput.py when making
the change.
---
Duplicate comments:
In `@libvirt/tests/src/virtual_network/qos/check_actual_network_throughput.py`:
- Around line 72-73: Record the initial firewalld state in fw_was_running (via
service.Factory.create_service("firewalld") and firewalld.status()) and when
restoring later (where the test currently unconditionally starts/stops firewalld
around line 170 and also at lines 123-125), only change the service if its
current state differs from fw_was_running; i.e., check fw_was_running before
calling start()/stop() so you restore the service to its original state only if
the test actually changed it.
- Around line 72-73: The variables vm_sess and host_nserver are sometimes left
undefined if an exception occurs before they are assigned; initialize them to
None before entering the try block (e.g., vm_sess = None; host_nserver = None)
and update the finally block to check "if vm_sess is not None" and "if
host_nserver is not None" before using/closing them; apply the same
initialization/check pattern for the similar scope around the code referenced at
159-164 so UnboundLocalError is avoided and the original exception is preserved.
🪄 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: bbe8e49f-a8fe-4abe-b633-85e0a1df25f4
📒 Files selected for processing (6)
libvirt/tests/cfg/virtual_network/qos/check_actual_network_throughput.cfglibvirt/tests/cfg/virtual_network/qos/check_bandwidth_by_domiftune.cfglibvirt/tests/cfg/virtual_network/qos/check_qos_floor.cfglibvirt/tests/cfg/virtual_network/qos/test_bandwidth_boundry.cfglibvirt/tests/src/virtual_network/qos/check_actual_network_throughput.pylibvirt/tests/src/virtual_network/qos/check_bandwidth_by_domiftune.py
🚧 Files skipped from review as they are similar to previous changes (3)
- libvirt/tests/cfg/virtual_network/qos/test_bandwidth_boundry.cfg
- libvirt/tests/cfg/virtual_network/qos/check_qos_floor.cfg
- libvirt/tests/cfg/virtual_network/qos/check_actual_network_throughput.cfg
Contributor
Author
|
This patch depends on #6845 |
Contributor
Author
|
Test pass |
Contributor
Author
|
Note: if you would like to use avocado runner to run these case, it should be like this command: |
1.Add some parameters to allow the framework provide guest xml 2.Add "only Linux" to limit guest type 3.Add a check about host bridge type for check_actual_network_throughput and check_bandwidth_by_domiftune 4.Change the method for specifying different bridge types via the test command intead of case cfg varaint for check_actual_network_throughput Signed-off-by: Lei Yang <leiyang@redhat.com>
yanglei-rh
force-pushed
the
LIBVIRTAT-22417
branch
from
April 8, 2026 02:51
f9b9822 to
1b2bacf
Compare
|
Closing this PR due to current team constraints. This is part of a broader effort to triage all in-flight work across our upstream repos. If this work is still needed, please feel free to reopen and it will be picked up. Apologies for any inconvenience. |
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.
1.Add some parameters to allow the framework provide guest xml
2.Add "only Linux" to limit guest type
3.Add a check about host bridge type for check_actual_network_throughput and check_bandwidth_by_domiftune
4.Change the method for specifying different bridge types via the test command intead of case cfg varaint for check_actual_network_throughput
ID: LIBVIRTAT-22417
Assisted-by: Gemini AI
Signed-off-by: Lei Yang leiyang@redhat.com
Summary by CodeRabbit