Repository navigation
fix(saiserver): support hybrid (untagged + tagged) VLAN membership on a port - #764
Open
mohankarthik wants to merge 3 commits into
Open
mohankarthik wants to merge 3 commits into
mohankarthik wants to merge 3 commits into
Conversation
This was referenced Oct 21, 2026
mohankarthik
force-pushed
the
split/pr3-hybrid-vlan-port
branch
3 times, most recently
from
October 26, 2026 04:46
1e8d014 to
164d61f
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.
mohankarthik
force-pushed
the
split/pr3-hybrid-vlan-port
branch
from
October 30, 2026 06:21
164d61f to
24a730a
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 #762, #763
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:
(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.
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:
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).
CreateVlanMember/RemoveVlanMember so removing one membership only
removes its own entry.
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.