[vpp] VPP SAI backend fixes: CPU queue, host-interface admin-up, L3-over-LAG/host-route forwarding, LAG egress-disable#1952
Conversation
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Backend portion of the L3-over-LAG forwarding fix (BVI/bond FDB handling in SwitchVppFdb.cpp). The UT-harness portion (run_test.sh LAG/SVI interface-IP assignment) and the related devdocs are in the harness PR. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
programNeighborHostRoute() left the host-route nexthop sw_if_index at 0, so ip_route_add_del() pinned the neighbor /32//128 host route to VPP's local0 and dropped traffic even though the correct adjacency over the real egress interface existed. Set sw_if_index = ~0 to resolve the egress via hwif_name. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
e3314e9 to
6af1c56
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Satisfy tests/swsslogentercheck.sh for static helpers introduced in the neighbor host-route refactor. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
It appears the checks are failing due to an upstream issue which should be resolved when #4696 is merged into sonic-swss. |
|
|
||
| if (is_add) | ||
| { | ||
| no_host_route = neighbor_no_host_route_from_attrs(attr_count, attr_list); |
There was a problem hiding this comment.
Please use SaiCachedObject and SaiDBObject, both are subclasses of SaiObject. It provides common functions like getAttribute
There was a problem hiding this comment.
Done. Implemented in 2667ad0e, the NO_HOST_ROUTE lookup now goes through SaiCachedObject::get_attr on create and SaiDBObject::get_attr via get_sai_object() on remove.
| return SAI_STATUS_SUCCESS; | ||
| } | ||
|
|
||
| sai_neighbor_entry_t nbr_entry; |
There was a problem hiding this comment.
The code is very similar to addRemoveIpNbr. Can we extend the function with an argument so it can optionally create host route?
There was a problem hiding this comment.
Yes, implemented in 2667ad0e, programNeighborHostRoute is removed and integrated into addRemoveIpNbr, which now takes program_adjacency and program_host_route flags so addIpNbr/removeIpNbr can call it for the adjacency phase and the host-route phases respectively.
| SWSS_LOG_ENTER(); | ||
|
|
||
| const struct timespec req = {2, 0}; | ||
| const struct timespec req = {5, 0}; |
There was a problem hiding this comment.
why do we need to increase the polling interval?
There was a problem hiding this comment.
Sorry, this was left from earlier attempts to fix event-handling issues (before the dedicated socket solution was implemented). I completed a full test run to verify reverting has no impact on our tests and reverted to 2s on e554f93a.
| if (ot == SAI_OBJECT_TYPE_LAG) | ||
| { | ||
| SWSS_LOG_ERROR("SAI_ROUTER_INTERFACE_ATTR_PORT_ID=%s expected to be PORT but is: %s", | ||
| platform_bond_info_t bond_info; |
There was a problem hiding this comment.
Is it better to move this into getTapNameFromPortId
There was a problem hiding this comment.
getTapNameFromPortId() is in the SwitchState base class. Since the class is shared by the non-VPP virtual switch and LAG->bond naming is VPP-specific, I centralized the LAG be<N> tap derivation into a new getTapNameFromPortOrLagId() helper instead in 31a2d2a5 and routed vpp_get_hwif_name / vpp_add_del_intf_ip_addr through it, removing the inline LAG blocks.
| hw_ifname = hw_subifname; | ||
| } else { | ||
| hw_ifname = hwifname; | ||
| if (ot == SAI_OBJECT_TYPE_LAG) |
There was a problem hiding this comment.
can we augment tap_to_hwif_name to convert be interface?
There was a problem hiding this comment.
Yes. In 31a2d2a5, tap_to_hwif_name now maps a be<N> tap to BondEthernet<N> when it's not in sonic_vpp_ifmap.iniso a LAG can flow through tap_to_hwif_name like a port.
| if (SAI_OBJECT_TYPE_ROUTER_INTERFACE == RealObjectIdManager::objectTypeQuery(next_hop_oid)) | ||
| { | ||
| // vpp_add_del_intf_ip_addr(route_entry.destination, next_hop_oid, is_add); | ||
| sai_status_t rif_status = vpp_add_del_intf_ip_addr(route_entry.destination, next_hop_oid, is_add); |
There was a problem hiding this comment.
ip is also added via route over port (line 164 in this file). If you enable this, we will see multiple attempts to set ip address on int. first will succeed and subsequent one fail because the address already exists. Is sai_test expecting ip to be created with rif nexthop?
There was a problem hiding this comment.
Looks like sai_test does create connected routes with a RIF next hop, but it does not expect the interface IP to be programmed by this path. So this vpp_add_del_intf_ip_addr call was redundant and introduces idempotency issues. Looks like this change was left over from earlier attempts to fix L3-over-LAG forwarding issues. I completed a full test run to verify reverting has no impact on our tests and reverted in d27b5ac6.
| } | ||
| } | ||
|
|
||
| create_vpp_nexthop_entry(nxt_grp_member, member_hwif, nexthop_type, &ip_route->nexthop[i]); |
There was a problem hiding this comment.
member_hwif is a local variable. create_vpp_nexthop_entry stores it in ip_route->nexthop[I]. It will become a dangling pointer when this block ends
There was a problem hiding this comment.
Thank you for catching this one. In eb837f2e, the resolved hwif names are now held in a std::vector<std::string> that outlives the ip_route_add_del_get_stats() call, so ip_route->nexthop[i].hwif_name no longer points at freed memory. I applied the same fix to the single-path route function.
| size_t i; | ||
| for (i = 0; i < nxthop_group->nmembers; i++) { | ||
| create_vpp_nexthop_entry(nxt_grp_member, hwif_name, nexthop_type, &ip_route->nexthop[i]); | ||
| const char *member_hwif = hwif_name; |
There was a problem hiding this comment.
This block and the one below can be extracted into a function like
const char* resolveNexthopMemberHwif(
In const nexthop_grp_member_t *member,
Out std::string &member_hwif);
There was a problem hiding this comment.
Done! Change made in 26b06c62, now extracted to resolveNexthopMemberHwif(const nexthop_grp_member_t *member, std::string &member_hwif) and used for both NHG loop and single-path route function.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
member_hwif_str was loop-scoped while create_vpp_nexthop_entry stored its c_str() and ip_route_add_del_get_stats ran after the loop, leaving dangling pointers in ip_route->nexthop[]. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Deduplicate RIF-to-hwif resolution used when programming multipath routes and single-path route updates. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Add getTapNameFromPortOrLagId for be<N> LCP taps, teach tap_to_hwif_name to map bond taps to BondEthernet<N>, and route vpp_get_hwif_name and vpp_add_del_intf_ip_addr through the shared helpers. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
Merge programNeighborHostRoute into addRemoveIpNbr with adjacency and host-route phase flags, and read NO_HOST_ROUTE via SaiCachedObject or SaiDBObject get_attr instead of manual attribute scans. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
The connected interface IP is supplied by the control plane (and, in the UT harness, by the interface-IP assignment mirrored into VPP), so programming it again on a ROUTER_INTERFACE-nexthop route is redundant and can fail with "address already exists" when the address is already present. Restore the branch to a no-op, matching master. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
The increase to 5s was residue from an earlier event-handling investigation that the dedicated event socket already resolved; revert to the original 2s to keep async link/BFD event draining prompt. Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi @yue-fred-gao, thanks for reviewing! I addressed/replied to each comment and also ran full test this evening to verify the changes in #1952 here and #2299-2301 in SAI did not impact the performance of the test harness. All tests previously passed are still passing. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Nicholas Ching <nicholaslching@gmail.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1163730: ✅Stage TestAsan:
✅Stage Test:
|
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1163730: ✅Stage TestAsan:
✅Stage Test:
|
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1163730: ✅Stage TestAsan:
✅Stage Test:
|
|
/azpw run |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1163730: ✅Stage Test:
|
|
Hello @saiarcot895 @lolyu! The changes in this PR have previously been reviewed by @yue-fred-gao, received final approval from @dypet, and are now passing checks. Our team at Cisco currently does not have anyone with merge permissions. When you have a chance, could you please take a quick look and help us merge into master for SAIVPP UT feature? |
|
The t1-lag-vpp sanity test, applied to both #1951 and #1952 has passed on the associated sonic-buildimage PR: sonic-net/sonic-buildimage#28145 I have attached the results below: |
Description of PR
Summary:
VPP SAI backend (
vslib/vpp/) correctness fixes uncovered while bringing the OCPsai_testsuite to green againstlibsaivs— neighbor/route/LAG/ECMP programming and forwarding. Pure backend; no harness or docs. Reviewers can start invslib/vpp/SwitchVppNbr.cppandvslib/vpp/SwitchVppFdb.cpp. Development context, before/after results, and the compatibility matrix are in the harness PR's.azure-pipelines/docker-sai-test-vpp/devdocs/.Fixes # (N/A — no upstream issue)
Type of change
Approach
What is the motivation for this PR?
Several VPP SAI backend gaps caused neighbor/route/LAG/ECMP tests to fail at create time or in the dataplane (e.g. host routes dropped at
local0, LAG-member egress-disable not enforced, missing CPU-queue parent). These fixes make the correspondingsai_testcases pass.How did you do it?
All changes under
vslib/vpp/:SwitchVppNbr.cpp: neighbor host route was pinned tolocal0because the host-route nexthopsw_if_indexwas left at 0; setsw_if_index = ~0soip_route_add_del()resolves the egress viahwif_name. Also program the implicit host route on neighbor add/remove and check RIF type before readingPORT_ID(avoids-7on non-PORT/SUB_PORT RIFs).SwitchVpp.cpp/.h,SwitchVppFdb.cpp: LAG-RIF neighbor/route create andSAI_LAG_MEMBER_ATTR_EGRESS_DISABLE(detach/reattach the VPP bond member); BVI/bond FDB handling for L3-over-LAG forwarding (backend portion — the harness IP-assignment half is in the harness PR).SwitchVppRif.cpp,SwitchVppRoute.cpp,SwitchVppNexthop.cpp: ROUTER_INTERFACE next-hop + LAGBondEthernet/be<N>interface-IP support, NHG-member idempotency,vppProcessEventssleep tuning.SwitchVppHostif.cpp: re-apply admin-up after host-interface create solinux_cpbrings the paired kernel netdev carrier up.SwitchVpp.cpp: CPU queue parent scheduler node for the CPU port.How did you verify/test it?
Validated with the
docker-sai-test-vppharness: the VPP SAI compatibility matrix improved to 34 PASS with these fixes; per-test before/after detail is in the harness PR'sdevdocs/.Any platform specific information?
VPP platform only (
vslib/vpp/). The L3-over-LAG fix is split across this PR (backend,SwitchVppFdb.cpp) and the harness PR (run_test.shLAG/SVI interface-IP assignment); both are needed for that dataplane feature end-to-end, but each merges independently and targetsmaster.Documentation
No new docs in this PR (kept backend-only). Development notes, root-cause write-ups, and the compatibility matrix live in the #1950 under
.azure-pipelines/docker-sai-test-vpp/devdocs/.