Skip to content

virsh_migrate_option_mix: Add missing vms parameter to check_result() #6744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hholoubk wants to merge 1 commit into
base: master
Choose a base branch
from
Open

virsh_migrate_option_mix: Add missing vms parameter to check_result() #6744

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ def cleanup_vm(vm, vm_name='', uri=''):
extra_opts=extra_options,
shell=True)
# Check migration result
obj_migration.check_result(obj_migration.ret, params)
obj_migration.check_result(obj_migration.ret, params, vms)
Copy link

@coderabbitai coderabbitai bot Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the specific file and the area around line 313
cd libvirt && find . -name "virsh_migrate_option_mix.py" -type f

Repository: autotest/tp-libvirt

Length of output: 119

🏁 Script executed:

# Search for all calls to check_result() in the codebase to verify consistency
rg -nP --type=py '\.check_result\s*\(' | head -50

Repository: autotest/tp-libvirt

Length of output: 6445

🏁 Script executed:

# Also search for the check_result method definition to understand the expected signature
rg -nP --type=py 'def check_result' | head -20

Repository: autotest/tp-libvirt

Length of output: 2202

🏁 Script executed:

# Read the specific file to see the change at line 313
wc -l libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py

Repository: autotest/tp-libvirt

Length of output: 129

🏁 Script executed:

# Read around line 313 to understand the context
sed -n '300,325p' libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py

Repository: autotest/tp-libvirt

Length of output: 1440

🏁 Script executed:

# Search for the MigrationTest class and its check_result method
rg -nP --type=py 'class.*Migration.*Test' | head -20

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Search specifically for obj_migration.check_result calls
rg -nP --type=py 'obj_migration\.check_result'

Repository: autotest/tp-libvirt

Length of output: 200

🏁 Script executed:

# Find the MigrationTest class definition
rg -nP --type=py 'class MigrationTest'

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Search for check_result method definition in migration-related files
rg -nP --type=py 'def check_result' provider/migration/ libvirt/tests/src/ | grep -i migration

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Read the migration_base.py to find the check_result method
find . -name "migration_base.py" -type f

Repository: autotest/tp-libvirt

Length of output: 101

🏁 Script executed:

# Search for check_result method definition in migration_base.py
rg -nP 'def check_result' ./provider/migration/migration_base.py -A 10

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Also check the imports and class definition in virsh_migrate_option_mix.py to understand obj_migration
head -100 libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py | grep -E '(import|class|obj_migration)'

Repository: autotest/tp-libvirt

Length of output: 514

🏁 Script executed:

# Look for other calls to check_result in virsh_migrate_option_mix.py
rg -nP 'check_result' libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py

Repository: autotest/tp-libvirt

Length of output: 137

🏁 Script executed:

# Let's read the migration_base.py file to find the check_result method
wc -l ./provider/migration/migration_base.py

Repository: autotest/tp-libvirt

Length of output: 105

🏁 Script executed:

# Search for check_result without the 'def' keyword in migration_base.py
rg -nP 'check_result' ./provider/migration/migration_base.py | head -30

Repository: autotest/tp-libvirt

Length of output: 351

🏁 Script executed:

# Look for the class definition in migration_base.py
rg -nP 'class ' ./provider/migration/migration_base.py | head -10

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Let's read the virsh_migrate_option_mix.py file more completely to understand the setup
grep -n 'class\|def\|obj_migration' libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py | head -50

Repository: autotest/tp-libvirt

Length of output: 1731

🏁 Script executed:

# Search for where obj_migration is instantiated
rg -nP 'obj_migration\s*=' libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py

Repository: autotest/tp-libvirt

Length of output: 116

🏁 Script executed:

# Let's search for migration.MigrationTest in the entire file
rg -nP 'migration\.' libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py

Repository: autotest/tp-libvirt

Length of output: 958

🏁 Script executed:

# Search for check_result in migration_base.py with a broader context
sed -n '1,100p' ./provider/migration/migration_base.py | grep -i class

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Search for class MigrationTest in migration_base.py
grep -n 'class MigrationTest' ./provider/migration/migration_base.py

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Search for all check_result definitions (not calls) in migration_base.py
grep -n 'def check_result' ./provider/migration/migration_base.py

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Let's read the migration_base.py file to understand the class structure better
sed -n '1,50p' ./provider/migration/migration_base.py

Repository: autotest/tp-libvirt

Length of output: 2189

🏁 Script executed:

# The MigrationTest is imported from virttest.migration, not from provider/migration
# Let's find where virttest.migration is located
find . -path "*/virttest/migration*" -type f | head -20

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Look for all check_result method definitions across the entire codebase
rg -nP 'def check_result.*\(.*ret.*params.*vms'

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# Let's search more broadly for check_result signatures
rg -B2 -A5 'def check_result' --type=py | grep -A 5 'migration'

Repository: autotest/tp-libvirt

Length of output: 45

🏁 Script executed:

# The MigrationTest class is imported from an external library (virttest)
# Let's search for all files that call obj_migration.check_result to see if there are consistency issues
rg -nP 'obj_migration.*check_result' --type=py

Repository: autotest/tp-libvirt

Length of output: 200

🏁 Script executed:

# Let's also check if there are any other migration test files that might call check_result
find libvirt/tests/src -name "*migrate*" -type f

Repository: autotest/tp-libvirt

Length of output: 1947

