[sonic-frr/bgpcfgd]: Add EVPN MAC synchronization over the FPM channel - #29312
Draft
tahmed-dev wants to merge 21 commits into
Draft
[sonic-frr/bgpcfgd]: Add EVPN MAC synchronization over the FPM channel#29312tahmed-dev wants to merge 21 commits into
tahmed-dev wants to merge 21 commits into
Conversation
Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
Prefer orchagent ZMQ publishing for EVPN-MH split-horizon and DF state, with direct Redis APP_DB fallback when ZMQ is disabled or unavailable. Drop the unused EVPN ES backup NHG FPM/APP_DB contract and add a CUnit harness for the SONiC FRR Redis/ZMQ provider. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
…hannel Phase 1 of moving MAC synchronization off the Linux kernel bridge FDB and onto the FPM channel. dplane_fpm_sonic gains an inbound MAC path: bridge FDB entries arriving on the FPM socket are fed through zebra_vxlan_local_mac_add_update()/_del(), the same entry point the kernel path uses, so the EVPN state machine and its multihoming semantics are unchanged. RTM_FPM_MAC_REPLAY_END is pinned to an explicit value. This enum is duplicated in sonic-swss fpmsyncd/fpm/fpm.h and the two copies have drifted, so a positional value would have resolved differently on each side and produced a silent wire mismatch. sonic-fdb-sync models FDB_SYNC|global with mac_sync_mode, defaulting to kernel so behaviour is unchanged unless explicitly enabled. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
fpmsyncd stamps each replayed local MAC with a generation and closes the replay with RTM_FPM_MAC_REPLAY_END. Record that generation on the MAC and, on the marker, delete every FPM-learned MAC still carrying an older one. Adds patch 0119 for the ZEBRA_MAC_FPM_LEARNED flag and the generation field on struct zebra_mac; the walk and delete live in the SONiC-owned plugin. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
fpm_read() runs on the FPM provider's own pthread, but the MAC path called zebra_vxlan_local_mac_add_update() directly and the end-of-replay sweep walked the EVPN hashes, both of which the main thread owns. The route case in the same switch marshals into a dplane context instead, and the RMAC walkers are all scheduled on zrouter.master. Parse the netlink message on the FPM thread and hand the MAC to the main thread, and queue the sweep the same way; the event queue is FIFO, so a MAC replayed in this session is stamped before the sweep inspects it. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
fpm_read_local_mac() parsed the netlink message and mutated zebra state in
the same function, so the parsing could only be exercised by running zebra
against a live FPM connection. There was no way to unit test it.
Move the decode into dplane_fpm_sonic/fpm_mac.c. That file depends only on
the kernel netlink headers, not on zebra, so it can be linked into a plain
test binary. Add a CUnit suite that covers the decode directly and register
it the same way sonic_frr_redis_interface_test is registered.
Pulling the parsing out also made two gaps in it visible, both fixed here:
* The message family was never checked, so a non AF_BRIDGE neighbour
message would have been treated as a MAC.
* NDA_LLADDR and NDA_VLAN were read without validating their payload
length, so a short attribute would have been over read.
The decode now writes the caller's struct only after the message has been
fully validated, so a caller can never act on a partially filled result.
Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
The FRR package build never ran a single unit test. dh_auto_test invokes "make test", and in this tree that target only builds the pceplib helper binaries and runs nothing, so every check program, including the SONiC ones in tests/zebra, was silently skipped. A test that is never executed cannot catch a regression. Override dh_auto_test to build the check programs and run the suite through tests/tests.xml instead. A plain "make check" is not usable here. It additionally runs pceplib's *_valgrind.sh wrappers, which reference a relative path that does not resolve in an out-of-tree build and fail with exit 127 regardless of the code under test. tests/tests.xml is FRR's own target for the unit suite and depends on $(check_PROGRAMS), so it builds and runs exactly what we want. The existing nocheck handling is preserved. Verified by breaking the FPM MAC decode on purpose: the package build fails, the failure is reported as a test failure rather than a build error, and no deb is produced. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
The reconnect walk covered L3VNI router MACs only. Nothing walked zevpn->mac_table, so a remote MAC that fpmsyncd installed before it restarted was never re-advertised, and a withdrawal for it could not be matched against anything. fpmsyncd could neither rebuild remote state nor safely remove what it held. Add a second walk, chained after the router MAC walk, that re-sends every ZEBRA_MAC_REMOTE entry. Local MACs are skipped because they originate from fpmsyncd. Each context is built the way zebra_evpn_mac.c builds it, including the mac->es case where the destination is a nexthop group rather than a VTEP address, so the replay reproduces what zebra would have sent rather than an approximation of it. RTM_FPM_MAC_REPLAY_END then marks the end of the walk. The type already exists for the local direction and carries no payload, so reusing it here needs no new enum value in either copy of the message type list, which is where these two codebases have drifted before. The marker schedules the socket write itself. Writing into obuf is not enough: every other sender ends with event_add_write(), and without it the marker waits for an unrelated enqueue to flush it, which on an idle session never arrives. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
config vrf add_vrf_vni_map writes a vni into VRF|<name>, and vrfmgrd acts on it, but nothing on the bgpcfgd path passes that vni to zebra. The zebra template renders vni only for VNET, and no manager watches the VRF table. The mapping therefore reaches the ASIC side while zebra never creates the L3VNI, leaving the VNI as L2 with the VRF merely as its tenant, and the CLI reports success throughout. frrcfgd already implements this in vrf_handler, so the model is supported and this is a gap in the default path rather than a missing feature. Add the equivalent manager for runtime updates, and the template branch so the binding survives a config reload. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
…he marker The end-of-replay marker was treated as proof that anything not replayed no longer exists, and the sweep deleted it on the spot. A replay is only as complete as its sender: fpmsyncd emits the marker even when it replayed nothing, so a short read deletes MACs that are still present, and since nothing re-drives an unchanged STATE_DB entry they stay deleted. Reproduced on hardware without an instrumented binary by removing the port field from a STATE_DB FDB entry, which loadLocalMacs() skips and processStateFdb() ignores rather than withdrawing. An fpmsyncd reconnect then deleted a local MAC that was still in STATE_DB, and it did not come back. Arm a hold-down instead. A refresh arriving inside the window carries the current generation and survives; only entries still stale at expiry are removed. A newer replay cancels the pending sweep, whose timer would otherwise remove the MACs that replay had just refreshed. Verified on hardware both ways: held then removed at expiry with no refresh, held then kept when a refresh arrived inside the window. fpmsyncd applies the same hold to the MACs it learns from zebra. The two constants are independent and nothing in the protocol negotiates them. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
The sweep predicate sat inline in the hash callback, where nothing can reach it without a running zebra, so the rule that decides whether a held MAC is retired had no coverage. Move it into fpm_mac.c beside the decode and test it, including that a MAC carrying the current generation survives, which is what a refresh arriving inside the hold-down relies on. Also state that the hold-down windows on the two sides are independent by design rather than a pair that must be kept equal. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
…ming EVPN multihoming programs its Ethernet Segment nexthops and nexthop groups through kernel_upd_mac_nh() and kernel_upd_mac_nhg(), which reach the kernel directly through netlink_talk() without ever building a dataplane context. Every other FDB operation goes through the dataplane, so a provider can be handed a MAC that refers to a group it was never told about. Add patch 0122, which fires a hook from the four kernel_*_mac_nh* entry points. The kernel path is byte for byte unchanged and the hook expands to nothing when no module is registered, so the patch is idempotent and is shaped to be offered upstream. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
In fpm mode the ASIC learns local MACs and hands them to zebra over FPM, and fdbsyncd no longer mirrors them into the kernel. Nothing was left populating the kernel bridge FDB for those MACs, so the kernel kept probing hosts the dataplane could already reach. Add patch 0123, exposing kernel_upd_local_mac()/kernel_del_local_mac(), and call them from the inbound FPM MAC path. The entry carries NTF_EXT_LEARNED so the bridge does not age a MAC it never learned. It is deliberately not tagged RTPROT_HW: nothing reads the protocol back now that SONiC no longer snoops the kernel, which is what lets the RTPROT_HW kernel patch be dropped later. Going straight to netlink rather than through dplane_local_mac_add() is required: a dataplane context would come back to this module as a MAC install and be echoed to fpmsyncd as a bogus remote entry. An externally learned update is not enough on its own. The kernel takes a separate path for NTF_EXT_LEARNED and ignores ndm_state there, so the message can create the entry but can never change the state of one that already exists, and zebra is not the only writer: its Ethernet Segment sync programs the same MAC as static. Send the update a second time without NTF_EXT_LEARNED, a flag the entry keeps once set, so the ordinary path honours ndm_state and forces it back to NUD_REACHABLE. Correcting the entry in place rather than deleting and re-adding it keeps the MAC forwarding throughout, so nothing is flooded meanwhile. Sticky MACs are left alone, NUD_NOARP being what they should carry. Both calls end in netlink_talk(), so report their return values rather than discarding them. A rejected write otherwise leaves no trace anywhere, and a MAC missing from the bridge FDB looks identical to one that was never sent. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
…down fpmsyncd builds its replay by reading STATE_DB synchronously, so the set it sends is complete at the moment it sends the marker. Nothing is still settling that the 120s hold-down was waiting for, and the event queue is FIFO, so every MAC replayed in the session is already stamped by the time the marker's handler runs. Sweep straight from fpm_local_mac_replay_end() and drop FPM_MAC_STALE_HOLD_SECONDS, t_mac_sweep, mac_sweep_generation and the separate sweep handler. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
Register the patch 0122 hooks and encode what zebra would have given the kernel: NHA_FDB to mark it an FDB nexthop, then NHA_GATEWAY for a single VTEP or NHA_GROUP for a set of member ids. The receiver therefore decodes exactly what it decodes from the kernel today. Writing into fnc->obuf is not enough on its own. An Ethernet Segment can settle with no other traffic behind it, so the write task is armed here rather than left to the next enqueue, which on an idle box never comes. fpmsyncd already gates publishing on fpm mode, so this is inert in kernel mode. fdbsyncd still reads the same groups from the kernel, so both write L2_NEXTHOP_GROUP_TABLE with identical content until that path is retired. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
…them Ethernet Segment nexthops are programmed outside the dataplane, so nothing re-sends them when the FPM session comes back. fpmsyncd would then hold MACs naming groups it had never been told about, and it drops those rather than publish a group it cannot resolve. Walk the L2 nexthops and then the groups built from them, between the RMAC and MAC walks. The order is load-bearing in both places: a group is only published once every member is known, and a MAC only once its group is. Also advances swss for the fdbsyncd retirement: d905e58b [fdbsyncd]: Stop publishing L2 nexthop groups in fpm mode Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
The encoding was inline in the send path, where it could only be exercised by building an FPM session. Move it into fpm_mac.c alongside the MAC decode, which already stays free of zebra state precisely so it can be tested directly, and have the send path call it. The five cases cover what fpmsyncd parses: an IPv4 and an IPv6 single VTEP, a group and its member ids, a delete that must still carry NHA_FDB, and the rejections. Verified by canary: dropping the NHA_FDB attribute turns the build red, which is the failure that would otherwise have fpmsyncd hand an Ethernet Segment to the L3 route path. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
Local MACs originate from fpmsyncd, so sending them back is a loop. It is also unsafe: dplane_local_mac_add() sets only the type of its stack vtep to IPADDR_NONE, and netlink_neigh_update_msg_encode() still emits a 16 byte NDA_DST built from that never-initialised union. fpmsyncd read it as a remote VTEP and published uninitialised zebra stack into APPL_DB. The kernel tolerates the attribute on a local entry, which is why this only surfaced once MACs crossed the FPM channel. Filter on DPLANE_MAC_REMOTE, which is the rule the replay walk already applied; only the incremental path was missing it. Seed the kernel entry before handing the MAC to zebra as well. A MAC on an Ethernet Segment makes zebra queue its own sync install marking the entry static, and that runs on the dplane thread, so seeding afterwards raced it and could downgrade a static entry back to extern_learn. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
The bridge FDB protocol names where a MAC came from, not who wrote the entry. A MAC the switch ASIC learned is hardware learnt however it reaches the kernel, so program it with RTPROT_HW, which is the same attribution fdbsyncd gave these MACs when it drove the kernel path itself. Remote MACs zebra installs keep RTPROT_ZEBRA, naming zebra as their source. That only holds if one writer owns the entry. zebra's Ethernet Segment sync installs the same MAC through the dataplane and would overwrite the attribution with its own, leaving two writers racing on one entry, so a pre-kernel dataplane provider now skips the kernel half of that write once FPM is driving local MACs. Remote MACs are left alone because zebra really is their source. Ownership is taken on the end-of-replay marker rather than on the first MAC to arrive over the channel. fpmsyncd sends the marker only in fpm mode, and sends it even when it replayed nothing, so it is the earliest honest statement that local MACs belong elsewhere and it covers boot, where the ASIC has learnt nothing yet while BGP has already converged and the Ethernet Segment sync would otherwise get an unsuppressed write in. Ownership is released when the channel drops, because nothing is feeding local MACs while fpmsyncd is gone and zebra has to write them itself until the next replay. Also drop the sonic-linux-kernel NTF_EXT_VALIDATED patch, which nothing consumes, and correct a claim in 0123 that nothing reads the protocol back: netlink_macfdb_change() does, to keep zebra from re-learning its own entries. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
EVPN_ETHERNET_SEGMENT reaches FRR only from the sonic-utilities CLI, at the moment the command is typed. Nothing reconciles CONFIG_DB into FRR afterwards and zebra.interfaces.conf.j2 renders no Ethernet Segment configuration, so the segments are lost on every reboot while CONFIG_DB keeps them. Both PEs then advertise the same MACs with no shared ESI. BGP reads that as a MAC move, so the two sides bump the sequence number at each other indefinitely: observed in the lab at sequence 11916 with both MACs flagged Duplicate and the VXLAN FDB rewritten about twice a second, until the segments were reapplied by hand. Add an EvpnMhEsMgr that pushes the segment to FRR from CONFIG_DB, mirroring the vtysh sequence the CLI already issues. It depends on PORTCHANNEL because the system MAC is what makes the ESI identical on both PEs, so the LAG table is now fed into the Directory as well. That dependency is satisfied as soon as the first PortChannel reaches the Directory, while the system MAC lookup is per interface, so a segment can still be handled before its own PortChannel arrives. Programming it then would emit an ESI without es-sys-mac, which the peer never matches, so the handler defers instead by returning False; the Manager base class queues it and retries on every dependency change, and the segment is programmed as soon as its PortChannel is known. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
This taught the bridge command to emit and print NDA_PROTOCOL on an FDB entry. The kernel side of that came from the sonic-linux-kernel protocol patch, which is gone, so the attribute is now discarded on add and never returned on dump. Nothing consumes it either. Its only caller was the fdbsyncd kernel-mode path, which is gated off in fpm mode, and the support probe backing that path can never succeed anyway because it tests "dev lo" and loopback is not an Ethernet device. Where a MAC came from now travels over the FPM channel instead, so the origin marker no longer needs a kernel or iproute2 carrier. Signed-off-by: Tamer Ahmed <tamerahmed@microsoft.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why I did it
EVPN multihoming needs locally learned MACs and EVPN remote MACs to converge in
a single place. Today
fdbsyncdreads the kernel bridge FDB and depends on aSONiC-only
NDA_PROTOCOLpatch carried insonic-linux-kernelandiproute2to tell hardware-learned entries apart from zebra's own writes.
This series moves that synchronization onto the FPM channel, so zebra is told
directly which MACs the ASIC learned. That removes the need for the out-of-tree
kernel patch and gives EVPN-MH a single writer for the kernel bridge FDB.
The design was reviewed with Patrice Brissette; per-MAC state is
[CP OR DP],and the kernel is still programmed in
fpmmode so the kernel ARP/ND statemachine is preserved.
Work item tracking
[Octans][FRR] Reconcile Phase 1 kernel and fpmsyncd FDB updates without duplicates)[Octans][Phase 1] EVPN multihoming for a dual-ToR frontend without hardware switchover)How I did it
dplane_fpm_sonic.cintofpm_mac.{c,h}, keptfree of zebra/FRR headers so it is unit testable, and wired the CUnit tests
into the package build (patch 0121).
explicit numeric values on both sides of the wire contract (patch 0077).
hold-down with an end-of-replay marker so a stale sweep cannot delete live
remote MACs (patches 0118-0120).
remote MACs can be carried as a nexthop group rather than a peer VTEP address
(patch 0122).
fpmmode,which is what removes the kernel patch dependency (patch 0123).
iproute2bridge-FDB protocol field patch.FDB_SYNCYANG model plus its tests, andbgpcfgdchanges to bindthe L3VNI to its VRF and restore EVPN Ethernet Segments into FRR on restart.
How to verify it
Unit tests, run by the package build:
Hardware validation on two Arista 7060X6-64PE-B (TH5) leaves in an EVPN-MH
dual-homed topology, 38/38 checks passed on both switches, covering:
local-inactivemarking in zebra[CP OR DP]rule, CP-only MACs still reachable through the tunnelmac_sync_modefpm->kernel->fpmownership handoverextern_learnandstaticDependency and merge order
This PR cannot build until
sonic-swss-common#1243 merges: it referencesAPP_EVPN_DF_TABLE_NAME, which does not exist on master yet. Opening as a draftfor that reason.
Merge order across the companion PRs:
sonic-swss-common[broadcom]: update sai to 3.0.3.3-1 #1243 - schema, blocks everythingsonic-swss[Juniper][QFX5210] Fixing a few platform issues #4857 - fpmsyncd/fdbsyncdsonic-utilities[Mellanox] Add a new SKU Mellanox-SN4600C-D112C8 #4833 -config fdb mac-sync-modeCLIsonic-linux-kernel[build]: support submodule patching for SONIC_DPKG_DEBS #619 - drops the kernel patches, must be last, sincemaster still carries the fdbsyncd plumbing those patches support
Which release branch to backport (provide reason below if selected)
Tested branch (Please provide the tested image version)
tahmed_fpm-mac-learning-master.0-68bbddeaf3on Arista 7060X6-64PE-B (TH5)Description for the changelog
Add EVPN MAC synchronization over the FPM channel, removing the kernel bridge
FDB protocol patch dependency for EVPN multihoming.
Link to config_db schema for YANG module changes
sonic-fdb-sync.yangadds theFDB_SYNCtable: