Handle unsupported architectures in virsh CPU models check! #6678
Conversation
Add a check to skip unsupported architectures when running virsh cpu-models. Logs the message and continues with the next architecture to avoid unnecessary failures as it is expected.. Signed-off-by: Anushree-Mathur <[email protected]>
WalkthroughThe change modifies the virsh cpu-models test to handle unsupported CPU architectures at runtime. When executing the cpu-models command, if the stderr contains "architecture is not supported by CPU driver", the test logs an info message and skips processing that architecture. For supported architectures, the existing exit status checks and comparison logic remain unchanged. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/virsh_cmd/host/virsh_cpu_models.py(1 hunks)
⏰ 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.8
- GitHub Check: Python 3.11
| # Check if the architecture is not supported; log it and skip to the next architecture. | ||
| if "architecture is not supported by CPU driver" in result.stderr: | ||
| logging.info("%s architecture is not supported by CPU driver as expected" % arch) | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
Consider checking exit status in addition to stderr.
The condition only checks stderr content without verifying that the command actually failed. If the command succeeds (exit status 0) but includes this text in stderr as a warning, the architecture would be incorrectly skipped.
Apply this diff for more defensive error handling:
# Check if the architecture is not supported; log it and skip to the next architecture.
-if "architecture is not supported by CPU driver" in result.stderr:
+if result.exit_status != 0 and "architecture is not supported by CPU driver" in result.stderr:
logging.info("%s architecture is not supported by CPU driver as expected" % arch)
continueAdditionally, the hardcoded error message string may be fragile if libvirt changes its error format.
Please verify the error message format is stable across libvirt versions:
🌐 Web query:
What error message does libvirt virsh cpu-models command return for unsupported architectures?
💡 Result:
It doesn't return an error — for architectures where libvirt has no built-in models it prints "all CPU models are accepted". [1][2]
Sources:
[1] libvirt virsh manpage (cpu-models).
[2] libvirt-devel discussion noting cpu-models prints "all CPU models are accepted" for aarch64/s390x.
Critical: The error message string is incorrect and will never match libvirt's actual output.
libvirt prints "all CPU models are accepted" for unsupported architectures, not "architecture is not supported by CPU driver". This means the condition on line 85 will never trigger, and unsupported architectures won't be skipped as intended.
Fix: Replace the error message string with the correct one:
# Check if the architecture is not supported; log it and skip to the next architecture.
-if "architecture is not supported by CPU driver" in result.stderr:
+if "all CPU models are accepted" in result.stderr:
logging.info("%s architecture is not supported by CPU driver as expected" % arch)
continue📝 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.
| # Check if the architecture is not supported; log it and skip to the next architecture. | |
| if "architecture is not supported by CPU driver" in result.stderr: | |
| logging.info("%s architecture is not supported by CPU driver as expected" % arch) | |
| continue | |
| # Check if the architecture is not supported; log it and skip to the next architecture. | |
| if "all CPU models are accepted" in result.stderr: | |
| logging.info("%s architecture is not supported by CPU driver as expected" % arch) | |
| continue |
🤖 Prompt for AI Agents
In libvirt/tests/src/virsh_cmd/host/virsh_cpu_models.py around lines 84 to 87,
the substring checked in result.stderr is wrong and will never match libvirt
output; replace the check for "architecture is not supported by CPU driver" with
"all CPU models are accepted" so the condition triggers for unsupported
architectures, keep the logging and continue behavior the same.
|
Before applying the patch: After applying the patch: In debug.log it will log the information as: |
1 participant
Add a check to skip unsupported architectures when running virsh cpu-models. Logs the message and continues with the next architecture to avoid unnecessary failures as it is expected..
Signed-off-by: Anushree-Mathur [email protected]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.