diff --git a/dataplane/saiserver/routing.go b/dataplane/saiserver/routing.go index 0ad054f5..9389efa2 100644 --- a/dataplane/saiserver/routing.go +++ b/dataplane/saiserver/routing.go @@ -1280,7 +1280,7 @@ type vlanMember struct { } type vlan struct { - mu sync.Mutex + mu sync.RWMutex saipb.UnimplementedVlanServer mgr *attrmgr.AttrMgr dataplane switchDataplaneAPI @@ -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 @@ -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 { @@ -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 { @@ -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 } @@ -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 @@ -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, @@ -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 { @@ -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 @@ -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() { @@ -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{} } diff --git a/dataplane/saiserver/vlan_test.go b/dataplane/saiserver/vlan_test.go index 75f5a548..1ce93000 100644 --- a/dataplane/saiserver/vlan_test.go +++ b/dataplane/saiserver/vlan_test.go @@ -17,6 +17,7 @@ package saiserver import ( "context" "fmt" + "sync" "testing" "google.golang.org/grpc" @@ -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()) + } +}