libvirt_mem:Add test for memory attach-detach with different option#6878
libvirt_mem:Add test for memory attach-detach with different option#6878sneh-3 wants to merge 1 commit into
Conversation
options are as --config, --live, --current, --persistent, --live --config Signed-off-by: Sneh Shikha Yadav <syadav@linux.ibm.com>
WalkthroughThis PR enhances the libvirt memory attach/detach test with comprehensive verification capabilities. It introduces four new utility functions to inspect guest memory and domain XML, then integrates them into the main test flow. The test configuration is updated with new baseline memory parameters, a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🤖 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/cfg/libvirt_mem.cfg`:
- Around line 190-193: The test variant current_shutoff sets start_vm = "no" but
the test harness in libvirt/tests/src/libvirt_mem.py still forces a guest start
whenever attach_device == "yes", so the case exercises the running (--current)
path instead of the shutoff path. Update the start logic in libvirt_mem.py (the
code that checks attach_device and starts the guest) to respect the start_vm
flag for this variant: when start_vm == "no" (or when the variant name is
current_shutoff) do not start the domain before invoking the attach/detach flow
so --current runs against a shutoff domain; alternatively add an explicit branch
for current_shutoff to skip the guest startup. Ensure the check references the
existing attach_device and start_vm variables (or the variant identifier) so
behavior matches the libvirt_mem.cfg setting.
In `@libvirt/tests/src/libvirt_mem.py`:
- Line 665: The branch uses detach_option without initializing it and reuses
attach_option in the detach path; initialize detach_option from params the same
way attach_option is loaded (e.g., detach_option = params.get("detach_option")
or equivalent), update the conditional to check if either attach_option or
detach_option is set, and replace uses of attach_option in the detach flow and
verification with detach_option so virsh.detach_device() and subsequent checks
exercise the configured detach-option variants.
- Around line 717-724: The verify_immediate_changes call is getting dicts from
get_domain_memory_xml (baseline_xml_active, after_attach_xml_active,
baseline_xml_inactive, after_attach_xml_inactive) but the helper expects scalar
counts and guest-memory values; update the call in the attach verification to
pass .get("memory_device_count") for each XML dict and the actual guest-memory
before/after attach (or alternatively change verify_immediate_changes to accept
the three models: active_xml, inactive_xml, guest_memory), and also stop
silently swallowing failures in the except block—replace the bare
logging.warning("Attach verification failed: %s", e) with either
logging.exception(...) followed by re-raising the exception or let the exception
propagate so the test fails on verification errors.
- Around line 892-897: The detach-only path can hit an UnboundLocalError because
dev_xml may never be defined before the detach block; ensure dev_xml is
initialized to None before the attach/ detach branches (or always create it when
detach_device is true). Specifically, add an initialization like dev_xml = None
at the start of the function handling attach/detach, or guard the detach branch
to call utils_hotplug.create_mem_xml (using tg_size, pg_size, mem_addr,
tg_sizeunit, pg_unit, tg_node, node_mask, mem_model, mem_discard)
unconditionally when detach_device is enabled so the code that checks "if
dev_xml is None:" no longer references an unbound variable.
🪄 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: c244e0fa-e418-4c89-8195-817019e42314
📒 Files selected for processing (2)
libvirt/tests/cfg/libvirt_mem.cfglibvirt/tests/src/libvirt_mem.py
| - current_shutoff: | ||
| attach_option = "--current" | ||
| detach_option = "--current" | ||
| start_vm = "no" |
There was a problem hiding this comment.
current_shutoff does not currently test a shutoff-domain --current flow.
This variant sets start_vm = "no", but libvirt/tests/src/libvirt_mem.py still starts the guest up front whenever attach_device = "yes". As written, this case will execute --current against a running VM and duplicate current_running semantics instead of covering the shutoff path.
🤖 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/cfg/libvirt_mem.cfg` around lines 190 - 193, The test variant
current_shutoff sets start_vm = "no" but the test harness in
libvirt/tests/src/libvirt_mem.py still forces a guest start whenever
attach_device == "yes", so the case exercises the running (--current) path
instead of the shutoff path. Update the start logic in libvirt_mem.py (the code
that checks attach_device and starts the guest) to respect the start_vm flag for
this variant: when start_vm == "no" (or when the variant name is
current_shutoff) do not start the domain before invoking the attach/detach flow
so --current runs against a shutoff domain; alternatively add an explicit branch
for current_shutoff to skip the guest startup. Ensure the check references the
existing attach_device and start_vm variables (or the variant identifier) so
behavior matches the libvirt_mem.cfg setting.
| session.close() | ||
|
|
||
| # Capture baseline for verification | ||
| if attach_option or detach_option: |
There was a problem hiding this comment.
Initialize and use detach_option explicitly.
detach_option is never loaded from params, so this branch raises NameError whenever attach_option is empty. The same root cause also means the detach path still uses attach_option for virsh.detach_device() and verification, so the new detach-option variants are not actually exercised as configured.
🧰 Tools
🪛 Ruff (0.15.15)
[error] 665-665: Undefined name detach_option
(F821)
🤖 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` at line 665, The branch uses detach_option
without initializing it and reuses attach_option in the detach path; initialize
detach_option from params the same way attach_option is loaded (e.g.,
detach_option = params.get("detach_option") or equivalent), update the
conditional to check if either attach_option or detach_option is set, and
replace uses of attach_option in the detach flow and verification with
detach_option so virsh.detach_device() and subsequent checks exercise the
configured detach-option variants.
| verify_immediate_changes(attach_option, baseline_xml_active, after_attach_xml_active, | ||
| baseline_xml_inactive, after_attach_xml_inactive, | ||
| operation="attach", test=test) | ||
| logging.info("After attach: active_xml=%s devices, inactive_xml=%s devices (will be reused as before-detach baseline)", | ||
| after_attach_xml_active.get('memory_device_count') if after_attach_xml_active else 'N/A', | ||
| after_attach_xml_inactive.get('memory_device_count') if after_attach_xml_inactive else 'N/A') | ||
| except Exception as e: | ||
| logging.warning("Attach verification failed: %s", e) |
There was a problem hiding this comment.
The new verification call is passing the wrong data types.
get_domain_memory_xml() returns dicts, but verify_immediate_changes() compares scalar XML counts and scalar guest-memory values. Passing baseline_xml_active/after_attach_xml_active plus the inactive-XML dicts here guarantees broken comparisons, and the warning-only except at Line 723 lets the test pass without validating the attach behavior. Please pass .get("memory_device_count") and real guest-memory before/after values, or widen the helper signature to model active XML, inactive XML, and guest memory separately.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 723-723: Do not catch blind exception: Exception
(BLE001)
🤖 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 717 - 724, The
verify_immediate_changes call is getting dicts from get_domain_memory_xml
(baseline_xml_active, after_attach_xml_active, baseline_xml_inactive,
after_attach_xml_inactive) but the helper expects scalar counts and guest-memory
values; update the call in the attach verification to pass
.get("memory_device_count") for each XML dict and the actual guest-memory
before/after attach (or alternatively change verify_immediate_changes to accept
the three models: active_xml, inactive_xml, guest_memory), and also stop
silently swallowing failures in the except block—replace the bare
logging.warning("Attach verification failed: %s", e) with either
logging.exception(...) followed by re-raising the exception or let the exception
propagate so the test fails on verification errors.
| # 1. Reuse dev_xml from attach operation instead of creating a new one | ||
| if dev_xml is None: | ||
| logging.warning("No dev_xml from attach, creating new one for detach") | ||
| dev_xml = utils_hotplug.create_mem_xml(tg_size, pg_size, mem_addr, tg_sizeunit, | ||
| pg_unit, tg_node, node_mask, mem_model, | ||
| mem_discard) |
There was a problem hiding this comment.
dev_xml can be unbound in detach-only variants.
The new detach block assumes dev_xml already exists, but detach-only cases do not go through any earlier assignment. That makes if dev_xml is None: raise UnboundLocalError before the detach command is even attempted. Initialize dev_xml = None before the attach branches, or create it unconditionally when detach_device is enabled.
🤖 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 892 - 897, The detach-only
path can hit an UnboundLocalError because dev_xml may never be defined before
the detach block; ensure dev_xml is initialized to None before the attach/
detach branches (or always create it when detach_device is true). Specifically,
add an initialization like dev_xml = None at the start of the function handling
attach/detach, or guard the detach branch to call utils_hotplug.create_mem_xml
(using tg_size, pg_size, mem_addr, tg_sizeunit, pg_unit, tg_node, node_mask,
mem_model, mem_discard) unconditionally when detach_device is enabled so the
code that checks "if dev_xml is None:" no longer references an unbound variable.
|
First of all please extend the PR description with ... what this do... why it is necessary... Second .. please check existing code .. mainly avocado-vt functions and existing memory providers in tp-libvirt. ... I will add detailed info directly |
|
There are some additional comments from AI that I believe are correct but will require much more detailed check and I am not so involved into the memory tests. |
2 participants
supported options are --config, --live, --current, --persistent, --live --config
Summary by CodeRabbit
New Features
Chores