Skip to content

Conversation

@liang-cong-red-hat
Copy link
Contributor

@liang-cong-red-hat liang-cong-red-hat commented Dec 10, 2025

Since libvirt 11.8.0, 0 memory size numa node is supported, requiring test cancellation for versions >= 11.8.0

Test result:
(1/1) type_specific.io-github-autotest-libvirt.memory.allocation.invalid_value.numa.zero: STARTED
(1/1) type_specific.io-github-autotest-libvirt.memory.allocation.invalid_value.numa.zero: CANCEL: Test is not supported since libvirt version:(11, 8, 0) (8.36 s)

Summary by CodeRabbit

  • Tests
    • Enhanced memory allocation tests with refined version-aware checks: tests now respect both "supported since" and "not supported since" markers to decide case cancellation.
    • Version markers were scoped to the specific variant, improving applicability of checks.
    • Adjusted test control flow so setup and execution occur after version checks, ensuring correct behavior across libvirt versions.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 10, 2025

Walkthrough

The change moves a top-level libvirt version constraint into the numa.zero variant in the memory invalid value config, adding func_supported_since_libvirt_ver = (9, 0, 0) and func_not_supported_since_libvirt_ver = (11, 8, 0) under numa.zero. The Python test now always calls libvirt_version.is_libvirt_feature_supported(params), reads func_not_supported_since_libvirt_ver from params, and cancels the test if the running libvirt version is at or beyond that threshold. setup_test() and run_test() are executed after these version checks. No other memory configurations or vm attributes were changed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Changes span two files and introduce new conditional logic around libvirt-version gating rather than a simple config move.
  • Review focus:
    • Verify the numa.zero placement and exact version tuples for func_supported_since_libvirt_ver and func_not_supported_since_libvirt_ver.
    • Confirm the test reads the new parameter name exactly (func_not_supported_since_libvirt_ver) and uses correct version comparison semantics.
    • Ensure the unconditional call to is_libvirt_feature_supported(params) plus the new cancellation logic do not change test execution order or skip unintended setup paths.
    • Check for typos in parameter names (consistency between config and test).

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding libvirt version-based test cancellation for NUMA nodes with 0 memory size, which aligns with the core modifications across both configuration and test files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@liang-cong-red-hat liang-cong-red-hat force-pushed the memory_invalid_value_add_version_check branch from a759127 to 60145d5 Compare December 10, 2025 02:21
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c99bc09 and a759127.

📒 Files selected for processing (2)
  • libvirt/tests/cfg/memory/memory_allocation/memory_invalid_value.cfg (1 hunks)
  • libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py (2 hunks)
🧰 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.
🪛 Ruff (0.14.8)
libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py

72-72: Use of possibly insecure function; consider using ast.literal_eval

(S307)

⏰ 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
🔇 Additional comments (2)
libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py (2)

81-84: Version check logic is correct.

The version checking logic appropriately cancels the test when libvirt version is >= 11.8.0, which aligns with the PR objective. The test will run on versions 9.0.0 through 11.7.x, where 0-memory NUMA nodes are expected to be invalid.

Note: After fixing the typo in line 72, update line 82 to reference the corrected variable name func_not_supported_since_libvirt_ver.

After fixing the typo in line 72, apply this corresponding change:

-        if func_not_suppored_since_libvirt_ver:
-            if libvirt_version.version_compare(*func_not_suppored_since_libvirt_ver):
-                test.cancel("Test is not supported since libvirt version:%s" % str(func_not_suppored_since_libvirt_ver))
+        if func_not_supported_since_libvirt_ver:
+            if libvirt_version.version_compare(*func_not_supported_since_libvirt_ver):
+                test.cancel("Test is not supported since libvirt version:%s" % str(func_not_supported_since_libvirt_ver))

85-86: Improved control flow with version checks before setup.

The reordering to execute setup_test() and run_test() after version validation is correct. This ensures that test setup and execution only occur when the libvirt version falls within the supported range, improving efficiency and preventing unnecessary operations.

… 0 memory size numa node

Since libvirt 11.8.0, 0 memory size numa node is supported, requiring test cancellation for versions >= 11.8.0

Signed-off-by: Liang Cong <lcong@redhat.com>
@liang-cong-red-hat liang-cong-red-hat force-pushed the memory_invalid_value_add_version_check branch from 60145d5 to 4d6bb74 Compare December 11, 2025 09:11
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py (1)

72-72: Consider using ast.literal_eval() for safer tuple parsing.

The static analysis tool flags eval() as potentially insecure (S307). While parameters originate from controlled configuration files and the same pattern exists at lines 76-77, using ast.literal_eval() is safer for parsing literal structures like tuples.

If you decide to address this, consider applying the change consistently to all eval() calls in this file for uniformity.

+import ast
+
 from virttest import virsh
-    func_not_supported_since_libvirt_ver = eval(params.get("func_not_supported_since_libvirt_ver", "()"))
+    func_not_supported_since_libvirt_ver = ast.literal_eval(params.get("func_not_supported_since_libvirt_ver", "()"))
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60145d5 and 4d6bb74.

📒 Files selected for processing (2)
  • libvirt/tests/cfg/memory/memory_allocation/memory_invalid_value.cfg (1 hunks)
  • libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libvirt/tests/cfg/memory/memory_allocation/memory_invalid_value.cfg
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 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/src/memory/memory_allocation/memory_invalid_value.py
📚 Learning: 2025-09-24T08:01:27.899Z
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.

Applied to files:

  • libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py
🪛 Ruff (0.14.8)
libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py

72-72: Use of possibly insecure function; consider using ast.literal_eval

(S307)

⏰ 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.11
  • GitHub Check: Python 3.9
  • GitHub Check: Python 3.12
  • GitHub Check: Python 3.8
🔇 Additional comments (1)
libvirt/tests/src/memory/memory_allocation/memory_invalid_value.py (1)

81-84: LGTM!

The version check logic correctly implements the dual-bound version constraint:

  1. Line 81 ensures minimum version support via is_libvirt_feature_supported(params) (which internally handles cancellation based on func_supported_since_libvirt_ver)
  2. Lines 82-84 add the upper bound check, cancelling tests for libvirt versions ≥ 11.8.0 where NUMA nodes with 0 memory size became valid

The PR test output confirms the expected behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants