Mailing List Archive

zebra route table fix
Hi,

Hasso came across the following long lost patch on marc.aimsgroup:

http://marc.theaimsgroup.com/?l=zebra&m=100314781512301&w=4

It appears to still be relevant, touches following files:

M zebra/connected.c
M zebra/kernel_socket.c
M zebra/zserv.c

Gilad, it seems sane, what do you think?

Patch as against quagga CVS is attached.

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
Nearly every complex solution to a programming problem that I
have looked at carefully has turned out to be wrong.
-- Brent Welch
Re: zebra route table fix [ In reply to ]
On Mon, 4 Aug 2003 13:04:26 +0100 (IST)
Paul Jakma <paul@clubi.ie> wrote:

> Hi,
>
> Hasso came across the following long lost patch on marc.aimsgroup:
>
> http://marc.theaimsgroup.com/?l=zebra&m=100314781512301&w=4
>
> It appears to still be relevant, touches following files:
>
> M zebra/connected.c
> M zebra/kernel_socket.c
> M zebra/zserv.c
>
> Gilad, it seems sane, what do you think?
>
> Patch as against quagga CVS is attached.

Uh! Reading the short article I got the impression that this is exactly the fix
for a problem I had for a long time. zebra 0.89 worked well regarding routing
tables, but following versions did not and always put the entries into the main
table.

BTW, this looks like quagga-plugin-linux "glue" code to me ;-)

Regards,
Stephan
Re: zebra route table fix [ In reply to ]
Paul Jakma wrote:

...
> It appears to still be relevant, touches following files:
>
> M zebra/connected.c
> M zebra/kernel_socket.c
> M zebra/zserv.c
>
> Gilad, it seems sane, what do you think?

Few doubts:

- zebra/connected.c: as a matter of fact, the so-called "VRF
infrastructure" lately introduced to zebra seems somewhat inconsistent
at first glance... what I can tell for sure is: (1) connected routes
must be added to the default table/vrf (is it true? that's the way it is
at the Linux kernel, at least wrt forwarding tables); (2) the default
table is always indexed as 0 (see zebra/zebra_rib.c/vrf_init()); from
(1) and (2) it holds that (3) connected routes must be added to table id
0. Now (4) RT_TABLE_MAIN is ambigous -- in the presence of rtnetlink, it
is 254 (the kernel default), otherwise it's set to 0; from (3) and (4)
it holds that this fix is correct only when RT_TABLE_MAIN isn't the
netlink value, otherwise I don't know exactly what's going to happen.
However, (5) if you look at zebra/zebra_rib.c/rib_add_ipv4(), you'll see
that the vrf_id argument isn't used at all, and the table being
retrieved is always the one indexed by zero, that is the default table.
So... my conclusions: (1) vrf_id argument in rib_add_ipv4() is
redundant, and it's only there for uniformness reasons (or future use);
(2) sending RT_TABLE_MAIN instead of 0 is meaningless, although it
appears more "clean" from the abstraction POV; (3) vrf_vector is built
in a strange way, I wonder why the default table isn't indexed by
RT_TABLE_MAIN instead of 0 -- any ideas? I would recommend to keep this
patch on hold until further clarifications are revealed.

- zebra/kernel_socket.c: I don't know the motivation for that patch, and
I don't know the socket kernel interface either -- it appears to make
sense, but bear in mind that zebra didn't deal with multi-table route
reflections / installation in the first place.

- zebra/zserv.c: personally, I object to this patch; I once posted
something similar to the zebra list (about 2 years ago), hoping to
achieve something of multi-context BGP or something. However, this was
proved to be a wrong approach:
zebra/zebra_rib.c/rib_add_ipv4_multipath() in fact adds all routes to
the default table (indexed by 0, see above); on the other, the route is
saved with a non-zero rib->table value; when installed to the kernel,
indeed the stated table value is used, and the expected effect is
achieved. But the question is: why would routes from a single (default)
RIB be installed to several FIBs? If the author (like myself, back then)
expected to be able to manage several /contexts/, i.e. several /RIBs/,
then it isn't the case, because a single RIB is not multi-layered wrt
table ID -- only a single route for each prefix may be "selected", even
though two identically indexed routes may refer to different "tables".
Moreover, a daemon cannot have more than two routes installed at the
same node (i.e. protocol type is unique wrt route node). Hence, I'd say
it wouldn't bring any benefit, and even if it does bring benefit then
it's a very restricted one, and on the other hand it does scatter
inconsistencies and potential hazards.

