Checkpoint 3 l2 fdb by thineshvs-google · Pull Request #722 · openconfig/lemming

thineshvs-google · 2026-09-04T06:23:08Z

No description provided.

…d entrypoint stubs (checkpoint 1)

…ng and aging events

…d SAI Bookworm build (checkpoint 2) - dataplane/saiserver/routing.go: - Dynamically set packet output decap pipeline on VLAN member ports based on VlanTaggingMode (TAGGED keeps 802.1Q header on wire; UNTAGGED decaps 802.1Q header). - Store BridgePortID in vlanMember struct for fast, reliable bridge port OID resolution. - dataplane/saiserver/switch.go: - Fixed onBridgeLearn bridge port resolution for all physical ports (including high port IDs) via member.BridgePortID and PopulateAttributes. - dataplane/saiserver/fdb.go & dataplane/forwarding/: - Fixed FlushFdbEntries and RemoveFdbEntry to synchronize state with internal fwdbridge.Table. - dataplane/standalone/sai/fdb.cc: - Added conversion from sai_fdb_entry_t to protobuf in SAI FDB entry API functions and bulk operations. - dataplane/standalone/BUILD & Dockerfile.saibuilder: - Added Debian 12 Bookworm container build definition with static libstdc++/libgcc linking for SONiC runtime.

mohankarthik

Leaving inline comments on the sections of #722 corresponding to Chunk 2 (#759), Chunk 3 (l2-chunk3-vlan-flood-tagging), and Chunk 7 (l2-chunk7-switch-bridge-learn) so these edge cases (hybrid untagged+tagged ports, LAG VLAN members, and vlan.mu synchronization) can be resolved directly as the remaining chunks are split out.

For reference, the main-compatible portions are already split out into small standalone PRs:

#762 (vlan.mu sync.RWMutex across all map accesses + RemoveVlanMember rollback on dataplane error)
#763 (RemoveVlanMember VLAN_ATTR_MEMBER_LIST cleanup)
#764 (composite (PACKET_PORT_INPUT, VLAN_TAG) key in VlanTable for hybrid untagged+tagged ports)
#765 (lag.mu synchronization + lag.Reset())
#766 (OBJECT_TYPE_LAG VLAN membership programming across physical member ports)

mohankarthik · 2026-10-21T11:12:11Z

+func (vlan *vlan) oidByVid(vid uint32) (uint64, bool) {
+	vlan.mu.Lock()
+	defer vlan.mu.Unlock()
+	oid, ok := vlan.oidByVId[vid]
+	return oid, ok
+}


Incomplete vlan.mu synchronization and lock held across dataplane RPCs:

memberByPortId (line 1254) and oidByVid (line 1267) acquire vlan.mu, but vidByOid (line 1234), memberByOid (line 1242), CreateVlan (lines 1274, 1302–1303), CreateVlanMember (line 1476 vlan.oidByVId[member.Vid]), and Reset() (lines 1562–1563) read and write vlan.oidByVId and vlan.vlans without holding vlan.mu, causing concurrent map read/write panics under -race.

In RemoveVlanMember (lines 1499–1500), vlan.mu.Lock(); defer vlan.mu.Unlock() is held across synchronous ObjectNID, TableEntryRemove, and PortUpdate dataplane RPCs, and VLAN_ATTR_MEMBER_LIST on the parent VLAN attribute is never updated to remove r.GetOid().

We split out the complete sync.RWMutex fix (releasing the lock before dataplane RPCs and rolling back if ObjectNID/TableEntryRemove fails) in #762 and the VLAN_ATTR_MEMBER_LIST cleanup in #763 so Chunk 2 / Chunk 3 can rebase directly on top of them.

mohankarthik · 2026-10-21T11:12:11Z

+	outputActions := []*fwdpb.ActionDesc{}
+	if r.GetVlanTaggingMode() == saipb.VlanTaggingMode_VLAN_TAGGING_MODE_UNTAGGED || r.GetVlanTaggingMode() == saipb.VlanTaggingMode_VLAN_TAGGING_MODE_PRIORITY_TAGGED {
+		outputActions = []*fwdpb.ActionDesc{
+			fwdconfig.Action(fwdconfig.DecapAction(fwdpb.PacketHeaderId_PACKET_HEADER_ID_ETHERNET_VLAN)).Build(),
+		}
+	}
+	portUpd := &fwdpb.PortUpdateRequest{
+		ContextId: &fwdpb.ContextId{Id: vlan.dataplane.ID()},
+		PortId:    &fwdpb.PortId{ObjectId: &fwdpb.ObjectId{Id: fmt.Sprint(portID)}},
+		Update: &fwdpb.PortUpdateDesc{
+			Port: &fwdpb.PortUpdateDesc_Kernel{
+				Kernel: &fwdpb.KernelPortUpdateDesc{
+					Inputs:  getPreIngressPipeline(),
+					Outputs: outputActions,
+				},
+			},
+		},
+	}
+	if _, err := vlan.dataplane.PortUpdate(ctx, portUpd); err != nil {
+		return nil, err
+	}


