win_driver_installer_test: Add viogpudo support for installer tests #4424
Conversation
WalkthroughAdded support for the VirtIO GPU DOD driver ("viogpudo") to the Windows driver installer tests. In provider/win_driver_installer_test.py the public lists 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
Comment |
6-dehan
force-pushed
the
KVMAUTOMA-4818-add-gpu-driver-rpm-acceptance-installer
branch
from
9e0aceb to
56cc08f
Compare
![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
🧹 Nitpick comments (1)
provider/win_driver_installer_test.py (1)
457-476: LGTM! Device status validation is correct.The function properly verifies the viogpudo driver by checking the device status through WMI. The status check correctly fails the test if the device is not in "OK" state.
Consider closing the session explicitly at the end of the function for better resource management:
# Output usually contains the number, e.g. "1024" if not any(char.isdigit() for char in output): test.warn("Could not retrieve resolution for viogpudo device: %s" % output) + session.close()While the current pattern is consistent with some other test functions in this file (e.g., rng_test, netkvm_test), explicitly closing the session is a better practice to prevent potential resource leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
provider/win_driver_installer_test.py(4 hunks)qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/win_driver_installer_test.py (1)
qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (6)
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (4)
61-61: LGTM! VGA configuration is appropriate for GPU driver testing.Setting
vga = "virtio"in the all_drivers variant ensures the VirtIO GPU device is available for the viogpudo driver tests.
107-107: LGTM! Driver list correctly updated.The addition of 'viogpudo' to the test_drivers list integrates the GPU driver into the all_drivers test variant.
115-115: LGTM! Test function mapping is correct.The driver_test_name_viogpudo parameter correctly maps to the test function implemented in the Python file.
195-200: LGTM! Configuration block follows established patterns.The with_viogpudo variant configuration is consistent with other driver variants (e.g., with_netkvm, with_balloon). The hardware ID, device name, and VGA setting match the implementation in the Python provider file.
provider/win_driver_installer_test.py (2)
31-31: LGTM! Driver metadata correctly added.The viogpudo driver has been properly added to all three lists (driver_name_list, device_hwid_list, device_name_list) at index 11, maintaining consistency. The hardware ID
PCI\VEN_1AF4&DEV_1050&SUBSYS_11001AF4&REV_01and device name "Red Hat VirtIO GPU DOD controller" match the configuration in the .cfg file.Also applies to: 46-46, 61-61
477-485: Resolution check provides useful validation.The horizontal resolution check serves as an additional indicator that the driver is active and functioning. Using
test.warninstead oftest.failis appropriate here since the primary validation (status check) has already passed, and resolution detection issues might occur due to environmental factors.
6-dehan
changed the title
6-dehan
force-pushed
the
KVMAUTOMA-4818-add-gpu-driver-rpm-acceptance-installer
branch
from
56cc08f to
dcfc8e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
provider/win_driver_installer_test.py (1)
457-485: Session not closed after use.The test function opens a session but doesn't close it. While some existing test functions in this file follow the same pattern (
rng_test,netkvm_test), others properly close the session (viofs_basic_io_test,balloon_test). For resource hygiene, consider closing the session.# Output usually contains the number, e.g. "1024" if not any(char.isdigit() for char in output): test.warn("Could not retrieve resolution for viogpudo device: %s" % output) + + session.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
provider/win_driver_installer_test.py(4 hunks)qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
provider/win_driver_installer_test.py (1)
qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (3)
provider/win_driver_installer_test.py (1)
19-32: Lists are consistent - LGTM.The
viogpudodriver entry is correctly added to all three parallel lists at the same index position, maintaining the required alignment for thezip()operations inwin_uninstall_all_driversanddriver_check.However, consider whether skip logic (similar to viomem at lines 113-115 and 185-187) is needed for viogpudo when
vga != "virtio". Currently, the configuration ensuresvga = "virtio"is set for all relevant variants, so this is not blocking.Also applies to: 34-47, 49-62
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (2)
58-116: Configuration for viogpudo in all_drivers variant is correct.The changes properly:
- Enable virtio GPU with
vga = "virtio"(line 61)- Add viogpudo to the test drivers list (line 107)
- Map the test function name (line 115)
This ensures the viogpudo driver is properly tested alongside other drivers.
195-200: New with_viogpudo variant is correctly configured.The single driver variant for viogpudo properly sets:
- The driver name, device name, and hardware ID consistent with the Python module
vga = "virtio"to ensure the VirtIO GPU device is present- The test function mapping
|
@leidwang please help review this when you're free. (14/16) Host_RHEL.m9.u8.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win2025.x86_64.io-github-autotest-qemu.win_virtio_driver_install_by_installer.driver_uninstall.q35: STARTED |
|
@xiagao Could you review this PR?Thanks. |
6-dehan
force-pushed
the
KVMAUTOMA-4818-add-gpu-driver-rpm-acceptance-installer
branch
from
be35af5 to
aac2305
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
provider/win_driver_installer_test.py (1)
457-479: Missing session cleanup and decorator.The function opens a session but doesn't close it, which could lead to resource leaks. Additionally, the
@error_context.context_awaredecorator is missing, which other similar test functions in this file use (e.g.,rng_test,fwcfg_test,vioser_test).♻️ Suggested improvements
+@error_context.context_aware def viogpudo_test(test, params, vm): """ viogpudo driver function test. :param test: kvm test object. :param params: the dict used for parameters. :param vm: vm object. """ session = vm.wait_for_login() error_context.context("Check viogpudo device status", LOG_JOB.info) # Check if the device is working properly cmd = params["viogpudo_status_check_cmd"] output = session.cmd_output(cmd).strip() if "OK" not in output: test.fail("viogpudo device status is not OK: %s" % output) # Check if resolution is detected (indicates driver is active) cmd = params["viogpudo_resolution_check_cmd"] output = session.cmd_output(cmd).strip() # Output usually contains the number, e.g. "1024" if not any(char.isdigit() for char in output): test.warn("Could not retrieve resolution for viogpudo device: %s" % output) + + session.close()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
provider/win_driver_installer_test.pyqemu/tests/cfg/win_virtio_driver_install_by_installer.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
provider/win_driver_installer_test.py (1)
qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (4)
provider/win_driver_installer_test.py (1)
19-32: Verify parallel list alignment for viogpudo.The three lists (
driver_name_list,device_hwid_list,device_name_list) must maintain strict parallel alignment since they're zipped together inwin_uninstall_all_driversanddriver_check. The newviogpudoentry is added at position 11 (0-indexed) in all three lists—this appears correct.However, there's an existing inconsistency:
device_name_listcontains both "Red Hat VirtIO SCSI controller" (for viostor) and "Red Hat VirtIO SCSI pass-through controller" (for vioscsi), butdevice_hwid_listhas only one vioscsi entry at index 2 while viostor is at index 1. This pre-existing alignment seems intentional, and the new viogpudo entries maintain consistency.Also applies to: 34-47, 49-62
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (3)
61-61: LGTM: VGA setting for all_drivers variant.Setting
vga = "virtio"enables the VirtIO GPU device for the all_drivers test variant, which is required for viogpudo driver testing.
107-107: LGTM: Test driver configuration.The
viogpudoentry intest_driversand the correspondingdriver_test_name_viogpudomapping are correctly configured to invoke theviogpudo_testfunction.Also applies to: 115-115
195-202: The hardware ID specificity concern is unfounded. The device HWID"PCI\\VEN_1AF4&DEV_1050&SUBSYS_11001AF4&REV_01"is consistently used across multiple config files (pvpanic_basic.cfg and win_virtio_driver_install_by_installer.cfg) with no simpler alternative patterns present in the codebase. This indicates the SUBSYS and REV values are intentional and required for proper device identification.Likely an incorrect or invalid review comment.
6-dehan
force-pushed
the
KVMAUTOMA-4818-add-gpu-driver-rpm-acceptance-installer
branch
from
aac2305 to
0123c73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @provider/win_driver_installer_test.py:
- Line 31: The tests added "viogpudo" to the driver lists but lack the
conditional skip used for "viomem"; update the functions
win_uninstall_all_drivers and driver_check to either accept a new boolean
parameter boot_with_viogpudo or otherwise gate operations on "viogpudo" using
the existing boot_with_viomem pattern; specifically, mirror the viomem checks
(where the device is skipped when the corresponding boot flag is false) for
viogpudo by adding boot_with_viogpudo and wrapping any uninstall/check logic
that references "viogpudo" with the same conditional skip used for viomem, or
remove viogpudo from the loops if it is guaranteed present.
🧹 Nitpick comments (3)
provider/win_driver_installer_test.py (3)
465-479: Session is not closed; consider closing for resource cleanup.The session created on line 465 is not closed at the end of the function. While some other test functions in this file also don't close sessions, functions like
viofs_basic_io_testandballoon_testdo callsession.close(). Consider adding cleanup for consistency.♻️ Suggested fix
# Output usually contains the number, e.g. "1024" if not any(char.isdigit() for char in output): test.warn("Could not retrieve resolution for viogpudo device: %s" % output) + session.close()
474-479: Resolution validation is too permissive.The check
any(char.isdigit() for char in output)would pass for error messages containing numbers (e.g., "Error 0x800401F3"). Consider validating that the output is a proper resolution value.♻️ Suggested fix
# Check if resolution is detected (indicates driver is active) cmd = params["viogpudo_resolution_check_cmd"] output = session.cmd_output(cmd).strip() - # Output usually contains the number, e.g. "1024" - if not any(char.isdigit() for char in output): + # Output should be a valid resolution value, e.g. "1024" + try: + resolution = int(output) + if resolution <= 0: + test.warn("Invalid resolution value for viogpudo device: %s" % output) + except ValueError: test.warn("Could not retrieve resolution for viogpudo device: %s" % output)
469-472: Consider adding a timeout for command execution.The
cmd_outputcalls don't specify a timeout. Other functions in this file (e.g., line 205) usetimeout=chk_timeoutto prevent hanging. Consider adding a timeout parameter for robustness.♻️ Suggested fix
+ chk_timeout = int(params.get("chk_timeout", 240)) + # Check if the device is working properly cmd = params["viogpudo_status_check_cmd"] - output = session.cmd_output(cmd).strip() + output = session.cmd_output(cmd, timeout=chk_timeout).strip() if "OK" not in output: test.fail("viogpudo device status is not OK: %s" % output) # Check if resolution is detected (indicates driver is active) cmd = params["viogpudo_resolution_check_cmd"] - output = session.cmd_output(cmd).strip() + output = session.cmd_output(cmd, timeout=chk_timeout).strip()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
provider/win_driver_installer_test.pyqemu/tests/cfg/win_virtio_driver_install_by_installer.cfg
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
provider/win_driver_installer_test.py (1)
qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
This patch adds support for testing the 'Red Hat VirtIO GPU DOD controller' (viogpudo) in the Windows driver installer test suite (install, repair, update, uninstall). Signed-off-by: Dehan Meng <[email protected]>
6-dehan
force-pushed
the
KVMAUTOMA-4818-add-gpu-driver-rpm-acceptance-installer
branch
from
0123c73 to
15a9f78
Compare
|
(1/5) Host_RHEL.m9.u8.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.Win2025.x86_64.io-github-autotest-qemu.win_virtio_driver_install_by_installer.driver_install.single_driver.with_viogpudo.q35: STARTED |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg:
- Around line 197-202: The device_hwid for the viogpudo entry is overly specific
("PCI\\VEN_1AF4&DEV_1050&SUBSYS_11001AF4&REV_01") and will miss other revisions;
update the viogpudo block (driver_name = viogpudo, device_hwid) to use the
broader vendor/device identifier "PCI\\VEN_1AF4&DEV_1050" or list multiple known
HWIDs for variants (e.g., include both REV_01 and REV_02 forms) so devcon find
matches all compatible devices.
- Line 63: The vga = "virtio" setting is currently placed at the all_drivers
variant and therefore applies to all six drivers; move it so it only applies to
the viogpudo tests unless you intentionally want VGA for every driver. Remove or
revert vga = "virtio" from the all_drivers block and add it only inside the
with_viogpudo (or the viogpudo single-driver) variant, ensuring the
viogpudo-specific configuration is the sole place that sets vga = "virtio", and
update or add a brief comment explaining the intent if you prefer to keep VGA
global.
🧹 Nitpick comments (1)
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (1)
48-49: Consider scoping these commands to the viogpudo variant.While the current global scope follows the pattern of other test-specific commands (like
read_rng_cmd), these PowerShell commands are only relevant for viogpudo tests. For better organization and to reduce global namespace pollution, consider moving them under thewith_viogpudovariant (lines 197-202) or conditionally defining them only when viogpudo is being tested.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
provider/win_driver_installer_test.pyqemu/tests/cfg/win_virtio_driver_install_by_installer.cfg
🚧 Files skipped from review as they are similar to previous changes (1)
- provider/win_driver_installer_test.py
🔇 Additional comments (2)
qemu/tests/cfg/win_virtio_driver_install_by_installer.cfg (2)
109-109: LGTM! Viogpudo integration follows established patterns.The addition of
viogpudoto thetest_driverslist and the definition ofdriver_test_name_viogpudoare consistent with how other drivers (viorng, viostor, vioscsi, etc.) are configured in the all_drivers variant.Also applies to: 117-117
48-49: Request: Include the Python test implementation file for review.The AI summary indicates that
provider/win_driver_installer_test.pywas modified to:
- Extend
driver_name_list,device_hwid_list, anddevice_name_list- Add a new
viogpudo_test(test, params, vm)functionHowever, this Python file is not included in the files for review. To ensure a comprehensive review, please include
provider/win_driver_installer_test.pyso we can verify:
- The viogpudo_test() function correctly uses
viogpudo_status_check_cmdandviogpudo_resolution_check_cmd- The lists are properly extended with the viogpudo configuration
- The test logic handles the viogpudo driver appropriately
- Error handling is implemented for the PowerShell commands
Also applies to: 197-202
3 participants
win_driver_installer_test: Add viogpudo support for installer tests
This patch adds support for testing the
'Red Hat VirtIO GPU DOD controller' (viogpudo)
in the Windows driver installer test suite
(install, repair, update, uninstall).
ID: 4817
Signed-off-by: Dehan Meng [email protected]
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.