Conversation
WalkthroughA new inner helper Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libvirt/tests/src/libvirt_mem.py`:
- Around line 148-166: The verification failure for vhost max_mem_regions
configuration and the exception handler are downgrading setup errors to
warnings, allowing the test to continue in an invalid state. When verification
confirms the value is not set to "512" (around line 155-156), raise an exception
instead of just logging a warning. In the exception handler (around line
165-166), re-raise the caught exception instead of logging a warning and
continuing, so that SLES-targeted setup failures properly fail or cancel the
test path rather than silently proceeding.
- Around line 135-137: The current pipeline in the vm_check assignment combines
virsh and grep in a single command, which means the exit_status will reflect
grep's result rather than virsh's failure. If virsh list fails but grep runs
against empty output without error, the condition will incorrectly assess the
situation as safe to proceed. Split the command execution into two separate
steps: first run virsh list --state-running independently and check its exit
status, then only proceed to parse the output with grep if virsh succeeds. This
ensures virsh failures are explicitly caught before attempting to process
output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 246a485b-d09a-43c6-957c-b8ee9d1ddada
📒 Files selected for processing (1)
libvirt/tests/src/libvirt_mem.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
libvirt/tests/src/libvirt_mem.py (3)
131-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a proper distro-detection API over shelling out to
cat/grepon/etc/os-release.
avocado.utils.distro.detect()provides structured Linux-distro detection and avoids the fragile substring match on/etc/os-release(which could theoretically hitID_LIKE/PRETTY_NAMEfalse positives, e.g. SLES-based derivatives).♻️ Proposed refactor
+from avocado.utils import distro + def setup_vhost_max_mem_regions(): ... try: - # Check if running on SLES - distro_check = process.run("cat /etc/os-release | grep -i sles", - shell=True, ignore_status=True) - if distro_check.exit_status == 0: + detected_distro = distro.detect() + if "sles" in detected_distro.name.lower():Please confirm
avocado.utils.distro.detect()is available/appropriate for this test's target Avocado version before adopting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libvirt/tests/src/libvirt_mem.py` around lines 131 - 134, The SLES detection in the test currently shells out to `cat`/`grep` against `/etc/os-release`, which is fragile and can match unintended fields. Replace that logic in the `libvirt_mem` test with Avocado’s structured distro detection via `avocado.utils.distro.detect()`, and use its returned distro info to decide whether to log the SLES-specific `max_mem_regions=512` message. Make sure `detect()` is available in the Avocado version targeted by this test before switching the existing `process.run` check.
135-150: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTOCTOU window between the "no running VMs" check and the module reload.
Between the
virsh list --state-runningcheck (lines 135-144) and themodprobe -r/reload (lines 147-150), another process could start a domain that depends on vhost, causing the unload to disrupt it. This is a narrow race in practice for a single-threaded test harness, so treat as a minor hardening opportunity rather than a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libvirt/tests/src/libvirt_mem.py` around lines 135 - 150, The vhost reconfiguration in the running-VM check has a small TOCTOU race between the `virsh list --state-running` validation and the `modprobe` unload/reload sequence. Harden `libvirt_mem` by rechecking for active VMs immediately before calling the `process.run` module removal/reload steps, and skip the configuration if any are detected; use the existing `vm_check`, `running_vms`, and `process.run` flow to keep the fix localized.
119-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winVhost module change is never reverted; persists beyond this test.
setup_vhost_max_mem_regions()unloads and reloadsvhost/vhost_net/vhost_scsi/vhost_vsockwithmax_mem_regions=512for the whole boot session, butrun()'sfinallyblock (lines 837-851) has no matching step to restore the previous module state. Any other test or process on the same host that runs afterward will silently inherit this modified vhost parameter instead of the distro default, which is easy to misdiagnose in shared/CI infrastructure.Consider restoring the module (unload/reload without
max_mem_regions, or track/restore the previous value) in thefinallyblock, mirroring howrestore_hugepages()undoessetup_hugepages().Also applies to: 837-851
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libvirt/tests/src/libvirt_mem.py` around lines 119 - 172, The vhost runtime tweak in setup_vhost_max_mem_regions is not reverted, so the modified max_mem_regions value leaks into later tests on the same host. Add matching cleanup in run()’s finally block to restore the prior vhost module state, either by unloading/reloading the vhost modules without the override or by saving and restoring the original parameter value. Use the existing setup_vhost_max_mem_regions helper as the place to capture state and mirror restore_hugepages-style teardown in run().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libvirt/tests/src/libvirt_mem.py`:
- Around line 131-134: The SLES detection in the test currently shells out to
`cat`/`grep` against `/etc/os-release`, which is fragile and can match
unintended fields. Replace that logic in the `libvirt_mem` test with Avocado’s
structured distro detection via `avocado.utils.distro.detect()`, and use its
returned distro info to decide whether to log the SLES-specific
`max_mem_regions=512` message. Make sure `detect()` is available in the Avocado
version targeted by this test before switching the existing `process.run` check.
- Around line 135-150: The vhost reconfiguration in the running-VM check has a
small TOCTOU race between the `virsh list --state-running` validation and the
`modprobe` unload/reload sequence. Harden `libvirt_mem` by rechecking for active
VMs immediately before calling the `process.run` module removal/reload steps,
and skip the configuration if any are detected; use the existing `vm_check`,
`running_vms`, and `process.run` flow to keep the fix localized.
- Around line 119-172: The vhost runtime tweak in setup_vhost_max_mem_regions is
not reverted, so the modified max_mem_regions value leaks into later tests on
the same host. Add matching cleanup in run()’s finally block to restore the
prior vhost module state, either by unloading/reloading the vhost modules
without the override or by saving and restoring the original parameter value.
Use the existing setup_vhost_max_mem_regions helper as the place to capture
state and mirror restore_hugepages-style teardown in run().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bbaf632-e271-4648-9cd4-6cbc5ec5b4b2
📒 Files selected for processing (1)
libvirt/tests/src/libvirt_mem.py
|
before fix: after fix: |
hholoubk
left a comment
hholoubk
left a comment
There was a problem hiding this comment.
There are several things that can be better, some of them kind of severe, some just unoptimized.
The main issue is, that test is changing host configuration and not recovering the environement after.
Commited changes will run regardless they are needed.
Suggested changes:
- Use the avocado cfg file to change the configuration of host only in cases that really need it.
- Do not hardcode set value, introduce the flag if it should be set in the test and the value, that should be set.
- move the check (if the value should be done) outside the setter method
- backup the value and recover it at the end of the test
- reconsider if the test shouldn't be canceled in case that there is any running VM.
| """ | ||
| try: | ||
| # Check if running on SLES | ||
| distro_check = process.run("cat /etc/os-release | grep -i sles", |
There was a problem hiding this comment.
Use avocado method to check the distro
already used in virsh_dump.py, libvirt_pci_passthrough_hotplug.py, etc.:
from avocado.utils import distro
detected = distro.detect()
if detected.name.lower() in ("suse", "sles"):
BUT
... there is better way how to do it via configuration.
| # Check if running on SLES | ||
| distro_check = process.run("cat /etc/os-release | grep -i sles", | ||
| shell=True, ignore_status=True) | ||
| if distro_check.exit_status == 0: |
There was a problem hiding this comment.
It is wrong approach mixing os detection into the value setting. Please move the whole detection outside (into the test run)
so the row will be
if detected.name.lower() in ("suse", "sles"):
setup_vhost_max_mem_regions()
| logging.info("No running VMs detected, proceeding with vhost configuration") | ||
| process.run("modprobe -r vhost_net vhost_scsi vhost_vsock vhost", | ||
| shell=True, ignore_status=True) | ||
| result = process.run("modprobe vhost max_mem_regions=512", |
There was a problem hiding this comment.
setting this value regardless what is actually set on the host is not good approach ...
Also setting the value without storing it and recover to previous value to return the host in same configuration as before the test is against best practices.
- store actual value
- check if it is lower than what you want
current = process.run(cmd, shell=True).stdout_text.strip()
needed = int(params.get("vhost_max_mem_regions", 512))
if current < needed:
# do the setting
return current
restore later in finally of run test.
1. check the running dostro and vm state 2. configures vhost max_mem_regions v2: fixed vhost max_mem_regions setup based on review comments - Removed OS detection from setup_vhost_max_mem_regions() - Introduced set_vhost_max_mem_regions flag in cfg to control when the setup runs - Make the value configurable via vhost_max_mem_regions param in cfg - Restore original vhost max_mem_regions in finally block Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
4c8ae26 to
63b9c8b
Compare
|
--logs-- |
2 participants
Summary by CodeRabbit