[vpp] saivpp VXLAN L3 decap: source-independent decap, inner source MAC, BD teardown fix - #1996
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
When a packet is routed into an L3 VXLAN tunnel, VPP builds the inner Ethernet header using the tunnel interface hardware MAC as the source. The interface default is VPPs auto-generated MAC (02:fe:..), which HW ASICs and the VNET decap tests do not expect. Override the tunnel interface MAC with the router MAC after creating the encap, via a new sw_interface_set_mac_by_index binary API helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
…ecap
Extend the VPP virtual-switch SAI tunnel adaptation (vslib/vpp) to
program source-independent VNET tunnel termination, so decap matches on
local dst + VNI while ignoring the outer source IP. This supports RIOT /
secondary-VTEP topologies where encapsulated traffic may arrive from a
VTEP other than the primary tunnel source, which previously failed to
decap.
vslib/vpp/TunnelManager.{cpp,h}: add the decap-any tunnel-term wiring.
Stacked on top of the existing tunnel inner-source-MAC override.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa
Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
eba10f6 to
717dd0a
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Address code-review findings on the L3 VXLAN VNET decap term handling in TunnelManager: - Refcount the VRF0 local-receive route by VTEP IP. Multiple VNIs can share one secondary VTEP, so program the route on the first term and remove it only on the last. Tearing down one VNI no longer removes the shared route and breaks decap for its siblings. - Check every VPP API return in create_vxlan_decap_term (interface state, L2 bridge bind, VRF set, both BVI address adds) and roll back the BD/BVI on failure instead of proceeding half-programmed and returning success. - Set decap_any on both L3 VNET decap tunnel requests so the decap term is source-independent. This covers the secondary-VTEP path (create_vxlan_decap_term) and the primary-VTEP L3 VNET path (tunnel_encap_nexthop_action, VIRTUAL_ROUTER_ID_TO_VNI mappers). L2 EVPN tunnels (create_l2_vxlan_tunnel_for_vni) intentionally leave decap_any unset to keep exact outer-source validation. SaiVppXlate signals the flag to VPP via the high bit of decap_next_index, forcing a valid default next index so the flag stays distinguishable. - Install L3 (VNI_TO_VIRTUAL_ROUTER_ID) decap terms from the tunnel-create path as well, so a TUNNEL created after its TUNNEL_MAP_ENTRY still gets its decap term programmed. A shared install_l3_vxlan_decap_terms helper keeps the map-entry and tunnel-create paths idempotent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
717dd0a to
2329bf9
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
remove_vpp_vxlan_decap deleted the bridge-domain while the VXLAN tunnel interface was still a member of it. The tunnel is only removed later by remove_vpp_vxlan_encap, so at BD-delete time the BD still had a member and vpp_bridge_domain_add_del(is_add=0) failed with -120 (bridge-domain in use). This surfaced as a syncd ERR log during L3 VNET decap teardown and tripped loganalyzer in the vxlan ECMP tests, although it was functionally benign (the BD was reclaimed once the tunnel was later removed). Detach the tunnel interface from the BD (set_sw_interface_l2_bridge_by_index with is_add=false) before freeing the bd_id and deleting the bridge-domain. Validated on a t1-lag-vpp testbed: vxlan/test_vxlan_ecmp.py no longer emits the -120 bridge-domain delete error and leaves zero bridge-domains in use. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
2329bf9 to
a9b16c4
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Bring the saivpp VXLAN branch up to date with sonic-net/sonic-sairedis master so it builds against and re-runs the current Azure pipeline. No code conflicts; merge is clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
The sonic-sairedis swsslogentercheck.sh test requires every method to call SWSS_LOG_ENTER() as its first statement. The vxlan_decap_term_set_dst helper in TunnelManager.cpp was missing it, failing "Run sonic sairedis unit tests" (1 of 11 tests) in the Build and BuildAsan legs. Add the macro to match the rest of the file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
3af0195 to
9aa0dca
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR enhances the sonic-vpp (vslib/vpp) VXLAN implementation to support correct L3 VNET decapsulation behavior (including source-independent decap) and hardens VXLAN decap setup/teardown to avoid known failure modes during tunnel lifecycle operations.
Changes:
- Add support for source-independent VXLAN decap terms (“decap_any”) by encoding a flag into
decap_next_indexfor patched VPP VXLAN behavior. - Add a VPP binary-API helper to set a tunnel interface MAC by
sw_if_index, and use it so L3 VXLAN inner source MAC matches the router MAC. - Add L3 secondary-VTEP decap term install/teardown logic and a late-tunnel hook to handle out-of-order tunnel vs. map-entry creation; adjust BD teardown ordering to avoid BD-in-use errors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vslib/vpp/vppxlate/SaiVppXlate.h | Adds decap-any flag encoding definitions, extends VXLAN tunnel struct, and declares sw_interface_set_mac_by_index. |
| vslib/vpp/vppxlate/SaiVppXlate.c | Implements sw_interface_set_mac_by_index and encodes decap_any into the wire decap_next_index. |
| vslib/vpp/TunnelManager.h | Introduces APIs and state for L3 secondary-VTEP decap term install/teardown and refcounting. |
| vslib/vpp/TunnelManager.cpp | Programs decap-any for L3 VNET, sets tunnel MAC to router MAC, adds L3 decap term install/remove logic, and fixes BD teardown ordering. |
| vslib/vpp/SwitchVpp.cpp | Invokes the late-tunnel hook after VXLAN tunnel creation to backfill missing L3 decap terms. |
|
Thanks — the updates in The IPv6 fix is better than what I suggested: forcing On the third stale reference at One thing I'd like to flag before this mergesI think the Copilot comment on the early return in sai_object_id_t term_oid;
sai_deserialize_object_id(map_entry_serialized_oid, term_oid);
if (m_vxlan_decap_term_map.find(term_oid) != m_vxlan_decap_term_map.end()) {
return SAI_STATUS_SUCCESS; // already installed
}but the work it guards is per-tunnel — the function goes on to loop over every tunnel referencing the mapper and create a decap term for each, storing them as a vector under that one key. So:
The pure late-tunnel case you designed the hook for is safe, because when no tunnel exists yet Making the guard per (map entry, VTEP) rather than per map entry would close it — e.g. keep the vector but skip only the tunnels whose VTEP IP already has a term recorded under that entry, so a newly-appearing tunnel still gets one. That also makes the function properly idempotent in the sense the comment claims. Worth confirming against a run of On the other three Copilot commentsFor what it's worth, I read the one about the The two about unchecked return codes ( |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Thanks @yejianquan — you're right on all three, and I've pushed the fix as Per-(map entry, VTEP) guardImplemented exactly as you suggested. I kept the vector under the map-entry key but replaced the whole-entry early-return with per-VTEP idempotency. The function now:
So the step-3 scenario you described — tunnel The two unchecked return codesBoth added as (non-fatal) error logs:
The
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
vslib/vpp/TunnelManager.cpp:470
- remove_vpp_vxlan_decap() returns the bridge-domain id to dynamic_bd_id_pool before attempting to delete the bridge domain. If vpp_bridge_domain_add_del(bd_id, false) fails (e.g., BD still in use), the id can be reallocated while the old bridge-domain still exists; the current code also logs a successful delete unconditionally.
This issue also appears on line 1305 of the same file.
m_switch_db->dynamic_bd_id_pool.free(tunnel_data.bd_id);
refresh_interfaces_list();
//bd is create automatically when the fist interface is add to it but requires manual deletion
vpp_bridge_domain_add_del(tunnel_data.bd_id, false);
vslib/vpp/TunnelManager.cpp:1309
- remove_vxlan_decap_term() frees the bridge-domain id back to dynamic_bd_id_pool before deleting the bridge-domain (and ignores the delete return code). If vpp_bridge_domain_add_del() fails, the id can be reused while the old bridge-domain still exists; the current code also logs successful teardown regardless of BD delete result.
snprintf(hw_bvi_ifname, sizeof(hw_bvi_ifname), "bvi%u", tunnel_data.bd_id);
delete_bvi_interface(hw_bvi_ifname);
m_switch_db->dynamic_bd_id_pool.free(tunnel_data.bd_id);
refresh_interfaces_list();
vpp_bridge_domain_add_del(tunnel_data.bd_id, false);
vslib/vpp/TunnelManager.cpp:1297
- remove_vxlan_decap_term() deletes the decap-only VXLAN tunnel without setting req.decap_any=true. Since vpp_vxlan_tunnel_add_del encodes the source-independent decap behavior via the high-bit flag, omitting it on delete can leave the decap-any entry/tunnel state behind in VPP.
req.instance = ~0;
req.vni = tunnel_data.vni;
req.decap_next_index = ~0;
sai_ip_address_t src_nc = tunnel_data.src_ip;
sai_ip_address_t_to_vpp_ip_addr_t(src_nc, req.src_address);
vslib/vpp/SwitchVpp.cpp:1528
- handle_l3_vxlan_tunnel_create() is invoked unconditionally even if create_l2_vxlan_tunnel() fails and SwitchVpp::create returns a failure status. That can program L3 secondary-VTEP decap state for a tunnel create that ultimately fails, potentially leaking VPP resources.
sai_status_t status = m_tunnel_mgr.create_l2_vxlan_tunnel(object_id, sw_if_index);
SWSS_LOG_INFO("L2 VXLAN tunnel create for %s: status=%d sw_if_index=%u",
serializedObjectId.c_str(), status, sw_if_index);
// Late-tunnel hook: install any L3 secondary-VTEP decap terms whose
// TUNNEL_MAP_ENTRY was created before this tunnel existed (M3).
m_tunnel_mgr.handle_l3_vxlan_tunnel_create(object_id);
return status;
install_l3_vxlan_decap_terms() guarded idempotency on the tunnel map entry OID as a whole, but the decap terms recorded under an entry are per-tunnel (one per VTEP source IP). A tunnel created after the entry was first processed (e.g. a second VTEP referencing the same mapper) hit the whole-entry early-return and never got its own decap term, so decap for that VTEP silently failed. Make idempotency per-(map entry, VTEP): skip only VTEPs already recorded under the entry (or installed earlier in the same pass) and append newly appearing VTEPs to the entry's vector instead of overwriting it. Reuse the existing saivs::sai_ip_address_equal helper for the VTEP comparison. Also check and log the previously ignored return codes of sw_interface_set_mac_by_index() (router MAC on the encap tunnel) and the unbridge set_sw_interface_l2_bridge_by_index() in remove_vpp_vxlan_decap() so these failures are diagnosable instead of silent. Verified on a t1-lag-vpp testbed: test_vxlan_multiple_tunnels.py 16/16 passed, BGP 24/24, no new cores. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
5553d17 to
fc3bb73
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@yejianquan quick follow up on your cross note about the map entry granularity early return in Fixed in Worth recording for the merge notes though. I ran For the record the run stayed healthy end to end, BGP 24 of 24 and no new cores. If you think it's worth locking down I can add a dedicated multi VTEP ordering case to the sonic-mgmt set as a follow up so this path gets explicit coverage. Co-authored with CoPilot |
| return ret; | ||
| } | ||
|
|
||
| int sw_interface_set_mac_by_index (uint32_t sw_if_index, uint8_t *mac_address) |
There was a problem hiding this comment.
This function is very similar to sw_interface_set_mac. Please refactor to reuse the common code
There was a problem hiding this comment.
Resolved. Please re-review @yue-fred-gao
| req.src_port = m_vxlan_port; | ||
| req.instance = ~0; | ||
| req.vni = tunnel_data.vni; | ||
| req.decap_next_index = ~0; |
There was a problem hiding this comment.
do we need to set decap_any flag for deletion?
There was a problem hiding this comment.
No. On delete VPP finds the tunnel by its key (dst, vni, encap_fib, src-port, plus src for v4), which does not include decap_next_index, then uses the flag it stored at create (t->decap_any) to also remove the wildcard entry and clear L3 promiscuous. So the remove request does not need to carry decap_any; setting it would be a no-op. See the delete path of vnet_vxlan_add_del_tunnel in patch 0017.
Co-authored with CoPilot
| return createMirrorSession(object_id, switch_id, attr_count, attr_list); | ||
| // Late-tunnel hook: install any L3 secondary-VTEP decap terms whose | ||
| // TUNNEL_MAP_ENTRY was created before this tunnel existed (M3). | ||
| m_tunnel_mgr.handle_l3_vxlan_tunnel_create(object_id); |
There was a problem hiding this comment.
This is only added to create. Do we need to handle tunnel being deleted when tunnel map entries are kept?
There was a problem hiding this comment.
The secondary VTEP decap terms are anchored to the map entries, not the tunnel: handle_l2_vxlan_tunnel_map_entry_removal frees them (via m_vxlan_decap_term_map) on TUNNEL_MAP_ENTRY removal. Only create needs the late tunnel hook, because a map entry can arrive before its tunnel; delete has no symmetric race since removing the entries frees the terms.
You are right about the gap: a TUNNEL deleted while its TUNNEL_MAP_ENTRYs are kept would leak, since nothing sweeps the terms then. Normal VNET teardown removes the entries first, so it does not happen in practice, but I added a symmetric handle_l3_vxlan_tunnel_removal sweep as defense in depth, pushed in 9ccc457. It runs before remove_internal (while the tunnel and its DECAP_MAPPERS links are still resolvable) and is refcount aware, since terms are deduplicated per VTEP: it frees a term only once no surviving VXLAN tunnel still references that mapper with the same VTEP source IP.
Co-authored with CoPilot
|
|
||
| is_local_skip = false; | ||
|
|
||
| // Primary-VTEP guard: if this VTEP IP already belongs to one of our own |
There was a problem hiding this comment.
I feel this is very SONiC specific behaviour. Is there stronger hint from other SAI API calls? I think I don't quite understand the sequence of SAI API calls leading to this. Can you please share the SAI calls captured from sairedis.rec? Essentially, how SONiC express primary and secondary vtep? how decap-any is derived from SAI attributes?
There was a problem hiding this comment.
It is not a heuristic. decap_any is set only on the L3 VNET path, both mapper directions. On the encap side (VIRTUAL_ROUTER_ID_TO_VNI, in tunnel_encap_nexthop_action) every tunnel built toward a remote peer is created with decap_any=true, so the local VTEP decaps VXLAN from any outer source. On the decap side (VNI_TO_VIRTUAL_ROUTER_ID, via install_l3_vxlan_decap_terms) the same flag is set, but that explicit term is only created for a VTEP source IP that is not one of our own interfaces: create_vxlan_decap_term probes with vpp_sw_interface_find_by_ip and skips the local (primary) VTEP, since it is already decapped by the encap path. The L2 EVPN path (VLAN mappers) never sets decap_any, so exact source matching is preserved there. So primary VTEP = the ENCAP_SRC_IP that matches a local interface (Loopback0); a non-local VTEP source is the only case that gets its own explicit term plus a VRF0 local-receive. Remote peer VTEPs, the tunnel destinations, are separate and only appear as tunnel-encap nexthops. The line at 1131 is just the placeholder dst for the decap only term: VPP needs a valid dst different from src, so I set a fixed high byte for v6 and 0.0.0.0 for v4. That is a VPP API constraint, not where decap-any is decided.
On a stronger SAI hint, and to answer how SONiC expresses this: I captured a sairedis.rec from a t1-lag-vpp VNET run. Per VNET, SONiC creates four tunnel maps (L2 VLAN<->VNI and L3 VirtualRouter<->VNI), one VXLAN TUNNEL with ENCAP_SRC_IP set to the local VTEP (10.1.0.32) and PEER_MODE P2MP, and a single TUNNEL_TERM_TABLE_ENTRY of type P2MP with DST_IP equal to that local VTEP and no SRC_IP. Per the SAI spec P2MP matches an exact dst with a masked (any) source, so that one term already expresses source independent (decap-any) decap for the local VTEP. Primary VTEP = ENCAP_SRC_IP (the same address as the term DST_IP); the remote peer VTEPs show up only as SAI_NEXT_HOP_TYPE_TUNNEL_ENCAP nexthops (IPs like 100.0.1.10), with no decap term of their own. So the SAI native decap-any signal is the P2MP term type, and a cleaner v2 would key VXLAN decap-any off it rather than the mapper type plus local-interface check. saivpp already reads the term type on the IPinIP path but the VXLAN term path currently just stores the object. Note this single-loopback topology never emits a non-local VTEP source, so the explicit secondary-term branch above is not exercised here; decap is entirely the primary encap path plus this one P2MP term.
Excerpt:
c|SAI_OBJECT_TYPE_TUNNEL:...6cc|TYPE=VXLAN|ENCAP_SRC_IP=10.1.0.32
|ENCAP_MAPPERS=[VLAN_ID_TO_VNI,VIRTUAL_ROUTER_ID_TO_VNI]
|DECAP_MAPPERS=[VNI_TO_VLAN_ID,VNI_TO_VIRTUAL_ROUTER_ID]|PEER_MODE=P2MP
c|SAI_OBJECT_TYPE_TUNNEL_TERM_TABLE_ENTRY:...6cd|TYPE=P2MP|VR_ID=...002
|DST_IP=10.1.0.32|TUNNEL_TYPE=VXLAN|ACTION_TUNNEL_ID=...6cc
c|SAI_OBJECT_TYPE_TUNNEL_MAP_ENTRY:...6ce|TYPE=VIRTUAL_ROUTER_ID_TO_VNI|VR=...002|VNI=10000
c|SAI_OBJECT_TYPE_TUNNEL_MAP_ENTRY:...6cf|TYPE=VNI_TO_VIRTUAL_ROUTER_ID|VNI=10000|VR=...002
c|SAI_OBJECT_TYPE_NEXT_HOP:...6d0|TYPE=TUNNEL_ENCAP|IP=100.0.1.10|TUNNEL_ID=...6cc
IPv6 VNET emits the same shape (term TYPE=P2MP, DST_IP=fc00:1::32).
Co-authored with CoPilot
There was a problem hiding this comment.
Thanks for the explanation.
There was a problem hiding this comment.
The vtep_ip is from encap_source_ip in tunnel object and you said "but that explicit term is only created for a VTEP source IP that is not one of our own interfaces". Does this mean the encap_source_ip can be an ip not a local interface address? is this ip advertised by bgp otherwise how remote can reach it?
There was a problem hiding this comment.
Correct. vtep_ip is the tunnel ENCAP_SRC_IP. In the standard VNET model that is always the local Loopback0 VTEP, which BGP advertises into the underlay so remote peers reach us, so it is never non local in a normal config. The primary VTEP guard therefore always fires and the secondary VTEP term branch below is a defensive path a single loopback config never hits
Co-authored with CoPilot
There was a problem hiding this comment.
The SAI_TUNNEL_ATTR_ENCAP_SRC_IP in above TUNNEL is 10.1.0.32, which is the normal case with loopback address. Is there a real usecase where the encap source ip is not a local ip? You said it is a defensive path so I am wondering if it is really necessary.
There was a problem hiding this comment.
Hi @yue-fred-gao , I modified the comment. It's not defensive but necessary. The encap source IP does not have to be a local IP. test_vxlan_multiple_tunnels configures :
- the loopback0 tunnel (10.1.0.32, local) and
- a second tunnel whose SRC is the Loopback0 IP BUT with the last octet set to 100 or 101. (e.g. 10.1.0.100). This address is not assigned to an interface. It then sends the outer packet to this special SRC and expects DUT to decap it.
The test assumes that on ASICs, (not specific to VPP) that the DUT is required to decap the special SRC even if not one of its interface address. VPP is the odd one out in how it gets there. VPP only local-receives an IP that is on an interface or has an explicit local route, so without this branch there is no local-receive for the special src, the outer packet is never punted to vxlan-input, and it is not decapped.
The change adds a VRF0 /32 or /128 VPP_NEXTHOP_LOCAL receive for exactly those non-interface VTEP IPs, while the primary Loopback0 VTEP is skipped by the local-interface guard.
In our run that is what takes the special-src subcases from failing to passing.
Co-authored with CoPilot
There was a problem hiding this comment.
Thanks. Honestly, I don't know how the secondly vtep works in production, especially how the second vtep's source ip is advertised. There is no integration with frr to advertise vtep's source ip via routing protocols. The PR is solid to support the feature. Just how it is intended to be used is unclear. If you can find a document to describe the use case and ways to advertise the source ip, that will be helpful to understand it.
There was a problem hiding this comment.
@yue-fred-gao . I looked at this a little bit more. So the "secondary" vtep is a sonic-mgmt test construct, and not really something that's done in production. It's needed to test decap for src that is not the box's interface addess.
There was a problem hiding this comment.
Thanks for the clarification. Maybe the sonic-mgmt goes too far to verify some behavior that is not required in production. It might even be some platform specific behavior. For example, some may implement vtep as an interface, which would naturally use vtep source ip as the pseduo interface ip. But sonic doesn't model vtep as an interface so the source ip is not added to the control plane and advertised by routing protocols.
Address review feedback on sonic-net#1996: sw_interface_set_mac duplicated the vl_api_sw_interface_set_mac_address body already present in sw_interface_set_mac_by_index. The name based variant now resolves the sw_if_index with get_swif_idx under VPP_LOCK, releases the lock, and delegates to sw_interface_set_mac_by_index for the actual VPP API call, leaving a single implementation. Return codes and the NULL mac guard are preserved (the guard now lives in the by-index variant). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Add a symmetric, refcount-aware handle_l3_vxlan_tunnel_removal that runs before remove_internal when a TUNNEL is deleted. It mirrors handle_l3_vxlan_tunnel_create: for the tunnel's ENCAP_SRC_IP, it sweeps the VNI_TO_VIRTUAL_ROUTER_ID decap mappers and frees the decap terms that VTEP source owns, but only once no surviving VXLAN tunnel still references the same mapper with the same source IP. Normal VNET teardown removes the TUNNEL_MAP_ENTRYs first, and handle_l2_vxlan_tunnel_map_entry_removal frees the terms then. This closes the gap where a TUNNEL is deleted while its map entries are kept, which would otherwise leak the terms since remove() had no TUNNEL case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
|
||
| is_local_skip = false; | ||
|
|
||
| // Primary-VTEP guard: if this VTEP IP already belongs to one of our own |
There was a problem hiding this comment.
The vtep_ip is from encap_source_ip in tunnel object and you said "but that explicit term is only created for a VTEP source IP that is not one of our own interfaces". Does this mean the encap_source_ip can be an ip not a local interface address? is this ip advertised by bgp otherwise how remote can reach it?
Source-independent (decap_any) L3 VXLAN decap is only correct for a P2MP tunnel, where the outer source is not a single fixed peer. For a P2P tunnel the outer source is the fixed remote VTEP (SAI marks SAI_TUNNEL_ATTR_ENCAP_DST_IP validonly when PEER_MODE == P2P), so it must keep exact outer-source validation. tunnel_encap_nexthop_action set req.decap_any unconditionally for every VIRTUAL_ROUTER_ID_TO_VNI mapper. Read SAI_TUNNEL_ATTR_PEER_MODE from the tunnel object once before the mapper loop and gate decap_any on P2MP. The attribute is CREATE_ONLY with a SAI default of P2MP, and this vslib's get_attr returns SAI_STATUS_ITEM_NOT_FOUND for an absent attribute, so soft-read it and default to P2MP rather than a hard read that would regress creation when SONiC omits the attribute. Also clarify in a comment that the create_vxlan_decap_term primary-VTEP guard always fires in the standard SONiC VNET model (ENCAP_SRC_IP is always the local Loopback0 VTEP advertised by BGP), so the explicit secondary-VTEP term below it is a defensive path for a non-local ENCAP_SRC_IP that a single-loopback config never hits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
The old comment claimed the secondary-VTEP decap term is a defensive path that a single-loopback config never hits. That is inaccurate: a VXLAN tunnel whose ENCAP_SRC_IP is not a local interface address falls through the primary-VTEP guard and installs the secondary-VTEP decap term plus the VRF0 local-receive. This path is exercised and required by the sonic-mgmt test_vxlan_multiple_tunnels case, which is enabled on the vpp asic. Comment only, no logic change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
### Description of PR Summary: Enable the L3 VXLAN test cases on the sonic-vpp platform and make the hash-distribution checks statistically robust on the vpp dataplane. Part of sonic-net/sonic-buildimage#25777 This is the test layer (layer 4) of a cross-repo feature and MUST merge after the dataplane and SAI layers are in a built sonic-vpp image, otherwise the newly-enabled tests fail in CI. Landing order: 1. sonic-net/sonic-platform-vpp#262 - VPP patches (first) 2. sonic-net/sonic-sairedis#1996 - saivpp adaptation 3. sonic-buildimage submodule bump (auto via mssonicbld) 4. this PR - tests (last) > Note for reviewers: the `t1-lag-vpp` Elastictest leg on this PR is > currently expected to fail. The PR CI builds a sonic-vpp KVM image from > **master** buildimage, whose `platform/vpp` and `src/sonic-sairedis` > submodules do not yet contain the dataplane (#262) and saivpp (#1996) > changes these tests exercise. Once #262 and #1996 merge and mssonicbld > advances the submodule pointers, the rebuilt image carries the VNET-decap > dataplane and this leg passes (validated locally: test_vnet_decap 4/4, > test_vxlan_multiple_tunnels 16/16 on a full image built from the paired > PRs). Changes: - conditional_mark (tests_mark_conditions.yaml and tests_mark_conditions_sonic_vpp.yaml) plus per-test skip markers in test_vnet_decap.py and test_vxlan_bfd_tsa.py: remove the asic_type vpp skip so test_vxlan_ecmp (random_hash and entropy), test_vnet_decap, and test_vxlan_bfd_tsa run on sonic-vpp. Originally from #25480 by Chenyang Wang; authorship preserved. - test_vxlan_ecmp.py: statistically-justified tolerance and packet-count for the random_hash and entropy distribution checks, paired with the VPP-side load-balance tolerance change so the 2-endpoint splits pass reliably rather than flaking at a too-tight bound. Supersedes debug PR #25480. ### Type of change - [x] Test case improvement - [x] Testbed and Framework(new/improvement) ### Back port request - [ ] 202311 - [ ] 202405 - [ ] 202411 - [ ] 202505 - [ ] 202511 - [ ] 202512 - [ ] 202605 ### Approach #### What is the motivation for this PR? The L3 VXLAN test cases were skipped on the vpp platform. With the dataplane (patches 0015-0017) and saivpp decap support in place, enable them and make the hash-distribution checks statistically robust so they do not flake. #### How did you do it? Removed the asic_type vpp skip conditions for the VXLAN tests and raised the packet-count/tolerance on the random_hash and entropy distribution checks in test_vxlan_ecmp.py. #### How did you verify/test it? Validated on a t1-lag-vpp testbed against a sonic-vpp image built from the paired PRs: fib/test_fib.py 16/16 hashing pass; vxlan/test_vxlan_ecmp.py random_hash and entropy pass; test_vnet_decap and test_vxlan_bfd_tsa green. --------- Signed-off-by: Aaron Bernardino <aaronber@microsoft.com> Co-authored-by: Chenyang Wang <chenyangw233@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa
Description of PR
Summary:
Enable L3 VXLAN decap on the sonic-vpp platform (saivpp / vslib/vpp) and fix
several pre-existing decap setup/teardown defects found during review.
Part of sonic-net/sonic-buildimage#25777
Depends on the VPP patch series in sonic-net/sonic-platform-vpp#262 being in the
built image first. All changes are confined to vslib/vpp.
Changes:
primary and secondary L3 VNET paths; L2 EVPN tunnels intentionally keep exact
outer-source validation.
sw_interface_set_mac_by_index binary-API helper, so the inner Ethernet header
matches what HW ASICs and the VNET decap tests expect (default was VPP's
auto-generated 02:fe:.. MAC).
shared VRF0 VTEP local-receive route so tearing down one VNI does not break
its siblings; check every VPP API return in create_vxlan_decap_term and roll
back the BD/BVI on failure instead of returning success half-programmed; and
install the L3 decap terms from the tunnel-create path too, so a tunnel
created after its map entry still gets programmed.
BD in remove_vpp_vxlan_decap: the tunnel was still a BD member at delete time
(it is removed later by remove_vpp_vxlan_encap), so
vpp_bridge_domain_add_del(is_add=0) failed with -120 (BD in use). Benign (BD
reclaimed later) but it tripped loganalyzer during teardown.
Files: vslib/vpp/{TunnelManager.cpp, TunnelManager.h, SwitchVpp.cpp,
vppxlate/SaiVppXlate.c, vppxlate/SaiVppXlate.h}.
Type of change
Approach
What is the motivation for this PR?
Enable correct L3 VXLAN decap on sonic-vpp and remove decap setup/teardown
defects: source-dependent decap, wrong inner source MAC, and the BD-in-use
teardown error.
How did you do it?
Changes in vslib/vpp (TunnelManager, SwitchVpp, SaiVppXlate) to install
source-independent decap terms, set the tunnel inner source MAC, and fix the BD
member-detach ordering on teardown.
How did you verify/test it?
Rebuilt syncd-vpp, hot-swapped onto a t1-lag-vpp testbed, and ran the VXLAN
regression suite: test_vnet_decap 4/4, test_vxlan_multiple_tunnels 16/16, and
vxlan/test_vxlan_ecmp.py with zero -120 bridge-domain-delete errors and zero
leaked bridge-domains after teardown.
Any platform specific information?
sonic-vpp (vslib/vpp) only; no other platform is affected.
Documentation
No doc/HLD changes.