Re: bgpd force fib sync in fetchtable
On Tue, Aug 02, 2022 at 12:58:13PM +0100, Stuart Henderson wrote:
> On 2022/08/02 12:34, Claudio Jeker wrote:
> > On startup we load the routing table in bgpd and at that moment a cleanup
> > of old bgpd routes should happen. I noticed this is not the case because
> > fib_sync is not set and so send_rtmsg() just returns.
> > I think we need to force fib_sync in fetchtable() to make sure the cleanup
> > happens correctly.
>
> How much of a problem is it that this clears any existing bgpd routes
> if "fib update no" is set?
>
> I guess in the common case there won't be any anyway, but is there
> any use-case for running two copies of bgpd, one with fib update, one
> without, on the same machine?
One case I see is if you run two bgpd in on two different rtables (not
rdomains). In that case the table used for nexthop lookups will be used by
both daemons. Not sure if this is an issue or not.
The problem with fib_sync being 0 comes from parse.y::parse_config()
if ((rr = add_rib("Adj-RIB-In")) == NULL)
fatal("add_rib failed");
rr->flags = F_RIB_NOFIB | F_RIB_NOEVALUATE;
if ((rr = add_rib("Loc-RIB")) == NULL)
fatal("add_rib failed");
rib_add_fib(rr, conf->default_tableid);
rr->flags = F_RIB_LOCAL;
First the Adj-RIB-In is added then the Loc-RIB. The Adj-RIB-In has
F_RIB_NOFIB | F_RIB_NOEVALUATE and so fib_sync is off when fetchtable is
called in ktable_new().
Afterwards the ktable is updated by Loc-RIB and fib_sync is enabled but
the cleanup did not happen.
So a quick fix is to switch the order in parse.y but heck that probably
breaks later on if used with other tables.
I guess fetchtable() needs to become smarter so that it can be called more
than once. That would also allow to handle RTM_DESYNC in bgpd.
> > OK?
> > --
> > :wq Claudio
> >
> > Index: kroute.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> > retrieving revision 1.285
> > diff -u -p -r1.285 kroute.c
> > --- kroute.c28 Jul 2022 14:05:13 - 1.285
> > +++ kroute.c2 Aug 2022 10:13:59 -
> > @@ -2726,6 +2726,7 @@ fetchtable(struct ktable *kt)
> > char*buf = NULL, *next, *lim;
> > struct rt_msghdr*rtm;
> > struct kroute_full kf;
> > + int fib_sync;
> >
> > mib[0] = CTL_NET;
> > mib[1] = PF_ROUTE;
> > @@ -2754,6 +2755,10 @@ fetchtable(struct ktable *kt)
> > }
> > }
> >
> > + /* force fib_sync on during fetch */
> > + fib_sync = kt->fib_sync;
> > + kt->fib_sync = 1;
> > +
> > lim = buf + len;
> > for (next = buf; next < lim; next += rtm->rtm_msglen) {
> > rtm = (struct rt_msghdr *)next;
> > @@ -2768,6 +2773,8 @@ fetchtable(struct ktable *kt)
> > else
> > kroute_insert(kt, &kf);
> > }
> > + kt->fib_sync = fib_sync;
> > +
> > free(buf);
> > return (0);
> > }
> >
>
--
:wq Claudio
Re: bgpd force fib sync in fetchtable
On 2022/08/02 12:34, Claudio Jeker wrote:
> On startup we load the routing table in bgpd and at that moment a cleanup
> of old bgpd routes should happen. I noticed this is not the case because
> fib_sync is not set and so send_rtmsg() just returns.
> I think we need to force fib_sync in fetchtable() to make sure the cleanup
> happens correctly.
How much of a problem is it that this clears any existing bgpd routes
if "fib update no" is set?
I guess in the common case there won't be any anyway, but is there
any use-case for running two copies of bgpd, one with fib update, one
without, on the same machine?
> OK?
> --
> :wq Claudio
>
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.285
> diff -u -p -r1.285 kroute.c
> --- kroute.c 28 Jul 2022 14:05:13 - 1.285
> +++ kroute.c 2 Aug 2022 10:13:59 -
> @@ -2726,6 +2726,7 @@ fetchtable(struct ktable *kt)
> char*buf = NULL, *next, *lim;
> struct rt_msghdr*rtm;
> struct kroute_full kf;
> + int fib_sync;
>
> mib[0] = CTL_NET;
> mib[1] = PF_ROUTE;
> @@ -2754,6 +2755,10 @@ fetchtable(struct ktable *kt)
> }
> }
>
> + /* force fib_sync on during fetch */
> + fib_sync = kt->fib_sync;
> + kt->fib_sync = 1;
> +
> lim = buf + len;
> for (next = buf; next < lim; next += rtm->rtm_msglen) {
> rtm = (struct rt_msghdr *)next;
> @@ -2768,6 +2773,8 @@ fetchtable(struct ktable *kt)
> else
> kroute_insert(kt, &kf);
> }
> + kt->fib_sync = fib_sync;
> +
> free(buf);
> return (0);
> }
>
Re: bgpd force fib sync in fetchtable
On Tue, Aug 02, 2022 at 01:44:42PM +0200, Theo Buehler wrote:
> On Tue, Aug 02, 2022 at 12:34:40PM +0200, Claudio Jeker wrote:
> > On startup we load the routing table in bgpd and at that moment a cleanup
> > of old bgpd routes should happen. I noticed this is not the case because
> > fib_sync is not set and so send_rtmsg() just returns.
> > I think we need to force fib_sync in fetchtable() to make sure the cleanup
> > happens correctly.
>
> If I understand correctly, this ignores the fs flag of ktable_new() for
> the fetchtable() call. In particular, 'fib-update no' is ignored in this
> situation. Is that intentional?
I think it is. At least I thought that even with 'fib-update no' bgpd
should cleanup the FIB. Now that may be a problem when people run multiple
bgpds with 'fib-update no'.
I see if I can find a solution that is a smaller hammer to fix this.
> >
> > OK?
> > --
> > :wq Claudio
> >
> > Index: kroute.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> > retrieving revision 1.285
> > diff -u -p -r1.285 kroute.c
> > --- kroute.c28 Jul 2022 14:05:13 - 1.285
> > +++ kroute.c2 Aug 2022 10:13:59 -
> > @@ -2726,6 +2726,7 @@ fetchtable(struct ktable *kt)
> > char*buf = NULL, *next, *lim;
> > struct rt_msghdr*rtm;
> > struct kroute_full kf;
> > + int fib_sync;
> >
> > mib[0] = CTL_NET;
> > mib[1] = PF_ROUTE;
> > @@ -2754,6 +2755,10 @@ fetchtable(struct ktable *kt)
> > }
> > }
> >
> > + /* force fib_sync on during fetch */
> > + fib_sync = kt->fib_sync;
> > + kt->fib_sync = 1;
> > +
> > lim = buf + len;
> > for (next = buf; next < lim; next += rtm->rtm_msglen) {
> > rtm = (struct rt_msghdr *)next;
> > @@ -2768,6 +2773,8 @@ fetchtable(struct ktable *kt)
> > else
> > kroute_insert(kt, &kf);
> > }
> > + kt->fib_sync = fib_sync;
> > +
> > free(buf);
> > return (0);
> > }
> >
>
--
:wq Claudio
Re: bgpd force fib sync in fetchtable
On Tue, Aug 02, 2022 at 12:34:40PM +0200, Claudio Jeker wrote:
> On startup we load the routing table in bgpd and at that moment a cleanup
> of old bgpd routes should happen. I noticed this is not the case because
> fib_sync is not set and so send_rtmsg() just returns.
> I think we need to force fib_sync in fetchtable() to make sure the cleanup
> happens correctly.
If I understand correctly, this ignores the fs flag of ktable_new() for
the fetchtable() call. In particular, 'fib-update no' is ignored in this
situation. Is that intentional?
>
> OK?
> --
> :wq Claudio
>
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.285
> diff -u -p -r1.285 kroute.c
> --- kroute.c 28 Jul 2022 14:05:13 - 1.285
> +++ kroute.c 2 Aug 2022 10:13:59 -
> @@ -2726,6 +2726,7 @@ fetchtable(struct ktable *kt)
> char*buf = NULL, *next, *lim;
> struct rt_msghdr*rtm;
> struct kroute_full kf;
> + int fib_sync;
>
> mib[0] = CTL_NET;
> mib[1] = PF_ROUTE;
> @@ -2754,6 +2755,10 @@ fetchtable(struct ktable *kt)
> }
> }
>
> + /* force fib_sync on during fetch */
> + fib_sync = kt->fib_sync;
> + kt->fib_sync = 1;
> +
> lim = buf + len;
> for (next = buf; next < lim; next += rtm->rtm_msglen) {
> rtm = (struct rt_msghdr *)next;
> @@ -2768,6 +2773,8 @@ fetchtable(struct ktable *kt)
> else
> kroute_insert(kt, &kf);
> }
> + kt->fib_sync = fib_sync;
> +
> free(buf);
> return (0);
> }
>
bgpd force fib sync in fetchtable
On startup we load the routing table in bgpd and at that moment a cleanup
of old bgpd routes should happen. I noticed this is not the case because
fib_sync is not set and so send_rtmsg() just returns.
I think we need to force fib_sync in fetchtable() to make sure the cleanup
happens correctly.
OK?
--
:wq Claudio
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.285
diff -u -p -r1.285 kroute.c
--- kroute.c28 Jul 2022 14:05:13 - 1.285
+++ kroute.c2 Aug 2022 10:13:59 -
@@ -2726,6 +2726,7 @@ fetchtable(struct ktable *kt)
char*buf = NULL, *next, *lim;
struct rt_msghdr*rtm;
struct kroute_full kf;
+ int fib_sync;
mib[0] = CTL_NET;
mib[1] = PF_ROUTE;
@@ -2754,6 +2755,10 @@ fetchtable(struct ktable *kt)
}
}
+ /* force fib_sync on during fetch */
+ fib_sync = kt->fib_sync;
+ kt->fib_sync = 1;
+
lim = buf + len;
for (next = buf; next < lim; next += rtm->rtm_msglen) {
rtm = (struct rt_msghdr *)next;
@@ -2768,6 +2773,8 @@ fetchtable(struct ktable *kt)
else
kroute_insert(kt, &kf);
}
+ kt->fib_sync = fib_sync;
+
free(buf);
return (0);
}
