Handling kqemu testcase as libvirt dropped it 2.19.2 onwards! #6749
Conversation
|
Warning Rate limit exceeded@Anushree-Mathur has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughA pre-check was added to the virsh_maxvcpus test that skips the test when libvirt version is 2.19.2 or newer and the virsh_maxvcpus option contains "kqemu". The new pre-check is placed before the option variable is initialized, which can raise a NameError at runtime. No other changes to control flow, error handling, or exported entities are present. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![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
Fix all issues with AI Agents 🤖
In @libvirt/tests/src/virsh_cmd/host/virsh_maxvcpus.py:
- Around line 118-121: Fix the indentation of the nested if so the line with
test.cancel uses 4-space indent (align under the inner if), and move the entire
libvirt_version.version_compare / "kqemu" check to immediately after parameter
retrieval (i.e., right after option = params.get("virsh_maxvcpus_options")) to
avoid expensive setup; update the test.cancel message to clearly reference the
libvirt version constraint only (e.g., mention libvirt 2.19.2) and ensure you
use libvirt_version.version_compare and option/test.cancel as the referenced
symbols.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/virsh_cmd/host/virsh_maxvcpus.py
🧰 Additional context used
🧠 Learnings (1)
📓 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.
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
⏰ 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.9
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
| # Checking libvirt version before running maxvcpus with kqemu | ||
| if libvirt_version.version_compare(2, 19, 2): | ||
| if "kqemu" in option: | ||
| test.cancel("kqemu is not supported in qemu 0.12 and libvirt 2.19.2 onwards") |
There was a problem hiding this comment.
Fix indentation error on line 121.
Line 121 has incorrect indentation with 8 spaces instead of the expected 4 spaces after the if statement on line 120. This violates Python's PEP 8 style guidelines and will be flagged by linters.
🔎 Proposed fix for indentation
# Checking libvirt version before running maxvcpus with kqemu
if libvirt_version.version_compare(2, 19, 2):
if "kqemu" in option:
- test.cancel("kqemu is not supported in qemu 0.12 and libvirt 2.19.2 onwards")
+ test.cancel("kqemu is not supported in qemu 0.12 and libvirt 2.19.2 onwards")Consider moving this check earlier to avoid unnecessary setup.
The kqemu version check is currently placed after the expensive domcapabilities XML parsing (lines 79-110). Moving this check to right after the parameter retrieval (around line 32) would skip unnecessary work when the test will be canceled anyway.
🔎 Suggested placement
Move the check to after line 32, right after retrieving the option parameter:
# get the params from subtests.
# params for general.
option = params.get("virsh_maxvcpus_options")
status_error = params.get("status_error")
connect_arg = params.get("connect_arg", "")
# Checking libvirt version before running maxvcpus with kqemu
if libvirt_version.version_compare(2, 19, 2):
if "kqemu" in option:
test.cancel("kqemu is not supported in qemu 0.12 and libvirt 2.19.2 onwards")Consider clarifying the error message.
The message "kqemu is not supported in qemu 0.12 and libvirt 2.19.2 onwards" mentions both qemu 0.12 and libvirt 2.19.2, which could be confusing. Consider simplifying to focus on the libvirt version constraint that's being checked.
🔎 Suggested message improvement
- test.cancel("kqemu is not supported in qemu 0.12 and libvirt 2.19.2 onwards")
+ test.cancel("kqemu is not supported in libvirt 2.19.2 and later versions")🤖 Prompt for AI Agents
In @libvirt/tests/src/virsh_cmd/host/virsh_maxvcpus.py around lines 118-121, Fix
the indentation of the nested if so the line with test.cancel uses 4-space
indent (align under the inner if), and move the entire
libvirt_version.version_compare / "kqemu" check to immediately after parameter
retrieval (i.e., right after option = params.get("virsh_maxvcpus_options")) to
avoid expensive setup; update the test.cancel message to clearly reference the
libvirt version constraint only (e.g., mention libvirt 2.19.2) and ensure you
use libvirt_version.version_compare and option/test.cancel as the referenced
symbols.
Anushree-Mathur
force-pushed
the
kqemu
branch
2 times, most recently
from
865ff9d to
a9b599f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @libvirt/tests/src/virsh_cmd/host/virsh_maxvcpus.py:
- Around line 28-35: The NameError arises because option is referenced before
assignment; move the libvirt version check that uses option (the call to
libvirt_version.version_compare(...) and the subsequent if "kqemu" in option
block that calls test.cancel(...)) to after you retrieve option from params via
option = params.get("virsh_maxvcpus_options"); also fix the cancel message text
to read "kqemu is not supported in libvirt 2.19.2 and qemu 0.12 onwards" so it’s
grammatically correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/virsh_cmd/host/virsh_maxvcpus.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: hholoubk
Repo: autotest/tp-libvirt PR: 6579
File: libvirt/tests/src/sriov/vIOMMU/iommu_device_lifecycle.py:95-97
Timestamp: 2025-09-24T08:01:27.899Z
Learning: In the libvirt test framework used in tp-libvirt, VM cleanup including destroying running VMs is handled by the teardown method (test_obj.teardown_iommu_test()) called in the finally block, so explicit VM destroy calls on timeout are not necessary according to the maintainers.
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.
🪛 Ruff (0.14.10)
libvirt/tests/src/virsh_cmd/host/virsh_maxvcpus.py
30-30: Undefined name option
(F821)
| # Checking libvirt version before running maxvcpus with kqemu | ||
| if libvirt_version.version_compare(2, 19, 2): | ||
| if "kqemu" in option: | ||
| test.cancel("kqemu is not supported libvirt 2.19.2 and qemu 0.12 onwards") | ||
|
|
||
| # get the params from subtests. | ||
| # params for general. | ||
| option = params.get("virsh_maxvcpus_options") |
There was a problem hiding this comment.
option is used before it's defined — causes NameError at runtime.
The variable option is referenced on line 30 before it's assigned on line 35. This will crash the test with NameError: name 'option' is not defined.
Move the version check to immediately after the parameter retrieval.
🔎 Proposed fix
def run(test, params, env):
"""
Test the command virsh maxvcpus
(1) Call virsh maxvcpus
(2) Call virsh -c remote_uri maxvcpus
(3) Call virsh maxvcpus with an unexpected option
"""
- # Checking libvirt version before running maxvcpus with kqemu
- if libvirt_version.version_compare(2, 19, 2):
- if "kqemu" in option:
- test.cancel("kqemu is not supported libvirt 2.19.2 and qemu 0.12 onwards")
-
# get the params from subtests.
# params for general.
option = params.get("virsh_maxvcpus_options")
status_error = params.get("status_error")
connect_arg = params.get("connect_arg", "")
+ # Checking libvirt version before running maxvcpus with kqemu
+ if libvirt_version.version_compare(2, 19, 2):
+ if "kqemu" in option:
+ test.cancel("kqemu is not supported in libvirt 2.19.2 onwards")
+
# params for transport connect.
local_ip = params.get("local_ip", "ENTER.YOUR.LOCAL.IP")Also note: the message has a minor grammar issue — "is not supported libvirt" should be "is not supported in libvirt".
🧰 Tools
🪛 Ruff (0.14.10)
30-30: Undefined name option
(F821)
🤖 Prompt for AI Agents
In @libvirt/tests/src/virsh_cmd/host/virsh_maxvcpus.py around lines 28-35, The
NameError arises because option is referenced before assignment; move the
libvirt version check that uses option (the call to
libvirt_version.version_compare(...) and the subsequent if "kqemu" in option
block that calls test.cancel(...)) to after you retrieve option from params via
option = params.get("virsh_maxvcpus_options"); also fix the cancel message text
to read "kqemu is not supported in libvirt 2.19.2 and qemu 0.12 onwards" so it’s
grammatically correct.
|
Before applying the patch : After applying the patch : |
Support for kqemu was dropped in libvirt by commit 8e91a400c, so it is failing as expected. I have added the condition to cancel this testcase if libvirt version is greater than 2.19.2! Signed-off-by: Anushree-Mathur <anushree.mathur@linux.ibm.com>
1 participant
Support for kqemu was dropped in libvirt by commit 8e91a400c, so it is failing as expected. I have added the condition to cancel this testcase if libvirt version is greater than 2.19.2!
Reference link to confirm dropping kqemu support : https://lists.libvirt.org/archives/list/devel%40lists.libvirt.org/thread/PXHESERQAWL2NFDGZWDYY4I4M567YXDX/?utm_source=chatgpt.com
Signed-off-by: Anushree-Mathur anushree.mathur@linux.ibm.com
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.