migration: Add case about suspend virsh command #6656
Conversation
This case is to verify that if virsh(management app) is suspended during migration, migration will succeed. ID: VIRT-298206 Signed-off-by: lcheng <[email protected]>
WalkthroughThis pull request introduces a new test scenario for destructive operations during live migration. It adds a configuration file defining settings for testing virsh suspension during migration, including precopy and postcopy variants with peer-to-peer and non-peer-to-peer sub-variants. A corresponding Python test script implements the test orchestration, handling migration setup, event monitoring, migration execution, verification of defaults, event output validation, and cleanup. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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/cfg/migration/destructive_operations_around_live_migration/suspend_virsh_during_migration.cfg (1)
18-19: Avoid duplicated migrate_src/dest state defaults
virsh_migrate_dest_stateandvirsh_migrate_src_stateare defined twice at the top level (Lines 18–19 and again at 30–31). Functionally the latter definitions win, but this duplication can be confusing when reading or modifying defaults.You can safely drop the earlier pair (or the later one, if you prefer) to keep a single source of truth.
Also applies to: 30-31
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/migration/destructive_operations_around_live_migration/suspend_virsh_during_migration.cfg(1 hunks)libvirt/tests/src/migration/destructive_operations_around_live_migration/suspend_virsh_during_migration.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/migration/destructive_operations_around_live_migration/suspend_virsh_during_migration.py (2)
provider/migration/base_steps.py (4)
MigrationBase(27-477)setup_connection(343-364)verify_default(309-324)cleanup_connection(366-378)provider/migration/migration_base.py (1)
monitor_event(180-211)
⏰ 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.11
- GitHub Check: Python 3.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
🔇 Additional comments (1)
libvirt/tests/src/migration/destructive_operations_around_live_migration/suspend_virsh_during_migration.py (1)
15-34: Migration orchestration and cleanup look correct
run()wiresMigrationBasesetup, event monitoring, migration execution, verification, and cleanup in a clear sequence, withcleanup_connection()guaranteed viafinally. This matches the expected migration flow and should integrate cleanly with the existing migration helpers.
| virsh_migrate_src_state = "running" | ||
| status_error = "yes" | ||
| - with_postcopy: | ||
| action_during_mig = '[{"func": "virsh.migrate_postcopy", "func_param": "'%s' % params.get('migrate_main_vm')", "after_event": "migration-iteration"}, {"func": "MigrationTest().do_cancel", "func_param": "signal.SIGTSTP", "after_event": "Suspended Post-copy"}]' |
There was a problem hiding this comment.
🧩 Analysis chain
Fix quoting in action_during_mig for with_postcopy
The action_during_mig value here is single‑quoted as a whole and also contains unescaped single quotes in '%s' and params.get('migrate_main_vm'). In the usual VT cfg workflow (where these values are interpreted as Python expressions/strings), this will very likely lead to a syntax/parse error for this scenario, unlike the simpler precopy case on Line 35.
Consider escaping the inner single quotes (or switching the outer string to double quotes) so that the expression remains valid, for example:
- action_during_mig = '[{"func": "virsh.migrate_postcopy", "func_param": "'%s' % params.get('migrate_main_vm')", "after_event": "migration-iteration"}, {"func": "MigrationTest().do_cancel", "func_param": "signal.SIGTSTP", "after_event": "Suspended Post-copy"}]'
+ action_during_mig = '[{"func": "virsh.migrate_postcopy", "func_param": "\'%s\' % params.get(\'migrate_main_vm\')", "after_event": "migration-iteration"}, {"func": "MigrationTest().do_cancel", "func_param": "signal.SIGTSTP", "after_event": "Suspended Post-copy"}]'This keeps the intended '%s' % params.get('migrate_main_vm') expression intact while avoiding conflicts with the outer quotes.
Please rerun the parser/tests for this config after adjusting the quoting to confirm it loads and the postcopy variants execute correctly.
I'll verify this quoting concern by examining the configuration file and understanding how the VT cfg framework processes these strings.
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate and examine the config file
fd "suspend_virsh_during_migration.cfg" --type f -x cat {}
Length of output: 3058
🏁 Script executed:
#!/bin/bash
# Search for similar action_during_mig patterns to understand the pattern
rg -n "action_during_mig\s*=" --type cfg -A 2 -B 2 | head -100Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for VT cfg parser code to understand string handling
fd -e py -e txt | xargs grep -l "cfg.*parse\|parse.*cfg" | grep -i parser | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any escape patterns used elsewhere in cfg files
rg "\\\\'" --type cfg | head -20Length of output: 27
---
🏁 Script executed:
```shell
#!/bin/bash
# Find VT cfg loader/parser code
fd -e py | xargs grep -l "action_during_mig\|load.*cfg\|parse.*cfg" | head -10
Length of output: 501
🏁 Script executed:
#!/bin/bash
# Search for test files that use this config
fd -e py | xargs grep -l "suspend_virsh_during_migration" | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for how action_during_mig values are processed/evaluated
fd -e py | xargs grep -n "action_during_mig" -B 3 -A 3 | head -60Length of output: 4819
🏁 Script executed:
#!/bin/bash
# Check if there's documentation on VT cfg string quoting rules
fd -e md -e txt -e rst | xargs grep -l "cfg\|quoting\|escape" | head -10Length of output: 72
---
🏁 Script executed:
```shell
#!/bin/bash
# Search more broadly for test methods that might use this config
fd -e py -path "*/libvirt/tests/src/migration/*" | xargs grep -l "migrate_postcopy\|postcopy" | head -10
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Try to find the main test that uses suspend_virsh_during_migration config
fd -e py | xargs grep -l "destructive_operations_around_live_migration" 2>/dev/null | head -5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Test the exact syntax error with Python eval
python3 << 'EOF'
# Simulate the problematic line 49 value
line_49_value = '[{"func": "virsh.migrate_postcopy", "func_param": "'%s' % params.get('migrate_main_vm')", "after_event": "migration-iteration"}]'
try:
result = eval(line_49_value)
print("SUCCESS: Parsed without error")
except SyntaxError as e:
print(f"SYNTAX ERROR: {e}")
print(f"Error at position: {e.offset}")
except Exception as e:
print(f"OTHER ERROR ({type(e).__name__}): {e}")
EOFLength of output: 295
🏁 Script executed:
#!/bin/bash
# Test the fixed version with escaped quotes
python3 << 'EOF'
# Test with escaped inner quotes
line_49_fixed = '[{"func": "virsh.migrate_postcopy", "func_param": "\'%s\' % params.get(\'migrate_main_vm\')", "after_event": "migration-iteration"}]'
try:
result = eval(line_49_fixed)
print("SUCCESS: Fixed version parses correctly")
print(f"Result type: {type(result)}, length: {len(result)}")
except Exception as e:
print(f"ERROR ({type(e).__name__}): {e}")
EOFLength of output: 78
🏁 Script executed:
#!/bin/bash
# Also test with double quotes for outer string
python3 << 'EOF'
# Test with double quotes for outer string
line_49_double_quotes = "[{\"func\": \"virsh.migrate_postcopy\", \"func_param\": \"'%s' % params.get('migrate_main_vm')\", \"after_event\": \"migration-iteration\"}]"
try:
result = eval(line_49_double_quotes)
print("SUCCESS: Double-quoted version parses correctly")
print(f"Result type: {type(result)}, length: {len(result)}")
except Exception as e:
print(f"ERROR ({type(e).__name__}): {e}")
EOFLength of output: 86
Fix critical quoting syntax error in action_during_mig
The action_during_mig value on line 49 will cause a SyntaxError when evaluated. The string is enclosed in single quotes but contains unescaped single quotes in '%s' and params.get('migrate_main_vm'), prematurely terminating the outer string literal. When eval(action_during_mig) is called in provider/migration/migration_base.py:55, this will fail at runtime.
Fix by escaping the inner single quotes:
- action_during_mig = '[{"func": "virsh.migrate_postcopy", "func_param": "'%s' % params.get('migrate_main_vm')", "after_event": "migration-iteration"}, {"func": "MigrationTest().do_cancel", "func_param": "signal.SIGTSTP", "after_event": "Suspended Post-copy"}]'
+ action_during_mig = '[{"func": "virsh.migrate_postcopy", "func_param": "\'%s\' % params.get(\'migrate_main_vm\')", "after_event": "migration-iteration"}, {"func": "MigrationTest().do_cancel", "func_param": "signal.SIGTSTP", "after_event": "Suspended Post-copy"}]'Alternatively, switch to double quotes for the outer string to allow the inner single quotes to remain unescaped.
📝 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.
| action_during_mig = '[{"func": "virsh.migrate_postcopy", "func_param": "'%s' % params.get('migrate_main_vm')", "after_event": "migration-iteration"}, {"func": "MigrationTest().do_cancel", "func_param": "signal.SIGTSTP", "after_event": "Suspended Post-copy"}]' | |
| action_during_mig = '[{"func": "virsh.migrate_postcopy", "func_param": "\'%s\' % params.get(\'migrate_main_vm\')", "after_event": "migration-iteration"}, {"func": "MigrationTest().do_cancel", "func_param": "signal.SIGTSTP", "after_event": "Suspended Post-copy"}]' |
🤖 Prompt for AI Agents
libvirt/tests/cfg/migration/destructive_operations_around_live_migration/suspend_virsh_during_migration.cfg
around line 49: the assigned literal for action_during_mig uses single quotes
around the whole string but also contains unescaped single quotes inside (e.g.
'%s' and params.get('migrate_main_vm')), which will raise a SyntaxError when
eval() is called; fix by making the outer string use double quotes (or properly
escape the inner single quotes) so the embedded single-quoted '%s' and
params.get(...) stay intact and the value becomes a valid Python expression for
eval; ensure the final string preserves the inner quoting and is syntactically
correct.
2 participants
This case is to verify that if virsh(management app) is suspended during migration, migration will succeed.
ID: VIRT-298206
Summary by CodeRabbit
Release Notes