Add focused VCPU migration test #6771
Conversation
- Create migration_options_vcpu.cfg with 4 VCPU variants (1, 4, 8, 16) - Implement migration_options_vcpu.py with focused test logic - Support precopy migration with bidirectional testing - Verify VCPU count preservation in XML after migration This replaces the vcpu tests from migrate_options_shared with a clean, maintainable implementation focused solely on VCPU migration testing. AI assisted code and commit. Human reviewed. Signed-off-by: hholoubk <[email protected]>
WalkthroughAdds a new libvirt test configuration block Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In `@libvirt/tests/src/migration/migration_options_vcpu.py`:
- Around line 73-74: vm.wait_for_login() returns an SSH session that must be
closed to avoid leaking connections; change the call to capture the returned
session (e.g., session = vm.wait_for_login()), use the session for any
interactions, and ensure you close it when done (session.close()) — preferably
via a context manager or try/finally around the code that uses the session so
the session is always closed even on exceptions.
- Around line 125-131: The current call to RemoteRunner.run with
ignore_status=False makes the subsequent check of ret.exit_status unreachable
because run will raise on non-zero exit; either change the call to
runner.run(cmd, ignore_status=True) and then keep the if ret.exit_status != 0 ->
test.fail(...) check, or remove the redundant if-block and let the exception
from runner.run propagate; update the invocation of RemoteRunner.run and the
associated ret/ret.exit_status usage in migration_options_vcpu.py accordingly
(symbols to locate: RemoteRunner, runner.run, ret, ret.exit_status, test.fail).
- Around line 154-164: The cleanup block should not use the static flag
migrate_vm_back in the condition; change the if to only check the VM runtime
state via libvirt.check_vm_state(vm_name, "running", uri=dest_uri) so you only
attempt migration when the VM is actually remote, and remove migrate_vm_back
from that conditional. Also replace the broad "except Exception as e" with
explicit exception handlers for the remote runner errors (e.g., except
remote.LoginError as e, except remote.LoginTimeoutError as e, and any other
specific RemoteRunner-related exceptions used elsewhere in the codebase) and log
the caught exception details in the existing logging.warning call to preserve
context.
| # Wait for VM to be ready | ||
| vm.wait_for_login() |
There was a problem hiding this comment.
Close the login session to avoid resource leak.
vm.wait_for_login() returns a session object that should be closed after use to release the SSH connection.
Proposed fix
# Wait for VM to be ready
- vm.wait_for_login()
+ session = vm.wait_for_login()
+ session.close()📝 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.
| # Wait for VM to be ready | |
| vm.wait_for_login() | |
| # Wait for VM to be ready | |
| session = vm.wait_for_login() | |
| session.close() |
🤖 Prompt for AI Agents
In `@libvirt/tests/src/migration/migration_options_vcpu.py` around lines 73 - 74,
vm.wait_for_login() returns an SSH session that must be closed to avoid leaking
connections; change the call to capture the returned session (e.g., session =
vm.wait_for_login()), use the session for any interactions, and ensure you close
it when done (session.close()) — preferably via a context manager or try/finally
around the code that uses the session so the session is always closed even on
exceptions.
| # If VM is on remote, migrate it back first | ||
| if migrate_vm_back or libvirt.check_vm_state(vm_name, "running", uri=dest_uri): | ||
| try: | ||
| runner = remote.RemoteRunner(host=server_ip, | ||
| username=server_user, | ||
| password=server_pwd) | ||
| src_uri = params.get("virsh_migrate_connect_uri", "qemu:///system") | ||
| cmd = "virsh migrate %s %s --live" % (vm_name, src_uri) | ||
| runner.run(cmd, ignore_status=True) | ||
| except Exception as e: | ||
| logging.warning("Failed to migrate VM back during cleanup: %s", e) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the RemoteRunner class definition and related files
find . -name "*.py" -type f | xargs grep -l "class RemoteRunner" | head -5Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Search for RemoteRunner and exception patterns
rg "class RemoteRunner" -A 30 --type pyRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Look for exception definitions in remote module
fd -e py "remote" | xargs grep -l "class.*Error\|class.*Exception" 2>/dev/null | head -10Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Check what exceptions are raised in remote.py or similar
rg "raise\s+" --type py -B 2 -A 2 | grep -A 5 -B 5 "RemoteRunner\|remote\.py" | head -50Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Find the test file to understand migrate_vm_back semantics
cat -n libvirt/tests/src/migration/migration_options_vcpu.py | sed -n '100,170p'Repository: autotest/tp-libvirt
Length of output: 3688
🏁 Script executed:
# Search for RemoteRunner in the repository
rg "class RemoteRunner" -A 50 --type py | head -100Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Search for exception definitions in remote module
rg "class.*Error|class.*Exception" --type py | grep -i remote | head -20Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Look for where RemoteRunner is imported from
rg "from.*remote|import.*remote" --type py | grep -v "test" | head -20Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Search for RemoteRunner usage to see what exceptions are caught
rg "RemoteRunner" -B 5 -A 10 --type py | grep -A 10 "except" | head -40Repository: autotest/tp-libvirt
Length of output: 2990
🏁 Script executed:
# Look for remote module location
find . -name "remote.py" -type fRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Check imports at the top of the test file
head -50 libvirt/tests/src/migration/migration_options_vcpu.py | grep -E "^import|^from"Repository: autotest/tp-libvirt
Length of output: 229
🏁 Script executed:
# Search for remote module definition
find . -path "*/virttest/*" -name "remote.py" -o -path "*/virttest/*" -name "remote" -type dRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Look for exception definitions related to RemoteRunner
rg "LoginTimeoutError|LoginError" --type py -B 2 -A 2Repository: autotest/tp-libvirt
Length of output: 50375
🏁 Script executed:
# Check what other tests catch when using RemoteRunner
rg "RemoteRunner" -B 2 -A 15 --type py | grep -B 10 -A 5 "except"Repository: autotest/tp-libvirt
Length of output: 4146
🏁 Script executed:
# Look at resource_abnormal.py to see the context of LoginTimeoutError
cat -n libvirt/tests/src/resource_abnormal.py | sed -n '1,50p'Repository: autotest/tp-libvirt
Length of output: 1805
Remove migrate_vm_back from cleanup condition and catch specific exceptions.
Two issues:
-
Logic bug: The cleanup condition
migrate_vm_back or libvirt.check_vm_state(...)is incorrect. Sincemigrate_vm_backis a static config flag never modified, if back-migration succeeded earlier (lines 112-133), the VM is already on source. But this condition still evaluates to True, triggering an unnecessary second migration attempt. Check only the VM state. -
Overly broad exception handling: Catching bare
Exceptionmasks specific errors.RemoteRunnerraisesLoginError,LoginTimeoutError, and similar specific exceptions that should be caught explicitly, consistent with patterns throughout the codebase.
Proposed fix
# If VM is on remote, migrate it back first
- if migrate_vm_back or libvirt.check_vm_state(vm_name, "running", uri=dest_uri):
+ if libvirt.check_vm_state(vm_name, "running", uri=dest_uri):
try:
runner = remote.RemoteRunner(host=server_ip,
username=server_user,
password=server_pwd)
src_uri = params.get("virsh_migrate_connect_uri", "qemu:///system")
cmd = "virsh migrate %s %s --live" % (vm_name, src_uri)
runner.run(cmd, ignore_status=True)
- except Exception as e:
+ except (remote.LoginError, remote.LoginTimeoutError) as e:
logging.warning("Failed to migrate VM back during cleanup: %s", e)🧰 Tools
🪛 Ruff (0.14.13)
163-163: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@libvirt/tests/src/migration/migration_options_vcpu.py` around lines 154 -
164, The cleanup block should not use the static flag migrate_vm_back in the
condition; change the if to only check the VM runtime state via
libvirt.check_vm_state(vm_name, "running", uri=dest_uri) so you only attempt
migration when the VM is actually remote, and remove migrate_vm_back from that
conditional. Also replace the broad "except Exception as e" with explicit
exception handlers for the remote runner errors (e.g., except remote.LoginError
as e, except remote.LoginTimeoutError as e, and any other specific
RemoteRunner-related exceptions used elsewhere in the codebase) and log the
caught exception details in the existing logging.warning call to preserve
context.
Replace manual migration implementation with provider.migration.base_steps.MigrationBase class to improve maintainability and consistency with other migration tests. Changes: - Use MigrationBase for standard migration operations (setup, migration, cleanup) - Move vcpu_num validation outside try block to fail early if not configured - Extract VCPU verification logic into separate helper functions - Remove manual XML backup/restore, VM startup, and RemoteRunner code - Remove unused imports (remote, libvirt) - Reduce code size from 173 to 143 lines while maintaining functionality AI assisted code and commit. Human reviewed. Signed-off-by: hholoubk <[email protected]>
The migrate_main_vm parameter is automatically set to ${main_vm} in the
avocado-vt base configuration (avocado-vt/virttest/shared/cfg/base.cfg:770),
so the fallback to params.get('main_vm') is unnecessary.
This change aligns the code with the standard pattern used in other migration
tests and provider/migration/base_steps.py.
AI assisted code and commit. Human reviewed.
Signed-off-by: hholoubk <[email protected]>
1 participant
This replaces the vcpu tests from migrate_options_shared with a clean, maintainable implementation focused solely on VCPU migration testing.
AI assisted code and commit. Human reviewed.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.