net, tests, hot-plug: Avoid redundant VMI interface lookup#5419
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesGuest device name resolution in l2bridge
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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
Branch Management
Custom Commands
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
requested review from
azhivovk,
frenzyfriday,
rnetser and
yossisegev
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5419 published |
Anatw
force-pushed
the
avoid_redundant_iface_lookup
branch
from
863db5c to
90822fa
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
Clean rebase detected — no code changes compared to previous head ( |
|
Refactored commit msg signature section |
|
|
||
| def set_secondary_static_ip_address(vm, ipv4_address, vmi_interface): | ||
| guest_vm_interface = get_guest_vm_interface_name_by_vmi_interface_name( | ||
| def set_secondary_static_ip_address(vm, ipv4_address, vmi_interface, guest_device_name=None): |
There was a problem hiding this comment.
Param typing and return type are missing
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5321. Overlapping filestests/network/l2_bridge/libl2bridge.py |
hot_plug_interface() already returns the full interface status including interfaceName. Pass it through to set_secondary_static_ip_address() instead of re-querying VMI status for the same value. Signed-off-by: Anat Wax <awax@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
Anatw
force-pushed
the
avoid_redundant_iface_lookup
branch
from
5e9e34e to
1f09d15
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/verified Tested on bm03-tlv2 uv run pytest -sv --jira --skip-deprecated-api-test --pdbcls=IPython.terminal.debugger:TerminalPdb --tc-format=python --cluster-sanity-skip-storage-check -m tier2 tests/network/l2_bridge/test_bridge_nic_hot_plug.py |
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:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: 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 — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== PR metadata =="
gh pr view 5419 --json baseRefName,headRefName,files,url --repo RedHatQE/openshift-virtualization-tests
echo
echo "== Changed files (name-only) =="
gh pr diff 5419 --name-only --repo RedHatQE/openshift-virtualization-tests
echo
echo "== Unified diff =="
gh pr diff 5419 --repo RedHatQE/openshift-virtualization-tests
echo
echo "== First changed file patch from GitHub API =="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5419/files --paginate \
--jq '.[] | {filename, status, additions, deletions, patch}'Length of output: 5184 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Outline: tests/network/l2_bridge/libl2bridge.py =="
ast-grep outline tests/network/l2_bridge/libl2bridge.py --view expanded || true
echo
echo "== Outline: tests/network/l2_bridge/test_bridge_nic_hot_plug.py =="
ast-grep outline tests/network/l2_bridge/test_bridge_nic_hot_plug.py --view expanded || true
echo
echo "== Relevant slice: tests/network/l2_bridge/libl2bridge.py =="
sed -n '1,340p' tests/network/l2_bridge/libl2bridge.py | nl -ba | sed -n '180,280p'
echo
echo "== Relevant slice: tests/network/l2_bridge/test_bridge_nic_hot_plug.py =="
sed -n '1,260p' tests/network/l2_bridge/test_bridge_nic_hot_plug.py | nl -ba | sed -n '1,240p'
echo
echo "== Direct callers/usages of set_secondary_static_ip_address in tests/ =="
rg -n -C2 '\bset_secondary_static_ip_address\s*\(' tests/
echo
echo "== Same-file callers of set_secondary_static_ip_address in tests/network/l2_bridge/libl2bridge.py =="
rg -n -C2 '\bset_secondary_static_ip_address\s*\(' tests/network/l2_bridge/libl2bridge.py
echo
echo "== Smoke-marked tests =="
rg -n '`@pytest`\.mark\.smoke' tests/
echo
echo "== Gating-marked tests =="
rg -n '`@pytest`\.mark\.gating' tests/
echo
echo "== Imports/usages of hot_plugged_interface_with_address and hot_plugged_second_interface_with_address =="
rg -n -C2 '\bhot_plugged_interface_with_address\b|\bhot_plugged_second_interface_with_address\b' tests/
echo
echo "== Any conftest under tests/network or repo root relevant to l2_bridge =="
fd -a 'conftest.py' tests . | sed 's#^\./##' | sort
for f in $(fd 'conftest.py' tests . | sort); do
echo
echo "--- $f ---"
rg -n -C2 'l2_bridge|set_secondary_static_ip_address|hot_plug_interface_and_set_address|get_guest_vm_interface_name_by_vmi_interface_name|hot_plugged_interface_with_address|hot_plugged_second_interface_with_address' "$f" || true
doneLength of output: 7298 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5419"
echo "== PR head SHA =="
gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid'
echo
echo "== tests/network/l2_bridge/libl2bridge.py lines 200-270 =="
awk 'NR>=200 && NR<=270 {printf "%4d:%s\n", NR, $0}' tests/network/l2_bridge/libl2bridge.py
echo
echo "== tests/network/l2_bridge/test_bridge_nic_hot_plug.py lines 1-240 =="
awk 'NR>=1 && NR<=240 {printf "%4d:%s\n", NR, $0}' tests/network/l2_bridge/test_bridge_nic_hot_plug.py
echo
echo "== tests/network/l2_bridge/test_bridge_nic_hot_plug.py lines 420-710 =="
awk 'NR>=420 && NR<=710 {printf "%4d:%s\n", NR, $0}' tests/network/l2_bridge/test_bridge_nic_hot_plug.py
echo
echo "== Direct callers/usages of set_secondary_static_ip_address =="
rg -n -C2 '\bset_secondary_static_ip_address\s*\(' tests/
echo
echo
echo "== Direct callers/usages of hot_plug_interface_and_set_address =="
rg -n -C2 '\bhot_plug_interface_and_set_address\s*\(' tests/
echo
echo "== Fixture/test usages of hot_plugged_interface_with_address or hot_plugged_second_interface_with_address =="
rg -n -C2 '\bhot_plugged_interface_with_address\b|\bhot_plugged_second_interface_with_address\b' tests/network/l2_bridge/test_bridge_nic_hot_plug.py
echo
echo "== Smoke-marked tests (all repo) =="
rg -n '`@pytest`\.mark\.smoke' tests/ || true
echo
echo "== Gating-marked tests (all repo) =="
rg -n '`@pytest`\.mark\.gating' tests/ || true
echo
echo "== Relevant conftest.py files under tests/network/l2_bridge ancestry =="
for f in tests/conftest.py tests/network/conftest.py tests/network/l2_bridge/conftest.py; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n -C2 'l2_bridge|set_secondary_static_ip_address|hot_plug_interface_and_set_address|get_guest_vm_interface_name_by_vmi_interface_name|hot_plugged_interface_with_address|hot_plugged_second_interface_with_address|label_schedulable_nodes' "$f" || true
fi
done
echo
echo "== GitHub diff files payload with line-bearing patch for anchoring =="
gh api "repos/RedHatQE/openshift-virtualization-tests/pulls/5419/files" --paginate \
--jq '.[] | select(.filename=="tests/network/l2_bridge/libl2bridge.py") | {filename, patch}'Length of output: 47038 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5419"
commit_id="1f09d15e13a19b18975fd10588c3a3d83deab303"
path="tests/network/l2_bridge/libl2bridge.py"
line="210"
body=$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — no smoke-marked tests were found in the affected call graph for `tests/network/l2_bridge/libl2bridge.py::set_secondary_static_ip_address()` or `tests/network/l2_bridge/libl2bridge.py::hot_plug_interface_and_set_address()`.
- **Run gating tests: False** — no gating-marked tests were found in the affected call graph.
**Affected tests to run**
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithOnlyPrimaryInterface::test_basic_connectivity_of_hot_plugged_interface` — `hot_plugged_interface_with_address` → `set_secondary_static_ip_address`
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithOnlyPrimaryInterface::test_connectivity_of_hot_plugged_jumbo_interface` — `hot_plugged_jumbo_interface_with_address` / `hot_plugged_jumbo_interface_in_utility_vm` → `hot_plug_interface_and_set_address` → `set_secondary_static_ip_address` (`special_infra`, `jumbo_frame`)
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithOnlyPrimaryInterface::test_connectivity_of_hot_plugged_sriov_interface` — direct call to `hot_plug_interface_and_set_address` → `set_secondary_static_ip_address` (`special_infra`, `sriov`)
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithSecondaryInterface::test_basic_connectivity_vm_with_secondary_and_hot_plugged_interfaces` — `hot_plugged_second_interface_with_address` → `set_secondary_static_ip_address`
**Real tests (cluster required)**
Error path (the fix):
`pytest tests/network/l2_bridge/test_bridge_nic_hot_plug.py -k 'test_basic_connectivity_vm_with_secondary_and_hot_plugged_interfaces' -sv`
Expected: the hot-plugged secondary interface gets its static IP via the passed `guest_device_name` path and connectivity succeeds without re-querying VMI interface status.
Happy path (regression):
`pytest tests/network/l2_bridge/test_bridge_nic_hot_plug.py -k 'test_connectivity_of_hot_plugged_jumbo_interface or test_connectivity_of_hot_plugged_sriov_interface' -sv`
Expected: the wrapper path `hot_plug_interface_and_set_address()` still configures the interface and end-to-end connectivity passes for jumbo and SR-IOV hot-plug flows.
EOF
)
gh api \
"repos/${repo}/pulls/${pr}/comments" \
-X POST \
-f body="$body" \
-f commit_id="$commit_id" \
-f path="$path" \
-F line="$line" \
-f side="RIGHT"Length of output: 3182 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5419"
body=$(cat <<'EOF'
**Test Execution Plan**
- **Run smoke tests: False** — no smoke-marked tests were found in the affected call graph for `tests/network/l2_bridge/libl2bridge.py::set_secondary_static_ip_address()` or `tests/network/l2_bridge/libl2bridge.py::hot_plug_interface_and_set_address()`.
- **Run gating tests: False** — no gating-marked tests were found in the affected call graph.
**Affected tests to run**
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithOnlyPrimaryInterface::test_basic_connectivity_of_hot_plugged_interface` — `hot_plugged_interface_with_address` → `set_secondary_static_ip_address`
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithOnlyPrimaryInterface::test_connectivity_of_hot_plugged_jumbo_interface` — `hot_plugged_jumbo_interface_with_address` / `hot_plugged_jumbo_interface_in_utility_vm` → `hot_plug_interface_and_set_address` → `set_secondary_static_ip_address` (`special_infra`, `jumbo_frame`)
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithOnlyPrimaryInterface::test_connectivity_of_hot_plugged_sriov_interface` — direct call to `hot_plug_interface_and_set_address` → `set_secondary_static_ip_address` (`special_infra`, `sriov`)
- `tests/network/l2_bridge/test_bridge_nic_hot_plug.py::TestHotPlugInterfaceToVmWithSecondaryInterface::test_basic_connectivity_vm_with_secondary_and_hot_plugged_interfaces` — `hot_plugged_second_interface_with_address` → `set_secondary_static_ip_address`
**Real tests (cluster required)**
Error path (the fix):
`pytest tests/network/l2_bridge/test_bridge_nic_hot_plug.py -k 'test_basic_connectivity_vm_with_secondary_and_hot_plugged_interfaces' -sv`
Expected: the hot-plugged secondary interface gets its static IP via the passed `guest_device_name` path and connectivity succeeds without re-querying VMI interface status.
Happy path (regression):
`pytest tests/network/l2_bridge/test_bridge_nic_hot_plug.py -k 'test_connectivity_of_hot_plugged_jumbo_interface or test_connectivity_of_hot_plugged_sriov_interface' -sv`
Expected: the wrapper path `hot_plug_interface_and_set_address()` still configures the interface and end-to-end connectivity passes for jumbo and SR-IOV hot-plug flows.
EOF
)
gh pr comment "$pr" --repo "$repo" --body "$body"Length of output: 2783
CRITICAL: I could not complete the required inline comment workflow because the GitHub integration does not have permission to write PR comments on Why this matters: the requested task requires creating either an inline review comment or, on fallback, a regular PR comment. Both API calls failed with Here is the prepared Test Execution Plan content: Test Execution Plan
Affected tests to run
Real tests (cluster required) Expected: the hot-plugged secondary interface gets its static IP via the passed Happy path (regression): Expected: the wrapper path |
10 participants
What this PR does / why we need it:
All callers of
set_secondary_static_ip_address()already have the guest device name from thehot_plug_interface()return value, but the function re-queries VMI status to look it up again viaget_guest_vm_interface_name_by_vmi_interface_name().Add an optional
guest_device_nameparameter so callers can pass the name they already have, skipping the redundant lookup. Callers without it still fall back to the VMI query.Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit