net: adapt test helpers to Fedora 43#4873
Open
servolkov wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR consolidates NAD interface lookup exception handling by moving ChangesException Relocation and IP Resolution Refactoring
🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
|
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.67%
=======================================
Files 25 25
Lines 2485 2487 +2
=======================================
+ Hits 2452 2454 +2
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}
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
7c38e3a to
25c680a
Compare
openshift-virtualization-qe-bot-3
added
size/M
and removed
lgtm-coderabbitai[bot]
size/S
labels
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
yossisegev
reviewed
May 17, 2026
| from libs.vm.spec import Network | ||
| from libs.vm.vm import BaseVirtualMachine | ||
|
|
||
| if TYPE_CHECKING: |
Contributor
There was a problem hiding this comment.
IIUC, this is done to avoid the enforcement of circular imports.
The commit message says, from what I understand, that what was cirucalrly imported is IfaceNotFound, so why does the import of BaseVirtualMachine need this defense?
I am confused...
Contributor
Author
There was a problem hiding this comment.
you are right to be confused. In reality a general crucially import issue is resolved by this PR - utilities use libs code and no libs that use utilities code. We are good here.
The current TYPE_CHECKING is for type annotations only, no real code is used. We could also use 'BaseVirtualMachine'(quotes) as type annotation to avoid this TYPE_CHECKING and real BaseVirtualMachine import, but I don't have strong opinion here.
Contributor
There was a problem hiding this comment.
This is a workaround, why can't we just fix it?
Conditional imports is a smell, the dependency is there in a cycle even if you do not see it.
That network utility needs to be taken down.
Here is my attempt to help in this: #4906
Contributor
Author
| def set_interface_state(self, network_name: str, state: str) -> None: | ||
| if not self._spec.template.spec.domain.devices: | ||
| raise IfaceNotFound(name=network_name) | ||
| raise VMInterfaceSpecNotFoundError(f"Interface {network_name} not found in VM {self.name} spec") |
Contributor
There was a problem hiding this comment.
Nice catch!
Separating SpecNotFound from StatusNotFound sounds like the correct approach.
+1
Contributor
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4788. Overlapping filesutilities/virt.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4772. Overlapping filestests/network/l2_bridge/libl2bridge.py |
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
9 participants
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.
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
Release Notes