Refactor routemanager

For a number of reasons:

1. Performance

With BGP the number of routes that can be installed on the system scales
up. Tests with > 1500 routes causes trouble. The current sync
implementation has exponential loops. On each iteration,
RouteListFiltered is called which dumps all the routes on the system and
then filters client side. netlink API is by value, and this causes high
GC crunch.

Changed sync implementation to have sequential loops and a single list
call to netlink. Changed to avoid listing routes in any other case.

sinc is still a heavy operation that keeps a lock and migh require
further optimizations. This is a first take.

2. Consistency

The used RoutePartiallyEqual had a different criteria than what was used
to actually delete a route from netlink, leaving the door open for
inconsistencies. This could be thought of as a compromise due to the fact
that routes can assume different defaults when installed than those
provided. Proposal is to do a bit better and ignore those defaults when
comparing against kernel routes as a unified criteria. Should fulfill our
use cases but might need further tweaking in the future. Potential
problem is that kernel switches bad an incorrect value to default
instead of throwing us an error which can cause a route to constantly
sync. Much of this is undocumented as far as I could tell but anyway has
not been observed with the specific routes that we install.

Also, the current implementation was validating and indexing by link index
which is actually not a required field of a route. Instead, key routes by
priority, prefix and table. Route manager assumes ownership of all the
routes with any of the provided keys and will delete unrecognized routes
on sync. This is inline to what our previous netlink delete was doing.

3. Other

* Reset ticker on each sync iteration to avoid backlogging.
* Cleaned up excessive logging

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Author: jcaamano

go-controller/pkg/node/controllers/egressip/egressip.go

@@ -1332,7 +1332,7 @@ func routeDifference(routesA, routesB []netlink.Route) []netlink.Route {
 	for _, routeA := range routesA {
 		found = false
 		for _, routeB := range routesB {
-			if routemanager.RoutePartiallyEqual(routeA, routeB) {
+			if util.RouteEqual(&routeA, &routeB) {
 				found = true
 				break
 			}

go-controller/pkg/node/controllers/egressip/egressip_test.go

@@ -1699,10 +1699,21 @@ func getNetlinkAddr(ip, netmask string) *netlink.Addr {
 // containsRoutes returns true if routes in routes1 are presents in routes routes2
 func containsRoutes(routes1 []netlink.Route, routes2 []netlink.Route) bool {
 	var found bool
+	eq := func(route1, route2 netlink.Route) bool {
+		// normalize fields that we don't set explicitly and just get set once
+		// the route is installed
+		if route1.Family == netlink.FAMILY_ALL {
+			route1.Family = route2.Family
+		}
+		if route1.Protocol == unix.RTPROT_UNSPEC {
+			route1.Protocol = route2.Protocol
+		}
+		return util.RouteEqual(&route1, &route2)
+	}
 	for _, route1 := range routes1 {
 		found = false
 		for _, route2 := range routes2 {
-			if routemanager.RoutePartiallyEqual(route1, route2) {
+			if eq(route1, route2) {
 				found = true
 				break
 			}

go-controller/pkg/node/gateway_init_linux_test.go

@@ -1756,8 +1756,7 @@ var _ = Describe("Gateway unit tests", func() {
 			netlinkMock.On("LinkByName", lnkAttr.Name).Return(lnk, nil)
 			netlinkMock.On("LinkByIndex", lnkAttr.Index).Return(lnk, nil)
 			netlinkMock.On("LinkSetUp", mock.Anything).Return(nil)
-			netlinkMock.On("RouteListFiltered", mock.Anything, mock.Anything, mock.Anything).Return(nil, nil)
-			netlinkMock.On("RouteAdd", mock.Anything, mock.Anything, mock.Anything).Return(nil)
+			netlinkMock.On("RouteReplace", mock.Anything, mock.Anything, mock.Anything).Return(nil)
 			wg := &sync.WaitGroup{}
 			rm := routemanager.NewController()
 			util.SetNetLinkOpMockInst(netlinkMock)
@@ -1786,29 +1785,20 @@ var _ = Describe("Gateway unit tests", func() {
 				Name:  "ens1f0",
 				Index: 5,
 			}
-			previousRoute := &netlink.Route{
-				Dst:       ipnet,
-				LinkIndex: 5,
-				Scope:     netlink.SCOPE_UNIVERSE,
-				Gw:        gwIPs[0],
-				MTU:       config.Default.MTU - 100,
-				Src:       srcIP,
-			}
-
 			expectedRoute := &netlink.Route{
 				Dst:       ipnet,
 				LinkIndex: 5,
 				Scope:     netlink.SCOPE_UNIVERSE,
 				Gw:        gwIPs[0],
 				MTU:       config.Default.MTU,
 				Src:       srcIP,
+				Table:     syscall.RT_TABLE_MAIN,
 			}
 
 			lnk.On("Attrs").Return(lnkAttr)
 			netlinkMock.On("LinkByName", lnkAttr.Name).Return(lnk, nil)
 			netlinkMock.On("LinkByIndex", lnkAttr.Index).Return(lnk, nil)
 			netlinkMock.On("LinkSetUp", mock.Anything).Return(nil)
-			netlinkMock.On("RouteListFiltered", mock.Anything, mock.Anything, mock.Anything).Return([]netlink.Route{*previousRoute}, nil)
 			netlinkMock.On("RouteReplace", expectedRoute).Return(nil)
 			wg := &sync.WaitGroup{}
 			rm := routemanager.NewController()