Conversation
WalkthroughA new negative test case for libvirt is introduced to validate VM creation failures when given invalid CPU topology configurations. This includes a Python test script that manipulates VM XML files and a configuration file specifying test variants with expected error messages for different incorrect CPU layouts. Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @LuyaoHuang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the test suite by adding new automated tests to validate libvirt's robust error handling for virtual machine CPU topology configurations. It introduces specific scenarios where intentionally incorrect CPU parameters are provided during VM definition, ensuring that the system correctly identifies and rejects these invalid setups with informative error messages, thereby preventing the creation of unsupportable virtual machines.
Highlights
- New Test Cases for CPU Topology Validation: I've introduced two new test cases (
one_clusterandmulti_clusters) undercpu.wrong_topologyto specifically validatelibvirt's behavior when defining VMs with invalid CPU topology configurations. - Error Handling Verification: These tests are designed to assert that
libvirtcorrectly rejects VM definitions with malformed CPU topologies, checking for specific error messages like "CPU topology doesn't match maximum vcpu count" or "clusters > 1 not supported by this machine's CPU topology". - Automated VM Definition and Validation: I've implemented the core test logic in
libvirt/tests/src/cpu/wrong_topology.pywhich dynamically constructs VM XML with specified CPU attributes andvcpucounts, attempts to define the VM usingvirsh.create, and then verifies the expected error status and message.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
| bkxml = vmxml.copy() | ||
| tmp_vm_name = 'tmp-vm-' + utils_misc.generate_random_string(3) | ||
| vcpu = int(params.get('vcpu')) | ||
| cpu_attrs = eval(params.get('cpu_attrs', {})) |
There was a problem hiding this comment.
Using eval can be dangerous if the input string comes from an untrusted source. It's generally better to use json.loads or ast.literal_eval for safer evaluation of data structures.1
import ast
# ...
cpu_attrs = ast.literal_eval(params.get('cpu_attrs', '{}'))Style Guide References
Footnotes
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
libvirt/tests/src/cpu/wrong_topology.py (2)
24-24: Fix the Yoda condition for better readability.The condition should be written in natural order for better readability.
- status_error = 'yes' == params.get('status_error', 'no') + status_error = params.get('status_error', 'no') == 'yes'
36-36: Rename unused variable to indicate it's intentionally unused.The variable
vis not used within the loop body and should be renamed to indicate this is intentional.- for k, v in list(os_attrs.items()): + for k, _v in list(os_attrs.items()):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/cpu/wrong_topology.py(1 hunks)libvirt/tests/src/cpu/wrong_topology.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
libvirt/tests/cfg/cpu/wrong_topology.py (1)
libvirt/tests/src/virsh_cmd/domain/virsh_attach_device.py (2)
start_vm(51-53)status_error(81-83)
🪛 Ruff (0.11.9)
libvirt/tests/src/cpu/wrong_topology.py
24-24: Yoda condition detected
Rewrite as params.get('status_error', 'no') == 'yes'
(SIM300)
36-36: Loop control variable v not used within loop body
Rename unused v to _v
(B007)
libvirt/tests/cfg/cpu/wrong_topology.py
1-1: SyntaxError: Invalid annotated assignment target
1-2: SyntaxError: Expected an expression
2-2: SyntaxError: Unexpected indentation
5-6: SyntaxError: Expected an expression
6-6: SyntaxError: Unexpected indentation
6-6: SyntaxError: Invalid annotated assignment target
6-7: SyntaxError: Expected an expression
7-7: SyntaxError: Unexpected indentation
9-9: SyntaxError: Simple statements must be separated by newlines or semicolons
9-9: SyntaxError: Simple statements must be separated by newlines or semicolons
9-9: SyntaxError: missing closing quote in string literal
9-10: SyntaxError: Expected a statement
10-10: SyntaxError: Expected a statement
10-11: SyntaxError: Expected an expression
11-11: SyntaxError: Unexpected indentation
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: missing closing quote in string literal
⏰ 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.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (6)
libvirt/tests/cfg/cpu/wrong_topology.py (3)
6-9: Verify the CPU topology calculation logic.For the
one_clustervariant:
- Configured vCPUs: 15
- Topology calculation: 2 sockets × 2 dies × 1 cluster × 2 cores × 2 threads = 16 vCPUs
- This mismatch (15 vs 16) should correctly trigger the expected error
The logic appears correct for testing the topology validation.
10-13: Verify the multi-cluster test logic.For the
multi_clustersvariant:
- Configured vCPUs: 32
- Topology calculation: 2 sockets × 2 dies × 2 clusters × 2 cores × 2 threads = 32 vCPUs
- The vCPU count matches, but the test expects an error about multiple clusters not being supported
This correctly tests the multiple cluster validation logic.
1-14: Ignore static analysis errors for configuration files.The static analysis errors are false positives since this appears to be a YAML-like configuration file for the libvirt test framework, not Python code. The syntax is appropriate for the test framework's configuration format.
libvirt/tests/src/cpu/wrong_topology.py (3)
19-21: Good use of backup and temporary VM naming.The code properly creates a backup of the original VM XML and generates a random temporary VM name to avoid conflicts. This is a good practice for test isolation.
34-42: Good handling of NVRAM cleanup.The code properly removes NVRAM-related OS attributes which could cause issues when creating a temporary VM with the same configuration. This shows good understanding of libvirt XML handling.
47-48: Verify partial error message matching in libvirt.check_result()Ensure that the call
libvirt.check_exit_status(result, status_error) libvirt.check_result(result, err_msg)in
• libvirt/tests/src/cpu/wrong_topology.py (lines 47–48)
will matcherr_msgas a substring or via regex against the actual libvirt error output (which may include extra context). Ifcheck_result()only does a full-string comparison, you will need to update the helper or switch to a regex-based expected message to avoid brittle failures.
| - multi_clusters: | ||
| vcpu = 32 | ||
| cpu_attrs = {'topology': {'sockets': '2', 'dies': '2', 'clusters': '2', 'cores': '2', 'threads': '2'}, 'mode': 'host-passthrough'} | ||
| err_msg = clusters > 1 not supported by this machine's CPU topology |
There was a problem hiding this comment.
Fix the unquoted error message.
The error message should be properly quoted to prevent parsing issues.
- err_msg = clusters > 1 not supported by this machine's CPU topology
+ err_msg = "clusters > 1 not supported by this machine's CPU topology"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err_msg = clusters > 1 not supported by this machine's CPU topology | |
| err_msg = "clusters > 1 not supported by this machine's CPU topology" |
🧰 Tools
🪛 Ruff (0.11.9)
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: Simple statements must be separated by newlines or semicolons
13-13: SyntaxError: missing closing quote in string literal
13-14: SyntaxError: Expected a statement
🤖 Prompt for AI Agents
In libvirt/tests/cfg/cpu/wrong_topology.py at line 13, the error message
assigned to err_msg is not enclosed in quotes, causing a syntax error. Fix this
by enclosing the entire error message string in either single or double quotes
to make it a valid string literal.
| - one_cluster: | ||
| vcpu = 15 | ||
| cpu_attrs = {'topology': {'sockets': '2', 'dies': '2', 'clusters': '1', 'cores': '2', 'threads': '2'}, 'mode': 'host-passthrough'} | ||
| err_msg = CPU topology doesn't match maximum vcpu count |
There was a problem hiding this comment.
Fix the unquoted error message.
The error message should be properly quoted to prevent parsing issues.
- err_msg = CPU topology doesn't match maximum vcpu count
+ err_msg = "CPU topology doesn't match maximum vcpu count"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err_msg = CPU topology doesn't match maximum vcpu count | |
| err_msg = "CPU topology doesn't match maximum vcpu count" |
🧰 Tools
🪛 Ruff (0.11.9)
9-9: SyntaxError: Simple statements must be separated by newlines or semicolons
9-9: SyntaxError: Simple statements must be separated by newlines or semicolons
9-9: SyntaxError: missing closing quote in string literal
9-10: SyntaxError: Expected a statement
🤖 Prompt for AI Agents
In libvirt/tests/cfg/cpu/wrong_topology.py at line 9, the error message assigned
to err_msg is not enclosed in quotes, causing a syntax error. Fix this by
enclosing the error message string in either single or double quotes to ensure
it is properly recognized as a string literal.
| bkxml = vmxml.copy() | ||
| tmp_vm_name = 'tmp-vm-' + utils_misc.generate_random_string(3) | ||
| vcpu = int(params.get('vcpu')) | ||
| cpu_attrs = eval(params.get('cpu_attrs', {})) |
There was a problem hiding this comment.
Replace eval() with ast.literal_eval() for security.
Using eval() is dangerous as it can execute arbitrary code. Use ast.literal_eval() instead for safe evaluation of dictionary literals.
+import ast- cpu_attrs = eval(params.get('cpu_attrs', {}))
+ cpu_attrs = ast.literal_eval(params.get('cpu_attrs', '{}'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cpu_attrs = eval(params.get('cpu_attrs', {})) | |
| # at top of the file, alongside other imports | |
| import ast | |
| # replace the unsafe eval() call with a safe literal_eval() | |
| cpu_attrs = ast.literal_eval(params.get('cpu_attrs', '{}')) |
🤖 Prompt for AI Agents
In libvirt/tests/src/cpu/wrong_topology.py at line 23, replace the use of eval()
with ast.literal_eval() to safely parse the 'cpu_attrs' parameter. Import the
ast module if not already imported, then change the code to use
ast.literal_eval(params.get('cpu_attrs', '{}')) to avoid executing arbitrary
code and ensure only literal structures are evaluated.
| try: | ||
| tmp_vmxml = vmxml.copy() | ||
| tmp_vmxml.setup_attrs(cpu=cpu_attrs) | ||
| tmp_vmxml.vcpu = vcpu | ||
|
|
||
| tmp_vmxml.vm_name = tmp_vm_name | ||
| tmp_vmxml.del_uuid() | ||
| osxml = tmp_vmxml.os | ||
| os_attrs = osxml.fetch_attrs() | ||
| for k, v in list(os_attrs.items()): | ||
| if k.startswith('nvram'): | ||
| os_attrs.pop(k) | ||
| new_os = vm_xml.VMOSXML() | ||
| new_os.setup_attrs(**os_attrs) | ||
| tmp_vmxml.os = new_os | ||
|
|
||
| tmp_vmxml.xmltreefile.write() | ||
| logging.debug(tmp_vmxml) | ||
|
|
||
| result = virsh.create(tmp_vmxml.xml, debug=True) | ||
| libvirt.check_exit_status(result, status_error) | ||
| libvirt.check_result(result, err_msg) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper error handling for VM cleanup.
The current cleanup only handles the original VM XML but doesn't ensure the temporary VM is properly cleaned up if it gets created unexpectedly.
try:
+ tmp_vm_created = False
tmp_vmxml = vmxml.copy()
tmp_vmxml.setup_attrs(cpu=cpu_attrs)
tmp_vmxml.vcpu = vcpu
tmp_vmxml.vm_name = tmp_vm_name
tmp_vmxml.del_uuid()
osxml = tmp_vmxml.os
os_attrs = osxml.fetch_attrs()
for k, _v in list(os_attrs.items()):
if k.startswith('nvram'):
os_attrs.pop(k)
new_os = vm_xml.VMOSXML()
new_os.setup_attrs(**os_attrs)
tmp_vmxml.os = new_os
tmp_vmxml.xmltreefile.write()
logging.debug(tmp_vmxml)
result = virsh.create(tmp_vmxml.xml, debug=True)
+ if result.exit_status == 0:
+ tmp_vm_created = True
libvirt.check_exit_status(result, status_error)
libvirt.check_result(result, err_msg)
finally:
+ # Clean up temporary VM if it was created
+ if 'tmp_vm_created' in locals() and tmp_vm_created:
+ try:
+ virsh.destroy(tmp_vm_name, ignore_status=True)
+ virsh.undefine(tmp_vm_name, ignore_status=True)
+ except Exception as e:
+ LOG.warning(f"Failed to cleanup temporary VM {tmp_vm_name}: {e}")
bkxml.sync()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| tmp_vmxml = vmxml.copy() | |
| tmp_vmxml.setup_attrs(cpu=cpu_attrs) | |
| tmp_vmxml.vcpu = vcpu | |
| tmp_vmxml.vm_name = tmp_vm_name | |
| tmp_vmxml.del_uuid() | |
| osxml = tmp_vmxml.os | |
| os_attrs = osxml.fetch_attrs() | |
| for k, v in list(os_attrs.items()): | |
| if k.startswith('nvram'): | |
| os_attrs.pop(k) | |
| new_os = vm_xml.VMOSXML() | |
| new_os.setup_attrs(**os_attrs) | |
| tmp_vmxml.os = new_os | |
| tmp_vmxml.xmltreefile.write() | |
| logging.debug(tmp_vmxml) | |
| result = virsh.create(tmp_vmxml.xml, debug=True) | |
| libvirt.check_exit_status(result, status_error) | |
| libvirt.check_result(result, err_msg) | |
| try: | |
| tmp_vm_created = False | |
| tmp_vmxml = vmxml.copy() | |
| tmp_vmxml.setup_attrs(cpu=cpu_attrs) | |
| tmp_vmxml.vcpu = vcpu | |
| tmp_vmxml.vm_name = tmp_vm_name | |
| tmp_vmxml.del_uuid() | |
| osxml = tmp_vmxml.os | |
| os_attrs = osxml.fetch_attrs() | |
| for k, _v in list(os_attrs.items()): | |
| if k.startswith('nvram'): | |
| os_attrs.pop(k) | |
| new_os = vm_xml.VMOSXML() | |
| new_os.setup_attrs(**os_attrs) | |
| tmp_vmxml.os = new_os | |
| tmp_vmxml.xmltreefile.write() | |
| logging.debug(tmp_vmxml) | |
| result = virsh.create(tmp_vmxml.xml, debug=True) | |
| if result.exit_status == 0: | |
| tmp_vm_created = True | |
| libvirt.check_exit_status(result, status_error) | |
| libvirt.check_result(result, err_msg) | |
| finally: | |
| # Clean up temporary VM if it was created | |
| if 'tmp_vm_created' in locals() and tmp_vm_created: | |
| try: | |
| virsh.destroy(tmp_vm_name, ignore_status=True) | |
| virsh.undefine(tmp_vm_name, ignore_status=True) | |
| except Exception as e: | |
| LOG.warning(f"Failed to cleanup temporary VM {tmp_vm_name}: {e}") | |
| bkxml.sync() |
🧰 Tools
🪛 Ruff (0.11.9)
36-36: Loop control variable v not used within loop body
Rename unused v to _v
(B007)
🤖 Prompt for AI Agents
In libvirt/tests/src/cpu/wrong_topology.py around lines 27 to 49, the current
code lacks proper error handling to clean up the temporary VM if it is created
unexpectedly. Add a try-except-finally structure or equivalent to ensure that if
the temporary VM is created during the test, it is properly destroyed or
undefined in the cleanup phase, preventing leftover resources. This involves
tracking the temporary VM creation status and invoking appropriate cleanup calls
in the error handling or finalization block.
- VIRT-17939 - [topology] Define or start VM with wrong cpu topology
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
libvirt/tests/src/cpu/wrong_topology.py (2)
23-23: Security vulnerability: Replace eval() with ast.literal_eval().Using
eval()poses a security risk as it can execute arbitrary code.The solution is to import
astand useast.literal_eval():+import ast- cpu_attrs = eval(params.get('cpu_attrs', {})) + cpu_attrs = ast.literal_eval(params.get('cpu_attrs', '{}'))
27-49: Critical: Add proper error handling for temporary VM cleanup.The current implementation lacks proper cleanup for the temporary VM if it gets created unexpectedly during the test, which could leave orphaned resources.
Add comprehensive error handling:
try: + tmp_vm_created = False tmp_vmxml = vmxml.copy() tmp_vmxml.setup_attrs(cpu=cpu_attrs) tmp_vmxml.vcpu = vcpu tmp_vmxml.vm_name = tmp_vm_name tmp_vmxml.del_uuid() osxml = tmp_vmxml.os os_attrs = osxml.fetch_attrs() - for k, v in list(os_attrs.items()): + for k, _v in list(os_attrs.items()): if k.startswith('nvram'): os_attrs.pop(k) new_os = vm_xml.VMOSXML() new_os.setup_attrs(**os_attrs) tmp_vmxml.os = new_os tmp_vmxml.xmltreefile.write() logging.debug(tmp_vmxml) result = virsh.create(tmp_vmxml.xml, debug=True) + if result.exit_status == 0: + tmp_vm_created = True libvirt.check_exit_status(result, status_error) libvirt.check_result(result, err_msg) finally: + # Clean up temporary VM if it was created + if 'tmp_vm_created' in locals() and tmp_vm_created: + try: + virsh.destroy(tmp_vm_name, ignore_status=True) + virsh.undefine(tmp_vm_name, ignore_status=True) + except Exception as e: + LOG.warning(f"Failed to cleanup temporary VM {tmp_vm_name}: {e}") bkxml.sync()
🧹 Nitpick comments (3)
libvirt/tests/src/cpu/wrong_topology.py (3)
24-24: Fix Yoda condition for better readability.The condition order should be reversed for improved readability.
- status_error = 'yes' == params.get('status_error', 'no') + status_error = params.get('status_error', 'no') == 'yes'
36-38: Fix unused loop variable.The loop variable
vis not used in the loop body and should be renamed to indicate this.- for k, v in list(os_attrs.items()): + for k, _v in list(os_attrs.items()):
13-16: Consider enhancing the docstring.The docstring could be more descriptive about the test's purpose and expected behavior.
- """ - Test Define VM with wrong cpu topology - """ + """ + Test that VM creation fails when configured with incorrect CPU topology. + + This negative test validates libvirt's error handling for invalid CPU + topology configurations by attempting to create a VM with wrong topology + parameters and verifying the expected error response. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/cpu/wrong_topology.cfg(1 hunks)libvirt/tests/src/cpu/wrong_topology.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libvirt/tests/cfg/cpu/wrong_topology.cfg
🧰 Additional context used
🪛 Ruff (0.11.9)
libvirt/tests/src/cpu/wrong_topology.py
24-24: Yoda condition detected
Rewrite as params.get('status_error', 'no') == 'yes'
(SIM300)
36-36: Loop control variable v not used within loop body
Rename unused v to _v
(B007)
⏰ 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.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (2)
libvirt/tests/src/cpu/wrong_topology.py (2)
1-6: LGTM - Imports look appropriate.The imports are well-organized and include all necessary modules for the test functionality.
8-10: LGTM - Constants and logging setup are appropriate.The VIRSH_ARGS constant and logger setup follow good practices for test infrastructure.
|
/gemini summary |
Summary of ChangesThis pull request enhances the Highlights
Changelog
Activity
|
1 participant
VIRT-17939 - [topology] Define or start VM with wrong cpu topology
Test result
(1/2) type_specific.local.cpu.wrong_topology.one_cluster: STARTED
(1/2) type_specific.local.cpu.wrong_topology.one_cluster: PASS (10.17 s)
(2/2) type_specific.local.cpu.wrong_topology.multi_clusters: STARTED
(2/2) type_specific.local.cpu.wrong_topology.multi_clusters: PASS (11.22 s)
Summary by CodeRabbit