Skip to content
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

Jump to bottom

feat(store): replace all 4 objects maps with gokv.Store abstraction #177

Merged
merged 7 commits into from
Oct 19, 2023
Merged

feat(store): replace all 4 objects maps with gokv.Store abstraction #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8e7221d
feat(store): use proto marshal instead of json
glimchb Sep 14, 2023
170d28b
feat(store): replace VRFs map with gokv.Store abstraction
glimchb Sep 14, 2023
97a17d1
feat(store): replace SVIs map with gokv.Store abstraction
glimchb Sep 14, 2023
464b0b0
feat(store): replace Bridges map with gokv.Store abstraction
glimchb Sep 18, 2023
4cd7b06
feat(store): replace Ports map with gokv.Store abstraction
glimchb Sep 18, 2023
9eb2859
feat(store): replace List methods with gokv.Store abstraction
glimchb Oct 19, 2023
1769edf
test(store): fix linter errcheck failure
glimchb Oct 19, 2023
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
1 change: 1 addition & 0 deletions cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func main() {

// Create KV store for persistence
options := redis.DefaultOptions
options.Codec = utils.ProtoCodec{}
store, err := redis.NewClient(options)
if err != nil {
log.Panic(err)
Expand Down
65 changes: 54 additions & 11 deletions pkg/evpn/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"fmt"
"log"
"sort"
"strings"

"github.com/google/uuid"
"github.com/opiproject/opi-evpn-bridge/pkg/models"
Expand Down Expand Up @@ -43,7 +44,12 @@
}
in.LogicalBridge.Name = resourceIDToFullName("bridges", resourceID)
// idempotent API when called with same key, should return same object
obj, ok := s.Bridges[in.LogicalBridge.Name]
obj := new(pb.LogicalBridge)
ok, err := s.store.Get(in.LogicalBridge.Name, obj)
if err != nil {
fmt.Printf("Failed to interact with store: %v", err)
return nil, err
}

Check warning on line 52 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L50-L52 

Added lines #L50 - L52 were not covered by tests
if ok {
log.Printf("Already existing LogicalBridge with id %v", in.LogicalBridge.Name)
return obj, nil
Expand All @@ -57,7 +63,11 @@
response.Status = &pb.LogicalBridgeStatus{OperStatus: pb.LBOperStatus_LB_OPER_STATUS_UP}
log.Printf("new object %v", models.NewBridge(response))
// save object to the database
s.Bridges[in.LogicalBridge.Name] = response
s.ListHelper[in.LogicalBridge.Name] = false
err = s.store.Set(in.LogicalBridge.Name, response)
if err != nil {
return nil, err
}

Check warning on line 70 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L69-L70 

Added lines #L69 - L70 were not covered by tests
return response, nil
}

Expand All @@ -68,7 +78,12 @@
return nil, err
}
// fetch object from the database
obj, ok := s.Bridges[in.Name]
obj := new(pb.LogicalBridge)
ok, err := s.store.Get(in.Name, obj)
if err != nil {
fmt.Printf("Failed to interact with store: %v", err)
return nil, err
}

Check warning on line 86 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L84-L86 

Added lines #L84 - L86 were not covered by tests
if !ok {
if in.AllowMissing {
return &emptypb.Empty{}, nil
Expand All @@ -81,7 +96,11 @@
return nil, err
}
// remove from the Database
delete(s.Bridges, obj.Name)
delete(s.ListHelper, obj.Name)
err = s.store.Delete(obj.Name)
if err != nil {
return nil, err
}

Check warning on line 103 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L102-L103 

Added lines #L102 - L103 were not covered by tests
return &emptypb.Empty{}, nil
}

Expand All @@ -92,7 +111,12 @@
return nil, err
}
// fetch object from the database
bridge, ok := s.Bridges[in.LogicalBridge.Name]
bridge := new(pb.LogicalBridge)
ok, err := s.store.Get(in.LogicalBridge.Name, bridge)
if err != nil {
fmt.Printf("Failed to interact with store: %v", err)
return nil, err
}

Check warning on line 119 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L117-L119 

Added lines #L117 - L119 were not covered by tests
if !ok {
// TODO: introduce "in.AllowMissing" field. In case "true", create a new resource, don't return error
err := status.Errorf(codes.NotFound, "unable to find key %s", in.LogicalBridge.Name)
Expand All @@ -115,7 +139,10 @@
}
response := protoClone(in.LogicalBridge)
response.Status = &pb.LogicalBridgeStatus{OperStatus: pb.LBOperStatus_LB_OPER_STATUS_UP}
s.Bridges[in.LogicalBridge.Name] = response
err = s.store.Set(in.LogicalBridge.Name, response)
if err != nil {
return nil, err
}

