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
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
Next Next commit
Fix vlan member creation with bridge ports
  • Loading branch information
@royyi8
royyi8 committed Apr 7, 2026
commit fab26d44ab58f9ffbad99db5aebef712bdd21e33
28 changes: 24 additions & 4 deletions dataplane/saiserver/routing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,14 +1303,30 @@ func (vlan *vlan) CreateVlanMember(ctx context.Context, r *saipb.CreateVlanMembe
if err != nil {
return nil, err
}
member := vlan.memberByPortId(r.GetBridgePortId()) // Keep the vlan member if this port was assigned to any VLAN.

// Get the port ID from the bridge port.
bpAttrReq := &saipb.GetBridgePortAttributeRequest{
Oid: r.GetBridgePortId(),
AttrType: []saipb.BridgePortAttr{saipb.BridgePortAttr_BRIDGE_PORT_ATTR_PORT_ID},
}
bpAttrResp := &saipb.GetBridgePortAttributeResponse{}
if err := vlan.mgr.PopulateAttributes(bpAttrReq, bpAttrResp); err != nil {
return nil, err
}

portID := bpAttrResp.GetAttr().GetPortId()

// For the case where this port was assigned to a prior vlan, store the
// vlan member to remove it from cache later.
member := vlan.memberByPortId(portID)

mOid := vlan.mgr.NextID()
nid, err := vlan.dataplane.ObjectNID(ctx, &fwdpb.ObjectNIDRequest{
ContextId: &fwdpb.ContextId{Id: vlan.dataplane.ID()},
ObjectId: &fwdpb.ObjectId{Id: fmt.Sprint(r.GetBridgePortId())},
ObjectId: &fwdpb.ObjectId{Id: fmt.Sprint(portID)},
})
if err != nil {
slog.InfoContext(ctx, "Failed to find NID for port", "bridge_port", r.GetBridgePortId(), "err", err)
slog.InfoContext(ctx, "Failed to find NID for port", "port_id", portID, "err", err)
return nil, err
}
vlanReq := fwdconfig.TableEntryAddRequest(vlan.dataplane.ID(), VlanTable).AppendEntry(
Expand All @@ -1331,8 +1347,10 @@ 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{Oid: mOid, PortID: r.GetBridgePortId(), Vid: vId, Mode: r.GetVlanTaggingMode()}
vlan.vlans[vOid][mOid] = &vlanMember{Oid: mOid, PortID: portID, Vid: vId, Mode: r.GetVlanTaggingMode()}
vlan.mu.Unlock()

// Fetch the original vlan from the old vlan member and remove the member from that vlan
if member != nil {
preVlanOid := vlan.oidByVId[member.Vid]
vlanAttrReq = &saipb.GetVlanAttributeRequest{Oid: preVlanOid, AttrType: []saipb.VlanAttr{saipb.VlanAttr_VLAN_ATTR_MEMBER_LIST}}
Expand Down Expand Up @@ -1444,6 +1462,8 @@ func (b *bridge) CreateBridgePort(ctx context.Context, req *saipb.CreateBridgePo
adminState := req.GetAdminState()
attrs := &saipb.BridgePortAttribute{
AdminState: proto.Bool(adminState),
PortId: proto.Uint64(req.GetPortId()),
Type: req.Type,
}
b.mgr.StoreAttributes(oid, attrs)
return &saipb.CreateBridgePortResponse{
Expand Down
4 changes: 3 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 @@ -219,6 +219,7 @@ type fakeSwitchDataplane struct {
gotPortCreateReqs []*fwdpb.PortCreateRequest
gotPortUpdateReqs []*fwdpb.PortUpdateRequest
gotObjectDeleteReqs []*fwdpb.ObjectDeleteRequest
gotObjectNIDReqs []*fwdpb.ObjectNIDRequest
gotFlowCounterCreateReqs []*fwdpb.FlowCounterCreateRequest
gotFlowCounterQueryReqs []*fwdpb.FlowCounterQueryRequest
gotEntryRemoveReqs []*fwdpb.TableEntryRemoveRequest
Expand Down Expand Up @@ -292,7 +293,8 @@ func (f *fakeSwitchDataplane) AttributeUpdate(context.Context, *fwdpb.AttributeU
return nil, nil
}

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

Expand Down
55 changes: 55 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 @@ -128,6 +128,11 @@ func TestVlanOperations(t *testing.T) {
mgr.StoreAttributes(1, &saipb.SwitchAttribute{
DefaultStpInstId: proto.Uint64(testStpInstId),
})
// Crete bridge port mapping in attrmgr for each port
mgr.StoreAttributes(11, &saipb.BridgePortAttribute{PortId: proto.Uint64(11)})
mgr.StoreAttributes(12, &saipb.BridgePortAttribute{PortId: proto.Uint64(12)})
mgr.StoreAttributes(13, &saipb.BridgePortAttribute{PortId: proto.Uint64(13)})
mgr.StoreAttributes(14, &saipb.BridgePortAttribute{PortId: proto.Uint64(14)})
ctx := context.TODO()

getVLANMembers := func(vlanOID uint64) ([]uint64, error) {
Expand Down Expand Up @@ -271,3 +276,53 @@ func newTestVlan(t testing.TB, api switchDataplaneAPI) (saipb.VlanClient, *attrm
})
return saipb.NewVlanClient(conn), mgr, stopFn
}

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

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

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

// Create mock bridge port mapping in attrmgr for given port id.
bridgePortOid := uint64(100)
testPortId := uint64(10)
mgr.StoreAttributes(bridgePortOid, &saipb.BridgePortAttribute{
PortId: proto.Uint64(testPortId),
})

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

// Verify that ObjectNID was queried with the port id.
found := false
for _, req := range dplane.gotObjectNIDReqs {
if req.GetObjectId().GetId() == "10" {
found = true
break
}
}
if !found {
t.Errorf("ObjectNID was not queried with port id 10, got reqs: %+v", dplane.gotObjectNIDReqs)
}
}