virsh_dump: drop --live option support #6584
Conversation
WalkthroughThis change removes --live-related dump options from specific config variants and adds a negative test for invalid --live usage. In code, it adds asynchronous waiting for logfile checks, introduces safer backup_xml handling, and refines error-handling flow to restore config, restart services, and early-return on expected errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tester
participant Test as virsh_dump.py
participant Log as check_logfile()
participant Wait as utils_misc.wait_for()
participant QEMU as QEMU Config
participant Libvirtd as libvirtd
Tester->>Test: Run virsh_dump test
alt invalid dump_image_format
Test->>Log: check_logfile(format)
Log->>Wait: wait_for(predicate, 30s)
Wait-->>Log: result (found/not found)
Log-->>Test: err_msg_found
Test->>QEMU: restore config
Test->>Libvirtd: restart socket
alt expected error found
Test-->>Tester: return (test passes this negative case)
else not found
Test-->>Tester: fail test
end
else valid path
Test->>Log: check_logfile(...) (async wait used)
Note right of Test: backup_xml.sync() only if backup_xml is set
Test-->>Tester: continue/complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
zhentang-tz
force-pushed
the
guest_kernel_debugging_fix
branch
2 times, most recently
from
8e3ae3e to
12ed210
Compare
The --live flag was removed from virsh dump upstream, so any positive test that still use it now fails immediately. ref: https://gitlab.com/libvirt/libvirt/-/issues/646 Delete all test cases that relied on --live, and add a negative test to make sure the command rejects the obsolete option. Additionally, fix an unstable race by using utils_misc.wait_for(). Preventing the race condition where the log file had not yet been fully generated when the first check occurred. Also fix an unstable teardown issue Signed-off-by: zhentang-tz <[email protected]>
zhentang-tz
force-pushed
the
guest_kernel_debugging_fix
branch
from
12ed210 to
c310b8d
Compare
|
JOB ID : de3026c2b931917a63af1bcf516d6864c9e07a2e |
![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 (3)
libvirt/tests/cfg/guest_kernel_debugging/virsh_dump.cfg(1 hunks)libvirt/tests/cfg/virsh_cmd/monitor/virsh_domstate.cfg(0 hunks)libvirt/tests/src/guest_kernel_debugging/virsh_dump.py(4 hunks)
💤 Files with no reviewable changes (1)
- libvirt/tests/cfg/virsh_cmd/monitor/virsh_domstate.cfg
🔇 Additional comments (4)
libvirt/tests/src/guest_kernel_debugging/virsh_dump.py (4)
69-69: LGTM: Defensive initialization prevents AttributeError.Initializing
backup_xmltoNoneensures the guardedsync()call in the finally block (lines 489-490) won't fail if an exception occurs before line 382.
338-338: LGTM: Fixes race condition with asynchronous wait.Using
utils_misc.wait_for()with a 30-second timeout properly addresses the race condition where the log file might not be fully written before the first check, as mentioned in the PR description.
353-359: LGTM: Improved error handling flow for invalid image format.The refactored logic correctly:
- Waits for the error message via
check_logfile()- Restores config and restarts services regardless of result
- Fails the test only if the expected error message is missing
- Returns early to skip remaining test logic
Note:
qemu_config.restore()will be called again in the finally block (line 491), but this should be safe if the method is idempotent.
489-490: LGTM: Guards against AttributeError.The conditional check prevents
AttributeErrorifbackup_xmlis stillNone(e.g., if an exception occurred before line 382).
| - invalid_option_live: | ||
| dump_options = "--live" |
There was a problem hiding this comment.
Remove trailing whitespace.
The new negative test variant correctly verifies that the obsolete --live option is rejected. However, Line 89 has trailing whitespace after "--live".
Apply this diff to remove the trailing whitespace:
- - invalid_option_live:
- dump_options = "--live"
+ - invalid_option_live:
+ dump_options = "--live"📝 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.
| - invalid_option_live: | |
| dump_options = "--live" | |
| - invalid_option_live: | |
| dump_options = "--live" |
🤖 Prompt for AI Agents
In libvirt/tests/cfg/guest_kernel_debugging/virsh_dump.cfg around lines 88-89,
the line defining dump_options for invalid_option_live contains trailing
whitespace after "--live"; remove the trailing space(s) so the line ends
immediately after the closing quote (no spaces or tabs after "--live"), and save
the file without introducing any other changes.
| - pause_dump: | ||
| paused_after_start_vm = "yes" | ||
| variants: | ||
| - live: |
There was a problem hiding this comment.
Maybe we can keep '--live' cases in tp-libvirt and remove '--live' cases from CI, so that others can still use them for testing.
| dump_options = "--live --reset" | ||
| - invalid_option4: | ||
| dump_options = "--crash --reset" | ||
| - invalid_option_live: |
There was a problem hiding this comment.
Maybe we need to add libvirt version check. For example, func_supported_since_libvirt_ver = (10, 4, 0)
2 participants
The --live flag was removed from virsh dump upstream, so any positive test that still use it now fails immediately. ref: https://gitlab.com/libvirt/libvirt/-/issues/646
Delete all test cases that relied on --live, and add a negative test to make sure the command rejects the obsolete option.
Additionally, fix an unstable race by using utils_misc.wait_for(). Preventing the race condition where the log file had not yet been fully generated when the first check occurred.
Summary by CodeRabbit
Tests
Bug Fixes