installer: Update network config for the IPs order issue#4443
Open
heywji wants to merge 1 commit intoautotest:masterfrom
Open
installer: Update network config for the IPs order issue#4443heywji wants to merge 1 commit intoautotest:masterfrom
heywji wants to merge 1 commit intoautotest:masterfrom
Conversation
This MR includes the following improvements: 1. Support multiple static IPs and DNS servers in test configuration. 2. Update `check_network_config` to verify all configured IPs and DNS entries. 3. Refactor network setup to use `netsh` for adding multiple addresses. Signed-off-by: Wenkang Ji <wji@redhat.com>
WalkthroughThe changes refactor the network configuration and validation process for Windows VirtIO driver installation tests. A configuration file shifts from a single static IP/DNS setup to a multi-address approach with separate commands for IP, DNS, and DHCP management. The test file adds netsh-based network information extraction, implements ordered IP list verification with timeout polling, supports multiple IP addresses with optional masks, replaces single DNS checks with list-based validation, and introduces network restoration to DHCP after testing completes. Enhanced logging is added throughout network operations. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Comment |
Contributor
Author
|
Test Results: |
![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
🤖 Fix all issues with AI agents
In `@qemu/tests/win_virtio_driver_update_by_installer.py`:
- Around line 135-148: The code calls utils_net.get_windows_nic_attribute into
static_dns_address and then uses "in" which will TypeError if
DNSServerSearchOrder is None; before iterating params.get("static_dns_list",
"").split(), add a null/empty guard for static_dns_address (e.g., if
static_dns_address is None or empty string) and handle it explicitly: either
normalize static_dns_address to an empty string/list so the membership checks
are safe, or call test.fail with a clear message that DNSServerSearchOrder is
missing; update the block around static_dns_address and the subsequent for-loop
(referencing static_dns_address, params.get("static_dns_list", ""),
utils_net.get_windows_nic_attribute, and test.fail) accordingly.
Comment on lines
135
to
+148
| static_dns_address = utils_net.get_windows_nic_attribute( | ||
| session_serial, | ||
| global_switch="nicconfig", | ||
| key="MACAddress", | ||
| value=f"{virtio_nic_mac}", | ||
| target="DNSServerSearchOrder", | ||
| ) | ||
| static_dns_address = static_dns_address.strip("{}").strip('"') | ||
| if static_dns_address != params["static_dns"]: | ||
| test.fail( | ||
| "Static dns is lost after upgrade driver, current dns " | ||
| "is %s" % static_dns_address | ||
| ) | ||
| test.log.info("Current guest DNS servers: %s", static_dns_address) | ||
| for dns in params.get("static_dns_list", "").split(): | ||
| if dns not in static_dns_address: | ||
| test.fail( | ||
| "Static dns is lost after upgrade driver, current dns " | ||
| "is %s" % static_dns_address | ||
| ) |
There was a problem hiding this comment.
Guard against missing DNS attributes.
If DNSServerSearchOrder is None, the in check will raise TypeError. Add a null/empty guard before iterating.
🩹 Suggested fix
static_dns_address = utils_net.get_windows_nic_attribute(
session_serial,
global_switch="nicconfig",
key="MACAddress",
value=f"{virtio_nic_mac}",
target="DNSServerSearchOrder",
)
test.log.info("Current guest DNS servers: %s", static_dns_address)
+ if not static_dns_address:
+ test.fail("No DNS servers found on the NIC after upgrade")
for dns in params.get("static_dns_list", "").split():
if dns not in static_dns_address:
test.fail(
"Static dns is lost after upgrade driver, current dns "
"is %s" % static_dns_address
)📝 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.
Suggested change
| static_dns_address = utils_net.get_windows_nic_attribute( | |
| session_serial, | |
| global_switch="nicconfig", | |
| key="MACAddress", | |
| value=f"{virtio_nic_mac}", | |
| target="DNSServerSearchOrder", | |
| ) | |
| static_dns_address = static_dns_address.strip("{}").strip('"') | |
| if static_dns_address != params["static_dns"]: | |
| test.fail( | |
| "Static dns is lost after upgrade driver, current dns " | |
| "is %s" % static_dns_address | |
| ) | |
| test.log.info("Current guest DNS servers: %s", static_dns_address) | |
| for dns in params.get("static_dns_list", "").split(): | |
| if dns not in static_dns_address: | |
| test.fail( | |
| "Static dns is lost after upgrade driver, current dns " | |
| "is %s" % static_dns_address | |
| ) | |
| static_dns_address = utils_net.get_windows_nic_attribute( | |
| session_serial, | |
| global_switch="nicconfig", | |
| key="MACAddress", | |
| value=f"{virtio_nic_mac}", | |
| target="DNSServerSearchOrder", | |
| ) | |
| test.log.info("Current guest DNS servers: %s", static_dns_address) | |
| if not static_dns_address: | |
| test.fail("No DNS servers found on the NIC after upgrade") | |
| for dns in params.get("static_dns_list", "").split(): | |
| if dns not in static_dns_address: | |
| test.fail( | |
| "Static dns is lost after upgrade driver, current dns " | |
| "is %s" % static_dns_address | |
| ) |
🤖 Prompt for AI Agents
In `@qemu/tests/win_virtio_driver_update_by_installer.py` around lines 135 - 148,
The code calls utils_net.get_windows_nic_attribute into static_dns_address and
then uses "in" which will TypeError if DNSServerSearchOrder is None; before
iterating params.get("static_dns_list", "").split(), add a null/empty guard for
static_dns_address (e.g., if static_dns_address is None or empty string) and
handle it explicitly: either normalize static_dns_address to an empty
string/list so the membership checks are safe, or call test.fail with a clear
message that DNSServerSearchOrder is missing; update the block around
static_dns_address and the subsequent for-loop (referencing static_dns_address,
params.get("static_dns_list", ""), utils_net.get_windows_nic_attribute, and
test.fail) accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This MR includes the following improvements:
check_network_configto verify all configured IPs and DNS entries.netshfor adding multiple addresses.Summary by CodeRabbit
Release Notes
New Features
Improvements