Fix: Add Route Reconciliation #1749
Conversation
aauren
added
the
override-stale
| @@ -0,0 +1,45 @@ | |||
| package pkg | |||
There was a problem hiding this comment.
@twz123 I wanted to get your feedback about this convention within Go.
As I started to get into the refactoring of InjectRoute, I found that I kept getting into circular dependency type situations between the bgp and routes packages.
After watching a video from Brian Ketelsen about structuring go programs, as well as several other posts, it seems like pulling up your domain models into the root of the project is a conventional way of working around circular dependencies.
Doing this with the below methods, did work out for me, but I noticed that this architecture is limited. For instance, I couldn't leave the TunnelCreator interface with EncapType and EncapPort because those are declared in the tunnels package. And for some reason, it didn't feel right to bring those up into this top level package either.
So I worked around it for now, but maybe this is showing that either I should have brought them up, or that maybe I'm embracing the wrong pattern here.
I would be curious for your thoughts.
| @@ -1,82 +0,0 @@ | |||
| package routes | |||
There was a problem hiding this comment.
@twz123 I ended up bringing this back into the routing package because I realized that it was behaving a bit more like an extension of the NetworkRoutesController, in that, in order to have it process route reconciliation I needed it to work with the routes source of truth, which is BGP. At that point it is no longer strictly related to host routes (although now that I'm thinking about it, it means that maybe host_route_sync.go isn't the right name for the file any more either.
Anyway, curious about your thoughts here as well.
pkg/routes/linux_routes.go
Outdated
| @@ -30,3 +32,109 @@ func DeleteByDestination(destinationSubnet *net.IPNet) error { | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func InjectRoute(subnet *net.IPNet, gw net.IP, ipa utils.NodeIPAware, tn pkg.Tunneler, rs pkg.RouteSyncer, | |||
There was a problem hiding this comment.
@twz123 - Now that this logic is disentangled with the BGP logic that used to be buried in here I like it a lot better. It feels a lot cleaner and more straight forward to me.
pkg/bgp/path.go
Outdated
| "k8s.io/klog/v2" | ||
| ) | ||
|
|
||
| func PathChanged(path *gobgpapi.Path, pl PeerLister, rs pkg.RouteSyncer, tc pkg.TunnelCleaner) error { |
There was a problem hiding this comment.
@twz123 - This feels like the right pattern, because it decouples the routes package from the bgp package by making it implementation agnostic. So I think that making all of the arguments to this function interfaces is the right thing to do, but wanted to see if you had any feedback.
aauren
force-pushed
the
aauren/issue1738
branch
from
c07f1fd
to
482ebec
Compare
| type LinuxRouter struct { | ||
| NodeIPA pkg.NodeIPAware | ||
| Tunneler pkg.Tunneler | ||
| RouteSyncer pkg.RouteSyncer |
There was a problem hiding this comment.
I don't particularly like the circular dependency here. LinuxRouter requires pkg.RouteSyncer so that it can update the state host routes. pkg.RouteSyncer in turn requires LinuxRouter so that it can reuse the InjectRoute() logic when new routes are found during checkCacheAgainstBGP().
It feels like there should be a better / cleaner way to do this.
Add a route reconciliation step to the host_route_sync file that uses the BGP state table as a system of record for the routes that should or should not be synced to the system. In theory, kube-router should be consistently informed of route changes via BGP Path change events that are sent to us from GoBGP. However, in practice it seems that some routes are getting stuck in our state table.
aauren
force-pushed
the
aauren/issue1738
branch
from
5e23294
to
25b6ff4
Compare
Host route syncing is an important part of kube-router that is run in its own goroutine, so we should be checking it for health the way we do the major controllers to make sure that it never stops functioning. Additionally, as the health check controller has been continuously added on to, we also make some modifications here to make it a bit more robust and scalable.
aauren
force-pushed
the
aauren/issue1738
branch
from
25b6ff4
to
c204b26
Compare
This PR is still very early and is not fully implemented, but I wanted to start to get some feedback going on the direction of this before I got too much further down the road.
The end goal of this PR is to enable route reconciliation within the route sync controller. However, in order to do that, I wanted to break out
injectRoute()from the NetworkRoutesController.Also, over the years, it has become obvious that the
injectRoute()function is hard for new users to parse, and there is often a lot of confusion about how it works because the logic is complicated. So this also attempts to split out the logic in inject route into BGP type logic as well as Route type logic.Final fix for: #1738