Babel: Clarifications on seqno request handling in bird

[Daniel Gröber]

Hi Juliusz,

On Mon, Feb 27, 2023 at 01:58:21PM +0100, Juliusz Chroboczek wrote:
> > I don't think RFC8966 is really framed in bird's "multi protocol" mindset
> 
> See the beginning of Section 3.7, which describes how a route
> redistributed from another protocol has router-id set to the local
> router's id.  Babel updates for the same prefix are processed as usual,
> with the routes inserted and maintained in Babel's route table but not
> necessarily installed into the kernel's forwarding table.

Right, what was missing from my thinking was the realization that sending
seqno requests for routes originated by us in this way doesn't really make
sense :)

> > so it's unclear to me whether this is something we have to fix or
> > not.
> 
> I'm not familiar with BIRD's internals, so I don't know.  There are two
> possible approaches:
> 
>   - drop all Babel routes for the static prefix ; or
>   - maintain routes as usual, but don't select them.
> 
> The latter is what babeld does (see xroute.c).

I belive my current rt_notify approach also has this behaviour, so that
should be fine then.

> >> A node that has lost all feasible routes to a given destination but still
> >> has unexpired unfeasible routes to that destination MUST send a seqno
> >> request;
> > 
> > I could for example read this as the above mentioned static route
> > constituting a feasible route received from a bird "internal" babel
> > neighbour which would make the behaviour described above perfectly fine,
> > no?
> 
> It is always safe to send a seqno request.  However, unless it is known
> that there is a Babel speaker at that address, it's pointless: a non-Babel
> node will not know how to react to a Babel message.
> 
> However, if there's a static route that's selected, there's not much point
> maintaining the feasibility of the Babel route; just let it become
> unfeasible, and optionally send a request when the static route is
> retracted.

When the static route is retracted we'll only send a seqno request when we
receive an unfeasable update. The problem in the implementation is we don't
know the router-id/seqno for the request anymore at this point.

I think this is fine since we're still sending seqno requests for any
unfeasable updates received while the static route is installed, so sending
another one when that is retracted shouldn't make a difference.

> > Further, looking at the code implementing RFC section 3.8.2.2 in
> > babel_handle_update:
> > 
> >     if (!feasible && (metric != BABEL_INFINITY) &&
> >         (!best || (r == best) || (metric < best->metric)))
> >         /*^ */
> >       babel_generate_seqno_request(p, e, s->router_id, s->seqno + 1, nbr);
> >
> > It's unclear to me what the (!best) condition corresponds to from the RFC
> > text.
> 
> That used to be clear in RFC 6126 (Section 3.8.2.2):
> 
>     Additionally, a node SHOULD send a unicast seqno request whenever it
>     receives an unfeasible update from a currently unselected neighbour
>     that is "good enough", i.e., that would lead to the received route
>     becoming selected were it feasible.
> 
> It became less clear in RFC 8966:
> 
>     Additionally, since metric computation does not necessarily coincide
>     with the delay in propagating updates, a node might receive an
>     unfeasible update from a currently unselected neighbour that is
>     preferable to the currently selected route (e.g., because it has
>     a much smaller metric); in that case, the node SHOULD send a unicast
>     seqno request to the neighbour that advertised the preferable update.
>
> It is definitely a good idea to send a request in this case, especially if
> you ignore unfeasible updates for unknown routes (first clause of Section
> 3.5.3).

That's a bug in the new RFC text then ;)

Actually I also missed another reason for the !best condition: to prevent
(metric < best->metric) from segfaulting.

Thanks,
--Daniel