network: remove VMI MAC lookup helper from utilities/network#4906
Open
EdDev wants to merge 1 commit into
Open
Conversation
Callers of utilities/network should not depend on it for MAC address lookups when libs/net/vmspec already provides the right abstraction. Replace get_vmi_mac_address_by_iface_name with lookup_iface_status using a no-op predicate and a short timeout (1s), reading .mac from the VMI status interface. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Edward Haas <edwardh@redhat.com>
Contributor
📝 WalkthroughWalkthroughThis PR replaces all calls to the deprecated ChangesMAC Lookup Refactoring
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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)
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4906 +/- ##
==========================================
- Coverage 98.68% 98.67% -0.01%
==========================================
Files 25 25
Lines 2504 2487 -17
==========================================
- Hits 2471 2454 -17
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-4
requested review from
dshchedr,
rlobillo and
servolkov
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/network/kubemacpool/test_kubemacpool.py`:
- Around line 47-51: The assert calling mac_pool.mac_is_within_range using
lookup_iface_status(...).mac should include a descriptive failure message;
update the assertion (and other similar asserts in this file) to pass a message
that includes VM identity and interface name (use vm.name or vm identifier and
vm.default_masquerade_iface_config.name) so test failures show which
VM/interface and MAC were out of range; locate uses of
mac_pool.mac_is_within_range and lookup_iface_status in this test and append an
f-string message describing vm, iface and the MAC value.
In `@tests/network/kubemacpool/utils.py`:
- Around line 148-151: The test assertions comparing MAC addresses lack failure
messages, making CI failures hard to triage; update the asserts around the MAC
checks (the one comparing iface.macAddress with lookup_iface_status(vm=vm,
iface_name=iface.name, ...) and the similar assert at the other block) to
include descriptive pytest failure messages that embed identifying context such
as vm (name/UID/namespace), iface.name, the expected value (iface.macAddress)
and the actual value returned by lookup_iface_status(). Use the same unique
symbols seen in the diff (iface.macAddress, iface.name, lookup_iface_status, vm)
so failures clearly show which VM/interface and what expected vs actual MAC
failed.
In `@tests/network/upgrade/test_upgrade_network.py`:
- Around line 86-90: The mac range assertions using mac_pool.mac_is_within_range
and lookup_iface_status(vm=vm, iface_name=upgrade_bridge_marker_nad.name, ...)
lack descriptive failure messages; update those assertions (including the
similar ones around the later block) to pass a clear pytest failure message that
includes VM identity (e.g., vm.name or vm.metadata.name), the interface name
(upgrade_bridge_marker_nad.name), and the observed MAC so that failures show
which VM/interface and which MAC fell outside the pool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6fea538-a825-4d42-8769-5d8bb2f54750
📒 Files selected for processing (5)
tests/network/kubemacpool/test_kubemacpool.pytests/network/kubemacpool/utils.pytests/network/l2_bridge/conftest.pytests/network/upgrade/test_upgrade_network.pyutilities/network.py
💤 Files with no reviewable changes (1)
- utilities/network.py
Comment on lines
47
to
51
| assert mac_pool.mac_is_within_range( | ||
| mac=get_vmi_mac_address_by_iface_name(vmi=vm.vmi, iface_name=vm.default_masquerade_iface_config.name), | ||
| mac=lookup_iface_status( | ||
| vm=vm, iface_name=vm.default_masquerade_iface_config.name, predicate=lambda _: True, timeout=1 | ||
| ).mac, | ||
| ) |
Contributor
There was a problem hiding this comment.
MEDIUM: Include failure message in MAC-range assertions
Please add a descriptive message to this assert (and the same pattern in similar updated checks in this file) so failures report VM/interface context directly.
As per coding guidelines: tests/**/*.py: include assertion failure messages in pytest asserts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/network/kubemacpool/test_kubemacpool.py` around lines 47 - 51, The
assert calling mac_pool.mac_is_within_range using lookup_iface_status(...).mac
should include a descriptive failure message; update the assertion (and other
similar asserts in this file) to pass a message that includes VM identity and
interface name (use vm.name or vm identifier and
vm.default_masquerade_iface_config.name) so test failures show which
VM/interface and MAC were out of range; locate uses of
mac_pool.mac_is_within_range and lookup_iface_status in this test and append an
f-string message describing vm, iface and the MAC value.
Comment on lines
+148
to
+151
| assert ( | ||
| iface.macAddress | ||
| == lookup_iface_status(vm=vm, iface_name=iface.name, predicate=lambda _: True, timeout=1).mac | ||
| ) |
Contributor
There was a problem hiding this comment.
HIGH: Add assertion failure context for MAC validation
These asserts currently fail without resource/interface context, which makes failures harder to triage in CI. Please add explicit failure messages.
Proposed patch
def assert_macs_preseved(vm):
for iface in vm.get_interfaces():
assert (
iface.macAddress
== lookup_iface_status(vm=vm, iface_name=iface.name, predicate=lambda _: True, timeout=1).mac
- )
+ ), f"MAC mismatch on VM {vm.name}, interface {iface.name}: expected {iface.macAddress}"
def assert_manual_mac_configured(vm, iface_config):
assert (
iface_config.mac_address
== lookup_iface_status(vm=vm, iface_name=iface_config.name, predicate=lambda _: True, timeout=1).mac
- )
+ ), f"Manual MAC mismatch on VM {vm.name}, interface {iface_config.name}: expected {iface_config.mac_address}"As per coding guidelines: tests/**/*.py: Use pytest assertions with failure messages (assert condition, 'descriptive message explaining failure').
Also applies to: 155-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/network/kubemacpool/utils.py` around lines 148 - 151, The test
assertions comparing MAC addresses lack failure messages, making CI failures
hard to triage; update the asserts around the MAC checks (the one comparing
iface.macAddress with lookup_iface_status(vm=vm, iface_name=iface.name, ...) and
the similar assert at the other block) to include descriptive pytest failure
messages that embed identifying context such as vm (name/UID/namespace),
iface.name, the expected value (iface.macAddress) and the actual value returned
by lookup_iface_status(). Use the same unique symbols seen in the diff
(iface.macAddress, iface.name, lookup_iface_status, vm) so failures clearly show
which VM/interface and what expected vs actual MAC failed.
Comment on lines
86
to
90
| assert mac_pool.mac_is_within_range( | ||
| mac=get_vmi_mac_address_by_iface_name(vmi=vm.vmi, iface_name=upgrade_bridge_marker_nad.name) | ||
| mac=lookup_iface_status( | ||
| vm=vm, iface_name=upgrade_bridge_marker_nad.name, predicate=lambda _: True, timeout=1 | ||
| ).mac | ||
| ) |
Contributor
There was a problem hiding this comment.
HIGH: Add descriptive messages to kubemacpool range assertions
Both updated assertions should include failure messages with VM/interface details to make upgrade-failure triage actionable.
As per coding guidelines: tests/**/*.py: pytest assertions must include failure messages.
Also applies to: 195-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/network/upgrade/test_upgrade_network.py` around lines 86 - 90, The mac
range assertions using mac_pool.mac_is_within_range and
lookup_iface_status(vm=vm, iface_name=upgrade_bridge_marker_nad.name, ...) lack
descriptive failure messages; update those assertions (including the similar
ones around the later block) to pass a clear pytest failure message that
includes VM identity (e.g., vm.name or vm.metadata.name), the interface name
(upgrade_bridge_marker_nad.name), and the observed MAC so that failures show
which VM/interface and which MAC fell outside the pool.
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4772. Overlapping filestests/network/l2_bridge/conftest.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
5 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:
Callers of
utilities/networkshould not depend on it for MAC address lookups whenlibs/net/vmspecalready provides the right abstraction. This removesget_vmi_mac_address_by_iface_nameand replaces all call sites withlookup_iface_statususing a no-op predicate and a short timeout (1s).Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
Tests
Refactor