Allow setting the BGP graceful restart deferral time. See RFC4724 4.1 #753
Conversation
|
I can confirm that this is a problem that has affected us, too. I've tested this PR in our cluster, and it works as expected. IMO, this PR is essential in order to be able to use the RollingUpdate strategy for the daemonset. |
|
@andrewsykim do you mind taking a look at this one? |
pkg/options/options.go
Outdated
| @@ -130,6 +131,8 @@ func (s *KubeRouterConfig) AddFlags(fs *pflag.FlagSet) { | |||
| "Each node in the cluster will setup BGP peering with rest of the nodes.") | |||
| fs.BoolVar(&s.BGPGracefulRestart, "bgp-graceful-restart", false, | |||
| "Enables the BGP Graceful Restart capability so that routes are preserved on unexpected restarts") | |||
| fs.Uint16Var(&s.BGPGracefulRestartDeferralTime, "bgp-graceful-restart-deferral-time", 10, | |||
There was a problem hiding this comment.
This should probably be a DurationVar and we should convert it to uint16 later?
There was a problem hiding this comment.
Or at least mention "in seconds" in the CLI help string
pkg/controllers/routing/bgp_peers.go
Outdated
| @@ -105,9 +105,11 @@ func (nrc *NetworkRoutingController) syncInternalPeers() { | |||
| n.GracefulRestart = config.GracefulRestart{ | |||
| Config: config.GracefulRestartConfig{ | |||
| Enabled: true, | |||
| DeferralTime: nrc.bgpGracefulRestartDeferralTime, | |||
There was a problem hiding this comment.
This is in seconds I'm guessing? Doesn't seem like gobgp documents this either.
|
@Cytrian this change is important to me, and I'm willing to complete the changes that @andrewsykim has requested if you're busy or no longer interested. Can you please let us know whether you'd prefer that someone else pick up the work in your stead? |
|
I'll do it, it's not too difficult. |
|
PR looks good, please squash commits into meaningful messages (the first one looks good) |
| IPTablesSyncPeriod: 5 * time.Minute, | ||
| IpvsGracefulPeriod: 30 * time.Second, | ||
| RoutesSyncPeriod: 5 * time.Minute, | ||
| BGPGracefulRestartDeferralTime: 360 * time.Second, |
There was a problem hiding this comment.
How was this default decided?
There was a problem hiding this comment.
FWIW, it's the gobgp default, so is fully backward compatible with prior kube-router releases.
There was a problem hiding this comment.
ah, good to know thanks!
GoBGP's default value for deferral time is 360 seconds. That means that the routes are not sent to the BGP peer until this timer is elapsed, so a server is unreachable for 360 seconds, when kube-router restarts. The new parameter is --bgp-graceful-restart-deferral-time duration_with_unit For example '--bgp-graceful-restart-deferral-time 10s'
|
Thanks! cc @murali-reddy @jimmy-zh |
…cloudnativelabs#753) GoBGP's default value for deferral time is 360 seconds. That means that the routes are not sent to the BGP peer until this timer is elapsed, so a server is unreachable for 360 seconds, when kube-router restarts. The new parameter is --bgp-graceful-restart-deferral-time duration_with_unit For example '--bgp-graceful-restart-deferral-time 10s'
…cloudnativelabs#753) GoBGP's default value for deferral time is 360 seconds. That means that the routes are not sent to the BGP peer until this timer is elapsed, so a server is unreachable for 360 seconds, when kube-router restarts. The new parameter is --bgp-graceful-restart-deferral-time duration_with_unit For example '--bgp-graceful-restart-deferral-time 10s' (cherry picked from commit 803bd90)
GoBGP default value for deferral time is 360 seconds. That means that the
routes are not sent to the BGP peer until this timer is elapsed.
We painfully noticed this when we restarted kube-router in our cluster. The pod routes were first stale on the routers. After they expired it took some more minutes until they were updated again. The pods and services on the node were not reachable during this time. We also were hit hard by #725, which caused regular kube-router restarts.