virsh_migrate_option_mix: Add missing vms parameter to check_result() #6744
Conversation
Fix TypeError by adding the required vms parameter to the check_result() method call. The upstream avocado-vt migration.MigrationTest.check_result() method signature was updated in commit 20eb8247e (Dec 16, 2025) to include the vms parameter for retrieving QEMU logs on migration failure. The vms list is already initialized (line 151) and populated with the VM instance (line 295), so this change simply passes it to check_result(). AI assisted code and commit. Human reviewed. Signed-off-by: hholoubk <hholoubk@redhat.com>
WalkthroughA method call to Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/virsh_cmd/domain/virsh_migrate_option_mix.py
⏰ 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.12
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.11
| shell=True) | ||
| # Check migration result | ||
| obj_migration.check_result(obj_migration.ret, params) | ||
| obj_migration.check_result(obj_migration.ret, params, vms) |
There was a problem hiding this comment.
🧩 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 fRepository: 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 -50Repository: 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 -20Repository: 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.pyRepository: 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.pyRepository: 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 -20Repository: 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 migrationRepository: 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 fRepository: 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 10Repository: 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.pyRepository: 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.pyRepository: 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 -30Repository: 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 -10Repository: 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 -50Repository: 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.pyRepository: 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.pyRepository: 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 classRepository: 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.pyRepository: 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.pyRepository: 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.pyRepository: 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 -20Repository: 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=pyRepository: 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 fRepository: 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.pyRepository: 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.pyRepository: 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 -20Repository: 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.pyRepository: 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.pyRepository: 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 -20Repository: 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.pyRepository: 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.pyRepository: 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 -20Repository: 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.pyRepository: 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 -nRepository: 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 -nRepository: 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.
1 participant
Fix TypeError by adding the required vms parameter to the check_result() method call. The upstream avocado-vt migration.MigrationTest.check_result() method signature was updated in commit 20eb8247e (Dec 16, 2025) to include the vms parameter for retrieving QEMU logs on migration failure.
The vms list is already initialized (line 151) and populated with the VM instance (line 295), so this change simply passes it to check_result().
AI assisted code and commit. Human reviewed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.