Support testing vsock driver in installer related test cases #4427
Conversation
WalkthroughThis change adds VirtIO Socket (viosock) support to Windows virtio driver installer tests. It updates provider/win_driver_installer_test.py to include "viosock" in driver_name_list, adds a matching device_name and device_hwid entry, and introduces a viosock_test stub. The test configuration qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg gains a with_viosock variant plus vsocks and boot_with_viosock settings. The installer update and uninstall tests are adjusted to skip or exclude viosock when boot_with_viosock is "no". Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
provider/win_driver_installer_test.py (3)
49-61: CRITICAL: Missing "VirtIO Socket Driver" entry in device_name_list.The device_name_list is missing the "VirtIO Socket Driver" entry, creating a length mismatch with driver_name_list (12 items) and device_hwid_list (12 items). This mismatch causes silent failures because:
- Multiple functions zip these three lists together (lines 109-110, 118-121, 183)
- Python's zip() truncates to the shortest list, causing viosock entries to be dropped
- This breaks driver installation, uninstallation, and verification flows for viosock
🔎 Apply this diff to add the missing entry:
device_name_list = [ "VirtIO Balloon Driver", "Red Hat VirtIO SCSI controller", "Red Hat VirtIO SCSI pass-through controller", "VirtIO RNG Device", "VirtIO FS Device", "VirtIO Serial Driver", "QEMU PVPanic Device", "Red Hat VirtIO Ethernet Adapter", "VirtIO Input Driver", "QEMU FwCfg Device", "VirtIO Viomem Driver", + "VirtIO Socket Driver", ]
100-117: Add boot_with_viosock check for consistency.The
win_uninstall_all_driversfunction checks for viomem at lines 112-114 but lacks a corresponding check for viosock. This inconsistency means viosock uninstall will be attempted even whenboot_with_viosockis "no", potentially causing failures.🔎 Apply this diff to add the missing check:
def win_uninstall_all_drivers(session, test, params): """ Uninstall all drivers from windows guests. :param session: The guest session object. :param test: kvm test object :param params: the dict used for parameters. """ devcon_path = params["devcon_path"] for driver_name, device_name, device_hwid in zip( driver_name_list, device_name_list, device_hwid_list ): if params.get("boot_with_viomem", "no") == "no": if driver_name == "viomem": continue + if params.get("boot_with_viosock", "no") == "no": + if driver_name == "viosock": + continue win_driver_utils.uninstall_driver( session, test, devcon_path, driver_name, device_name, device_hwid )
167-212: Add boot_with_viosock check for consistency.The
driver_checkfunction includes a viomem check at lines 184-186 but is missing a corresponding viosock check. This means driver verification will fail when viosock is not present in the guest (boot_with_viosock="no").🔎 Apply this diff to add the missing check:
for driver_name, device_name in zip(driver_name_list, device_name_list): if params.get("boot_with_viomem", "no") == "no": if driver_name == "viomem": continue + if params.get("boot_with_viosock", "no") == "no": + if driver_name == "viosock": + continue error_context.context("%s Driver Check" % driver_name, LOG_JOB.info)
🧹 Nitpick comments (2)
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (1)
269-278: Complete the TODO for the required_virtio_win version.The with_viosock variant configuration is well-structured and consistent with other driver variants. However, line 272 contains a TODO comment that should be resolved before merging.
Could you clarify the minimum required virtio-win version for viosock support? Once confirmed, the TODO can be replaced with the correct version constraint, similar to line 256:
required_virtio_win = [1.9.40.0, ).provider/win_driver_installer_test.py (1)
456-464: Address the TODO or track it as a follow-up task.The viosock_test stub function contains a TODO comment without implementation. Please either:
- Implement the basic viosock connectivity test now, or
- Create a tracking issue for the follow-up work
Would you like me to help draft a basic viosock connectivity test implementation, or should this be tracked as a separate issue?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
provider/win_driver_installer_test.py(3 hunks)qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg(2 hunks)qemu/tests/win_virtio_driver_installer_uninstall.py(1 hunks)qemu/tests/win_virtio_driver_update_by_installer.py(1 hunks)
🔇 Additional comments (4)
qemu/tests/win_virtio_driver_update_by_installer.py (1)
126-128: LGTM! Skip logic is consistent with existing patterns.The conditional block correctly skips viosock driver installation when
boot_with_viosockis set to "no", mirroring the established pattern for viomem.qemu/tests/win_virtio_driver_installer_uninstall.py (1)
99-100: LGTM! Uninstall skip logic is properly implemented.The conditional correctly excludes "VirtIO Socket Driver" from the uninstall list when
boot_with_viosockis "no", maintaining consistency with the viomem handling pattern.qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (1)
116-116: LGTM! VSock device mapping is properly configured.The vsocks device mapping correctly references vhost_vsock0, aligning with the new viosock driver testing support.
provider/win_driver_installer_test.py (1)
46-46: LGTM! Hardware ID is correctly formatted.The viosock hardware ID entry is properly added with correct raw string formatting, matching the PCI device identifiers for VirtIO Socket devices.
leidwang
force-pushed
the
vsock_installer_testing
branch
from
666d514 to
97cf356
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (1)
271-271: TODO comment indicates incomplete implementation.The TODO comment suggests that this viosock variant configuration may be incomplete. This aligns with the empty
viosock_teststub inprovider/win_driver_installer_test.py(lines 457-465).Do you want me to open an issue to track the completion of the viosock test implementation?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
provider/win_driver_installer_test.py(4 hunks)qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg(2 hunks)qemu/tests/win_virtio_driver_installer_uninstall.py(1 hunks)qemu/tests/win_virtio_driver_update_by_installer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/win_virtio_driver_update_by_installer.py
🔇 Additional comments (2)
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (1)
116-116: LGTM! VSock device added for all_drivers variant.The addition of
vsocks = vhost_vsock0enables VSock device testing in the all_drivers configuration variant.provider/win_driver_installer_test.py (1)
31-31: LGTM! VirtIO Socket driver metadata correctly added.The viosock driver has been properly integrated into the three core lists:
driver_name_list(line 31)device_hwid_list(line 46) with correct PCI hardware IDs and raw string formattingdevice_name_list(line 61)The list indices remain aligned, maintaining the mapping consistency between driver names, hardware IDs, and device names.
Also applies to: 46-46, 61-61
| def viosock_test(test, params, vm): | ||
| """ | ||
| Connect VM via vsock bridge service | ||
| :param test: kvm test object. | ||
| :param params: the dict used for parameters. | ||
| :param vm: vm object. | ||
| """ | ||
| # TODO |
There was a problem hiding this comment.
Incomplete implementation: viosock_test is an empty stub.
The viosock_test function is called by the test framework (via driver_test_names = ${driver_name} in cfg line 277) but contains only a TODO comment with no actual test logic. This means the viosock driver functionality will not be tested when the test runs, potentially allowing regressions to go undetected.
Do you want me to:
- Generate a basic implementation for vsock bridge connectivity testing?
- Open an issue to track this incomplete work?
- Suggest adding a skip/xfail marker until the implementation is complete?
🤖 Prompt for AI Agents
In provider/win_driver_installer_test.py around lines 457 to 465, viosock_test
is a TODO stub and must be implemented to actually verify vsock bridge
connectivity: open the host side vsock endpoint (or ensure the bridge service is
running), obtain the target VM identifier/address from the vm fixture, attempt a
connection to a known test port on the guest over vsock, send and receive a
small payload to verify bidirectional transfer, assert success with clear error
messages, and perform proper cleanup and timeouts; if the test environment may
lack vsock support add an environment check to skip the test with an explanatory
skip/xfail.
There was a problem hiding this comment.
Yes, I will add something to the function, so I added TODO here
There was a problem hiding this comment.
@leidwang, understood! Since you plan to implement this in a future update, would you like me to create an issue to track the viosock_test implementation work? This would help ensure the TODO doesn't get forgotten.
leidwang
force-pushed
the
vsock_installer_testing
branch
from
97cf356 to
ac0404b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
provider/win_driver_installer_test.py (1)
457-465: Incomplete implementation: viosock_test is an empty stub.The
viosock_testfunction remains unimplemented with only a TODO comment. This issue was previously flagged in earlier reviews and has not yet been addressed.
🧹 Nitpick comments (1)
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (1)
272-284: LGTM! VirtIO Socket variant properly configured.The with_viosock variant block is now complete with the
boot_with_viosock = yesparameter properly defined (line 276), addressing the critical issue from previous reviews. The configuration correctly includes:
- Host version restrictions matching the all_drivers variant
- Required virtio-win version constraint
- Complete driver metadata (name, device name, hardware IDs)
- Proper test framework integration via driver_test_names
Note: The TODO comment on line 274 remains, but the configuration appears functionally complete. Consider removing the TODO or documenting what additional work is pending.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
provider/win_driver_installer_test.py(4 hunks)qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg(2 hunks)qemu/tests/win_virtio_driver_installer_uninstall.py(1 hunks)qemu/tests/win_virtio_driver_update_by_installer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/win_virtio_driver_update_by_installer.py
🔇 Additional comments (5)
provider/win_driver_installer_test.py (3)
31-31: LGTM! VirtIO Socket driver properly added to driver list.The addition of "viosock" to the driver_name_list is correct and maintains consistency with the corresponding entries in device_hwid_list (line 46) and device_name_list (line 61).
46-46: LGTM! Hardware IDs correctly defined.The VirtIO Socket device hardware IDs are properly formatted and positioned to match the corresponding "viosock" entry in driver_name_list.
61-61: LGTM! Device name properly added.The "VirtIO Socket Driver" entry correctly completes the VirtIO Socket support additions, maintaining proper alignment with driver_name_list and device_hwid_list.
qemu/tests/win_virtio_driver_installer_uninstall.py (1)
99-100: LGTM! Conditional exclusion correctly implemented.The VirtIO Socket Driver exclusion logic properly mirrors the existing viomem pattern and correctly references the
boot_with_viosockparameter. The corresponding parameter is now properly defined in the configuration file (qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg, line 276), resolving the concern raised in previous reviews.qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (1)
116-119: LGTM! VirtIO Socket configuration properly added to all_drivers variant.The vsock device and boot_with_viosock parameter are correctly configured with appropriate host version restrictions matching the pattern used for viomem. The default enablement with per-host overrides ensures proper test coverage across supported platforms.
1.repaire virtio-win drivers via installer. 2.uninstall virtio-win drivers via installer. 3.Check viosock device works well if driver installed via installer. 4.Upgrade virtio-win drivers via installer from the old driver version. 5.Upgrade virtio-win drivers via installer from the previous installer version. ID:4312,4877,4878,4879,4883 Signed-off-by: Leidong Wang <[email protected]>
leidwang
force-pushed
the
vsock_installer_testing
branch
from
ac0404b to
af09b49
Compare
1 participant
1.repaire virtio-win drivers via installer.
2.uninstall virtio-win drivers via installer.
3.Check viosock device works well if driver installed via installer.
4.Upgrade virtio-win drivers via installer from the old driver version.
5.Upgrade virtio-win drivers via installer from the previous installer version.
ID:4312,4877,4878,4879,4883
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.