Skip to content

Vlan membership table fix #683

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

Merged
sonikajindal merged 2 commits into from
Apr 8, 2026
+180 −12
Merged

Vlan membership table fix #683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fab26d4
Fix vlan member creation with bridge ports
royyi8 Apr 4, 2026
22af1ab
Remove member from cache to fix vlan double delete error
royyi8 Apr 7, 2026
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
Prev Previous commit
Remove member from cache to fix vlan double delete error
  • Loading branch information
@royyi8
royyi8 committed Apr 8, 2026
commit 22af1ab0f2bbc1b5e0e1864a62696425a1c813fe
34 changes: 30 additions & 4 deletions dataplane/saiserver/routing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1273,9 +1273,17 @@ func (vlan *vlan) RemoveVlan(ctx context.Context, r *saipb.RemoveVlanRequest) (*
if vId == DefaultVlanId {
return nil, fmt.Errorf("cannot remove default VLAN")
}
for _, v := range vlan.vlans[r.GetOid()] {

vlan.mu.Lock()
var memberOids []uint64
for mOid := range vlan.vlans[r.GetOid()] {
memberOids = append(memberOids, mOid)
}
vlan.mu.Unlock()

for _, mOid := range memberOids {
_, err := attrmgr.InvokeAndSave(ctx, vlan.mgr, vlan.RemoveVlanMember, &saipb.RemoveVlanMemberRequest{
Oid: v.Oid,
Oid: mOid,
})
if err != nil {
return nil, err
Expand Down Expand Up @@ -1375,8 +1383,23 @@ func (vlan *vlan) CreateVlanMember(ctx context.Context, r *saipb.CreateVlanMembe
}

func (vlan *vlan) RemoveVlanMember(ctx context.Context, r *saipb.RemoveVlanMemberRequest) (*saipb.RemoveVlanMemberResponse, error) {
member := vlan.memberByOid(r.GetOid())
if member == nil {
vlan.mu.Lock()
defer vlan.mu.Unlock()

var member *vlanMember
var targetVlanOid uint64
found := false

for vlanOid, members := range vlan.vlans {
if m, ok := members[r.GetOid()]; ok {
member = m
targetVlanOid = vlanOid
found = true
break
}
}

if !found {
return nil, fmt.Errorf("cannot find member with OID %d", r.GetOid())
}
nid, err := vlan.dataplane.ObjectNID(ctx, &fwdpb.ObjectNIDRequest{
Expand All @@ -1391,6 +1414,9 @@ func (vlan *vlan) RemoveVlanMember(ctx context.Context, r *saipb.RemoveVlanMembe
fwdconfig.EntryDesc(fwdconfig.ExactEntry(fwdconfig.PacketFieldBytes(fwdpb.PacketFieldNum_PACKET_FIELD_NUM_PACKET_PORT_INPUT).WithUint64(nid.GetNid())))).Build()); err != nil {
return nil, err
}

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

return &saipb.RemoveVlanMemberResponse{}, nil
}

Expand Down
4 changes: 2 additions & 2 deletions dataplane/saiserver/routing_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1063,7 +1063,7 @@ func TestRemoveRouterInterface(t *testing.T) {
FieldId: &fwdpb.PacketFieldId{Field: &fwdpb.PacketField{
FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_PACKET_PORT_INPUT,
}},
Bytes: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
Bytes: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05},
}, {
FieldId: &fwdpb.PacketFieldId{Field: &fwdpb.PacketField{
FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_VLAN_TAG,
Expand All @@ -1089,7 +1089,7 @@ func TestRemoveRouterInterface(t *testing.T) {
FieldId: &fwdpb.PacketFieldId{Field: &fwdpb.PacketField{
FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_PACKET_PORT_INPUT,
}},
Bytes: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00},
Bytes: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05},
}, {
FieldId: &fwdpb.PacketFieldId{Field: &fwdpb.PacketField{
FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_VLAN_TAG,
Expand Down
13 changes: 12 additions & 1 deletion dataplane/saiserver/switch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@ package saiserver

import (
"context"
"fmt"
"io"
"log"
"net"
"strconv"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -248,6 +250,12 @@ func (f *fakeSwitchDataplane) TableEntryAdd(_ context.Context, req *fwdpb.TableE
}

func (f *fakeSwitchDataplane) TableEntryRemove(_ context.Context, req *fwdpb.TableEntryRemoveRequest) (*fwdpb.TableEntryRemoveReply, error) {
for _, prev := range f.gotEntryRemoveReqs {
if cmp.Equal(prev, req, protocmp.Transform()) {
return nil, fmt.Errorf("double table entry removal detected: %v", req)
}
}

f.gotEntryRemoveReqs = append(f.gotEntryRemoveReqs, req)
return nil, nil
}
Expand Down Expand Up @@ -295,7 +303,10 @@ func (f *fakeSwitchDataplane) AttributeUpdate(context.Context, *fwdpb.AttributeU

func (f *fakeSwitchDataplane) ObjectNID(_ context.Context, req *fwdpb.ObjectNIDRequest) (*fwdpb.ObjectNIDReply, error) {
f.gotObjectNIDReqs = append(f.gotObjectNIDReqs, req)
return nil, nil

// Derive unique nid by using request object id to prevent collisions in tests.
id, _ := strconv.ParseUint(req.GetObjectId().GetId(), 10, 64)
return &fwdpb.ObjectNIDReply{Nid: id}, nil
}

func (f *fakeSwitchDataplane) InjectPacket(_ *fwdpb.ContextId, _ *fwdpb.PortId, _ fwdpb.PacketHeaderId, pkt []byte, _ []*fwdpb.ActionDesc, _ bool, _ fwdpb.PortAction) error {
Expand Down
54 changes: 54 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 @@ -326,3 +326,57 @@ func TestCreateVlanMemberLogicalMapping(t *testing.T) {
t.Errorf("ObjectNID was not queried with port id 10, got reqs: %+v", dplane.gotObjectNIDReqs)
}
}

func TestRemoveVlan(t *testing.T) {
dplane := &fakeSwitchDataplane{}
c, mgr, stopFn := newTestVlan(t, dplane)
defer stopFn()
ctx := context.TODO()

mgr.StoreAttributes(1, &saipb.SwitchAttribute{
DefaultStpInstId: proto.Uint64(testStpInstId),
})

// Create vlan.
vlanResp, err := c.CreateVlan(ctx, &saipb.CreateVlanRequest{
Switch: 1,
VlanId: proto.Uint32(10),
})
if err != nil {
t.Fatalf("CreateVlan failed: %v", err)
}
vlanOid := vlanResp.Oid

// Create mapping for mock bridge ports.
bridgePortOid := uint64(100)
physicalPortId := uint64(10)
mgr.StoreAttributes(bridgePortOid, &saipb.BridgePortAttribute{
PortId: proto.Uint64(physicalPortId),
})

// Create vlan member with bridge port.
memberResp, err := c.CreateVlanMember(ctx, &saipb.CreateVlanMemberRequest{
Switch: 1,
VlanId: &vlanOid,
BridgePortId: &bridgePortOid,
})
if err != nil {
t.Fatalf("CreateVlanMember failed: %v", err)
}

// Remove vlan member.
_, err = c.RemoveVlanMember(ctx, &saipb.RemoveVlanMemberRequest{
Oid: memberResp.Oid,
})
if err != nil {
t.Fatalf("RemoveVlanMember failed: %v", err)
}

// Remove vlan.
_, err = c.RemoveVlan(ctx, &saipb.RemoveVlanRequest{
Oid: vlanOid,
})
if err != nil {
t.Fatalf("RemoveVlan failed: %v", err)
}
}
Loading