Skip to content

Conversation

@maxime-leroy
Copy link
Collaborator

@maxime-leroy maxime-leroy commented Nov 19, 2025

Add a smoke test that brings up a FRR+Grout router and an FRR BGP peer connected over IPv6 (BGP6) and exchanges VPNv4 routes using SRv6. The test passes when both sides learn the remote IPv4 /24 and hosts in each VRF can ping each other.

Summary by CodeRabbit

  • Tests

    • Added an end-to-end shell test that validates VPNv4 route exchange over BGP6 with SRv6 across multi-namespace FRR deployments, automating topology and SRv6/BGP setup, route-learning checks with timeouts, connectivity verification, and clear failure reporting.
  • Bug Fixes

    • Simplified datapath handling to consistently derive interface, stats, and tracing from the actual packet interface, improving VRF and stats reporting and removing an obsolete invalid-interface code path.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds smoke/bgp6_srv6_frr_test.sh, a shell test that builds multi-namespace FRR instances, configures IPv6/SRv6 locators, VRFs, and BGP6/VPNv4 sessions via vtysh/grcli, polls for VPNv4 route exchange with timeouts, and verifies end-to-end connectivity with pings. Updates modules/infra/datapath/loop_xvrf.c to remove the INVALID_IFACE path and use eth_data->iface consistently for interface selection, stats, tracing flags, and VRF id.

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a smoke test for BGP6 with VPN4 SRv6, which matches the primary file addition and PR objectives.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8584559 and 6aa4bdf.

⛔ Files ignored due to path filters (1)
  • docs/graph.svg is excluded by !**/*.svg
📒 Files selected for processing (2)
  • modules/infra/datapath/loop_xvrf.c (1 hunks)
  • smoke/bgp6_srv6_frr_test.sh (1 hunks)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
smoke/bgp6_srv6_frr_test.sh (1)

72-72: Remove duplicate command.

Line 72 duplicates the link-up command from line 69 with no intervening changes. This is harmless but unnecessary.

 ip -n ns-a link set eth0 up
-ip -n ns-a link set eth0 up
 ip -n ns-a addr add 16.0.0.2/24 dev eth0
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37521c7 and 3e24d96.

📒 Files selected for processing (1)
  • smoke/bgp6_srv6_frr_test.sh (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.sh

⚙️ CodeRabbit configuration file

**/*.sh: - Don't bother about unquoted shell variables.

Files:

  • smoke/bgp6_srv6_frr_test.sh
🧠 Learnings (2)
📓 Common learnings
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:88-122
Timestamp: 2025-09-09T06:02:47.703Z
Learning: When parsing IPv6 routing headers in SRv6 processing, bounds checking for the full extension header length and validation of the routing header structure is needed before accessing fields like hdr_len, last_entry, segments_left, and type.
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.

Applied to files:

  • smoke/bgp6_srv6_frr_test.sh
🧬 Code graph analysis (1)
smoke/bgp6_srv6_frr_test.sh (2)
smoke/_init_frr.sh (5)
  • create_interface (8-23)
  • set_vrf_iface (68-77)
  • set_ip_address (25-56)
  • set_ip_route (79-119)
  • start_frr_on_namespace (239-286)
smoke/_init.sh (2)
  • netns_add (113-121)
  • fail (104-107)
🪛 Shellcheck (0.11.0)
smoke/bgp6_srv6_frr_test.sh

[warning] 38-38: Quote this to prevent word splitting.

(SC2046)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
  • GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
  • GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
  • GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
  • GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
  • GitHub Check: rpm
  • GitHub Check: deb
🔇 Additional comments (1)
smoke/bgp6_srv6_frr_test.sh (1)

40-46: Verify whether set_vrf_iface call is necessary.

Based on prior learnings from this codebase, VRF interfaces (gr-vrf) are automatically created when an interface is added to a non-existing VRF via port_add. Line 42 calls set_vrf_iface 1 explicitly after line 41 creates interface p1 in vrf 1. Verify whether line 42 is still needed or if it is now redundant given automatic VRF creation.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
modules/infra/datapath/loop_xvrf.c (1)

35-56: Iface/ptype invariants and minor stats comment nit

  • Lines 38–41: with the else falling through to IP6_INPUT, any packet that is not tagged RTE_PTYPE_L3_IPV4 will now be treated as IPv6. This assumes only IPv4/IPv6 L3 traffic ever reaches loop_xvrf. Please confirm that there is no path that could deliver non‑L3 or other types here (e.g. ARP, unknown), otherwise they’d be misrouted to ip6_input instead of being dropped/handled via a dedicated edge.
  • Lines 47–54: we now unconditionally dereference eth_data->iface for stats and tracing (id, flags, vrf_id). That’s fine if the graph always sets eth_data->iface before reaching this node, but it removes the previous safety net (INVALID_IFACE/GR_VRF_ID_ALL). Worth double‑checking that there is no control‑plane or corner path where eth_data->iface could be NULL or carry a dummy iface, otherwise this becomes a hard crash. If the invariant holds, an inline comment or an assert(eth_data->iface != NULL); could make it explicit.
  • Line 46–49: the comment says “increment tx stats” but the code updates rx_packets/rx_bytes. If that’s intentional (RX from the VRF’s perspective), consider clarifying the comment to avoid confusion.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 76ea8c9 and d0142b9.

