Repository navigation
fix(saiserver): remove VLAN member OID from MEMBER_LIST in RemoveVlanMember - #763
Open
mohankarthik wants to merge 2 commits into
Open
mohankarthik wants to merge 2 commits into
mohankarthik wants to merge 2 commits into
Conversation
This was referenced Oct 21, 2026
mohankarthik
force-pushed
the
split/pr2-remove-vlan-member-list
branch
from
October 22, 2026 04:09
bf01daf to
43a8ad4
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.
mohankarthik
force-pushed
the
split/pr2-remove-vlan-member-list
branch
from
October 26, 2026 04:23
43a8ad4 to
7a7efd2
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
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.