From ccfbeca4a0b79007295fd362ac7eb05750d375e4 Mon Sep 17 00:00:00 2001 From: Boris Glimcher Date: Thu, 31 Aug 2023 23:24:50 +0300 Subject: [PATCH] fix(bridge): avoid LinkByName netlink calls Main reason is perfoamnce. If we have the relevant info in the cache/DB, we could just use it instead of netlink get calls. Fixes #123 Signed-off-by: Boris Glimcher --- pkg/evpn/bridge.go | 7 +------ pkg/evpn/bridge_test.go | 17 +---------------- pkg/evpn/svi.go | 7 +------ pkg/evpn/svi_test.go | 15 +-------------- pkg/evpn/vrf.go | 7 +------ 5 files changed, 5 insertions(+), 48 deletions(-) diff --git a/pkg/evpn/bridge.go b/pkg/evpn/bridge.go index 17571376..9bf5be3c 100644 --- a/pkg/evpn/bridge.go +++ b/pkg/evpn/bridge.go @@ -60,12 +60,6 @@ func (s *Server) CreateLogicalBridge(_ context.Context, in *pb.CreateLogicalBrid } // create vxlan only if VNI is not empty if in.LogicalBridge.Spec.Vni != nil { - bridge, err := s.nLink.LinkByName(tenantbridgeName) - if err != nil { - err := status.Errorf(codes.NotFound, "unable to find key %s", tenantbridgeName) - log.Printf("error: %v", err) - return nil, err - } // Example: ip link add vxlan- type vxlan id local dstport 4789 nolearning proxy myip := make(net.IP, 4) // TODO: remove hard-coded 167772162 == "10.0.0.2" @@ -79,6 +73,7 @@ func (s *Server) CreateLogicalBridge(_ context.Context, in *pb.CreateLogicalBrid return nil, err } // Example: ip link set vxlan- master br-tenant addrgenmode none + bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} if err := s.nLink.LinkSetMaster(vxlan, bridge); err != nil { fmt.Printf("Failed to add Vxlan to bridge: %v", err) return nil, err diff --git a/pkg/evpn/bridge_test.go b/pkg/evpn/bridge_test.go index 3f2f2de1..4c42806e 100644 --- a/pkg/evpn/bridge_test.go +++ b/pkg/evpn/bridge_test.go @@ -117,14 +117,6 @@ func Test_CreateLogicalBridge(t *testing.T) { "", true, }, - "failed LinkByName call": { - testLogicalBridgeID, - &testLogicalBridge, - nil, - codes.NotFound, - "unable to find key br-tenant", - false, - }, "failed LinkAdd call": { testLogicalBridgeID, &testLogicalBridge, @@ -189,16 +181,12 @@ func Test_CreateLogicalBridge(t *testing.T) { } // TODO: refactor this mocking - if strings.Contains(name, "failed LinkByName") { - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(nil, errors.New(tt.errMsg)).Once() - } else if strings.Contains(name, "failed LinkAdd") { + if strings.Contains(name, "failed LinkAdd") { // myip := net.ParseIP("10.0.0.2") myip := make(net.IP, 4) binary.BigEndian.PutUint32(myip, 167772162) vxlanName := fmt.Sprintf("vni%d", *testLogicalBridge.Spec.Vni) vxlan := &netlink.Vxlan{LinkAttrs: netlink.LinkAttrs{Name: vxlanName}, VxlanId: int(*testLogicalBridge.Spec.Vni), Port: 4789, Learning: false, SrcAddr: myip} - bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(bridge, nil).Once() mockNetlink.EXPECT().LinkAdd(vxlan).Return(errors.New(tt.errMsg)).Once() } else if strings.Contains(name, "failed LinkSetMaster") { myip := make(net.IP, 4) @@ -206,7 +194,6 @@ func Test_CreateLogicalBridge(t *testing.T) { vxlanName := fmt.Sprintf("vni%d", *testLogicalBridge.Spec.Vni) vxlan := &netlink.Vxlan{LinkAttrs: netlink.LinkAttrs{Name: vxlanName}, VxlanId: int(*testLogicalBridge.Spec.Vni), Port: 4789, Learning: false, SrcAddr: myip} bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(bridge, nil).Once() mockNetlink.EXPECT().LinkAdd(vxlan).Return(nil).Once() mockNetlink.EXPECT().LinkSetMaster(vxlan, bridge).Return(errors.New(tt.errMsg)).Once() } else if strings.Contains(name, "failed LinkSetUp") { @@ -215,7 +202,6 @@ func Test_CreateLogicalBridge(t *testing.T) { vxlanName := fmt.Sprintf("vni%d", *testLogicalBridge.Spec.Vni) vxlan := &netlink.Vxlan{LinkAttrs: netlink.LinkAttrs{Name: vxlanName}, VxlanId: int(*testLogicalBridge.Spec.Vni), Port: 4789, Learning: false, SrcAddr: myip} bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(bridge, nil).Once() mockNetlink.EXPECT().LinkAdd(vxlan).Return(nil).Once() mockNetlink.EXPECT().LinkSetMaster(vxlan, bridge).Return(nil).Once() mockNetlink.EXPECT().LinkSetUp(vxlan).Return(errors.New(tt.errMsg)).Once() @@ -225,7 +211,6 @@ func Test_CreateLogicalBridge(t *testing.T) { vxlanName := fmt.Sprintf("vni%d", *testLogicalBridge.Spec.Vni) vxlan := &netlink.Vxlan{LinkAttrs: netlink.LinkAttrs{Name: vxlanName}, VxlanId: int(*testLogicalBridge.Spec.Vni), Port: 4789, Learning: false, SrcAddr: myip} bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(bridge, nil).Once() mockNetlink.EXPECT().LinkAdd(vxlan).Return(nil).Once() mockNetlink.EXPECT().LinkSetMaster(vxlan, bridge).Return(nil).Once() mockNetlink.EXPECT().LinkSetUp(vxlan).Return(nil).Once() diff --git a/pkg/evpn/svi.go b/pkg/evpn/svi.go index 8b97c335..272bd575 100644 --- a/pkg/evpn/svi.go +++ b/pkg/evpn/svi.go @@ -78,12 +78,7 @@ func (s *Server) CreateSvi(_ context.Context, in *pb.CreateSviRequest) (*pb.Svi, return nil, err } // not found, so create a new one - bridge, err := s.nLink.LinkByName(tenantbridgeName) - if err != nil { - err := status.Errorf(codes.NotFound, "unable to find key %s", tenantbridgeName) - log.Printf("error: %v", err) - return nil, err - } + bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} vid := uint16(bridgeObject.Spec.VlanId) // Example: bridge vlan add dev br-tenant vid self if err := s.nLink.BridgeVlanAdd(bridge, vid, false, false, true, false); err != nil { diff --git a/pkg/evpn/svi_test.go b/pkg/evpn/svi_test.go index 5b82839f..147e111d 100644 --- a/pkg/evpn/svi_test.go +++ b/pkg/evpn/svi_test.go @@ -186,14 +186,6 @@ func Test_CreateSvi(t *testing.T) { fmt.Sprintf("unable to find key %v", "unknown-vrf-id"), false, }, - "failed LinkByName call": { - testSviID, - &testSvi, - nil, - codes.NotFound, - "unable to find key br-tenant", - false, - }, "failed BridgeVlanAdd call": { testSviID, &testSvi, @@ -252,17 +244,13 @@ func Test_CreateSvi(t *testing.T) { opi.Bridges[testLogicalBridgeName] = &testLogicalBridge // TODO: refactor this mocking - if strings.Contains(name, "failed LinkByName") { - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(nil, errors.New(tt.errMsg)).Once() - } else if strings.Contains(name, "failed BridgeVlanAdd") { + if strings.Contains(name, "failed BridgeVlanAdd") { vid := uint16(testLogicalBridge.Spec.VlanId) bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(bridge, nil).Once() mockNetlink.EXPECT().BridgeVlanAdd(bridge, vid, false, false, true, false).Return(errors.New(tt.errMsg)).Once() } else if strings.Contains(name, "failed LinkAdd") { vid := uint16(testLogicalBridge.Spec.VlanId) bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(bridge, nil).Once() mockNetlink.EXPECT().BridgeVlanAdd(bridge, vid, false, false, true, false).Return(nil).Once() vlanName := fmt.Sprintf("vlan%d", vid) vlandev := &netlink.Vlan{LinkAttrs: netlink.LinkAttrs{Name: vlanName, ParentIndex: bridge.Attrs().Index}, VlanId: int(vid)} @@ -270,7 +258,6 @@ func Test_CreateSvi(t *testing.T) { } else if strings.Contains(name, "failed LinkSetHardwareAddr") { vid := uint16(testLogicalBridge.Spec.VlanId) bridge := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: tenantbridgeName}} - mockNetlink.EXPECT().LinkByName(tenantbridgeName).Return(bridge, nil).Once() mockNetlink.EXPECT().BridgeVlanAdd(bridge, vid, false, false, true, false).Return(nil).Once() vlanName := fmt.Sprintf("vlan%d", vid) vlandev := &netlink.Vlan{LinkAttrs: netlink.LinkAttrs{Name: vlanName, ParentIndex: bridge.Attrs().Index}, VlanId: int(vid)} diff --git a/pkg/evpn/vrf.go b/pkg/evpn/vrf.go index 47fbfc5b..ffa76079 100644 --- a/pkg/evpn/vrf.go +++ b/pkg/evpn/vrf.go @@ -193,13 +193,8 @@ func (s *Server) DeleteVrf(_ context.Context, in *pb.DeleteVrfRequest) (*emptypb } // use netlink to find BRIDGE device bridgeName := fmt.Sprintf("br%d", *obj.Spec.Vni) - bridgedev, err := s.nLink.LinkByName(bridgeName) + bridgedev := &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: bridgeName}} log.Printf("Deleting BRIDGE %v", bridgedev) - if err != nil { - err := status.Errorf(codes.NotFound, "unable to find key %s", bridgeName) - log.Printf("error: %v", err) - return nil, err - } // bring link down if err := s.nLink.LinkSetDown(bridgedev); err != nil { fmt.Printf("Failed to up link: %v", err)