🏁 Script executed:

# Read the full section around line 313 in context (300-350) to understand the change better
sed -n '290,350p' libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py

Repository: autotest/tp-libvirt

Length of output: 3147

🏁 Script executed:

# Check if there are any other test files in the migration directory that might use check_result
grep -r 'obj_migration.*check_result\|migration.*check_result' libvirt/tests/src/migration/ libvirt/tests/src/virsh_cmd/domain/ --include="*.py"

Repository: autotest/tp-libvirt

Length of output: 310

🏁 Script executed:

# Let's verify the vms list initialization in the file
sed -n '140,160p' libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py

Repository: autotest/tp-libvirt

Length of output: 625

🏁 Script executed:

# Let's check the migrate_options_shared.py file to see how it uses check_result
grep -n 'migration_test\|check_result' libvirt/tests/src/migration/migrate_options_shared.py | head -20

Repository: autotest/tp-libvirt

Length of output: 733

🏁 Script executed:

# Read the context around the check_result call in migrate_options_shared.py
grep -B 5 -A 5 'check_result' libvirt/tests/src/migration/migrate_options_shared.py

Repository: autotest/tp-libvirt

Length of output: 435

🏁 Script executed:

# Let's see what migration_test is in that file
grep -n 'migration_test\s*=' libvirt/tests/src/migration/migrate_options_shared.py

Repository: autotest/tp-libvirt

Length of output: 114

🏁 Script executed:

# This is a critical finding! Let's verify the difference between the two calls
# In virsh_migrate_option_mix.py: check_result(obj_migration.ret, params, vms)
# In migrate_options_shared.py: check_result(mig_result, params)

# Let's check if there's a version or commit log that explains this change
git log --oneline --all 2>/dev/null | grep -i 'migration\|check_result' | head -20

Repository: autotest/tp-libvirt

Length of output: 431

🏁 Script executed:

# Let's also check if virsh_migrate_option_mix.py was the only file modified in this PR
# by looking at the full git diff
git diff HEAD~1..HEAD --name-only 2>/dev/null || echo "Git history not available"

Repository: autotest/tp-libvirt

Length of output: 88

🏁 Script executed:

# Let's read the migrate_options_shared.py to understand the context better around line 1420
sed -n '1410,1445p' libvirt/tests/src/migration/migrate_options_shared.py

Repository: autotest/tp-libvirt

Length of output: 1819

🏁 Script executed:

# This is critical! The git log shows the upstream API was changed to require vms parameter
# But migrate_options_shared.py still uses the old signature (2 params instead of 3)
# Let's verify if migrate_options_shared.py needs to be updated too

# First, check when migrate_options_shared.py was last modified
git log --oneline -n 1 libvirt/tests/src/migration/migrate_options_shared.py 2>/dev/null || echo "Git history not available"

Repository: autotest/tp-libvirt

Length of output: 431

🏁 Script executed:

# Let's check the full context around the do_migration call in migrate_options_shared.py
# to see if vms is available in that scope
sed -n '1415,1445p' libvirt/tests/src/migration/migrate_options_shared.py

Repository: autotest/tp-libvirt

Length of output: 1497

🏁 Script executed:

# Verify: is there a vms list in migrate_options_shared.py before the check_result call?
grep -n 'vms\s*=' libvirt/tests/src/migration/migrate_options_shared.py | head -20

Repository: autotest/tp-libvirt

Length of output: 90

🏁 Script executed:

# Critical finding! Let's check if there are any other check_result calls in migrate_options_shared.py
# after the vms list is created
grep -n 'check_result' libvirt/tests/src/migration/migrate_options_shared.py

Repository: autotest/tp-libvirt

Length of output: 127

🏁 Script executed:

# Let's see the full section that shows all check_result calls in migrate_options_shared.py
sed -n '1415,1450p' libvirt/tests/src/migration/migrate_options_shared.py | cat -n

Repository: autotest/tp-libvirt

Length of output: 2065

🏁 Script executed:

# Let's confirm the line numbers one more time for migrate_options_shared.py
sed -n '1418,1422p' libvirt/tests/src/migration/migrate_options_shared.py | cat -n

Repository: autotest/tp-libvirt

Length of output: 273

LGTM! Correctly fixes the TypeError by passing the required vms parameter.

The change properly addresses the upstream API signature change (commit 20eb8247e, Dec 16, 2025) by passing the vms list that is initialized at line 151 and populated at line 295. This ensures the check_result() method has access to VM instances for retrieving QEMU logs on migration failure.

However, a related file needs attention: libvirt/tests/src/migration/migrate_options_shared.py:1420 contains an identical check_result() call (migration_test.check_result(mig_result, params)) that still uses the old two-parameter signature and was not updated to include the vms parameter. This file should also be updated to maintain API consistency across the codebase.

🤖 Prompt for AI Agents 
In libvirt/tests/src/migration/migrate_options_shared.py around line 1420, the
call migration_test.check_result(mig_result, params) uses the old two-argument
signature; update it to pass the vms list as the third argument
(migration_test.check_result(mig_result, params, vms)). Ensure the vms variable
referenced is the same list initialized/populated earlier in this module (or
bring it into scope if needed) so check_result() receives the VM instances for
QEMU log retrieval on failure.


# Check "postcopy-active" event after postcopy migration
logging.debug("Check postcopy-active event after postcopy migration")
Expand Down