Check warning on line 145 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L142-L145 

Added lines #L142 - L145 were not covered by tests
return response, nil
}

Expand All @@ -126,7 +153,12 @@
return nil, err
}
// fetch object from the database
bridge, ok := s.Bridges[in.Name]
bridge := new(pb.LogicalBridge)
ok, err := s.store.Get(in.Name, bridge)
if err != nil {
fmt.Printf("Failed to interact with store: %v", err)
return nil, err
}

Check warning on line 161 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L159-L161 

Added lines #L159 - L161 were not covered by tests
if !ok {
err := status.Errorf(codes.NotFound, "unable to find key %s", in.Name)
return nil, err
Expand Down Expand Up @@ -157,10 +189,21 @@
}
// fetch object from the database
Blobarray := []*pb.LogicalBridge{}
for _, bridge := range s.Bridges {
r := protoClone(bridge)
r.Status = &pb.LogicalBridgeStatus{OperStatus: pb.LBOperStatus_LB_OPER_STATUS_UP}
Blobarray = append(Blobarray, r)
for key := range s.ListHelper {
if !strings.HasPrefix(key, "//network.opiproject.org/bridges") {
continue

Check warning on line 194 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L194 

Added line #L194 was not covered by tests
}
bridge := new(pb.LogicalBridge)
ok, err := s.store.Get(key, bridge)
if err != nil {
fmt.Printf("Failed to interact with store: %v", err)
return nil, err
}

Check warning on line 201 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L199-L201 

Added lines #L199 - L201 were not covered by tests
if !ok {
err := status.Errorf(codes.NotFound, "unable to find key %s", key)
return nil, err
}

Check warning on line 205 in pkg/evpn/bridge.go

View check run for this annotation

Codecov / codecov/patch

pkg/evpn/bridge.go#L203-L205 

Added lines #L203 - L205 were not covered by tests
Blobarray = append(Blobarray, bridge)
}
// sort is needed, since MAP is unsorted in golang, and we might get different results
sortLogicalBridges(Blobarray)
Expand Down
22 changes: 12 additions & 10 deletions pkg/evpn/bridge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
pb "github.com/opiproject/opi-api/network/evpn-gw/v1alpha1/gen/go"
pc "github.com/opiproject/opi-api/network/opinetcommon/v1alpha1/gen/go"

"github.com/opiproject/opi-evpn-bridge/pkg/utils"
"github.com/opiproject/opi-evpn-bridge/pkg/utils/mocks"
)

