fix sriov vfio variant driver vf pci param #6615
Conversation
Refactor provider infrastructure to use the new recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). Modified files: - provider/save/save_base.py: pre_save_setup() and post_save_check() - provider/migration/base_steps.py: PrepareVMTest and VerifyTest classes - provider/migration/migration_base.py: poweroff_vm_on_dest() and do_io_error_on_dest() - provider/interface/check_points.py: check_vm_network_accessed() Depends on: avocado-framework/avocado-vt#4248 Signed-off-by: hholoubk <[email protected]>
Refactor all SR-IOV tests to use recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This eliminates code duplication and makes the intent clearer across all SR-IOV test categories: vIOMMU, VM lifecycle, device plug/unplug, failover scenarios, and VFIO variant driver tests. Modified test categories: - vIOMMU tests (5 files): Device settings, DMA translation, address width, driver reload, hotplug - VM lifecycle tests (5 files): Suspend/resume, reboot, managedsave, IOMMU+hostdev, exclusive check - Plug/unplug tests (4 files): Released hostdev, interface/device attach/detach with flags, VFIO variant - Failover tests (3 files): Migration, lifecycle operations, attach/detach hostdev - VFIO tests (1 file): Migration with VFIO variant driver Total: 18 files modified, cleaner code with single-line console recreation. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
Refactor all migration tests to use the recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This simplifies serial console management when switching connection URIs during migration operations. The pattern is especially useful after migrating VMs to remote hosts where the serial console needs to be recreated to match the new connection context. Modified test categories: - Memory migration tests: migrate_mem.py - Network migration tests: migrate_network.py (3 occurrences) - Migration with panic device: migrate_with_panic_device.py - Migration with virtiofs: internally/externally launched virtiofs devices (2 files) - Migration with vTPM: shared_tpm, vtpm_dev, external_tpm (3 files) All tests now use a cleaner pattern: vm.connect_uri = dest_uri vm_session = vm.wait_for_serial_login(timeout=240, recreate_serial_console=True) Total: 8 files modified with 31 fewer lines of console management code. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
Refactor network and interface tests to use the recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This simplifies console management in scenarios where the console needs to be recreated after state changes such as: - After libvirtd restart - After VM save/restore or suspend/resume operations - After VM start or device hotplug Modified test files: - virtual_network/iface_bridge.py: Console recreation after libvirtd restart - virtual_network/passt/passt_lifecycle.py: Console after restore/resume - virtual_interface/interface_update_device_negative.py: Console after VM start - virsh_cmd/domain/virsh_domif_setlink_getlink.py: Console for link state check - virtio/virtio_page_per_vq.py: Console after device attach All tests now use a cleaner single-line pattern for console recreation. Note: iface_hotplug.py was not modified as it creates console without immediately calling wait_for_serial_login(), which doesn't match the refactoring pattern. Total: 5 files modified with 9 fewer lines of console management code. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
Refactor snapshot tests to use the recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This simplifies console management after snapshot revert operations, which require console recreation since the VM state may have changed. Modified test files: - snapshot/revert_snapshot_after_xml_updated.py: Console after VM start/revert - snapshot/revert_disk_external_snap.py: Console after snapshot revert Both tests handle console recreation after reverting to snapshots, where the VM state is restored and the old console session is no longer valid. Total: 2 files modified with 4 fewer lines of console management code. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
Refactor virtio test to use the recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This simplifies console management after VM start and device hotplug operations in virtio page_per_vq testing. Modified test file: - virtio/virtio_page_per_vq.py: Console after VM start/device hotplug The test handles console recreation after VM initialization where the console needs to be properly set up for testing virtio device attributes. Total: 1 file modified with 2 fewer lines of console management code. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
…n tests Refactor migration with copy storage tests to use the recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This simplifies console management when connecting to the migrated VM on the destination host after switching the connection URI. Modified test files: - migration_with_copy_storage/migration_with_backingchain.py: Console to remote host - migration_with_copy_storage/disk_io_load_in_vm.py: Console to remote host after migration Both tests handle console recreation when switching from source to destination host connection, where the old console must be cleaned up and recreated with the new URI. Total: 2 files modified with 4 fewer lines of console management code. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
Refactor nwfilter binding list test to use the recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This simplifies console management before interface detach operations. Modified test file: - nwfilter/nwfilter_binding_list.py: Console recreation before interface detach (also fixed bug: .close -> .close()) Total: 1 file modified with 2 fewer lines of console management code. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
Refactor BIOS boot order tests to use the recreate_serial_console parameter instead of manually calling cleanup_serial_console() and create_serial_console() before wait_for_serial_login(). This simplifies console management after VM boot operations, ensuring proper console state for various boot scenarios including OVMF and SeaBIOS. Modified test files: - bios/boot_order_ovmf.py: Console after VM start for OVMF boot testing - bios/boot_order_seabios.py: Console after VM start for SeaBIOS boot testing Both tests handle console recreation after VM start operations where the console needs to be reset to properly capture boot messages and login prompts. Total: 2 files modified with 9 fewer lines of console management code. Depends on: avocado-framework/avocado-vt#4248 Depends on: autotest#6596 Signed-off-by: hholoubk <[email protected]>
…ole-migration', 'refactor-serial-console-network', 'refactor-serial-console-snapshot', 'refactor-serial-console-bios', 'refactor-serial-console-migration-storage', 'refactor-serial-console-virtio' and 'refactor-serial-console-misc' into test-all-serial-console-changes
The local parse_iface_dict() function had incorrect parameter name (pf_pci instead of vf_pci), causing vf_pci to be undefined and resulting in error: "readlink /sys/bus/pci/devices/None/virtfn0". This change: - Removes duplicate parse_iface_dict() function - Uses existing sriov_vfio.parse_iface_dict() provider function - Removes unused utils_net import AI assisted code and commit. Human reviewed. Signed-off-by: hholoubk <[email protected]>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRefactors many tests to replace explicit serial console cleanup/creation with a single vm.wait_for_serial_login(..., recreate_serial_console=True) call. A few cases add session.close() after use and minor utility changes. No public signatures changed in tests; the VM API is used with the new recreate_serial_console parameter. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant VM
participant SerialConsole as Serial Console
Tester->>VM: wait_for_serial_login(timeout, recreate_serial_console=True)
alt recreate_serial_console=True
VM->>SerialConsole: cleanup existing (if any)
VM->>SerialConsole: create/attach new
end
VM->>SerialConsole: open session, wait for login
SerialConsole-->>VM: session handle
VM-->>Tester: session
note right of VM: Explicit cleanup/create calls removed at call sites
sequenceDiagram
autonumber
actor Tester
participant VM
participant SerialConsole as Serial Console
rect rgba(230,240,255,0.5)
note over Tester,SerialConsole: Old flow (removed)
Tester->>VM: cleanup_serial_console()
Tester->>VM: create_serial_console()
Tester->>VM: wait_for_serial_login(timeout)
end
rect rgba(230,255,230,0.5)
note over Tester,SerialConsole: New flow
Tester->>VM: wait_for_serial_login(timeout, recreate_serial_console=True)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.py (1)
55-61: Close the previous session before creating a new one.Line 55 creates a session that is reused until line 61, where a new session is created via another
wait_for_serial_logincall. The previous session should be closed before creating the new one to avoid resource leaks.Apply this diff to close the previous session:
test.log.info("TEST_STEP3: Reboot the vm") virsh.reboot(vm.name, debug=True, ignore_status=False) + vm_session.close() vm_session = vm.wait_for_serial_login(timeout=240)libvirt/tests/src/sriov/failover/sriov_failover_migration.py (1)
1-122: Critical inconsistency: PR objectives don't match file changes.The PR title and description claim to fix
parse_iface_dict()parameter issues (pf_pcivsvf_pci) and remove unusedutils_netimports. However, this file contains none of those changes:
- No local
parse_iface_dict()function exists (lines 53, 67 call methods on SRIOV objects)- No
utils_netimport to remove- The actual change adds
recreate_serial_console=True(line 75), which isn't mentioned in the PR objectivesThis suggests either:
- The wrong file was included in this PR
- The PR description is incorrect or outdated
- The intended changes exist in other files not provided for review
Please verify this file should be part of PR #6615 and clarify the relationship between the stated objectives and the serial console refactoring.
🧹 Nitpick comments (8)
libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_suspend_resume.py (1)
32-32: Inconsistency between PR objectives and actual code changes.The PR objectives describe fixing a
parse_iface_dict()bug wherepf_pciwas incorrectly used instead ofvf_pci, causing "readlink /sys/bus/pci/devices/None/virtfn0" errors. However, the only change visible in this file is adding therecreate_serial_console=Trueparameter towait_for_serial_login(), which is unrelated to the described bug fix.Please verify that the correct changes are included in this PR, or update the PR description to accurately reflect the serial console refactoring being performed.
Minor: Consider explicitly closing the session.
While the current implementation is likely acceptable for test scripts, as a best practice, explicitly close
vm_sessionafter use (e.g., after line 53 in the check_points call) to prevent potential resource leaks.Apply this diff to close the session:
else: br_name = None if test_scenario == 'failover': br_name = br_dict['source'].get('bridge') check_points.check_vm_network_accessed(vm_session, tcpdump_iface=br_name, tcpdump_status_error=True) + vm_session.close()libvirt/tests/src/sriov/vIOMMU/viommu_unload_driver.py (1)
130-136: Add session cleanup for resource management.The
vm_session(created at line 67) andvm_serial(created at line 114) are not explicitly closed in thefinallyblock, which could lead to resource leaks.Apply this diff to add proper session cleanup:
finally: test.log.info("TEST_TEARDOWN: Cleanup the env.") + if 'vm_serial' in locals(): + vm_serial.close() + if 'vm_session' in locals(): + vm_session.close() for thread in threads: thread.join() for copied_file in host_file_paths: process.system("rm -rf %s" % copied_file) test_obj.teardown_iommu_test()libvirt/tests/src/sriov/vIOMMU/intel_iommu_with_dma_translation.py (1)
35-35: Consider closing the session explicitly.The addition of
recreate_serial_console=Trueconsolidates serial console lifecycle management, which aligns with the broader refactoring pattern described in the AI summary. However,vm_sessionis used on lines 36, 37, and 43 but never explicitly closed, which could lead to resource leaks if the test fails before teardown.Consider adding
vm_session.close()before thefinallyblock or using a context manager if supported:if with_more_vcpus: s, o = vm_session.cmd_status_output("cat /proc/cpuinfo | grep processor| wc -l") if int(o) != int(guest_vcpus): test.fail("Can't get expected vCPU number, the actual number is %s" % o) + vm_session.close() finally: test_obj.teardown_iommu_test()Note: The PR description mentions fixing sriov vfio parameter issues (parse_iface_dict, pf_pci vs vf_pci), but this file only contains serial console changes. This may indicate that the main fixes are in other files not included in this review subset.
libvirt/tests/src/sriov/failover/sriov_failover_lifecycle.py (2)
68-68: Consider adding explicit timeout for consistency.Lines 32 specifies
timeout=240while line 68 omits the timeout parameter. While relying on the default timeout may be acceptable, explicitly specifying the timeout improves consistency and makes the intent clearer.Consider applying this diff:
- vm_session = vm.wait_for_serial_login(recreate_serial_console=True) + vm_session = vm.wait_for_serial_login(timeout=240, recreate_serial_console=True)
79-79: Consider adding explicit timeout for consistency.Line 32 specifies
timeout=240while line 79 omits the timeout parameter. While relying on the default timeout may be acceptable, explicitly specifying the timeout improves consistency and makes the intent clearer.Consider applying this diff:
- vm_session = vm.wait_for_serial_login(recreate_serial_console=True) + vm_session = vm.wait_for_serial_login(timeout=240, recreate_serial_console=True)libvirt/tests/src/sriov/vIOMMU/intel_iommu_aw_bits.py (1)
31-33: Align PR scope or split changes
The PR description targets avf_pcifix inparse_iface_dict(), yet this change refactors serial-console handling. Either split the serial-console refactoring into its own PR or update the description to cover both.libvirt/tests/src/sriov/plug_unplug/sriov_attach_released_hostdev.py (1)
42-48: Close the old session before reassignment to prevent resource leak.When
test_scenario == "to_2nd_vm", thevm_sessionvariable is reassigned at line 45 without closing the previous session created at line 30. This could leave the first session's resources unclosed.Apply this diff to properly close the old session:
if test_scenario == "to_2nd_vm": test_vm = vm_list[1] libvirt_vmxml.remove_vm_devices_by_type(test_vm, 'interface') + vm_session.close() vm_session = get_vm_session(test_vm) else: test_vm = vmlibvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py (1)
54-56: Consider reusing the parse_iface_dict() result.Line 54 calls
sroiv_test_obj.parse_iface_dict()and stores the result iniface_dicts, but line 56 calls the same method again. Ifparse_iface_dict()is expensive or has side effects, this redundancy is wasteful.Apply this diff to reuse the result:
if need_sriov: iface_dicts = sroiv_test_obj.parse_iface_dict() test.log.debug(iface_dicts) - test_obj.params["iface_dict"] = str(sroiv_test_obj.parse_iface_dict()) + test_obj.params["iface_dict"] = str(iface_dicts) iface_dict = test_obj.parse_iface_dict()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
libvirt/tests/src/bios/boot_order_ovmf.py(1 hunks)libvirt/tests/src/bios/boot_order_seabios.py(1 hunks)libvirt/tests/src/migration/migrate_mem.py(1 hunks)libvirt/tests/src/migration/migrate_network.py(3 hunks)libvirt/tests/src/migration/migrate_with_panic_device.py(1 hunks)libvirt/tests/src/migration/migration_with_virtiofs/migration_with_externally_launched_virtiofs_dev.py(1 hunks)libvirt/tests/src/migration/migration_with_virtiofs/migration_with_internally_launched_virtiofs_dev.py(1 hunks)libvirt/tests/src/migration/migration_with_vtpm/migration_with_external_tpm.py(1 hunks)libvirt/tests/src/migration/migration_with_vtpm/migration_with_shared_tpm.py(1 hunks)libvirt/tests/src/migration/migration_with_vtpm/migration_with_vtpm_dev.py(1 hunks)libvirt/tests/src/migration_with_copy_storage/disk_io_load_in_vm.py(1 hunks)libvirt/tests/src/migration_with_copy_storage/migration_with_backingchain.py(1 hunks)libvirt/tests/src/nwfilter/nwfilter_binding_list.py(1 hunks)libvirt/tests/src/snapshot/revert_disk_external_snap.py(1 hunks)libvirt/tests/src/snapshot/revert_snapshot_after_xml_updated.py(1 hunks)libvirt/tests/src/sriov/failover/sriov_failover_at_dt_hostdev.py(1 hunks)libvirt/tests/src/sriov/failover/sriov_failover_lifecycle.py(3 hunks)libvirt/tests/src/sriov/failover/sriov_failover_migration.py(1 hunks)libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py(2 hunks)libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_with_flags.py(1 hunks)libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_interface_with_flags.py(1 hunks)libvirt/tests/src/sriov/plug_unplug/sriov_attach_released_hostdev.py(1 hunks)libvirt/tests/src/sriov/vIOMMU/hotplug_device_with_iommu_enabled.py(1 hunks)libvirt/tests/src/sriov/vIOMMU/intel_iommu_aw_bits.py(1 hunks)libvirt/tests/src/sriov/vIOMMU/intel_iommu_with_dma_translation.py(1 hunks)libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py(1 hunks)libvirt/tests/src/sriov/vIOMMU/viommu_unload_driver.py(1 hunks)libvirt/tests/src/sriov/vfio/sriov_migration_vfio_variant_driver.py(1 hunks)libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_exclusive_check_running_domain.py(1 hunks)libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_iommu_at_dt_hostdev.py(1 hunks)libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_managedsave.py(1 hunks)libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_reboot.py(1 hunks)libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_suspend_resume.py(1 hunks)libvirt/tests/src/virsh_cmd/domain/virsh_domif_setlink_getlink.py(1 hunks)libvirt/tests/src/virtio/virtio_page_per_vq.py(1 hunks)libvirt/tests/src/virtual_interface/interface_update_device_negative.py(1 hunks)libvirt/tests/src/virtual_network/iface_bridge.py(1 hunks)libvirt/tests/src/virtual_network/passt/passt_lifecycle.py(1 hunks)provider/interface/check_points.py(1 hunks)provider/migration/base_steps.py(2 hunks)provider/migration/migration_base.py(2 hunks)provider/save/save_base.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (1)
provider/sriov/sriov_vfio.py (1)
parse_iface_dict(17-39)
🪛 Ruff (0.14.0)
libvirt/tests/src/bios/boot_order_seabios.py
274-274: Do not catch blind exception: Exception
(BLE001)
275-275: Use explicit conversion flag
Replace with conversion flag
(RUF010)
libvirt/tests/src/bios/boot_order_ovmf.py
27-27: Do not catch blind exception: Exception
(BLE001)
⏰ 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.8
- GitHub Check: Python 3.11
🔇 Additional comments (50)
libvirt/tests/src/snapshot/revert_disk_external_snap.py (1)
41-41: LGTM! Clean refactor for serial console handling.The use of
recreate_serial_console=Trueappropriately consolidates serial console cleanup and recreation after the snapshot revert. This is correct because the VM state changes after reverting, requiring a fresh serial console connection.However, there's a significant inconsistency: The PR objectives describe fixing "sriov vfio variant driver vf pci param" issues and removing duplicate
parse_iface_dict()functions, but this file is a snapshot reversion test with no relation to SR-IOV networking. This suggests either:
- The PR objectives/title are incorrect for this PR
- This file was included in the wrong PR
- Multiple unrelated changes were bundled together
Please verify this file belongs in this PR.
libvirt/tests/src/sriov/vIOMMU/viommu_unload_driver.py (2)
1-136: Inconsistency: PR objectives do not match the actual changes shown.The PR objectives describe:
- Fixing an incorrect parameter name (
pf_pciinstead ofvf_pci) in a localparse_iface_dict()function- Removing a duplicate
parse_iface_dict()function- Removing an unused
utils_netimportHowever, the actual changes shown only include:
- Line 114: Serial console refactoring with
recreate_serial_console=TrueAdditionally:
utils_netis still imported (line 9) and actively used (line 71)- No local
parse_iface_dict()function is visible in this fileparse_iface_dict()is called ontest_obj(line 59), which is aviommu_base.VIOMMUTestinstanceThis suggests the PR objectives may be describing changes in a different file not included in this review.
114-114: LGTM: Serial console refactoring is appropriate.The use of
recreate_serial_console=Trueconsolidates serial console management into a single call, replacing the previous pattern of explicit cleanup and recreation. This aligns with the broader test suite pattern described in the AI summary.libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_exclusive_check_running_domain.py (1)
33-33: LGTM! Console refactoring aligns with test suite standardization.The addition of
recreate_serial_console=Trueproperly simplifies serial console management by consolidating recreation into the login call, as described in the AI summary. This eliminates the need for manual cleanup/creation steps.Note: The PR description mentions fixing a
parse_iface_dict()parameter issue (pf_pci vs vf_pci), but those changes are not visible in this file. They likely exist in other files within the PR.libvirt/tests/src/migration_with_copy_storage/disk_io_load_in_vm.py (1)
58-58: LGTM!The refactoring correctly consolidates serial console recreation into the login call. The session is properly closed after use.
libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_managedsave.py (1)
28-28: LGTM!The refactoring correctly consolidates serial console recreation into the login call, and the session is immediately closed as it's only needed for verification.
libvirt/tests/src/bios/boot_order_seabios.py (2)
273-273: LGTM!The refactoring correctly consolidates serial console recreation into the login call in the success path.
280-280: LGTM!The refactoring correctly consolidates serial console recreation into the login call in the error path. This call is expected to raise
LoginTimeoutErroras part of the negative test scenario.libvirt/tests/src/migration/migrate_with_panic_device.py (1)
60-60: LGTM!The refactoring correctly consolidates serial console recreation into the login call. The session is properly closed after use on line 72.
libvirt/tests/src/sriov/vfio/sriov_migration_vfio_variant_driver.py (1)
36-36: Verify that the default timeout is acceptable.The refactoring correctly consolidates serial console recreation into the login call. However, no timeout is specified, unlike other similar changes in this PR (e.g., lines 58 in disk_io_load_in_vm.py uses timeout=360, line 55 in sriov_vm_lifecycle_reboot.py uses timeout=240). Ensure that the default timeout is appropriate for this migration scenario.
provider/interface/check_points.py (1)
24-24: LGTM!The refactoring correctly consolidates serial console recreation into the login call. Note that no timeout is specified (using default) and the session is not explicitly closed in this function, which appears intentional as the caller may be responsible for session lifecycle management.
libvirt/tests/src/virtual_interface/interface_update_device_negative.py (1)
27-28: LGTM!The refactoring correctly consolidates serial console recreation into the login call with a parameterized timeout. The session is properly closed immediately after creation.
libvirt/tests/src/sriov/failover/sriov_failover_lifecycle.py (1)
32-32: LGTM: Serial console recreation during login.The change to use
recreate_serial_console=Trueconsolidates serial console lifecycle management into the login call, removing the need for explicit cleanup/creation steps.libvirt/tests/src/virsh_cmd/domain/virsh_domif_setlink_getlink.py (1)
260-262: LGTM: Proper serial console recreation with credentials.The change correctly consolidates serial console lifecycle management into the login call while preserving the username and password parameters needed for authentication.
libvirt/tests/src/migration/migration_with_virtiofs/migration_with_externally_launched_virtiofs_dev.py (1)
79-79: LGTM: Serial console recreation in verify step.The change correctly consolidates serial console lifecycle management into the login call with an appropriate timeout for the post-migration verification phase.
libvirt/tests/src/nwfilter/nwfilter_binding_list.py (1)
123-123: LGTM: Efficient boot wait pattern.The chained
.close()call correctly creates and immediately closes the session, ensuring the guest has booted while properly cleaning up resources. This is an efficient pattern for boot verification without maintaining an open session.libvirt/tests/src/virtio/virtio_page_per_vq.py (1)
102-102: LGTM: Serial console recreation in run_test.The change correctly consolidates serial console lifecycle management into the login call, and the returned session is properly used for subsequent network checks and then closed.
libvirt/tests/src/migration/migration_with_vtpm/migration_with_external_tpm.py (1)
58-68: LGTM: Proper session lifecycle management.The changes correctly:
- Use
recreate_serial_console=Trueto consolidate console creation during login- Explicitly close the session after command execution (line 62)
- Perform additional cleanup for the remote case after restoring the connection URI
The mixed approach (automated recreation + manual cleanup) is intentional and appropriate for handling the remote connection context.
libvirt/tests/src/migration/migration_with_vtpm/migration_with_shared_tpm.py (1)
74-84: LGTM: Consistent session lifecycle management.The changes correctly follow the same pattern as migration_with_external_tpm.py:
- Use
recreate_serial_console=Truefor console creation during login- Explicitly close the session after command execution
- Perform additional cleanup for the remote case after restoring the connection URI
This consistent approach across migration tests ensures reliable serial console handling.
libvirt/tests/src/migration_with_copy_storage/migration_with_backingchain.py (1)
151-162: LGTM: Proper serial console handling with explicit cleanup.The changes correctly use
recreate_serial_console=Truefor the initial login (line 151) while retaining explicit cleanup at line 161 after all remote operations complete. This ensures proper resource cleanup before restoring the connection URI, which is appropriate for this migration verification flow.libvirt/tests/src/bios/boot_order_ovmf.py (1)
26-26: LGTM: Serial console refactor is correct.The consolidation of serial console cleanup/creation into a single
wait_for_serial_login(recreate_serial_console=True)call simplifies the code and centralizes console lifecycle management.Also applies to: 33-33
libvirt/tests/src/migration/migrate_mem.py (1)
282-282: LGTM: Serial console recreation is correct.Using
recreate_serial_console=Trueproperly handles console lifecycle after migration back, simplifying the code.provider/save/save_base.py (1)
21-21: LGTM: Console lifecycle simplification is correct.Consolidating serial console recreation into the login step in both
pre_save_setupandpost_save_checkimproves code clarity.Also applies to: 49-49
libvirt/tests/src/virtual_network/iface_bridge.py (1)
284-284: LGTM: Console recreation after libvirtd restart.Using
recreate_serial_console=Trueafter restarting libvirtd is the correct approach to ensure the serial console is properly reinitialized.libvirt/tests/src/migration/migrate_network.py (1)
316-316: LGTM: Serial console handling in migration flow.All three locations correctly use
recreate_serial_console=Trueto handle console lifecycle during migration stages (before migration, after migration, and after migration back).Also applies to: 343-343, 390-390
libvirt/tests/src/sriov/failover/sriov_failover_at_dt_hostdev.py (1)
35-35: LGTM: Console recreation after VM start.Using
recreate_serial_console=Trueafter starting the VM properly handles console initialization.libvirt/tests/src/virtual_network/passt/passt_lifecycle.py (1)
129-129: LGTM: Console recreation after restore/resume.The change aligns well with the comment above it and properly handles console recreation after VM restore/resume operations.
provider/migration/migration_base.py (2)
261-261: LGTM: Console recreation in migration helper functions.Both
poweroff_vmandwrite_vm_disk_on_destcorrectly userecreate_serial_console=Trueto handle console lifecycle.Also applies to: 777-777
264-264: Verify if manual cleanup is still needed.After replacing the manual console creation with
recreate_serial_console=Trueat line 261, the explicitcleanup_serial_console()call at line 264 might be redundant since the session is already closed at line 263. Please verify if this cleanup is still necessary or if it can be removed.provider/migration/base_steps.py (3)
1-604: Verify that the correct file is included in this PR.The PR objectives describe fixing SR-IOV VFIO parameter issues (specifically changing
pf_pcitovf_pciin aparse_iface_dict()function and removing unusedutils_netimports), but this file (provider/migration/base_steps.py) contains only migration-related serial console refactoring. There is no SR-IOV, VFIO,parse_iface_dict(), orutils_netcontent in this file.Please confirm whether:
- This file was included in the PR by mistake, or
- The PR objectives are outdated/incorrect, or
- This is part of a larger PR with multiple unrelated changes
96-96: LGTM: Clean consolidation of serial console management.The refactoring to use
wait_for_serial_login(recreate_serial_console=True).close()properly consolidates serial console recreation and login into a single call, with the session correctly closed afterward.
296-301: LGTM: Correct serial console management in ping check.The refactoring properly uses
wait_for_serial_login(timeout=360, recreate_serial_console=True)to recreate and access the serial console, with the session correctly closed at line 301. The 360-second timeout is appropriate for migration scenarios.libvirt/tests/src/sriov/vIOMMU/hotplug_device_with_iommu_enabled.py (1)
67-69: LGTM! Consolidates serial console management.The addition of
recreate_serial_console=Trueproperly consolidates the serial console lifecycle management, eliminating the need for explicit cleanup and recreation calls. This follows the refactoring pattern applied consistently across the PR.libvirt/tests/src/sriov/plug_unplug/sriov_attach_released_hostdev.py (1)
23-23: Serial console handling improved, but PR description mentions different changes.The addition of
recreate_serial_console=Truecorrectly consolidates serial console lifecycle management into a single call, improving code clarity.However, the PR description states:
- "Remove the duplicate parse_iface_dict() function"
- "Use the existing sriov_vfio.parse_iface_dict() provider function instead"
- "Remove an unused utils_net import"
None of these changes are visible in this file. The only change shown is the serial console handling improvement. Please verify that the PR description accurately reflects the changes, or confirm that the parse_iface_dict() fixes were applied to different files not included in this review.
libvirt/tests/src/sriov/failover/sriov_failover_migration.py (1)
71-81: <!-- [scratchpad]
[task review_file_1/1]
[observations]
- File: libvirt/tests/src/sriov/failover/sriov_failover_migration.py.
verify_network()definesvm_session = vm.wait_for_serial_login(...)at line 75 and uses it until line 81 without.close().- Search results show many patterns in codebase: some sessions closed (
.close()), e.g., interface_update_device_negative.py, scripts under migration, etc., but many not closed explicitly.
[analysis]- In this file, likely other sessions are not closed either.
- Possibly session is closed by framework tear-down or not critical.
- No explicit
.close()calls in this file.
[pending]- Determine file-level usage: search for
.close(in this file.- Check if larger pattern: functions often not closing when returning session to caller; e.g., functions named
verify_networkmight rely on caller to close.
[actions]- Run grep for
.closein this file.- Display file snippet around verify_network and other session usage.
[/scratchpad]libvirt/tests/src/snapshot/revert_snapshot_after_xml_updated.py (1)
71-71: Simplify serial console handling
Addingrecreate_serial_console=Truetovm.wait_for_serial_login()correctly replaces manual console cleanup/creation in this snapshot test.libvirt/tests/src/sriov/vIOMMU/iommu_device_settings.py (2)
2-2: Verify that utils_net import is intentionally retained.The PR objectives mention removing "an unused utils_net import," but
utils_net.pingis used at line 85 for the non-SRIOV network checking path. If the intention was to remove this import from other files only, this is correct. However, if it was meant to be removed from this file as well, the non-SRIOV network check at line 85 would need to be refactored.
37-39: LGTM!The addition of
recreate_serial_console=Truealigns with the broader refactoring to consolidate serial console setup into the login call. This ensures a fresh serial console is available when the VM session is established.libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_vfio_variant_driver.py (4)
1-11: Inconsistency between PR objectives and AI-generated summary.The PR objectives describe fixing a specific bug (incorrect parameter
pf_pcivsvf_pciin a localparse_iface_dict()function), while the AI-generated summary describes a broad refactoring effort to replace serial console cleanup/creation across many tests. Additionally, files 2 and 3 in this review contain only serial console changes and are not mentioned in the PR objectives.Please clarify whether this PR includes both the bug fix and the serial console refactoring, or if the AI summary and additional files are from a different PR.
45-45: LGTM! Bug fix correctly addresses the parameter issue.The fix correctly uses
vf_pci(obtained from line 44) instead of the incorrectpf_pcithat would have been used by the removed local function. This matches the provider function signature atprovider/sriov/sriov_vfio.py:16-38and resolves the error "readlink /sys/bus/pci/devices/None/virtfn0" mentioned in the PR objectives.
82-82: Good practice: Properly closes the session after reboot.The change appropriately uses
recreate_serial_console=Trueafter the VM reboot (line 80) to handle console state changes, and correctly calls.close()on the returned session to prevent resource leaks.
1-11: utils_net import removal safe. No references toutils_netin sriov_attach_detach_device_vfio_variant_driver.py; removal is safe.libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_device_with_flags.py (1)
79-79: File not mentioned in PR objectives - verify this change belongs in this PR.This file is not mentioned in the PR objectives, which specifically state the bug fix is in
sriov_attach_detach_device_vfio_variant_driver.py. The serial console refactoring in this file appears to be part of a broader effort described in the AI summary, but it's unclear if it should be included in this PR that's focused on fixing thevf_pci/pf_pciparameter bug.Please confirm whether this change is intentionally part of this PR or if it belongs to a separate PR.
libvirt/tests/src/sriov/plug_unplug/sriov_attach_detach_interface_with_flags.py (1)
57-57: File not mentioned in PR objectives - verify this change belongs in this PR.Like the previous file, this file is not mentioned in the PR objectives. The serial console refactoring appears to be part of the broader effort described in the AI summary, but the PR objectives specifically describe fixing a bug in
sriov_attach_detach_device_vfio_variant_driver.py.Please confirm whether these additional changes are intentionally bundled in this PR or if they belong to a separate PR.
libvirt/tests/src/migration/migration_with_vtpm/migration_with_vtpm_dev.py (2)
1-236: This file does not match the PR objectives.The PR objectives describe fixing an SR-IOV VFIO variant driver issue (incorrect parameter name
pf_pcivsvf_pci, removing duplicateparse_iface_dict()function), but this file is about migration with vTPM devices and has no relation to SR-IOV or VFIO. Either:
- This is the wrong file included in the PR
- The PR objectives are incorrect
- This is a separate unrelated change bundled in the same PR
Please verify that this file was intentionally included in this PR.
35-35: Approve addition of recreate_serial_console=True. Verify whether the manualvm.cleanup_serial_console()at libvirt/tests/src/migration/migration_with_vtpm/migration_with_vtpm_dev.py:41–42 is still required givenwait_for_serial_login(recreate_serial_console=True)handles console recreation.libvirt/tests/src/migration/migration_with_virtiofs/migration_with_internally_launched_virtiofs_dev.py (2)
67-67: Remove outdated cleanup logic verification
This file does not contain anycleanup_serial_console()orcreate_serial_console()calls—there is no removed logic to verify.Likely an incorrect or invalid review comment.
1-98: Ignore migration_with_internally_launched_virtiofs_dev.py: not modified in this PR. This file isn’t part of the changes and can be excluded from review.Likely an incorrect or invalid review comment.
libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_iommu_at_dt_hostdev.py (2)
1-91: Inconsistency detected between PR description and code changes.The PR description states:
- "A local parse_iface_dict() function used an incorrect parameter name (pf_pci instead of vf_pci)..."
- "Remove the duplicate parse_iface_dict() function"
- "Remove an unused utils_net import"
However, the only visible change in this file is the addition of
recreate_serial_console=Trueon line 34, which is unrelated to vf_pci parameters or parse_iface_dict(). Line 73 showssriov_test_obj.parse_iface_dict()but is not marked as changed, and there's no utils_net import visible in this file.Please verify:
- Is this the correct PR, or was the wrong description provided?
- Are the parse_iface_dict() and vf_pci fixes in other files not included in this review?
- If this PR only contains serial console refactoring, the description should be updated to match.
34-34: Close the serial session to avoid leaks.Add a
vm_session.close()call at the end ofrun_test()in libvirt/tests/src/sriov/vm_lifecycle/sriov_vm_lifecycle_iommu_at_dt_hostdev.py, and confirm thatrecreate_serial_console=Trueproperly tears down any existing console before recreating.
The test was missing VF creation, causing failure with: 'readlink /sys/bus/pci/devices/<PCI>/virtfn0' - no such file Changes: - Add need_sriov=yes and vf_no=4 to enable automatic VF setup/teardown - Use SRIOVTest class (like viommu tests) for proper VF management - Make test_pf architecture-specific (x86_64 only) - Enable aarch64 to auto-detect SR-IOV interface - Add fallback for backward compatibility - Fix pf_name usage in ping test This allows the test to work on both x86_64 and aarch64 systems with proper VF lifecycle management. AI assisted code and commit. Human reviewed. Signed-off-by: hholoubk <[email protected]>
The VFIO migration error message format differs between kernel/libvirt versions and architectures (x86_64 vs aarch64): Old format: 'VFIO migration is not supported with postcopy migration' New format: 'cannot migrate domain: <PCI>: VFIO migration is not supported in kernel' Update err_msg to use regex pattern that matches both formats: 'VFIO migration is not supported|cannot migrate domain.*VFIO migration is not supported' This allows the test to pass on different environments. AI assisted code and commit. Human reviewed. Signed-off-by: hholoubk <[email protected]>
1 participant
The local parse_iface_dict() function had incorrect parameter name
(pf_pci instead of vf_pci), causing vf_pci to be undefined and
resulting in error: "readlink /sys/bus/pci/devices/None/virtfn0".
This change:
Summary by CodeRabbit
Refactor
Bug Fixes
Tests