Repository navigation
fix(saiserver): program LAG VLAN memberships on LAG member ports - #766
Open
mohankarthik wants to merge 5 commits into
Open
mohankarthik wants to merge 5 commits into
mohankarthik wants to merge 5 commits into
Conversation
This was referenced Oct 21, 2026
mohankarthik
force-pushed
the
split/pr5-lag-vlan-members
branch
3 times, most recently
from
October 26, 2026 04:46
1cefba5 to
bb6b61e
Compare
The vlan server's oidByVId and vlans maps are read and written from concurrent SAI gRPC handlers, but only some writers held vlan.mu: vidByOid, memberByOid, memberByPortId, CreateVlan, the previous-VLAN lookup in CreateVlanMember, and Reset all accessed the maps unlocked. - Take vlan.mu in vidByOid, memberByOid, memberByPortId, CreateVlan and Reset. - Add an oidByVid helper and use it for the duplicate-VID check in CreateVlan and the previous-VLAN lookup in CreateVlanMember (returning an error instead of silently using OID 0 when the VID is unknown). - Return an error instead of panicking on a nil-map write if the parent VLAN disappears between vidByOid and the member insert. - In RemoveVlanMember, claim (delete) the member under the lock and release vlan.mu before issuing ObjectNID/TableEntryRemove so the lock is never held across dataplane RPCs. If the dataplane removal fails, the claim is undone (restoreMember) so the removal can be retried. Adds TestVlanConcurrentAccess, which races SAI create/remove calls against the lookup helpers and fails under -race without this change, and TestRemoveVlanMemberRetryAfterDataplaneError.
…Member RemoveVlanMember dropped the member from the vlan server's internal map and the VlanTable, but left its OID in the parent VLAN's VLAN_ATTR_MEMBER_LIST attribute, so GetVlanAttribute(MEMBER_LIST) continued to report removed members. Prune the removed OID from the parent VLAN's MEMBER_LIST in RemoveVlanMember once the dataplane removal has succeeded, mirroring what CreateVlanMember already does when it moves a port out of its previous VLAN. A failed removal leaves MEMBER_LIST untouched so it stays consistent with the restored member. TestVlanOperations previously combined its member-count checks with '&&', which hid this bug; switch them to '||', print the observed lists, and assert the removed OID is no longer listed. TestRemoveVlanMemberRetryAfterDataplaneError now also checks MEMBER_LIST across the failed and retried removal.
… a port A port may have one untagged/priority-tagged VLAN membership (its PVID) plus any number of tagged memberships. The vlan server did not model this: - CreateVlanMember evicted whatever membership memberByPortId returned (an arbitrary map entry) for every new member, so adding a tagged VLAN to a port silently dropped its untagged VLAN membership (or vice versa) from MEMBER_LIST and the internal map. A later RemoveVlanMember for the evicted OID then failed. - VlanTable was keyed only on PACKET_PORT_INPUT, so every membership of a port overwrote the same entry and the first RemoveVlanMember on a multi-VLAN port deleted it, making the next RemoveVlanMember fail with a missing-key error. Changes: - Key VlanTable on (PACKET_PORT_INPUT, VLAN_TAG). Untagged and priority-tagged members program (port, 0) -> push 802.1Q + set VID; tagged members program (port, VID) with no actions. CreatePort's PVID entry moves to (port, 0). - Add programPortVlan/unprogramPortVlan helpers and use them from CreateVlanMember/RemoveVlanMember so removing one membership only removes its own entry. - Add untaggedMemberByPortId and only evict a prior untagged/priority- tagged membership when the new member is untagged/priority-tagged; tagged memberships never evict other memberships. VlanTable is not referenced by any port pipeline on main, so this change does not alter forwarding; it makes SAI VLAN member bookkeeping and table programming consistent for hybrid and trunk ports. Adds TestCreateVlanMemberHybridPort and extends TestCreateVlanMemberTaggingMode to assert the (port, VLAN_TAG) key.
…reset The lag server's memberships map is mutated from concurrent CreateLagMember/RemoveLagMember gRPC handlers without synchronization, RemoveLagMember never forgets a removed membership, and saiSwitch.Reset (called from SAI re-initialization together with attrmgr.Reset, which restarts OID allocation) never resets the lag server. After a reset, stale memberships keyed by recycled OIDs can make RemoveLagMember detach the wrong port from the wrong LAG. - Add lag.mu and take it in Reset, CreateLagMember and RemoveLagMember. - Claim (delete) the membership under the lock in RemoveLagMember and restore it if the dataplane AggregateDel fails, so the removal can be retried. - Call sw.Lag.Reset() from saiSwitch.Reset(). Adds TestRemoveLagMemberForgetsMembership, TestRemoveLagMemberRetryAfterDataplaneError, TestSwitchResetClearsLagMemberships and TestLagMemberConcurrent (run with -race).
VlanTable entries are keyed on PACKET_PORT_INPUT, which carries the physical ingress port. When a LAG was added to a VLAN, CreateVlanMember programmed an entry keyed on the LAG's aggregate-port NID, which can never match, and LAG membership changes never updated VLAN programming. - Record OBJECT_TYPE_LAG in CreateLag before returning so the vlan server can identify LAG bridge-port targets. - Add lag.memberPorts and vlan.targetPhysicalPorts, and program / unprogram LAG VLAN memberships on every current physical member port in CreateVlanMember and RemoveVlanMember. A failed RemoveVlanMember re-programs the ports it already unprogrammed and restores the member, so it can be retried. - Add vlan.lagVlanMembers and use it from CreateLagMember / RemoveLagMember so ports joining or leaving a LAG inherit or drop the LAG's VLAN memberships. - Add vlan.memberMu, held by CreateVlanMember, RemoveVlanMember, CreateLagMember and RemoveLagMember across their state change and VlanTable programming. Snapshotting the other side's membership under vlan.mu/lag.mu alone is not sufficient: the dataplane add/remove issued after the snapshot can still be reordered (e.g. RemoveVlanMember unprograms (port, VLAN) before a concurrent CreateLagMember has programmed it), leaking or failing the entry. Lock order is memberMu -> vlan.mu -> lag.mu; memberMu is only taken by SAI handlers and never from dataplane callbacks, so holding it across dataplane RPCs cannot deadlock with the bridge learn callback path. - Wire the vlan and lag servers together in newSwitch. Depends on the hybrid-port (port, VLAN_TAG) VlanTable keying and on the LAG membership mutex. Adds TestVlanLagMembership and TestVlanLagMembershipConcurrent (run with -race), which fails without memberMu.
mohankarthik
force-pushed
the
split/pr5-lag-vlan-members
branch
from
October 30, 2026 06:21
bb6b61e to
dd27208
Compare
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
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.
Depends on #764, #765
VlanTable entries are keyed on PACKET_PORT_INPUT, which carries the
physical ingress port. When a LAG was added to a VLAN, CreateVlanMember
programmed an entry keyed on the LAG's aggregate-port NID, which can
never match, and LAG membership changes never updated VLAN programming.
server can identify LAG bridge-port targets.
unprogram LAG VLAN memberships on every current physical member port
in CreateVlanMember and RemoveVlanMember. A failed RemoveVlanMember
re-programs the ports it already unprogrammed and restores the
member, so it can be retried.
RemoveLagMember so ports joining or leaving a LAG inherit or drop the
LAG's VLAN memberships.
CreateLagMember and RemoveLagMember across their state change and
VlanTable programming. Snapshotting the other side's membership under
vlan.mu/lag.mu alone is not sufficient: the dataplane add/remove
issued after the snapshot can still be reordered (e.g. RemoveVlanMember
unprograms (port, VLAN) before a concurrent CreateLagMember has
programmed it), leaking or failing the entry. Lock order is
memberMu -> vlan.mu -> lag.mu; memberMu is only taken by SAI handlers
and never from dataplane callbacks, so holding it across dataplane
RPCs cannot deadlock with the bridge learn callback path.
Depends on the hybrid-port (port, VLAN_TAG) VlanTable keying and on the
LAG membership mutex.
Adds TestVlanLagMembership and TestVlanLagMembershipConcurrent (run
with -race), which fails without memberMu.