Expand Down Expand Up @@ -258,7 +259,7 @@ func Test_CreateLogicalBridge(t *testing.T) {
ctx := context.Background()
mockNetlink := mocks.NewNetlink(t)
mockFrr := mocks.NewFrr(t)
store := gomap.NewStore(gomap.DefaultOptions)
store := gomap.NewStore(gomap.Options{Codec: utils.ProtoCodec{}})
opi := NewServerWithArgs(mockNetlink, mockFrr, store)
conn, err := grpc.DialContext(ctx,
"",
Expand All @@ -276,7 +277,7 @@ func Test_CreateLogicalBridge(t *testing.T) {
client := pb.NewLogicalBridgeServiceClient(conn)

if tt.exist {
opi.Bridges[testLogicalBridgeName] = protoClone(&testLogicalBridgeWithStatus)
_ = opi.store.Set(testLogicalBridgeName, &testLogicalBridgeWithStatus)
}
if tt.out != nil {
tt.out = protoClone(tt.out)
Expand Down Expand Up @@ -427,7 +428,7 @@ func Test_DeleteLogicalBridge(t *testing.T) {
ctx := context.Background()
mockNetlink := mocks.NewNetlink(t)
mockFrr := mocks.NewFrr(t)
store := gomap.NewStore(gomap.DefaultOptions)
store := gomap.NewStore(gomap.Options{Codec: utils.ProtoCodec{}})
opi := NewServerWithArgs(mockNetlink, mockFrr, store)
conn, err := grpc.DialContext(ctx,
"",
Expand All @@ -445,7 +446,7 @@ func Test_DeleteLogicalBridge(t *testing.T) {
client := pb.NewLogicalBridgeServiceClient(conn)

fname1 := resourceIDToFullName("bridges", tt.in)
opi.Bridges[testLogicalBridgeName] = protoClone(&testLogicalBridgeWithStatus)
_ = opi.store.Set(testLogicalBridgeName, &testLogicalBridgeWithStatus)

if tt.on != nil {
tt.on(mockNetlink, mockFrr, tt.errMsg)
Expand Down Expand Up @@ -519,7 +520,7 @@ func Test_UpdateLogicalBridge(t *testing.T) {
ctx := context.Background()
mockNetlink := mocks.NewNetlink(t)
mockFrr := mocks.NewFrr(t)
store := gomap.NewStore(gomap.DefaultOptions)
store := gomap.NewStore(gomap.Options{Codec: utils.ProtoCodec{}})
opi := NewServerWithArgs(mockNetlink, mockFrr, store)
conn, err := grpc.DialContext(ctx,
"",
Expand All @@ -537,7 +538,7 @@ func Test_UpdateLogicalBridge(t *testing.T) {
client := pb.NewLogicalBridgeServiceClient(conn)

if tt.exist {
opi.Bridges[testLogicalBridgeName] = protoClone(&testLogicalBridgeWithStatus)
_ = opi.store.Set(testLogicalBridgeName, &testLogicalBridgeWithStatus)
}
if tt.out != nil {
tt.out = protoClone(tt.out)
Expand Down Expand Up @@ -601,7 +602,7 @@ func Test_GetLogicalBridge(t *testing.T) {
ctx := context.Background()
mockNetlink := mocks.NewNetlink(t)
mockFrr := mocks.NewFrr(t)
store := gomap.NewStore(gomap.DefaultOptions)
store := gomap.NewStore(gomap.Options{Codec: utils.ProtoCodec{}})
opi := NewServerWithArgs(mockNetlink, mockFrr, store)
conn, err := grpc.DialContext(ctx,
"",
Expand All @@ -618,7 +619,7 @@ func Test_GetLogicalBridge(t *testing.T) {
}(conn)
client := pb.NewLogicalBridgeServiceClient(conn)

opi.Bridges[testLogicalBridgeName] = protoClone(&testLogicalBridgeWithStatus)
_ = opi.store.Set(testLogicalBridgeName, &testLogicalBridgeWithStatus)

request := &pb.GetLogicalBridgeRequest{Name: tt.in}
response, err := client.GetLogicalBridge(ctx, request)
Expand Down Expand Up @@ -706,7 +707,7 @@ func Test_ListLogicalBridges(t *testing.T) {
ctx := context.Background()
mockNetlink := mocks.NewNetlink(t)
mockFrr := mocks.NewFrr(t)
store := gomap.NewStore(gomap.DefaultOptions)
store := gomap.NewStore(gomap.Options{Codec: utils.ProtoCodec{}})
opi := NewServerWithArgs(mockNetlink, mockFrr, store)
conn, err := grpc.DialContext(ctx,
"",
Expand All @@ -723,7 +724,8 @@ func Test_ListLogicalBridges(t *testing.T) {
}(conn)
client := pb.NewLogicalBridgeServiceClient(conn)

opi.Bridges[testLogicalBridgeName] = protoClone(&testLogicalBridgeWithStatus)
_ = opi.store.Set(testLogicalBridgeName, &testLogicalBridgeWithStatus)
opi.ListHelper[testLogicalBridgeName] = false
opi.Pagination["existing-pagination-token"] = 1

request := &pb.ListLogicalBridgesRequest{PageSize: tt.size, PageToken: tt.token}
Expand Down
10 changes: 2 additions & 8 deletions pkg/evpn/evpn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,8 @@ type Server struct {
pb.UnimplementedSviServiceServer
pb.UnimplementedLogicalBridgeServiceServer
pb.UnimplementedBridgePortServiceServer
Bridges map[string]*pb.LogicalBridge
Ports map[string]*pb.BridgePort
Svis map[string]*pb.Svi
Vrfs map[string]*pb.Vrf
Pagination map[string]int
ListHelper map[string]bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glimchb since ListHelper is used as a set, it is more optimal to use struct{} as a value to save up 1 byte per entry (empty struct occupies 0 bytes).

nLink utils.Netlink
frr utils.Frr
tracer trace.Tracer
Expand All @@ -65,10 +62,7 @@ func NewServerWithArgs(nLink utils.Netlink, frr utils.Frr, store gokv.Store) *Se
log.Panic("nil for Store is not allowed")
}
return &Server{
Bridges: make(map[string]*pb.LogicalBridge),
Ports: make(map[string]*pb.BridgePort),
Svis: make(map[string]*pb.Svi),
Vrfs: make(map[string]*pb.Vrf),
ListHelper: make(map[string]bool),
Pagination: make(map[string]int),
nLink: nLink,
frr: frr,
Expand Down
Loading
Loading