Per-port KernelPortUpdateDesc.Outputs VLAN decap breaks hybrid ports and LAGs:

Setting DecapAction(PACKET_HEADER_ID_ETHERNET_VLAN) on KernelPortUpdateDesc.Outputs (in ports.go:309-311 and routing.go:1421-1441) applies a single egress action to the entire port rather than per-(port, VLAN):

Hybrid ports (untagged native VLAN + tagged VLANs on the same port): Whichever CreateVlanMember call runs last overwrites Outputs for the entire port. If an UNTAGGED VLAN (e.g. Vlan42) is programmed after a TAGGED VLAN (e.g. Vlan666), Outputs is set to DecapAction, stripping the 802.1Q tag from Vlan666 egress frames. Conversely, if TAGGED runs after UNTAGGED, Outputs is cleared ([]), leaving 802.1Q tags on Vlan42 egress frames.

LAG (OBJECT_TYPE_LAG) members: When portID is a LAG OID (PortChannel), calling PortUpdate with PortUpdateDesc_Kernel fails because the port is an AggregatePort, not a KernelPort.

RemoveVlanMember: RemoveVlanMember never updates or restores Outputs.

Suggested fix for Chunk 3:
Instead of mutating KernelPortUpdateDesc.Outputs, add an exact-match EgressVlanTable ("egress-vlan", keyed on (PACKET_FIELD_NUM_TARGET_EGRESS_PORT, PACKET_FIELD_NUM_VLAN_TAG) with default ContinueAction) right before ACTION_TYPE_OUTPUT in getL2Pipeline() (ports.go:103-113), and program DecapAction(PACKET_HEADER_ID_ETHERNET_VLAN) into EgressVlanTable only for (portNID, vid) entries whose mode is UNTAGGED or PRIORITY_TAGGED (see #756 for the reference implementation). For ingress hybrid port classification ((PACKET_PORT_INPUT, VLAN_TAG) composite key in VlanTable), see #764.

mohankarthik · 2026-10-21T11:12:11Z

+	floodID := fmt.Sprintf("vlan_flood_%d", vId)
+	_, _ = vlan.dataplane.PortUpdate(ctx, &fwdpb.PortUpdateRequest{
+		ContextId: &fwdpb.ContextId{Id: vlan.dataplane.ID()},
+		PortId:    &fwdpb.PortId{ObjectId: &fwdpb.ObjectId{Id: floodID}},
+		Update: &fwdpb.PortUpdateDesc{
+			Port: &fwdpb.PortUpdateDesc_AggregateAdd{
+				AggregateAdd: &fwdpb.AggregatePortAddMemberUpdateDesc{
+					PortId:        &fwdpb.PortId{ObjectId: &fwdpb.ObjectId{Id: fmt.Sprint(portID)}},
+					InstanceCount: 1,
+				},
+			},
+		},
+	})


vlan_flood_<vId> aggregate port updates need OBJECT_TYPE_LAG expansion and hybrid-port safety:

If a port is a hybrid member of the same or multiple VLANs or if RemoveVlanMember removes one membership while another remains, AggregateAdd / AggregateDel on vlan_flood_<vId> should ensure physical ports are not added twice or removed while still a member.

When r.GetBridgePortId() wraps a LAG (OBJECT_TYPE_LAG), portID is the LAG OID rather than physical port OIDs, and member ports can also be added/removed dynamically via CreateLagMember / RemoveLagMember. Once fix(saiserver): program LAG VLAN memberships on LAG member ports #766 lands, FloodTable / vlan_flood_<vId> in Chunk 3 can use targetPhysicalPorts(portID) so LAG VLAN members flood across constituent physical ports.

mohankarthik · 2026-10-21T11:12:11Z

+	var vlanOID uint64
+	var vlanID uint32 = uint32(DefaultVlanId)
+
+	if sw.vlan != nil {
+		if member := sw.vlan.memberByPortId(portNum); member != nil {
+			vlanID = member.Vid
+			if member.BridgePortID != 0 {
+				bridgePortOID = member.BridgePortID
+			}
+			if oid, ok := sw.vlan.oidByVid(vlanID); ok {
+				vlanOID = oid
+				slog.Info("onBridgeLearn: found VLAN for port", "portNum", portNum, "vlanID", vlanID, "vlanOID", vlanOID)
+			} else {
+				slog.Warn("onBridgeLearn: failed to get OID for VLAN", "vlanID", vlanID)
+			}
+		} else {
+			slog.Warn("onBridgeLearn: no VLAN member found for port", "portNum", portNum)
+		}
+	} else {


onBridgeLearn resolves VLAN via memberByPortId(portNum), which is ambiguous on hybrid/trunk ports and fails on LAG member ports:

In bridge.go:169 (merged in Chunk 1 feat(forwarding): add BridgeLearnAction builder, exact RemoveKey, and… #757), t.LearnCallback(req.mac, string(fwdport.GetID(port).GetObjectId().GetId())) passes only (mac, portID) without the packet's PACKET_FIELD_NUM_VLAN_TAG.

In switch.go:1331, sw.vlan.memberByPortId(portNum) iterates vlan.vlans and returns the first matching vlanMember for portNum.

On a hybrid or trunk port belonging to multiple VLANs (e.g. Vlan42 untagged + Vlan666 tagged), Go map iteration order is non-deterministic, so learned MACs get attributed to the wrong vlanOID.

On a LAG member physical port, member.PortID in vlan.vlans is the LAG OID (PortChannel), so memberByPortId(portNum) returns nil and falls back to DefaultVlanId (VLAN 1), and the BRIDGE_PORT_ATTR_PORT_ID lookup (lines 1349–1362) fails to find the LAG's bridge port OID.

Suggested fix for Chunk 7:

Update learnReq / Learn() in dataplane/forwarding/fwdtable/bridge/bridge.go to read PACKET_FIELD_NUM_VLAN_TAG from the packet and pass vid uint16 to LearnCallback(req.mac, portID, vid).

In onBridgeLearn, look up the vlanMember by (portNum, uint32(vid)) (checking both direct physical port membership and parent LAG membership via sw.Lag when portNum is a LAG member port; see feat(dataplane): L2 FDB learning, per-(port, VLAN) ingress/egress tables, hybrid VLANs, and LAG VLAN programming (ref for #722) #756).

thineshvs-google and others added 7 commits July 27, 2026 13:24

Add SAI L2 bridging, FDB learning, and BUM flooding support

ca43c61

add notification to syncd for fdb updates

c06fd33

Add dynamic bridge MAC learning notification handler

0a282c1

l2 corrected - show vlan brief

8a29b02

fix(dataplane): register SAI object types for bridge ports, VLANs, an…

ddd8fc5

…d entrypoint stubs (checkpoint 1)

feat(dataplane): implement FlushFdbEntries, dynamic/static FDB tracki…

a5c31ca

…ng and aging events

thineshvs-google requested a review from a team as a code owner September 4, 2026 06:23

mohankarthik mentioned this pull request Oct 13, 2026

feat(dataplane): L2 FDB learning, per-(port, VLAN) ingress/egress tables, hybrid VLANs, and LAG VLAN programming (ref for #722) #756

Open

mohankarthik reviewed Oct 21, 2026

View reviewed changes

mohankarthik mentioned this pull request Oct 22, 2026

feat(saiserver): add FDB/Flood tables, VLAN flood ports, and L2 tagging #767

Merged

thineshvs-google mentioned this pull request Oct 22, 2026

feat(saiserver): implement SAI FDB entry CRUD and event pub/sub #769

Open

Navigation Menu

Repository navigation

Checkpoint 3 l2 fdb - #722

Checkpoint 3 l2 fdb#722
thineshvs-google wants to merge 7 commits into
openconfig:mainfrom
thineshvs-google:checkpoint-3-l2-fdb

thineshvs-google commented Sep 4, 2026

Uh oh!

mohankarthik left a comment

Uh oh!

mohankarthik Oct 21, 2026

Uh oh!

mohankarthik Oct 21, 2026

Uh oh!

mohankarthik Oct 21, 2026

Uh oh!

mohankarthik Oct 21, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

thineshvs-google commented Sep 4, 2026

Uh oh!

mohankarthik left a comment

Choose a reason for hiding this comment

Uh oh!

mohankarthik Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

mohankarthik Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

mohankarthik Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

mohankarthik Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants