Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The proposed stripping cannot resolve the described failure because int() already accepts surrounding whitespace.
Key Test Cases:
verify_cpu_offline_channel_add
Impacted LISA Features:
NetworkInterface
Tested Azure Marketplace Images:
- canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates ethtool channel parsing by trimming captured values before integer conversion.
Changes:
- Strips whitespace from current and maximum channel values.
| File | Description |
|---|---|
lisa/tools/ethtool.py |
Adjusts channel-value conversion. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
AI Test Case SelectionSelected 4 test case(s): verify_device_channels_change,verify_device_enabled_features,verify_device_statistics,verify_device_gro_lro_settings_change Marketplace image: Result: Succeeded |
|
paxue Please provide test matrix which can repro this issue, and provide the results of before and after the fix. |
bd2e71a to
1e14684
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The parser now silently accepts malformed numeric prefixes, while the whitespace test does not reproduce the reported failure.
Review effort: Balanced
Findings: 2
Open (2)
Resolved since last review (1)
| # Combined: 1 | ||
| _channel_count_param_pattern = re.compile( | ||
| r"(?P<param>Combined):[ \t]*(?P<value>.*)$", re.MULTILINE | ||
| r"(?P<param>Combined):[ \t]*(?P<value>\d+)", re.MULTILINE |
| def test_device_channel_parsing_with_trailing_spaces(self) -> None: | ||
| """Test parsing when output has trailing spaces.""" | ||
| raw_output = ( | ||
| "Channel parameters for eth0:\nPre-set maximums:\n" | ||
| "RX: 0\nTX: 0\nOther: 0\n" | ||
| "Combined: 32 \n" | ||
| "Current hardware settings:\n" | ||
| "RX: 0\nTX: 0\nOther: 0\n" | ||
| "Combined: 2 " # noqa: W291 |
AI Test Case SelectionSelected 5 test case(s): verify_device_channels_change,verify_device_enabled_features,verify_device_gro_lro_settings_change,verify_device_rss_hash_key_change,verify_device_rx_hash_level_change Marketplace image: Result: Succeeded |
| @@ -0,0 +1,105 @@ | |||
| # Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
remove this, no need to add selftests for each PR
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 3
Open (4)
.strip()was added before converting regex groups toint(), but the… · New[ \\t]*and separately handles\\r?. You can… · New
| r"(?P<param>Combined):[ \t]*(?P<value>\d+)[ \t]*\r?$", | ||
| re.MULTILINE, |
| # Combined: 1 | ||
| _channel_count_param_pattern = re.compile( | ||
| r"(?P<param>Combined):[ \t]*(?P<value>.*)$", re.MULTILINE | ||
| r"(?P<param>Combined):[ \t]*(?P<value>\d+)[ \t]*\r?$", |
|
paxue Please keep the PR updated in a timely manner and provide the test matrix that this PR is intended to fix. Thanks! |
Description
Fix parsing issue in ethtool channel info where regex-captured values with trailing whitespace caused
int()conversion to fail.Root Cause
The DeviceChannel class in ethtool.py parsed device channel info using regex patterns that captured values with potential trailing whitespace. When converting these values to integers using
int(), the whitespace caused a ValueError.Changes
.strip()to remove leading/trailing whitespace before converting regex group values to integerslisa/tools/ethtool.pyTesting
Fixes
verify_cpu_offline_channel_addtestRelated Tests