passt_function: Add qemu warning check for vhostuser #6691
Conversation
It covers the test for https://issues.redhat.com/browse/RHEL-80533 Signed-off-by: Han Han <hhan@redhat.com>
WalkthroughThis pull request adds QEMU log file monitoring capability to passt network function tests. Configuration changes add explicit qemu_log_file paths for both root and non-root user variants. The test code introduces a new optional qemu_log_file parameter and extends the existing warning check logic to additionally inspect the QEMU log file for warning entries when vhostuser and warning_check are enabled. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
| user_id = 107 | ||
| log_dir = /run/user/${user_id} | ||
| socket_dir = f'/run/libvirt/qemu/passt/' | ||
| qemu_log_file = /var/log/libvirt/qemu/avocado-vt-vm1.log |
There was a problem hiding this comment.
Hi, @nanli1 , is there any way to replace the hard-coding guest name with a variable?
There was a problem hiding this comment.
@qiankehan hi hhan, we could use main_vm parameter
qemu_log_file = /var/log/libvirt/qemu/${main_vm}.log
we could also use libvirt.check_logfile("fail|error", qemu_log_file, str_in_log=False) to check
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
libvirt/tests/cfg/virtual_network/passt/passt_function.cfg (1)
17-24: Avoid hard‑coding the QEMU log filename; derive it from the VM name insteadBoth
qemu_log_fileentries hard‑codeavocado-vt-vm1.log. Ifmain_vm/unpr_vm_nameis ever changed or differs between root and unprivileged runs, the Python side will grep the wrong log file and silently miss QEMU warnings.Consider instead passing either:
- a directory (e.g.
/var/log/libvirt/qemuand/home/${test_user}/.cache/libvirt/qemu) and let the test build<vm_name>.log, or- a single template parameter (e.g.
qemu_log_dirorqemu_log_file_pattern) used by the Python code to compute the exact path from the actualvm_name.This reduces duplication and keeps log paths aligned with VM naming.
libvirt/tests/src/virtual_network/passt/passt_function.py (1)
76-76: Validateqemu_log_filewhen vhostuser and warning_check are enabled
qemu_log_fileis taken directly from params with no validation. If another cfg reuses this test withvhostuser=yesandwarning_check=yesbut forgets to setqemu_log_file, the later grep will run against"None"/ a non‑existent file and the test will silently skip QEMU warning coverage.To make misconfiguration obvious, consider guarding this early, for example:
warning_check = params.get('warning_check') qemu_log_file = params.get('qemu_log_file') if vhostuser and warning_check and not qemu_log_file: test.cancel('qemu_log_file must be set when vhostuser and warning_check are enabled')This keeps current behavior for correctly configured jobs while failing fast on incomplete configs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/passt/passt_function.cfg(1 hunks)libvirt/tests/src/virtual_network/passt/passt_function.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/passt/passt_function.cfglibvirt/tests/src/virtual_network/passt/passt_function.py
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/passt/passt_function.py (2)
libvirt/tests/src/virtual_network/passt/passt_reconnect.py (1)
run(28-148)libvirt/tests/src/virtual_network/passt/passt_transfer_file.py (1)
run(116-197)
🪛 Ruff (0.14.6)
libvirt/tests/src/virtual_network/passt/passt_function.py
143-143: Function call with shell=True parameter identified, security issue
(S604)
150-150: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
| if vhostuser: | ||
| if warning_check: | ||
| # Exclude the warning like: 'warning : qemuDomainObjTaintMsg:5529 : Domain ... is tainted: custom-monitor' | ||
| check_warning = process.run('grep -v "qemuDomainObjTaintMsg.*custom-monitor {0}"|grep "warning :"'.format(libvirtd_debug_file), | ||
| shell=True, | ||
| ignore_status=True, | ||
| logger=LOG) | ||
| if check_warning.exit_status == 0: | ||
| test.fail('Get warnings in virtqemud log: {0}'.format(check_warning.stdout_text)) | ||
|
|
||
| check_qemu_warning = process.run('grep warning: {0}'.format(qemu_log_file), | ||
| shell=True, | ||
| ignore_status=True, | ||
| logger=LOG) | ||
| if not check_qemu_warning.exit_status: | ||
| test.fail('Get warnings in qemu log {0}: {1}'.format(qemu_log_file, check_qemu_warning.stdout_text)) |
There was a problem hiding this comment.
Clear QEMU log before test run to avoid false positives and assert log presence
The new QEMU log warning check is useful, but there are a couple of reliability issues:
-
Stale warnings across runs (flaky failures)
grep warning: {qemu_log_file}runs on whatever is currently in the log file. Unlikelibvirtd_debug_file, this QEMU log is never removed or truncated before starting the VM in this test. If previous runs left warnings in the same log, this test will fail even when the current run is clean.Suggest explicitly clearing the QEMU log before
vm.start()whenvhostuserandwarning_checkare enabled, e.g.:if vhostuser and warning_check and qemu_log_file: if os.path.exists(qemu_log_file): os.remove(qemu_log_file)
This mirrors the “fresh log per run” behavior you already enforce for
libvirtd_debug_fileand avoids history‑dependent failures. -
No assertion that the QEMU log actually exists
Ifqemu_log_filepoints to the wrong place or QEMU logging is disabled,grepwill return a non‑zero status and the currentif not check_qemu_warning.exit_status:condition will quietly treat that as “no warnings”. That means the test can give a false sense of safety while never reading any QEMU log at all.After the VM run and before grepping, consider asserting that the log file exists when
warning_checkis on, for example:if vhostuser and warning_check: if not os.path.exists(qemu_log_file): test.fail(f'QEMU log file "{qemu_log_file}" not found for warning check')
These changes would make the new warning coverage both more robust and less flaky, without altering the intended pass/fail semantics when the log is correctly produced.
🧰 Tools
🪛 Ruff (0.14.6)
143-143: Function call with shell=True parameter identified, security issue
(S604)
150-150: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
In libvirt/tests/src/virtual_network/passt/passt_function.py around lines
140-155, the QEMU log grep can use stale data and may silently pass when the
QEMU log doesn't exist; before starting the VM when vhostuser and warning_check
are enabled, ensure the qemu_log_file is cleared (remove or truncate if it
exists) so each run has a fresh log, and after the VM run but before grepping
assert that qemu_log_file exists (fail the test if it does not) so the grep
actually checks current QEMU output rather than old data or a missing file.
2 participants
It covers the test for https://issues.redhat.com/browse/RHEL-80533
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.