max_vpus: Add version parsing and conditional logic for pc-q35-rhel machine types #6742
Conversation
WalkthroughThe change modifies a single test file to add regex-based version parsing and handling for QEMU machine-type keys. It introduces helper Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 1
🧹 Nitpick comments (1)
libvirt/tests/src/cpu/max_vcpus.py (1)
125-135: Consider simplifying version validation logic.The control flow is correct, but there's a minor redundancy: if you add proper error handling to
_parse_version()(as suggested in the earlier comment), thelen(version) != 3check on lines 128-129 becomes unreachable for invalid versions, since_parse_version()would raise an exception first.You can either:
- Remove lines 128-129 entirely and rely on
_parse_version()to validate the format, or- Keep it as a defensive assertion for future safety
Option 1: Remove redundant check (preferred if _parse_version has error handling)
if key.startswith('pc-q35-rhel'): version_str = key.replace('pc-q35-rhel', '') version = _parse_version(version_str) - if len(version) != 3: - test.error('version of machine type is not expected as x.x.x but %s' % version_str) if key == "pc-q35-rhel7.3.0":
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/cpu/max_vcpus.py
🧰 Additional context used
🪛 Gitleaks (8.30.0)
libvirt/tests/src/cpu/max_vcpus.py
[high] 130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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.8
- GitHub Check: Python 3.9
🔇 Additional comments (1)
libvirt/tests/src/cpu/max_vcpus.py (1)
2-2: LGTM!The regex module import is appropriate for the version parsing functionality added in this PR.
liang-cong-red-hat
force-pushed
the
max_vcpus_add_adaption_for_higher_q35_machine
branch
from
8c359b0 to
f1a6e85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
libvirt/tests/src/cpu/max_vcpus.py (1)
64-73: Missing error handling for regex match failure (previously flagged).This issue was already identified in a previous review: if
re.searchreturnsNone(when the version string doesn't match the expected pattern, such as "9.6", "9", or non-numeric versions), the code will raiseAttributeErrorwhen callingmatches.groups(). Please add a check afterre.searchto handle this case.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/cpu/max_vcpus.py
🧰 Additional context used
🪛 Gitleaks (8.30.0)
libvirt/tests/src/cpu/max_vcpus.py
[high] 130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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.11
- GitHub Check: Python 3.12
- GitHub Check: Python 3.8
🔇 Additional comments (1)
libvirt/tests/src/cpu/max_vcpus.py (1)
125-146: Version comparison logic is sound.The implementation correctly handles different pc-q35-rhel machine type versions:
- Version parsing is only attempted for keys starting with 'pc-q35-rhel' (line 125)
- Special case for pc-q35-rhel7.3.0 is preserved (line 130)
- New generalized comparison for version >= 9.6.0 addresses the issue described in the PR objectives (line 134)
- Short-circuit evaluation at line 134 prevents
NameErrorby only accessingversionwhen it's guaranteed to be defined- Fallback to libvirt version comparisons handles older machine types appropriately
The approach aligns well with the stated goal of fixing incorrect q35_max_vcpus_num reporting for newer RHEL versions.
…achine types 1. Add _parse_version function to convert 'x.x.x' to integer tuple; 2. Enhance version comparison logic for machine type pc-q35-rhel9.6.0+; Signed-off-by: Liang Cong <lcong@redhat.com>
liang-cong-red-hat
force-pushed
the
max_vcpus_add_adaption_for_higher_q35_machine
branch
from
f1a6e85 to
bf0cac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libvirt/tests/src/cpu/max_vcpus.py (1)
127-135: Consider checking special case before parsing version.The code is functionally correct, but for the special case "pc-q35-rhel7.3.0" (line 130), the version is parsed on line 129 but never used. While this doesn't affect correctness, checking the special case before parsing would avoid unnecessary work.
💡 Optional refactor to improve efficiency
if key.startswith('pc-q35') or key == 'q35': exp_val = report_num_q35_7_8 - if key.startswith('pc-q35-rhel'): - version_str = key.replace('pc-q35-rhel', '') - version = _parse_version(version_str) if key == "pc-q35-rhel7.3.0": exp_val = report_num_q35_73 elif key == 'q35': exp_val = report_num_q35_9_6 elif key.startswith('pc-q35-rhel'): + version_str = key.replace('pc-q35-rhel', '') + version = _parse_version(version_str) - elif key.startswith('pc-q35-rhel') and version >= (9, 6, 0): + if version >= (9, 6, 0): - exp_val = report_num_q35_9_6 + exp_val = report_num_q35_9_6 + else: + if libvirt_version.version_compare(7, 0, 0): + exp_val = report_num_q35_8_4 + elif libvirt_version.version_compare(6, 6, 0): + exp_val = report_num_q35_8_3 else: if libvirt_version.version_compare(7, 0, 0): exp_val = report_num_q35_8_4 elif libvirt_version.version_compare(6, 6, 0): exp_val = report_num_q35_8_3
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/cpu/max_vcpus.py
🧰 Additional context used
🪛 Gitleaks (8.30.0)
libvirt/tests/src/cpu/max_vcpus.py
[high] 130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
🔇 Additional comments (3)
libvirt/tests/src/cpu/max_vcpus.py (3)
2-2: LGTM!The
remodule import is properly added to support version parsing.
64-75: LGTM!The version parsing function is well-implemented with proper error handling. The check for
Nonereturn fromre.search(line 73-74) addresses the concern raised in the previous review, ensuring malformed version strings produce clear error messages rather than AttributeError.
130-130: Static analysis false positive.The Gitleaks warning on line 130 is a false positive. The string "pc-q35-rhel7.3.0" is a QEMU machine type identifier, not an API key.
1 participant
Before fix: test case may fail for "Test failed as the q35_max_vcpus_num for machine type:pc-q35-rhel9.8.0 in virsh_capa is wrong. Expected: 710, Actual: 4096."
After fix:
(1/1) type_specific.io-github-autotest-libvirt.max_vcpus.virsh_capabilities: STARTED
(1/1) type_specific.io-github-autotest-libvirt.max_vcpus.virsh_capabilities: PASS (6.28 s)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.