Note: my general opinion about multiple-contexts in zebra is that (a) it
should go through some abstract model, e.g. the Cisco-derived VRF model,
that is decoupled from the OS forwarding model; (b) it must be written
top down, in the sense that first the abstract layer must be fully
implemented and valid, and only then tie abstract VRF to (possibly) real
forwarding contexts (in this context, I must state that implementing
VRFs in Linux using forwarding tables isn't sufficient by most means, I
would check on Jim Leu's linux-vrf implementation); (c) playing with
table ID's and so must be handled with care. I definitely wouldn't
incorporate a partial patch like this to the repository, unless there's
a specific, agreed issue it is aimed to solve.

(Do I sound too conservative? ;-> )

Gilad
Re: zebra route table fix [ In reply to ]
On Mon, 4 Aug 2003, Gilad Arnold wrote:

>
> Paul Jakma wrote:
>
> ...
> > It appears to still be relevant, touches following files:
> >
> > M zebra/connected.c
> > M zebra/kernel_socket.c
> > M zebra/zserv.c
> >
> > Gilad, it seems sane, what do you think?
>
> Few doubts:
>
> - zebra/connected.c: as a matter of fact, the so-called "VRF
> infrastructure" lately introduced to zebra seems somewhat
> inconsistent at first glance... what I can tell for sure is: (1)
> connected routes must be added to the default table/vrf (is it
> true?

Ok, well on linux the kernel takes care fo this right?

anyway, the patch changes 0 to the correct define, right?

> that's the way it is at the Linux kernel, at least wrt
> forwarding tables); (2) the default table is always indexed as 0
> (see zebra/zebra_rib.c/vrf_init()); from (1) and (2) it holds that
> (3) connected routes must be added to table id 0. Now (4)
> RT_TABLE_MAIN is ambigous -- in the presence of rtnetlink, it is
> 254 (the kernel default), otherwise it's set to 0;

hmm...

> from (3) and (4) it holds that this fix is correct only when
> RT_TABLE_MAIN isn't the netlink value, otherwise I don't know
> exactly what's going to happen.

hmmm..

> However, (5) if you look at zebra/zebra_rib.c/rib_add_ipv4(),
> you'll see that the vrf_id argument isn't used at all, and the
> table being retrieved is always the one indexed by zero, that is
> the default table. So... my conclusions: (1) vrf_id argument in
> rib_add_ipv4() is redundant, and it's only there for uniformness
> reasons (or future use);

future use i guess. the vrf code present is nothing but stub code
really.

> (2) sending RT_TABLE_MAIN instead of 0 is meaningless, although it
> appears more "clean" from the abstraction POV; (3) vrf_vector is
> built in a strange way, I wonder why the default table isn't
> indexed by RT_TABLE_MAIN instead of 0 -- any ideas? I would
> recommend to keep this patch on hold until further clarifications
> are revealed.

agreed.

RT_TABLE_MAIN - is this valid for VRF? Or does this overload this
define? (afaict - it isnt actually clear).

> - zebra/kernel_socket.c: I don't know the motivation for that patch, and
> I don't know the socket kernel interface either -- it appears to make
> sense, but bear in mind that zebra didn't deal with multi-table route
> reflections / installation in the first place.

i have a feeling this is the critical change :). Basically the
problem
AIUI is that current behaviour is:

- zebra reads from the table specified (as well as RTM_MAIN)
- zebra only inserts to main, not to the specified table.

this change fixes the latter i suspect.

> - zebra/zserv.c: personally, I object to this patch; I once posted
> something similar to the zebra list (about 2 years ago), hoping to
> achieve something of multi-context BGP or something. However, this was
> proved to be a wrong approach:
> zebra/zebra_rib.c/rib_add_ipv4_multipath() in fact adds all routes to
> the default table (indexed by 0, see above);

yep.

> on the other, the route is saved with a non-zero rib->table value;
> when installed to the kernel, indeed the stated table value is
> used, and the expected effect is achieved. But the question is: why
> would routes from a single (default) RIB be installed to several
> FIBs?

no idea.

> If the author (like myself, back then) expected to be able to
> manage several /contexts/, i.e. several /RIBs/, then it isn't the
> case, because a single RIB is not multi-layered wrt table ID --
> only a single route for each prefix may be "selected", even though
> two identically indexed routes may refer to different "tables".
> Moreover, a daemon cannot have more than two routes installed at
> the same node (i.e. protocol type is unique wrt route node). Hence,
> I'd say it wouldn't bring any benefit, and even if it does bring
> benefit then it's a very restricted one, and on the other hand it
> does scatter inconsistencies and potential hazards.

hmm... basically, i think table/routing context support is premature
in zebra. (i'm bearing your next paragraph in mind here).

> Note: my general opinion about multiple-contexts in zebra is that
> (a) it should go through some abstract model, e.g. the
> Cisco-derived VRF model, that is decoupled from the OS forwarding
> model; (b) it must be written top down, in the sense that first the
> abstract layer must be fully implemented and valid, and only then
> tie abstract VRF to (possibly) real forwarding contexts (in this
> context, I must state that implementing VRFs in Linux using
> forwarding tables isn't sufficient by most means, I would check on
> Jim Leu's linux-vrf implementation);

Possibly :)

Though, I'd rather wait and see what the linux kernel people
eventually adopt. (cause its impossible to know what they'll do).

> (c) playing with table ID's and so must be handled with care. I
> definitely wouldn't incorporate a partial patch like this to the
> repository, unless there's a specific, agreed issue it is aimed to
> solve.

Yes, i agree with you. To my mind the table command just should not
have gone into zebra. There are too many inconsistencies/issues not
thought about to make it useful really. Should really never have gone
in.

I agree it needs much more though.

I think though we have to deal with the fact that zebra has this
table command. Either it should behave as one would expect wrt route
insertions to kernel - or the table stuff should just all be ripped
out for now.

> (Do I sound too conservative? ;-> )

no :)

> Gilad

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
There is very little future in being right when your boss is wrong.