migrate: New case that migrate VM with repetition #6738
Conversation
WalkthroughAdds a new Windows-only test configuration file libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg and a new test module libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py. The config defines a migrate_with_repetition test type with NFS-backed storage, increased open-files limit, OVMF kill options, local/remote auth and IPs, migration options (e.g., --live --p2p --auto-converge), post-migration checks, and a TCP variant. The Python module implements run(test, params, env) to perform an initial migration, optional network checks, then repeat forward/back migrations for a configurable count (default 2000) with cleanup on completion. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/migrate_with_ping_pong.py (1)
35-46: Consider catching more specific exceptions.The current implementation catches bare
Exception, which is overly broad. While acceptable in test code for providing clear failure messages across many iterations, catching more specific exception types would improve error handling precision.🔎 View suggested improvement
If the migration framework raises specific exception types, consider catching them explicitly:
except Exception as e: - test.fail(f"Migration failed at iteration {idx + 1}: {str(e)}") + test.fail(f"Migration failed at iteration {idx + 1}: {e!s}")Additionally, if you know the specific exception types raised by MigrationBase methods (e.g.,
migration.MigrationError), consider:except (migration.MigrationError, LibvirtError) as e: test.fail(f"Migration failed at iteration {idx + 1}: {e!s}")Note: Based on static analysis hints (Ruff BLE001, RUF010).
libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfg (1)
22-25: Consider setting ping_pong_repeat_times explicitly.The Python test uses a default of 2000 iterations for
ping_pong_repeat_timesif not specified. For clarity and easier configuration adjustments, consider setting this explicitly in the config file.🔎 Suggested addition:
virsh_migrate_options = "--live --p2p --verbose --auto-converge" ping_vm_after_migration = yes + ping_pong_repeat_times = 2000
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfg(1 hunks)libvirt/tests/src/virtual_network/qemu/migrate_with_ping_pong.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfg
📚 Learning: 2025-11-18T08:47:14.465Z
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfglibvirt/tests/src/virtual_network/qemu/migrate_with_ping_pong.py
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfg
🪛 Ruff (0.14.8)
libvirt/tests/src/virtual_network/qemu/migrate_with_ping_pong.py
45-45: Do not catch blind exception: Exception
(BLE001)
46-46: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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.9
- GitHub Check: Python 3.8
- GitHub Check: Python 3.11
- GitHub Check: Python 3.12
🔇 Additional comments (6)
libvirt/tests/src/virtual_network/qemu/migrate_with_ping_pong.py (4)
1-13: LGTM!The imports, function signature, and decorator usage are appropriate for a tp-libvirt migration test.
14-15: LGTM!VM retrieval and MigrationBase initialization follow standard patterns.
48-49: LGTM!The finally block ensures proper cleanup of the migration connection regardless of test outcome.
24-28: Consider adding parameter validation before ping operation.While
ping_vm_after_migrationis conditionally enabled, passingparams.get("virsh_migrate_desturi")directly toping_vm()without checking if it'sNoneor empty could fail silently or produce unclear errors if the parameter is missing. Either validate the parameter existence or provide a sensible default.libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfg (2)
1-9: Verify the Windows-only restriction is intentional.The configuration restricts this test to Windows guests with
only Windows. Please confirm whether:
- This is a deliberate limitation (e.g., test infrastructure availability, specific Windows migration scenarios)
- The test should eventually support Linux guests
If the restriction is intentional, consider adding a comment explaining the rationale.
27-30: LGTM!The TCP variant configuration uses proper variable substitution and follows tp-libvirt cartesian config patterns.
| local_ip = "${migrate_source_host}" | ||
| local_pwd = "${migrate_source_host}" |
There was a problem hiding this comment.
Fix incorrect password variable assignment.
Line 18 assigns local_pwd to "${migrate_source_host}", which is a hostname, not a password. This will cause authentication failures.
🔎 Apply this diff to fix the password variable:
local_ip = "${migrate_source_host}"
- local_pwd = "${migrate_source_host}"
+ local_pwd = "${migrate_source_pwd}"Note: Line 20 correctly uses ${migrate_dest_pwd}, confirming this pattern.
📝 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.
| local_ip = "${migrate_source_host}" | |
| local_pwd = "${migrate_source_host}" | |
| local_ip = "${migrate_source_host}" | |
| local_pwd = "${migrate_source_pwd}" |
🤖 Prompt for AI Agents
In libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfg around
lines 17 to 18, local_pwd is incorrectly set to the hostname variable
"${migrate_source_host}"; change it to the corresponding source password
variable by replacing the value with "${migrate_source_pwd}" so local_pwd holds
the correct password for the source host.
There was a problem hiding this comment.
@nanli1, thank you for confirming! Please let me know once you've applied the fix so I can mark this as resolved.
🧠 Learnings used
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Learnt from: smitterl
Repo: autotest/tp-libvirt PR: 6072
File: libvirt/tests/src/passthrough/pci/libvirt_pci_passthrough_hotplug.py:239-241
Timestamp: 2025-11-18T08:47:14.465Z
Learning: In tp-libvirt tests, `utils_test.ping` is a valid function that exists in the avocado-vt framework (virttest.utils_test module). It is an alias to `utils_net.ping` and supports parameters including dest, count, interface, timeout, and session.
|
Host_RHEL.m9.u8.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win2016.x86_64.io-github-autotest-libvirt.virtual_network.qemu_test.migrate_with_ping_pong.tcp.q35: PASS (63141.20 s) |
|
Hi @nanli1 @yanglei-rh |
| local_ip = "${migrate_source_host}" | ||
| local_pwd = "${migrate_source_host}" |
libvirt/tests/src/virtual_network/qemu/migrate_with_ping_pong.py
Outdated
Show resolved
Hide resolved
libvirt/tests/src/virtual_network/qemu/migrate_with_ping_pong.py
Outdated
Show resolved
Hide resolved
libvirt/tests/cfg/virtual_network/qemu/migrate_with_ping_pong.cfg
Outdated
Show resolved
Hide resolved
YongxueHong
force-pushed
the
LIBVIRTAT-22191
branch
from
5d1e0c0 to
9ae42e1
Compare
|
Hi @nanli1 @yanglei-rh |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py (1)
35-46: Ping-pong migration logic is correct.The alternating migration pattern correctly implements the ping-pong behavior. After the initial forward migration, the loop alternates between migrating back and forward for the specified iterations.
Optional: Consider more specific exception handling
While catching
Exceptionensures any migration failure is reported, consider catching more specific exceptions if possible, or at least logging the exception type for debugging.Additionally, you can use the
!sconversion flag instead ofstr()for cleaner f-string formatting:- except Exception as e: - test.fail(f"Migration failed at iteration {idx + 1}: {str(e)}") + except Exception as e: + test.fail(f"Migration failed at iteration {idx + 1}: {e!s}")However, in the context of stress testing where any failure should stop the test, the current implementation is acceptable.
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg (1)
30-33: TCP variant configuration is correct.The TCP transport variant is properly configured with appropriate destination URI. The use of variable substitution
${migrate_dest_host}follows cartesian config conventions.Optional: Consider adding more transport variants
While the TCP variant is functional, you may want to consider adding additional migration transport variants in the future, such as:
- SSH transport (qemu+ssh://)
- TLS transport (qemu+tls://)
This would provide broader test coverage for different migration scenarios. However, this is not necessary for the initial implementation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg
🧬 Code graph analysis (1)
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py (2)
provider/guest_os_booting/guest_os_booting_base.py (1)
get_vm(18-56)provider/migration/base_steps.py (3)
MigrationBase(27-477)setup_connection(343-364)cleanup_connection(366-378)
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py
45-45: Do not catch blind exception: Exception
(BLE001)
46-46: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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.8
- GitHub Check: Python 3.9
- GitHub Check: Python 3.12
🔇 Additional comments (9)
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py (6)
1-2: LGTM!The imports are appropriate for the migration test functionality.
5-13: LGTM!The function signature and docstring follow the test framework conventions correctly.
14-15: LGTM!The VM retrieval and MigrationBase initialization follow standard patterns.
17-28: LGTM!The initial migration setup and optional network accessibility check are correctly implemented. The error contexts provide clear logging for test progress tracking.
30-33: LGTM!The repetition setup is straightforward. The default value of 2000 iterations is appropriate for stress testing, and the parameter allows configuration flexibility.
48-49: LGTM!The finally block ensures proper cleanup of migration connections and VM state, even if the test fails.
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg (3)
1-9: LGTM!The test configuration correctly sets up VM lifecycle management and increases the open file limit to handle the many file descriptors created during 2000 migrations. The OVMF nvram cleanup ensures proper state cleanup between runs.
Based on learnings, the
vt_ulimit_nofile = 10240parameter was already explained by the developer as necessary to avoid "Too many open files" errors during repeated migrations.
11-20: LGTM!The NFS storage configuration is correctly set up for shared disk access during migration. The use of variable substitution (e.g.,
${migrate_source_host}) follows the cartesian config parser conventions. The mount options include appropriate timeout and retry settings for resilience.
22-28: LGTM!The migration options are well-suited for repeated live migrations. The
--auto-convergeflag is particularly important for ensuring migrations complete successfully even under load. The post-migration network accessibility check with 5 pings provides basic validation without excessive overhead.
YongxueHong
changed the title
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py
Outdated
Show resolved
Hide resolved
mplement a new test case to validate VM migration using a ping-pong pattern for a specified number of iterations. Signed-off-by: Yongxue Hong <yhong@redhat.com>
YongxueHong
force-pushed
the
LIBVIRTAT-22191
branch
from
9ae42e1 to
3f20641
Compare
|
Hi @nanli1 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py (1)
28-44: Consider adding periodic post-migration checks in the repetition loop.The ping-pong migration logic correctly alternates between source and destination. However, post-migration checks (network accessibility, disk checks) are only performed after the initial migration but not during the 2000 loop iterations. This could mask network or system issues that emerge during repeated migrations.
Consider adding periodic checks (e.g., every Nth iteration) to improve test reliability:
🔎 Suggested enhancement
for idx in range(migrate_repeat_times): try: error_context.context( f"Ping-pong migration iteration: {idx + 1}", test.log.info ) if idx % 2 == 0: migration_obj.run_migration_back() else: migration_obj.run_migration() + + # Perform periodic checks every 100 iterations + if (idx + 1) % 100 == 0: + dest_uri = params.get("virsh_migrate_desturi") if idx % 2 == 1 else None + migration_obj.migration_test.post_migration_check( + [vm], params, dest_uri=dest_uri + ) except Exception as e: - test.fail(f"Migration failed at iteration {idx + 1}: {str(e)}") + test.fail(f"Migration failed at iteration {idx + 1}: {e!s}")Note: Static analysis suggests using
{e!s}instead of{str(e)}for clearer conversion semantics (optional style improvement).libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg (1)
22-28: Consider enabling disk checks for improved coverage.The migration options (--live --p2p --auto-converge) are well-chosen for testing migration resilience, and network accessibility checks are appropriately enabled. However,
simple_disk_check_after_mig = nomay miss storage issues during repeated migrations.Given the long test runtime (~17.5 hours), this may be an intentional tradeoff. However, consider enabling periodic disk checks if storage validation is a test objective.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
📚 Learning: 2025-11-24T10:44:47.801Z
Learnt from: qiankehan
Repo: autotest/tp-libvirt PR: 6652
File: libvirt/tests/cfg/virtual_network/passt/passt_function.cfg:106-106
Timestamp: 2025-11-24T10:44:47.801Z
Learning: In libvirt/tests/cfg/virtual_network/passt/passt_function.cfg, passt test cases have only one interface, so running `dhcpcd` without specifying an interface is safe and doesn't cause network disruption.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfglibvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py
📚 Learning: 2025-12-12T10:00:09.383Z
Learnt from: yanglei-rh
Repo: autotest/tp-libvirt PR: 6675
File: libvirt/tests/cfg/virtual_network/qemu/nic_bonding.cfg:16-16
Timestamp: 2025-12-12T10:00:09.383Z
Learning: In tp-libvirt test config files (*.cfg under libvirt/tests/cfg), the cartesian config parser supports variable substitution using the ${variable_name} syntax. Variables defined in the cfg (e.g., filesize = 4000) are substituted into strings that reference them (e.g., count=${filesize}) before the Python test code receives the parameters. Treat this as a general behavior for these cfg files: define variables early, reference them with ${var}, and verify substitutions in tests to avoid hard-coded literals.
Applied to files:
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg
🪛 Ruff (0.14.10)
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py
43-43: Do not catch blind exception: Exception
(BLE001)
44-44: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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.9
- GitHub Check: Python 3.12
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
🔇 Additional comments (7)
libvirt/tests/src/virtual_network/qemu/migrate_with_repetition.py (4)
1-6: LGTM!The imports and function decorator are appropriate for the migration test framework.
14-15: LGTM!Standard VM and migration object initialization for the test framework.
17-26: LGTM!The initial migration sequence correctly sets up the connection, migrates, and performs post-migration checks. The network accessibility check is properly handled via
post_migration_checkas configured in the cfg file.
46-47: LGTM!The finally block ensures proper cleanup of migration connections regardless of test outcome.
libvirt/tests/cfg/virtual_network/qemu/migrate_with_repetition.cfg (3)
1-9: LGTM!The basic configuration correctly restricts the test to Windows, sets up VM lifecycle management, and configures the ulimit for handling many file descriptors during repeated migrations. The OVMF nvram cleanup option is appropriate for UEFI firmware VMs.
11-20: LGTM!The NFS storage configuration with soft mount options and retry parameters is well-suited for repeated migrations, preventing indefinite hangs while allowing transient failures to be retried. Variable substitution for IPs and credentials follows the correct cartesian config syntax.
30-33: LGTM!The TCP variant correctly configures the migration destination URI using variable substitution. The qemu+tcp protocol is appropriate for network-based migration testing.
3 participants
mplement a new test case to validate VM migration using a ping-pong pattern for a specified number of iterations.
ID: LIBVIRTAT-22191
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.