net: adapt test helpers to Fedora 43#4873
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughCentralizes interface-related exceptions in libs/net/vmspec, defers a runtime VM type import, updates VM and network utilities to use vmspec lookup/error helpers (including IP resolution via lookup_iface_status_ip), updates tests to expect the new exceptions, and extends console escape stripping. ChangesException Centralization and Network Utility Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Linked repositories: Couldn't analyze 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-2
requested review from
EdDev,
RoniKishner,
dshchedr,
frenzyfriday,
nirdothan,
orelmisan,
rnetser and
yossisegev
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4873 +/- ##
==========================================
- Coverage 98.67% 98.65% -0.02%
==========================================
Files 25 25
Lines 2487 2459 -28
==========================================
- Hits 2454 2426 -28
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
servolkov
force-pushed
the
fix/fedora-43-fix-net
branch
from
7c38e3a to
25c680a
Compare
|
/verified Also verified same tests but with old fedora-41 - all PASSED. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get current changed files and diff for position mapping
echo "=== Changed files ==="
gh pr diff 4873 --name-only
echo "=== PR head SHA ==="
gh pr view 4873 --json headRefOid -q '.headRefOid'
echo "=== Full diff (first 80 lines) ==="
gh pr diff 4873 | head -80
echo "=== Check smoke test dependencies on changed functions (virt.py, network.py) ==="
echo "-- Files with vm_console_run_commands and smoke --"
rg -l "vm_console_run_commands" tests/ --type py | xargs grep -l "pytest.mark.smoke" 2>/dev/null || echo "NONE"
echo "-- Files with get_ip_from_vm_or_virt_handler_pod and smoke --"
rg -l "get_ip_from_vm_or_virt_handler_pod" tests/ --type py | xargs grep -l "pytest.mark.smoke" 2>/dev/null || echo "NONE"
echo "-- Direct smoke marks in l2_bridge tests --"
rg -n "pytest.mark.smoke" tests/network/l2_bridge/ --type py || echo "NONE"Length of output: 4648 🏁 Script executed: #!/bin/bash
# Post inline review comment with Test Execution Plan on first changed line of libs/net/vmspec.py
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
/repos/RedHatQE/openshift-virtualization-tests/pulls/4873/reviews \
-f commit_id="fb3143cd0a8a55fc956bdd8ba455cc4ad0cce87f" \
-f body="" \
-f event="COMMENT" \
-f "comments[][path]=libs/net/vmspec.py" \
-F "comments[][position]=1" \
-f "comments[][body]=## Test Execution Plan
**Run smoke tests: False**
> Verified: no \`@pytest.mark.smoke\`-marked tests import or depend on \`vm_console_run_commands\` or \`get_ip_from_vm_or_virt_handler_pod\`. No smoke marks exist in \`tests/network/l2_bridge/\`.
---
### Tests to run
| Test | Reason |
|------|--------|
| \`tests/network/l2_bridge/test_bridge_nic_hot_plug.py\` | \`IfaceNotFound\` → \`VMInterfaceSpecNotFoundError\` rename; import updated here and in \`libl2bridge.py\` |
| \`tests/network/connectivity/test_pod_network.py\` | \`get_ip_from_vm_or_virt_handler_pod\` race-condition fix directly affects pod-network connectivity tests |
| \`tests/network/\` files using \`vm_console_run_commands\` | OSC 8003 regex fix in \`utilities/virt.py\` affects all console-output parsing (e.g., migration, jumbo-frame, service-mesh tests)"Length of output: 3068 |
IfaceNotFound was a single exception incorrectly covering both spec and status lookup failures with a misleading "Interface not found for NAD" message. Replace each usage with the semantically correct exception: - VMInterfaceSpecNotFoundError for spec.domain.devices.interfaces lookups (libs/vm/vm.py) - VMInterfaceStatusNotFoundError for vmi.status.interfaces lookups (utilities/network.py, tests/network/l2_bridge/) This also eliminates the only libs/ -> utilities/network import, breaking the circular dependency that prevented utilities/network.py from importing libs/net/vmspec. Signed-off-by: Sergei Volkov <sevolkov@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Fedora 43 (systemd 256+) emits OSC 8003 session-tracking escape sequences in the terminal. The existing regex in vm_console_run_commands only stripped CSI sequences (ESC[…), leaving OSC sequences (ESC]…) in the parsed output. This broke any caller that parses console output, e.g. json.loads() in ip_specification tests. Extend the regex to also strip OSC sequences terminated by BEL or ST, per ECMA-48 specification. Signed-off-by: Sergei Volkov <sevolkov@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
The function read vm.vmi.interfaces[0]["ipAddresses"] once without waiting. With Fedora 43 the guest agent reports IPs slower than Fedora 41, so ipAddresses is still None right after AgentConnected becomes True, causing TypeError on iteration. Delegate the VM branch to lookup_iface_status_ip which polls the VMI with a watcher until IPs are reported. Signed-off-by: Sergei Volkov <sevolkov@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
servolkov
force-pushed
the
fix/fedora-43-fix-net
branch
from
fb3143c to
339b773
Compare
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:failed
|
D/S test |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4954. Overlapping filestests/network/l2_bridge/libl2bridge.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4660. Overlapping filestests/network/l2_bridge/libl2bridge.py |
Anatw
left a comment
Anatw
left a comment
There was a problem hiding this comment.
Thank you Sergei for making or code base better
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4916. Overlapping filesutilities/virt.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5103. Overlapping filesutilities/virt.py |
11 participants
What this PR does / why we need it:
Fix test failures caused by the Fedora 41 → 43 container image upgrade (PR #4322):
Console output parsing broken — Fedora 43 (systemd 256+) emits OSC 8003
session-tracking escape sequences. The ANSI regex in
vm_console_run_commandsonly stripped CSI sequences, leaving OSC markers in parsed output. This caused
json.loads()to fail intest_vm_is_started_with_successful_connectivity.IP lookup race condition —
get_ip_from_vm_or_virt_handler_podreadvmi.interfaces[0]["ipAddresses"]once without waiting. Fedora 43's guest agentreports IPs slower than Fedora 41, so
ipAddresseswas stillNoneright afterAgentConnectedbecameTrue, crashingtest_connectivity_over_pod_network.Which issue(s) this PR fixes:
failures from PR #4322
Special notes for reviewer:
Importing
lookup_iface_status_ipfromlibs/net/vmspecintoutilities/network.pywasblocked by the circular dependency caused by
IfaceNotFoundliving inutilities/network.py.IfaceNotFoundwas a single exception incorrectly covering both spec and status lookupfailures with a misleading "Interface not found for NAD" message. Replaced each usage
with the semantically correct exception.
Removing
IfaceNotFoundis a prerequisite for the race condition fix.jira-ticket:
NONE
Summary by CodeRabbit
Bug Fixes
Refactor
Tests