⛔ Files ignored due to path filters (1)
  • docs/graph.svg is excluded by !**/*.svg
📒 Files selected for processing (1)
  • modules/infra/datapath/loop_xvrf.c (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}

⚙️ CodeRabbit configuration file

**/*.{c,h}: - gr_vec_*() functions cannot fail. No need to check their return value.

  • gr_vec_free(x) always sets x = NULL. There is no risk of double free.
  • ec_node_*() functions consume their ec_node arguments. No leaks on error.
  • rte_node->ctx is an uint8_t array of size 16, not a pointer.
  • Don't suggest to replace assert() with graceful error checking.
  • We compile with -std=gnu2x. Unnamed parameters in function definitions are valid.

Files:

  • modules/infra/datapath/loop_xvrf.c
🧠 Learnings (5)
📓 Common learnings
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.
📚 Learning: 2025-11-05T14:28:28.817Z
Learnt from: rjarry
Repo: DPDK/grout PR: 372
File: modules/infra/datapath/loop_input.c:53-56
Timestamp: 2025-11-05T14:28:28.817Z
Learning: In modules/infra/datapath/loop_input.c, the l3_edges array is intentionally indexed using network byte order (rte_be16_t) for EtherType values. Both loopback_input_add_type() registration and loopback_input_process() lookup use network byte order consistently, so no byte order conversion is needed when accessing l3_edges.

Applied to files:

  • modules/infra/datapath/loop_xvrf.c
📚 Learning: 2025-11-05T13:55:26.189Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 372
File: smoke/cross_vrf_forward_test.sh:18-18
Timestamp: 2025-11-05T13:55:26.189Z
Learning: In the DPDK/grout codebase, VRF interfaces (named gr-vrf<id>) are automatically created when an interface is added to a non-existing VRF using port_add. The VRF creation is handled automatically by the event system in vrf_netlink.c, so no explicit VRF interface creation commands are needed in test scripts.

Applied to files:

  • modules/infra/datapath/loop_xvrf.c
📚 Learning: 2025-09-09T09:22:31.596Z
Learnt from: maxime-leroy
Repo: DPDK/grout PR: 309
File: modules/srv6/datapath/srv6_local.c:97-101
Timestamp: 2025-09-09T09:22:31.596Z
Learning: In SRv6 IPv6 extension header parsing, when is_ipv6_ext[proto] is true, rte_ipv6_get_next_ext() will not return a negative value, making error checking unnecessary. An assert can be used as a defensive measure for future DPDK compatibility.

Applied to files:

  • modules/infra/datapath/loop_xvrf.c
📚 Learning: 2025-11-03T13:28:19.489Z
Learnt from: rjarry
Repo: DPDK/grout PR: 379
File: modules/infra/datapath/port_tx.c:36-46
Timestamp: 2025-11-03T13:28:19.489Z
Learning: In DPDK graph node process callbacks, the return value is only used for statistics and does not affect packet flow or scheduling through the graph. Nodes can return 0 when they haven't processed packets (e.g., when dropping or redirecting due to port state).

Applied to files:

  • modules/infra/datapath/loop_xvrf.c
🧬 Code graph analysis (1)
modules/infra/datapath/loop_xvrf.c (3)
modules/infra/control/gr_iface.h (1)
  • iface_get_stats (102-104)
modules/infra/datapath/gr_mbuf.h (1)
  • gr_mbuf_is_traced (52-54)
modules/infra/datapath/trace.c (1)
  • gr_mbuf_trace_add (592-619)

@maxime-leroy maxime-leroy force-pushed the bgp6_srv6_test branch 2 times, most recently from 3f52ba3 to 8584559 Compare November 20, 2025 08:24
Add a smoke test that brings up a FRR+Grout router and an FRR BGP peer
connected over IPv6 (BGP6) and exchanges VPNv4 routes using SRv6.
The test passes when both sides learn the remote IPv4 /24 and hosts in
each VRF can ping each other.

Signed-off-by: Maxime Leroy <maxime@leroys.fr>
Reviewed-by: Robin Jarry <rjarry@redhat.com>
The interface ifindex and its vrf_id happen to match for most VRFs,
except for VRF 0 where they differ. When tracing packets, we should
record the actual VRF identifier rather than the interface index.

Signed-off-by: Maxime Leroy <maxime@leroys.fr>
Reviewed-by: Robin Jarry <rjarry@redhat.com>
ip_output/ip6_output already performs:
    iface = iface_from_id(nh->iface_id);
    mbuf_data(mbuf)->iface = iface;

xvrf_loop_process was doing this again unnecessarily. Remove the
redundant code.

Signed-off-by: Maxime Leroy <maxime@leroys.fr>
Reviewed-by: Robin Jarry <rjarry@redhat.com>
@rjarry rjarry merged commit 8e31c2f into DPDK:main Nov 20, 2025
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants