net, evpn: refactor helpers for independent endpoint deployment#5248
net, evpn: refactor helpers for independent endpoint deployment#5248servolkov wants to merge 1 commit into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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
Branch Management
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-2
requested review from
frenzyfriday,
nirdothan and
rnetser
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/network/bgp/evpn/test_evpn_connectivity.py (1)
215-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH:
new_l2_endpointis created but never torn downAfter switching to endpoint-object teardown, this test removes the old endpoint but does not clean the newly deployed
new_l2_endpoint. That leaves endpoint lifecycle asymmetric and risks stale netns/veth artifacts in the FRR pod.Suggested fix
new_l2_endpoint = deploy_evpn_l2_endpoint( pod=frr_external_pod.pod, endpoint_ips=[_L2_ENDPOINT_IPV4, _L2_ENDPOINT_IPV6], ) - - assert_evpn_workloads_connectivity( - target_vm=vm_evpn_target, - ref_vm=vm_source_provider, - l2_endpoint=new_l2_endpoint, - l3_endpoint=external_l3_endpoint, - subtests=subtests, - ) + try: + assert_evpn_workloads_connectivity( + target_vm=vm_evpn_target, + ref_vm=vm_source_provider, + l2_endpoint=new_l2_endpoint, + l3_endpoint=external_l3_endpoint, + subtests=subtests, + ) + finally: + teardown_evpn_l2_endpoint(endpoint=new_l2_endpoint)🤖 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/bgp/evpn/test_evpn_connectivity.py` around lines 215 - 231, The test creates new_l2_endpoint by calling deploy_evpn_l2_endpoint() but never cleans it up, leaving the endpoint lifecycle asymmetric and risking stale artifacts in the FRR pod. After the assert_evpn_workloads_connectivity() call completes, add a teardown_evpn_l2_endpoint(endpoint=new_l2_endpoint) call to properly clean up the newly deployed endpoint and maintain symmetric resource management consistent with how external_l2_endpoint is torn down at the beginning.
🤖 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/bgp/evpn/libevpn.py`:
- Around line 214-218: The UUID suffix generation using uuid.uuid4().hex[:3]
only provides 4096 unique combinations. To improve collision resistance for
potential higher parallelism or longer-lived namespaces in future test
scenarios, extend the hex suffix length from 3 characters to 4 or 5 characters
in the suffix assignment. This change requires modifying only the string slicing
operation where the suffix is generated (uuid.uuid4().hex[:3]) to use a longer
substring, which will then be propagated to all the dependent variables like
netns, veth_pod, and veth_ep that use this suffix.
---
Outside diff comments:
In `@tests/network/bgp/evpn/test_evpn_connectivity.py`:
- Around line 215-231: The test creates new_l2_endpoint by calling
deploy_evpn_l2_endpoint() but never cleans it up, leaving the endpoint lifecycle
asymmetric and risking stale artifacts in the FRR pod. After the
assert_evpn_workloads_connectivity() call completes, add a
teardown_evpn_l2_endpoint(endpoint=new_l2_endpoint) call to properly clean up
the newly deployed endpoint and maintain symmetric resource management
consistent with how external_l2_endpoint is torn down at the beginning.
🪄 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: 817e2f6a-73c7-454e-a8cc-4d135427ab67
📒 Files selected for processing (3)
tests/network/bgp/evpn/conftest.pytests/network/bgp/evpn/libevpn.pytests/network/bgp/evpn/test_evpn_connectivity.py
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4994. Overlapping filestests/network/bgp/evpn/test_evpn_connectivity.py |
|
/verified |
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
# Check for smoke and gating markers in evpn tests
echo "=== Smoke markers in evpn ==="
rg -n '`@pytest.mark.smoke`' tests/network/bgp/evpn/
echo "=== Gating markers in evpn ==="
rg -n '`@pytest.mark.gating`' tests/network/bgp/evpn/
echo "=== All test functions in evpn ==="
rg -n '^def test_' tests/network/bgp/evpn/
echo "=== All smoke marker files ==="
rg -l '`@pytest.mark.smoke`' tests/network/bgp/
echo "=== All gating marker files ==="
rg -l '`@pytest.mark.gating`' tests/network/bgp/
echo "=== Fixtures used by test_source_provider_migration ==="
rg -n 'test_source_provider_migration' tests/network/bgp/evpn/
echo "=== All test functions in test_evpn_connectivity.py ==="
rg -n 'def test_' tests/network/bgp/evpn/test_evpn_connectivity.py
echo "=== libevpn imports in test files ==="
rg -rn 'from.*libevpn import\|import.*libevpn' tests/
echo "=== deploy_evpn_bridge callers ==="
rg -n 'deploy_evpn_bridge\|deploy_evpn_l2_endpoint\|deploy_evpn_l3_endpoint\|teardown_evpn_l2_endpoint\|teardown_evpn_l3_endpoint\|deploy_evpn_l3_vrf\|teardown_evpn_l3_vrf' tests/Length of output: 3201 🏁 Script executed: #!/bin/bash
echo "=== test_source_provider_migration signature ==="
rg -n -A 15 'def test_source_provider_migration' tests/network/bgp/evpn/test_evpn_connectivity.py
echo "=== external_l3_vrf fixture usage ==="
rg -n 'external_l3_vrf\|external_l3_endpoint\|external_l2_endpoint' tests/network/bgp/evpn/test_evpn_connectivity.py
echo "=== conftest fixtures defined ==="
rg -n 'def external_l' tests/network/bgp/evpn/conftest.pyLength of output: 1362 🏁 Script executed: #!/bin/bash
# Get the PR diff to find first changed line in conftest.py
gh pr diff 5248 --repo RedHatQE/openshift-virtualization-tests -- tests/network/bgp/evpn/conftest.py | head -40
echo "=== PR number and files ==="
gh pr view 5248 --repo RedHatQE/openshift-virtualization-tests --json files --jq '.files[].path' | head -10Length of output: 555 🏁 Script executed: #!/bin/bash
# Get the PR diff via API to find file positions
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/files \
--jq '.[] | select(.filename == "tests/network/bgp/evpn/conftest.py") | {filename, patch: .patch}' | head -60
echo "=== Get PR commit SHA ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248 \
--jq '{head_sha: .head.sha, base_sha: .base.sha}'Length of output: 2770 🏁 Script executed: #!/bin/bash
# Post inline review comment on first changed line of conftest.py
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/reviews \
--method POST \
--field commit_id="7d5c838432948b48069b9595e09ad4c246a18e46" \
--field body="" \
--field event="COMMENT" \
--field "comments[][path]=tests/network/bgp/evpn/conftest.py" \
--field "comments[][line]=30" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=**Test Execution Plan**
**Run smoke tests: False** — No \`@pytest.mark.smoke\` markers found in \`tests/network/bgp/evpn/\` or any parent directory. Verified with \`rg -l '`@pytest.mark.smoke`' tests/network/bgp/\`.
**Run gating tests: False** — No \`@pytest.mark.gating\` markers found in \`tests/network/bgp/evpn/\` or any parent directory. Verified with \`rg -l '`@pytest.mark.gating`' tests/network/bgp/\`.
---
**Affected tests to run** — all changes are scoped to \`tests/network/bgp/evpn/\`:
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_stretched_l2_connectivity_udn_vm_and_external_provider\` — uses \`external_l2_endpoint\` (teardown path changed: now passes \`EvpnEndpoint\` object)
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_stretched_l2_connectivity_is_preserved_over_live_migration\` — uses \`external_l2_endpoint\` fixture
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_udn_vm_and_external_provider\` — uses \`external_l3_endpoint\` (now depends on new \`external_l3_vrf\` fixture; VRF lifecycle separated)
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_is_preserved_over_live_migration\` — uses \`external_l3_endpoint\` fixture
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_source_provider_migration\` — directly modified; \`cudn_evpn_layer2\` param replaced with \`external_l2_endpoint\`
---
**Real tests (cluster required)**
Happy path (regression — verify all existing EVPN scenarios still pass):
\`\`\`
pytest tests/network/bgp/evpn/
\`\`\`
Expected: 7/7 pass.
Fix path (verify new \`external_l3_vrf\` shared-infra fixture and independent endpoint deployment):
\`\`\`
pytest tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_udn_vm_and_external_provider tests/network/bgp/evpn/test_evpn_connectivity.py::test_source_provider_migration -v
\`\`\`
Expected: L3 VRF deployed once via \`external_l3_vrf\`; \`external_l3_endpoint\` and \`external_l2_endpoint\` each get UUID-suffixed netns/veth without conflicts."Length of output: 4306 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 5248 --json headRefOid,commits --jq '{head: .headRefOid}'
echo "=== files changed now ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/files --jq '.[].filename'
echo "=== diff for libevpn.py and conftest.py ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/files --jq '.[] | select(.filename=="tests/network/bgp/evpn/conftest.py") | .patch' | head -60
echo "=== check for smoke/gating markers ==="
rg -l '`@pytest.mark.smoke`' tests/network/bgp/ || true
rg -l '`@pytest.mark.gating`' tests/network/bgp/ || trueLength of output: 3067 🏁 Script executed: #!/bin/bash
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/reviews \
--method POST \
--field commit_id="48f365337133047a3e74249cff687348dfa219af" \
--field body="" \
--field event="COMMENT" \
--field "comments[][path]=tests/network/bgp/evpn/conftest.py" \
--field "comments[][line]=30" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=**Test Execution Plan**
**Run smoke tests: False** — No \`@pytest.mark.smoke\` markers found under \`tests/network/bgp/\` (verified with \`rg -l '`@pytest.mark.smoke`' tests/network/bgp/\`, no matches).
**Run gating tests: False** — No \`@pytest.mark.gating\` markers found under \`tests/network/bgp/\` (verified with \`rg -l '`@pytest.mark.gating`' tests/network/bgp/\`, no matches).
---
**Affected tests to run** — all changes are scoped to \`tests/network/bgp/evpn/\`. Latest commit (48f3653) extracted \`deploy_evpn_l3_vrf\`/\`teardown_evpn_l3_vrf\` into a dedicated \`external_l3_vrf\` fixture, removing the redundant VRF cleanup previously inlined in \`external_l3_endpoint\`:
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_stretched_l2_connectivity_udn_vm_and_external_provider\` — uses \`external_l2_endpoint\` (teardown now passes \`EvpnEndpoint\` object)
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_stretched_l2_connectivity_is_preserved_over_live_migration\` — uses \`external_l2_endpoint\` fixture
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_udn_vm_and_external_provider\` — uses \`external_l3_endpoint\`, which now depends on new module-scoped \`external_l3_vrf\` fixture
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_is_preserved_over_live_migration\` — uses \`external_l3_endpoint\` fixture
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_source_provider_migration\` — directly modified; \`cudn_evpn_layer2\` param replaced with \`external_l2_endpoint\`
---
**Real tests (cluster required)**
Happy path (regression — full suite):
\`\`\`
pytest tests/network/bgp/evpn/
\`\`\`
Expected: 7/7 pass (author reported this locally per PR thread).
Fix path (verify VRF-only teardown ordering and shared-infra reuse across L3 endpoint + VRF fixtures):
\`\`\`
pytest tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_udn_vm_and_external_provider tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_is_preserved_over_live_migration -v
\`\`\`
Expected: \`external_l3_vrf\` deploys/tears down VRF once (module scope); \`external_l3_endpoint\` teardown only removes its own netns/veth without double-tearing-down the VRF."Length of output: 4608 Test execution plan posted as an inline review comment on |
Separate shared EVPN infrastructure (bridge VLAN/VNI mappings, L3 VRF) from per-endpoint setup so multiple endpoints can coexist independently. - Move VLAN/VNI mappings into deploy_evpn_bridge (shared, created once) - Extract deploy_evpn_l3_vrf/teardown_evpn_l3_vrf from L3 endpoint - Use uuid-suffixed netns/veth names for endpoint isolation - Teardown functions take EvpnEndpoint instead of raw pod/vni args - Simplify endpoint teardown to netns removal only (kernel auto-removes veth peer) - Update test_source_provider_migration to use new signatures Signed-off-by: Sergei Volkov <sevolkov@redhat.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
servolkov
force-pushed
the
net/evpn-helpers-refactor
branch
from
48f3653 to
2488600
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
|
/verified |
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
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248 --jq '{head_sha: .head.sha, base_sha: .base.sha}'
echo "=== files changed ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/files --jq '.[].filename'
echo "=== diff for conftest.py first hunk ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/files --jq '.[] | select(.filename=="tests/network/bgp/evpn/conftest.py") | .patch' | head -30Length of output: 1942 🏁 Script executed: #!/bin/bash
echo "=== smoke markers ==="
rg -l '`@pytest`\.mark\.smoke' tests/network/bgp/ || echo "none"
echo "=== gating markers ==="
rg -l '`@pytest`\.mark\.gating' tests/network/bgp/ || echo "none"
echo "=== check git log for latest commits ==="
git log --oneline -5Length of output: 625 🏁 Script executed: #!/bin/bash
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5248/reviews \
--method POST \
--field commit_id="2488600e13f5d00d0989ecca20a50e033370ca42" \
--field body="" \
--field event="COMMENT" \
--field "comments[][path]=tests/network/bgp/evpn/conftest.py" \
--field "comments[][line]=30" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=**Test Execution Plan**
**Run smoke tests: False** — No \`@pytest.mark.smoke\` markers found under \`tests/network/bgp/\` (verified with \`rg -l '`@pytest.mark.smoke`' tests/network/bgp/\`, no matches).
**Run gating tests: False** — No \`@pytest.mark.gating\` markers found under \`tests/network/bgp/\` (verified with \`rg -l '`@pytest.mark.gating`' tests/network/bgp/\`, no matches).
---
**Affected tests to run** — all changes are scoped to \`tests/network/bgp/evpn/\`:
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_stretched_l2_connectivity_udn_vm_and_external_provider\` — uses \`external_l2_endpoint\` (teardown now passes \`EvpnEndpoint\` object)
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_stretched_l2_connectivity_is_preserved_over_live_migration\` — uses \`external_l2_endpoint\` fixture
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_udn_vm_and_external_provider\` — uses \`external_l3_endpoint\`, which now depends on module-scoped \`external_l3_vrf\` fixture (shared VRF setup/teardown)
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_is_preserved_over_live_migration\` — uses \`external_l3_endpoint\` fixture
- \`tests/network/bgp/evpn/test_evpn_connectivity.py::test_source_provider_migration\` — directly modified; \`cudn_evpn_layer2\` param replaced with \`external_l2_endpoint\`
---
**Real tests (cluster required)**
Happy path (regression — full suite):
\`\`\`
pytest tests/network/bgp/evpn/
\`\`\`
Expected: 7/7 pass (author verified locally per PR thread comments).
Fix path (verify shared \`external_l3_vrf\` lifecycle and UUID-suffixed netns/veth isolation across endpoints):
\`\`\`
pytest tests/network/bgp/evpn/test_evpn_connectivity.py::test_routed_l3_connectivity_udn_vm_and_external_provider tests/network/bgp/evpn/test_evpn_connectivity.py::test_source_provider_migration -v
\`\`\`
Expected: \`external_l3_vrf\` deploys/tears down VRF once (module scope); \`external_l3_endpoint\`/\`external_l2_endpoint\` each get unique netns/veth without conflicts and teardown removes only their own namespace."Length of output: 4443 Test execution plan posted as an inline review comment on |
10 participants
What this PR does / why we need it:
Separate shared EVPN infrastructure (bridge VLAN/VNI mappings, L3 VRF) from per-endpoint setup so multiple endpoints can coexist independently.
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
NONE
Summary by CodeRabbit
Release Notes