Hi Toke, On Tue, Feb 28, 2023 at 12:20:22PM +0100, Toke Høiland-Jørgensen wrote:
I've thought about this some more, I think we absolutely shouldn't expose the smooted metric to filters. It's an implementation detail. There's a bunch of other valid ways to implement this sort of dampening/hysteresis no reason to expose it to filter users really.
How would you even use this for any practical filtering anyway? Reject routes based on `smoothed_metric - metric > threshold` or even better, increase metric based on smoothed_metric difference (oscillations galore haha). Hardly seems useful to me :)
AFAICT it's perfectly fine to have EAs that filters can't access though, that's the situation we're currently in for the router-id after all so that's not really a blocker.
My thinking was that filters may want to do something like:
if (metric == smoothed_metric) metric += 100; /* route is stable, we can apply our policy */
but I honestly don't know if that's useful for anything in reality :)
Hmm, I suppose that is a valid use-case. I'd rather expose a boolean flag for that rather than the smoothed metric itself.
I've been playing with rebasing your smoothing patch on mine now and I see a couple of options. My favorite so far is strategically replacing relevant babel_announce_rte calls with this:
static void babel_sched_announce_rte(struct babel_proto *p, struct babel_route *r) { struct babel_config *cf = (void *) p->p.cf;
if (!cf->metric_decay || !e->selected || e->selected->metric < r->metric) /* When best route's metric is less than the one we're about to announce * it's safe to do so immediately even if metric smoothing is on. Note * e->selected takes export filters into account. */ babel_announce_rte(p, r->e, r); else e->scheduled = r; }
The clever bit is that announcing a route that's no better than the selected one will have no impact on route selection. Same applies if another protocol's route is active (i.e. when e->selected is NULL).
Why is it better to do this pre-filtering instead of just having the smoothed metric be part of the check in babel_rte_better()?
I think you're looking at this from the wrong angle. By not having a smoothed metric attribute we have to update to begin with we save on all this timer logic that would constantly re-announce routes to core. This way the number of core announcements stays exactly the same as before and we don't really loose anything.
Only think I'm still missing is a way to reframe the metric smoothing math such that I can install a timer for when the next scheduled route update should go through, though perhaps polling in the global timer hook is fine?
I think it's probably simpler to just re-announce any route that's still converging every time we go through the routing table.
Simpler, yes, but I do want to be able to maintain internet scale routing tables through babel eventually so slashing every little bit helps :)
Bear in mind that the currently selected route can also be converging, so predicting when two routes "cross" gets complicated quickly. Simpler to just do a periodic update and redo the comparison every time this update happens.
I feel like that's an artifact specific to the "metric smoothing" approach to route dampening not a feature though. The way I see it the behaviour we really want is to delay any change in selected route for a time related to the metric difference. Think back to what the purpose of the metric smoothing is in the first place: to limit oscillations of the selected route, which this will do just as well.
Another question is if the smoothing should incorporate any changes to the metric that the filters do? It probably should, right?
I don't think that'll give us very useful behaviour TBH. Perhaps the way to look at it is that we're not trying to smooth the (administrative) metric but rather only the link cost component which is the bit that is actually fluctuating? Problem is we only have these as seperate numbers for our local metrics/costs not the ones from other nodes.
Maybe we need a protocol extension to seperate these two concepts, hmm. I have been thinking it would be nice to have seperate rtt and administrative metric numbers just for network debugging, perhaps it would make sense for general operation too.
I'm not sure this is a good idea from a protocol perspective. If you want a global view of the network, you should really be running a link-state protocol ;)
I don't agree with that. It's not as if I want per-hop information. Just a sum of RTTs along the path and a sum of administrative metric along the path rather than have those jumbled together into one number. Since babel is quite flexible in the actual metric math that would allow interesting ways of weighing each metric component rather than just having everything be linear. For debugging this would be useful as you can see that this path in front of you actually has a crazy RTT rather than someone just having fiddled with their rxcost.
Also see above for how "lovely" all the necessary lookups are going to be if we go down this path, haha.
Whoops, that was from an earlier draft. This was about what it would look like if we update the smoothed metric in rte_better. Something like this: struct babel_proto *p_new = (struct babel_proto*)new->src->proto, *p_old = (struct babel_proto*)old->src->proto; struct babel_iface *ifa_new, *ifa_old; struct babel_neighbor *nbr_new, *nbr_old; struct babel_entry *ent_new, *ent_old; struct babel_route *route_new, *route_old; ifa_new = babel_find_iface(p_new, new->attrs->nh.iface); ifa_old = babel_find_iface(p_old, old->attrs->nh.iface); nbr_new = babel_find_neighbor(ifa_new, new->attrs->from); nbr_old = babel_find_neighbor(ifa_old, old->attrs->from); ent_new = babel_find_entry(p_new, new->net->n.addr); ent_old = babel_find_entry(p_old, old->net->n.addr); route_new = babel_find_route(ent_new, nbr_new); route_old = babel_find_route(ent_old, nbr_old); babel_update_smoothed_metric(p_new, route_new); babel_update_smoothed_metric(p_old, route_old); yikes. Don't want to go down that road, got enough of these lookups in rt_notify already :) --Daniel