From fff911d0667f71e4ea9606d0d25e8d543cfd68b5 Mon Sep 17 00:00:00 2001 From: Guo-shiuan Wang Date: Thu, 22 Feb 2024 17:42:44 +0000 Subject: [PATCH] Implement the removal of NextHopGroup and NextHopGroupMember. --- dataplane/dplanerc/routes.go | 4 +- dataplane/saiserver/routing.go | 73 +++++++++-- dataplane/saiserver/routing_test.go | 187 ++++++++++++++++++++++++++++ 3 files changed, 250 insertions(+), 14 deletions(-) diff --git a/dataplane/dplanerc/routes.go b/dataplane/dplanerc/routes.go index b571bcdd..293e6ae8 100644 --- a/dataplane/dplanerc/routes.go +++ b/dataplane/dplanerc/routes.go @@ -120,7 +120,7 @@ func (ni *Reconciler) StartRoute(ctx context.Context, client *ygnmi.Client) erro return ygnmi.Continue } for i, nh := range route.GetNextHops().GetHops() { - hopID, err = ni.createNextHop(ctx, nh) + hID, err := ni.createNextHop(ctx, nh) if err != nil { log.Warningf("failed to create next hop: %v", err) return ygnmi.Continue @@ -128,7 +128,7 @@ func (ni *Reconciler) StartRoute(ctx context.Context, client *ygnmi.Client) erro _, err = ni.nextHopGroupClient.CreateNextHopGroupMember(ctx, &saipb.CreateNextHopGroupMemberRequest{ Switch: ni.switchID, NextHopGroupId: &group.Oid, - NextHopId: &hopID, + NextHopId: &hID, Weight: proto.Uint32(uint32(route.GetNextHops().Weights[i])), }) if err != nil { diff --git a/dataplane/saiserver/routing.go b/dataplane/saiserver/routing.go index a626daa8..352fc7e5 100644 --- a/dataplane/saiserver/routing.go +++ b/dataplane/saiserver/routing.go @@ -116,7 +116,7 @@ func newNextHopGroup(mgr *attrmgr.AttrMgr, dataplane switchDataplaneAPI, s *grpc return n } -// CreateNextHopGroupMember creates a next hop group. +// CreateNextHopGroup creates a next hop group. func (nhg *nextHopGroup) CreateNextHopGroup(_ context.Context, req *saipb.CreateNextHopGroupRequest) (*saipb.CreateNextHopGroupResponse, error) { id := nhg.mgr.NextID() @@ -130,16 +130,17 @@ func (nhg *nextHopGroup) CreateNextHopGroup(_ context.Context, req *saipb.Create }, nil } -// CreateNextHopGroupMember adds a next hop to a next hop group. -func (nhg *nextHopGroup) CreateNextHopGroupMember(ctx context.Context, req *saipb.CreateNextHopGroupMemberRequest) (*saipb.CreateNextHopGroupMemberResponse, error) { - memberID := nhg.mgr.NextID() - group := nhg.groups[req.GetNextHopGroupId()] +// updateNextHopGroupMember updates the next hop group. +// If m is nil, remove mid from the group(key: nhgid), otherwise add m to group with mid as the key. +func updateNextHopGroupMember(ctx context.Context, nhg *nextHopGroup, nhgid, mid uint64, m *groupMember) (*fwdpb.TableEntryAddReply, error) { + group := nhg.groups[nhgid] if group == nil { - return nil, status.Errorf(codes.FailedPrecondition, "group %v does not exist", req.GetNextHopGroupId()) + return nil, status.Errorf(codes.FailedPrecondition, "group %d does not exist", nhgid) } - group[memberID] = &groupMember{ - nextHop: req.GetNextHopId(), - weight: req.GetWeight(), + if m != nil { + group[mid] = m + } else { + delete(group, mid) } var actLists []*fwdpb.ActionList for _, member := range group { @@ -191,7 +192,7 @@ func (nhg *nextHopGroup) CreateNextHopGroupMember(ctx context.Context, req *saip Entry: &fwdpb.EntryDesc_Exact{ Exact: &fwdpb.ExactEntryDesc{ Fields: []*fwdpb.PacketFieldBytes{{ - Bytes: binary.BigEndian.AppendUint64(nil, req.GetNextHopGroupId()), + Bytes: binary.BigEndian.AppendUint64(nil, nhgid), FieldId: &fwdpb.PacketFieldId{ Field: &fwdpb.PacketField{ FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_NEXT_HOP_GROUP_ID, @@ -204,11 +205,59 @@ func (nhg *nextHopGroup) CreateNextHopGroupMember(ctx context.Context, req *saip Actions: actions, }}, } - if _, err := nhg.dataplane.TableEntryAdd(ctx, entries); err != nil { + return nhg.dataplane.TableEntryAdd(ctx, entries) +} + +// RemoveNextHopGroup removes the next hop group specified in the OID. +func (nhg *nextHopGroup) RemoveNextHopGroup(_ context.Context, req *saipb.RemoveNextHopGroupRequest) (*saipb.RemoveNextHopGroupResponse, error) { + oid := req.GetOid() + if _, ok := nhg.groups[oid]; !ok { + return nil, status.Errorf(codes.FailedPrecondition, "group %d does not exist", oid) + } + // Emit an error if the NHG still has members. + if len(nhg.groups[oid]) != 0 { + return nil, status.Errorf(codes.FailedPrecondition, "cannot remove non-empty group %d", oid) + } + delete(nhg.groups, oid) + return &saipb.RemoveNextHopGroupResponse{}, nil +} + +// CreateNextHopGroupMember adds a next hop to a next hop group. +func (nhg *nextHopGroup) CreateNextHopGroupMember(ctx context.Context, req *saipb.CreateNextHopGroupMemberRequest) (*saipb.CreateNextHopGroupMemberResponse, error) { + nhgid := req.GetNextHopGroupId() + mid := nhg.mgr.NextID() + m := &groupMember{ + nextHop: req.GetNextHopId(), + weight: req.GetWeight(), + } + if _, err := updateNextHopGroupMember(ctx, nhg, nhgid, mid, m); err != nil { return nil, err } + return &saipb.CreateNextHopGroupMemberResponse{Oid: mid}, nil +} - return &saipb.CreateNextHopGroupMemberResponse{Oid: memberID}, nil +// RemoveNextHopGroupMember remove the next hop group member specified in the OID. +// Only need to remove with the desc. +func (nhg *nextHopGroup) RemoveNextHopGroupMember(ctx context.Context, req *saipb.RemoveNextHopGroupMemberRequest) (*saipb.RemoveNextHopGroupMemberResponse, error) { + locateMember := func(oid uint64) (uint64, uint64, error) { + for nhgid, nhg := range nhg.groups { + for mid := range nhg { + if mid == oid { + return nhgid, mid, nil + } + } + } + return 0, 0, fmt.Errorf("cannot find member with id=%d", oid) + } + nhgid, mid, err := locateMember(req.GetOid()) + if err != nil { + return nil, err + } + + if _, err := updateNextHopGroupMember(ctx, nhg, nhgid, mid, nil); err != nil { + return nil, err + } + return &saipb.RemoveNextHopGroupMemberResponse{}, nil } type nextHop struct { diff --git a/dataplane/saiserver/routing_test.go b/dataplane/saiserver/routing_test.go index c43291d5..7b2ff73d 100644 --- a/dataplane/saiserver/routing_test.go +++ b/dataplane/saiserver/routing_test.go @@ -113,6 +113,57 @@ func TestCreateNextHopGroup(t *testing.T) { } } +func TestRemoveNextHopGroup(t *testing.T) { + tests := []struct { + desc string + memberReq *saipb.CreateNextHopGroupMemberRequest + oid uint64 // specify this if you want an arbitrary OID to remove. + wantErr string + }{{ + desc: "success", + }, { + desc: "fail: group not found", + oid: 15, // a non-existing OID. + wantErr: "group 15 does not exist", + }, { + desc: "fail: group not empty", + memberReq: &saipb.CreateNextHopGroupMemberRequest{ + NextHopId: proto.Uint64(1), + Weight: proto.Uint32(100), + }, + wantErr: "cannot remove non-empty group", + }} + nhgReq := &saipb.CreateNextHopGroupRequest{ + Type: saipb.NextHopGroupType_NEXT_HOP_GROUP_TYPE_DYNAMIC_UNORDERED_ECMP.Enum(), + } + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + dplane := &fakeSwitchDataplane{} + c, _, stopFn := newTestNextHopGroup(t, dplane) + defer stopFn() + ctx := context.Background() + resp, err := c.CreateNextHopGroup(ctx, nhgReq) + if err != nil { + t.Fatalf("unexpected err: %s", err) + } + oid := tt.oid + if oid == 0 { + oid = resp.Oid + } + if tt.memberReq != nil { + tt.memberReq.NextHopGroupId = proto.Uint64(oid) + if _, err := c.CreateNextHopGroupMember(ctx, tt.memberReq); err != nil { + t.Fatalf("unexpected err: %s", err) + } + } + _, gotErr := c.RemoveNextHopGroup(context.TODO(), &saipb.RemoveNextHopGroupRequest{Oid: oid}) + if diff := errdiff.Check(gotErr, tt.wantErr); diff != "" { + t.Fatalf("RemoveNextHopGroup() unexpected err: %s", diff) + } + }) + } +} + func TestCreateNextHopGroupMember(t *testing.T) { tests := []struct { desc string @@ -217,6 +268,142 @@ func TestCreateNextHopGroupMember(t *testing.T) { } } +func TestRemoveNextHopGroupMember(t *testing.T) { + tests := []struct { + desc string + mid uint64 + reqIndex int + attrID string + wantAttr *saipb.NextHopGroupMemberAttribute + wantReq *fwdpb.TableEntryAddRequest + wantErr string + }{{ + desc: "success", + mid: 0, + reqIndex: 2, + attrID: "3", + wantReq: &fwdpb.TableEntryAddRequest{ + ContextId: &fwdpb.ContextId{Id: "foo"}, + TableId: &fwdpb.TableId{ObjectId: &fwdpb.ObjectId{Id: NHGTable}}, + Entries: []*fwdpb.TableEntryAddRequest_Entry{{ + EntryDesc: &fwdpb.EntryDesc{ + Entry: &fwdpb.EntryDesc_Exact{ + Exact: &fwdpb.ExactEntryDesc{ + Fields: []*fwdpb.PacketFieldBytes{{ + FieldId: &fwdpb.PacketFieldId{Field: &fwdpb.PacketField{ + FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_NEXT_HOP_GROUP_ID, + }}, + Bytes: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01}, + }}, + }, + }, + }, + Actions: []*fwdpb.ActionDesc{{ + ActionType: fwdpb.ActionType_ACTION_TYPE_SELECT_ACTION_LIST, + Action: &fwdpb.ActionDesc_Select{ + Select: &fwdpb.SelectActionListActionDesc{ + SelectAlgorithm: fwdpb.SelectActionListActionDesc_SELECT_ALGORITHM_CRC32, + FieldIds: []*fwdpb.PacketFieldId{ + {Field: &fwdpb.PacketField{FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_IP_PROTO}}, + {Field: &fwdpb.PacketField{FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_IP_ADDR_SRC}}, + {Field: &fwdpb.PacketField{FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_IP_ADDR_DST}}, + {Field: &fwdpb.PacketField{FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_L4_PORT_SRC}}, + {Field: &fwdpb.PacketField{FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_L4_PORT_DST}}, + }, + ActionLists: []*fwdpb.ActionList{{ + Weight: 66, + Actions: []*fwdpb.ActionDesc{{ + ActionType: fwdpb.ActionType_ACTION_TYPE_UPDATE, + Action: &fwdpb.ActionDesc_Update{ + Update: &fwdpb.UpdateActionDesc{ + FieldId: &fwdpb.PacketFieldId{Field: &fwdpb.PacketField{FieldNum: fwdpb.PacketFieldNum_PACKET_FIELD_NUM_NEXT_HOP_ID}}, + Type: fwdpb.UpdateType_UPDATE_TYPE_SET, + Field: &fwdpb.PacketFieldId{Field: &fwdpb.PacketField{}}, + Value: []byte{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x0c}, + }, + }, + }}, + }}, + }, + }, + }, { + ActionType: fwdpb.ActionType_ACTION_TYPE_LOOKUP, + Action: &fwdpb.ActionDesc_Lookup{ + Lookup: &fwdpb.LookupActionDesc{ + TableId: &fwdpb.TableId{ObjectId: &fwdpb.ObjectId{Id: NHTable}}, + }, + }, + }}, + }}, + }, + wantAttr: &saipb.NextHopGroupMemberAttribute{ + NextHopGroupId: proto.Uint64(1), + NextHopId: proto.Uint64(12), + Weight: proto.Uint32(66), + }, + }, { + desc: "fail: member not found", + mid: 100, + wantErr: "cannot find member with id", + }} + + // Creates two members. + createReqs := []*saipb.CreateNextHopGroupMemberRequest{ + { + NextHopId: proto.Uint64(11), + Weight: proto.Uint32(33), + }, { + NextHopId: proto.Uint64(12), + Weight: proto.Uint32(66), + }, + } + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + dplane := &fakeSwitchDataplane{} + c, mgr, stopFn := newTestNextHopGroup(t, dplane) + ctx := context.Background() + r, err := c.CreateNextHopGroup(ctx, &saipb.CreateNextHopGroupRequest{Type: saipb.NextHopGroupType_NEXT_HOP_GROUP_TYPE_DYNAMIC_UNORDERED_ECMP.Enum()}) + if err != nil { + t.Fatal(err) + } + defer stopFn() + mid := tt.mid // mid is either the mid specified in test case, or the mid of the first added member. + if mid == 0 { + for _, req := range createReqs { + req.NextHopGroupId = &r.Oid + resp, err := c.CreateNextHopGroupMember(ctx, req) + if err != nil { + t.Fatal("unexpected error: %v", err) + } + // Stores the first member ID. + if mid == 0 { + mid = resp.GetOid() + } + } + } + + _, err = c.RemoveNextHopGroupMember(ctx, &saipb.RemoveNextHopGroupMemberRequest{Oid: mid}) + if diff := errdiff.Check(err, tt.wantErr); diff != "" { + t.Fatalf("RemoveNextHopGroupMember() unexpected err: %s", diff) + } + if tt.wantErr != "" { + return + } + + if d := cmp.Diff(dplane.gotEntryAddReqs[tt.reqIndex], tt.wantReq, protocmp.Transform()); d != "" { + t.Errorf("RemoveNextHopGroupMember() failed: diff(-got,+want)\n:%s", d) + } + attr := &saipb.NextHopGroupMemberAttribute{} + if err := mgr.PopulateAllAttributes(tt.attrID, attr); err != nil { + t.Fatal(err) + } + if d := cmp.Diff(attr, tt.wantAttr, protocmp.Transform()); d != "" { + t.Errorf("RemoveNextHopGroupMember() failed: diff(-got,+want)\n:%s", d) + } + }) + } +} + func TestCreateNextHop(t *testing.T) { tests := []struct { desc string