Skip to content

fix(saiserver): synchronize vlan server map access with vlan.mu #762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mohankarthik wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(saiserver): synchronize vlan server map access with vlan.mu #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 44 additions & 11 deletions dataplane/saiserver/routing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,7 @@ type vlanMember struct {
}

type vlan struct {
mu sync.Mutex
mu sync.RWMutex
saipb.UnimplementedVlanServer
mgr *attrmgr.AttrMgr
dataplane switchDataplaneAPI
Expand All @@ -1300,6 +1300,8 @@ func newVlan(mgr *attrmgr.AttrMgr, dataplane switchDataplaneAPI, s *grpc.Server)
}

func (vlan *vlan) vidByOid(oid uint64) (uint32, error) {
vlan.mu.RLock()
defer vlan.mu.RUnlock()
for v, o := range vlan.oidByVId {
if o == oid {
return v, nil
Expand All @@ -1309,6 +1311,8 @@ func (vlan *vlan) vidByOid(oid uint64) (uint32, error) {
}

func (vlan *vlan) memberByOid(oid uint64) *vlanMember {
vlan.mu.RLock()
defer vlan.mu.RUnlock()
for _, v := range vlan.vlans {
for mOid, member := range v {
if mOid == oid {
Expand All @@ -1320,8 +1324,8 @@ func (vlan *vlan) memberByOid(oid uint64) *vlanMember {
}

func (vlan *vlan) memberByPortId(oid uint64) *vlanMember {
vlan.mu.Lock()
defer vlan.mu.Unlock()
vlan.mu.RLock()
defer vlan.mu.RUnlock()
for _, v := range vlan.vlans {
for _, member := range v {
if member.PortID == oid {
Expand All @@ -1332,9 +1336,10 @@ func (vlan *vlan) memberByPortId(oid uint64) *vlanMember {
return nil
}

// oidByVid returns the VLAN OID for the given VLAN ID.
func (vlan *vlan) oidByVid(vid uint32) (uint64, bool) {
vlan.mu.Lock()
defer vlan.mu.Unlock()
vlan.mu.RLock()
defer vlan.mu.RUnlock()
oid, ok := vlan.oidByVId[vid]
return oid, ok
}
Expand Down Expand Up @@ -1368,8 +1373,10 @@ func (vlan *vlan) CreateVlan(ctx context.Context, r *saipb.CreateVlanRequest) (*
TamObject: []uint64{},
}
vlan.mgr.StoreAttributes(id, attrs)
vlan.mu.Lock()
vlan.vlans[id] = map[uint64]*vlanMember{}
vlan.oidByVId[r.GetVlanId()] = id
vlan.mu.Unlock()
return &saipb.CreateVlanResponse{
Oid: id,
}, nil
Expand Down Expand Up @@ -1468,7 +1475,12 @@ func (vlan *vlan) CreateVlanMember(ctx context.Context, r *saipb.CreateVlanMembe
vlanAttrResp.GetAttr().MemberList = append(vlanAttrResp.GetAttr().MemberList, mOid)
vlan.mgr.StoreAttributes(vOid, vlanAttrResp.GetAttr())
vlan.mu.Lock()
vlan.vlans[vOid][mOid] = &vlanMember{
members, ok := vlan.vlans[vOid]
if !ok {
vlan.mu.Unlock()
return nil, fmt.Errorf("cannot find VLAN with OID %d", vOid)
}
members[mOid] = &vlanMember{
Oid: mOid,
PortID: portID,
Vid: vId,
Expand All @@ -1479,7 +1491,10 @@ func (vlan *vlan) CreateVlanMember(ctx context.Context, r *saipb.CreateVlanMembe

// Fetch the original vlan from the old vlan member and remove the member from that vlan
if member != nil {
preVlanOid, _ := vlan.oidByVid(member.Vid)
preVlanOid, ok := vlan.oidByVid(member.Vid)
if !ok {
return nil, fmt.Errorf("cannot find VLAN OID for VID %d", member.Vid)
}
vlanAttrReq = &saipb.GetVlanAttributeRequest{Oid: preVlanOid, AttrType: []saipb.VlanAttr{saipb.VlanAttr_VLAN_ATTR_MEMBER_LIST}}
vlanAttrResp = &saipb.GetVlanAttributeResponse{}
if err := vlan.mgr.PopulateAttributes(vlanAttrReq, vlanAttrResp); err != nil {
Expand All @@ -1502,9 +1517,11 @@ func (vlan *vlan) CreateVlanMember(ctx context.Context, r *saipb.CreateVlanMembe
}

func (vlan *vlan) RemoveVlanMember(ctx context.Context, r *saipb.RemoveVlanMemberRequest) (*saipb.RemoveVlanMemberResponse, error) {
// Claim the member under the lock so that concurrent removals of the same
// OID cannot both proceed, then release the lock before issuing dataplane
// RPCs so vlan.mu is never held across calls into the dataplane. If the
// dataplane removal fails, the claim is undone so the call can be retried.
vlan.mu.Lock()
defer vlan.mu.Unlock()

var member *vlanMember
var targetVlanOid uint64
found := false
Expand All @@ -1519,26 +1536,40 @@ func (vlan *vlan) RemoveVlanMember(ctx context.Context, r *saipb.RemoveVlanMembe
}

if !found {
vlan.mu.Unlock()
return nil, fmt.Errorf("cannot find member with OID %d", r.GetOid())
}
delete(vlan.vlans[targetVlanOid], r.GetOid())
vlan.mu.Unlock()

nid, err := vlan.dataplane.ObjectNID(ctx, &fwdpb.ObjectNIDRequest{
ContextId: &fwdpb.ContextId{Id: vlan.dataplane.ID()},
ObjectId: &fwdpb.ObjectId{Id: fmt.Sprint(member.PortID)},
})
if err != nil {
slog.InfoContext(ctx, "Failed to find NID for port", "bridge_port", member.PortID, "err", err)
vlan.restoreMember(targetVlanOid, member)
return nil, err
}
if _, err := vlan.dataplane.TableEntryRemove(ctx, fwdconfig.TableEntryRemoveRequest(vlan.dataplane.ID(), VlanTable).AppendEntry(
fwdconfig.EntryDesc(fwdconfig.ExactEntry(fwdconfig.PacketFieldBytes(fwdpb.PacketFieldNum_PACKET_FIELD_NUM_PACKET_PORT_INPUT).WithUint64(nid.GetNid())))).Build()); err != nil {
vlan.restoreMember(targetVlanOid, member)
return nil, err
}

delete(vlan.vlans[targetVlanOid], r.GetOid())

return &saipb.RemoveVlanMemberResponse{}, nil
}

// restoreMember re-inserts a member claimed by RemoveVlanMember after its
// dataplane removal failed, unless the parent VLAN has since been removed.
func (vlan *vlan) restoreMember(vlanOid uint64, member *vlanMember) {
vlan.mu.Lock()
defer vlan.mu.Unlock()
if members, ok := vlan.vlans[vlanOid]; ok {
members[member.Oid] = member
}
}

func (vlan *vlan) CreateVlanMembers(ctx context.Context, r *saipb.CreateVlanMembersRequest) (*saipb.CreateVlanMembersResponse, error) {
resp := &saipb.CreateVlanMembersResponse{}
for _, req := range r.GetReqs() {
Expand All @@ -1553,6 +1584,8 @@ func (vlan *vlan) CreateVlanMembers(ctx context.Context, r *saipb.CreateVlanMemb

func (vlan *vlan) Reset() {
slog.Info("resetting vlan")
vlan.mu.Lock()
defer vlan.mu.Unlock()
vlan.oidByVId = map[uint32]uint64{}
vlan.vlans = map[uint64]map[uint64]*vlanMember{}
}
Expand Down
169 changes: 169 additions & 0 deletions dataplane/saiserver/vlan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package saiserver
import (
"context"
"fmt"
"sync"
"testing"

"google.golang.org/grpc"
Expand Down Expand Up @@ -461,3 +462,171 @@ func TestCreateVlanMemberTaggingMode(t *testing.T) {
}
}

// lockedVlanDataplane serializes access to the fakeSwitchDataplane methods used
// by the vlan server so that concurrency tests only flag races in the vlan server.
type lockedVlanDataplane struct {
*fakeSwitchDataplane
mu sync.Mutex
}

func (l *lockedVlanDataplane) ObjectNID(ctx context.Context, req *fwdpb.ObjectNIDRequest) (*fwdpb.ObjectNIDReply, error) {
l.mu.Lock()
defer l.mu.Unlock()
return l.fakeSwitchDataplane.ObjectNID(ctx, req)
}

func (l *lockedVlanDataplane) TableEntryAdd(ctx context.Context, req *fwdpb.TableEntryAddRequest) (*fwdpb.TableEntryAddReply, error) {
l.mu.Lock()
defer l.mu.Unlock()
return l.fakeSwitchDataplane.TableEntryAdd(ctx, req)
}

func (l *lockedVlanDataplane) TableEntryRemove(ctx context.Context, req *fwdpb.TableEntryRemoveRequest) (*fwdpb.TableEntryRemoveReply, error) {
l.mu.Lock()
defer l.mu.Unlock()
return l.fakeSwitchDataplane.TableEntryRemove(ctx, req)
}

// TestVlanConcurrentAccess exercises the vlan server's internal maps from
// concurrent SAI calls and lookup helpers. It is intended to be run with -race.
func TestVlanConcurrentAccess(t *testing.T) {
const workers = 8
dplane := &lockedVlanDataplane{fakeSwitchDataplane: &fakeSwitchDataplane{}}
var v *vlan
conn, mgr, stopFn := newTestServer(t, func(mgr *attrmgr.AttrMgr, srv *grpc.Server) {
v = newVlan(mgr, dplane, srv)
})
defer stopFn()
c := saipb.NewVlanClient(conn)
ctx := context.Background()

mgr.StoreAttributes(1, &saipb.SwitchAttribute{DefaultStpInstId: proto.Uint64(testStpInstId)})
for i := 0; i < workers; i++ {
portID := uint64(1000 + i)
mgr.StoreAttributes(portID+10000, &saipb.BridgePortAttribute{PortId: proto.Uint64(portID)})
}

done := make(chan struct{})
var readers sync.WaitGroup
readers.Add(1)
go func() {
defer readers.Done()
for {
select {
case <-done:
return
default:
}
for i := 0; i < workers; i++ {
v.memberByPortId(uint64(1000 + i))
if oid, ok := v.oidByVid(uint32(100 + i)); ok {
v.vidByOid(oid)
v.memberByOid(oid)
}
}
}
}()

var writers sync.WaitGroup
errCh := make(chan error, workers)
for i := 0; i < workers; i++ {
writers.Add(1)
go func(i int) {
defer writers.Done()
vResp, err := c.CreateVlan(ctx, &saipb.CreateVlanRequest{Switch: 1, VlanId: proto.Uint32(uint32(100 + i))})
if err != nil {
errCh <- fmt.Errorf("CreateVlan(%d) failed: %v", 100+i, err)
return
}
mResp, err := c.CreateVlanMember(ctx, &saipb.CreateVlanMemberRequest{
Switch: 1,
VlanId: proto.Uint64(vResp.GetOid()),
BridgePortId: proto.Uint64(uint64(1000+i) + 10000),
})
if err != nil {
errCh <- fmt.Errorf("CreateVlanMember(%d) failed: %v", 100+i, err)
return
}
if _, err := c.RemoveVlanMember(ctx, &saipb.RemoveVlanMemberRequest{Oid: mResp.GetOid()}); err != nil {
errCh <- fmt.Errorf("RemoveVlanMember(%d) failed: %v", mResp.GetOid(), err)
return
}
if _, err := c.RemoveVlan(ctx, &saipb.RemoveVlanRequest{Oid: vResp.GetOid()}); err != nil {
errCh <- fmt.Errorf("RemoveVlan(%d) failed: %v", vResp.GetOid(), err)
}
}(i)
}
writers.Wait()
close(done)
readers.Wait()
close(errCh)
for err := range errCh {
t.Error(err)
}

for i := 0; i < workers; i++ {
if _, ok := v.oidByVid(uint32(100 + i)); ok {
t.Errorf("VLAN %d still present after RemoveVlan", 100+i)
}
if m := v.memberByPortId(uint64(1000 + i)); m != nil {
t.Errorf("port %d still has VLAN member %+v after RemoveVlanMember", 1000+i, m)
}
}
}

// failingRemoveDataplane fails the next TableEntryRemove call when failRemove is set.
type failingRemoveDataplane struct {
*fakeSwitchDataplane
failRemove bool
}

func (f *failingRemoveDataplane) TableEntryRemove(ctx context.Context, req *fwdpb.TableEntryRemoveRequest) (*fwdpb.TableEntryRemoveReply, error) {
if f.failRemove {
f.failRemove = false
return nil, fmt.Errorf("injected TableEntryRemove failure")
}
return f.fakeSwitchDataplane.TableEntryRemove(ctx, req)
}

func TestRemoveVlanMemberRetryAfterDataplaneError(t *testing.T) {
dplane := &failingRemoveDataplane{fakeSwitchDataplane: &fakeSwitchDataplane{}}
var v *vlan
conn, mgr, stopFn := newTestServer(t, func(mgr *attrmgr.AttrMgr, srv *grpc.Server) {
v = newVlan(mgr, dplane, srv)
})
defer stopFn()
c := saipb.NewVlanClient(conn)
ctx := context.Background()

mgr.StoreAttributes(1, &saipb.SwitchAttribute{DefaultStpInstId: proto.Uint64(testStpInstId)})
const bridgePortOid, portID = uint64(100), uint64(10)
mgr.StoreAttributes(bridgePortOid, &saipb.BridgePortAttribute{PortId: proto.Uint64(portID)})

vlanResp, err := c.CreateVlan(ctx, &saipb.CreateVlanRequest{Switch: 1, VlanId: proto.Uint32(10)})
if err != nil {
t.Fatalf("CreateVlan failed: %v", err)
}
memberResp, err := c.CreateVlanMember(ctx, &saipb.CreateVlanMemberRequest{
Switch: 1,
VlanId: proto.Uint64(vlanResp.GetOid()),
BridgePortId: proto.Uint64(bridgePortOid),
})
if err != nil {
t.Fatalf("CreateVlanMember failed: %v", err)
}

dplane.failRemove = true
if _, err := c.RemoveVlanMember(ctx, &saipb.RemoveVlanMemberRequest{Oid: memberResp.GetOid()}); err == nil {
t.Fatalf("RemoveVlanMember with failing dataplane succeeded, want error")
}
if m := v.memberByOid(memberResp.GetOid()); m == nil {
t.Fatalf("VLAN member %d forgotten after failed RemoveVlanMember, want it retained for retry", memberResp.GetOid())
}

if _, err := c.RemoveVlanMember(ctx, &saipb.RemoveVlanMemberRequest{Oid: memberResp.GetOid()}); err != nil {
t.Fatalf("RemoveVlanMember retry failed: %v", err)
}
if m := v.memberByOid(memberResp.GetOid()); m != nil {
t.Errorf("VLAN member %d still present after successful RemoveVlanMember", memberResp.GetOid())
}
}
Loading