Route priority support for ospf6d
Hi,
this diff adds priority support to ospf6d.
Mostly based on the following ospfd commit:
cvs diff -D "2008-12-11" -D "2008-12-13"
Additionally I removed the RTF_UP from hdr.rtm_flags in send_rtmsg().
Ospfd and bgpd also don't set the flag.
The next steps will be to add support for ospf6ctl fib reload and
to invent rtmsg_process() to reduce duplicate code in fetchtable()
and dispatch_rtmsg().
OK ?
friehm
Index: usr.sbin/ospf6ctl/ospf6ctl.c
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/ospf6ctl.c,v
retrieving revision 1.44
diff -u -p -r1.44 ospf6ctl.c
--- usr.sbin/ospf6ctl/ospf6ctl.c22 Dec 2016 23:03:55 - 1.44
+++ usr.sbin/ospf6ctl/ospf6ctl.c31 May 2017 12:22:20 -
@@ -1256,7 +1256,8 @@ void
show_fib_head(void)
{
printf("flags: * = valid, O = OSPF, C = Connected, S = Static\n");
- printf("%-6s %-20s %-17s\n", "Flags", "Destination", "Nexthop");
+ printf("%-6s %-4s %-20s %-17s\n",
+ "Flags", "Prio", "Destination", "Nexthop");
}
int
@@ -1286,6 +1287,7 @@ show_fib_msg(struct imsg *imsg)
printf(" ");
printf(" ");
+ printf("%4d ", k->priority);
if (asprintf(&p, "%s/%u", log_in6addr(&k->prefix),
k->prefixlen) == -1)
err(1, NULL);
Index: usr.sbin/ospf6d/kroute.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.51
diff -u -p -r1.51 kroute.c
--- usr.sbin/ospf6d/kroute.c30 May 2017 12:42:31 - 1.51
+++ usr.sbin/ospf6d/kroute.c31 May 2017 12:22:20 -
@@ -62,7 +62,8 @@ int kroute_compare(struct kroute_node *,
intkr_change_fib(struct kroute_node *, struct kroute *, int, int);
intkr_delete_fib(struct kroute_node *);
-struct kroute_node *kroute_find(const struct in6_addr *, u_int8_t);
+struct kroute_node *kroute_find(const struct in6_addr *, u_int8_t,
+ u_int8_t);
struct kroute_node *kroute_matchgw(struct kroute_node *,
struct in6_addr *, unsigned int);
int kroute_insert(struct kroute_node *);
@@ -215,6 +216,7 @@ kr_change_fib(struct kroute_node *kr, st
kn->r.nexthop = kroute[i].nexthop;
kn->r.scope = kroute[i].scope;
kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED;
+ kn->r.priority = RTP_OSPF;
kn->r.ext_tag = kroute[i].ext_tag;
rtlabel_unref(kn->r.rtlabel); /* for RTM_CHANGE */
kn->r.rtlabel = kroute[i].rtlabel;
@@ -238,31 +240,10 @@ kr_change(struct kroute *kroute, int krc
kroute->rtlabel = rtlabel_tag2id(kroute->ext_tag);
- kr = kroute_find(&kroute->prefix, kroute->prefixlen);
- if (kr != NULL) {
- if (kr->r.flags & F_KERNEL) {
- /* a non-ospf route already exists. not a problem */
- if (!(kr->r.flags & F_BGPD_INSERTED)) {
- do {
- kr->r.flags |= F_OSPFD_INSERTED;
- kr = kr->next;
- } while (kr);
- return (0);
- }
- /*
-* XXX as long as there is no multipath support in
-* bgpd this is safe else we end up in a bad situation.
-*/
- /*
-* ospf route has higher pref
-* - reset flags to the ospf ones
-* - use RTM_CHANGE
-* - zero out ifindex (this is no longer relevant)
-*/
- action = RTM_CHANGE;
- } else if (kr->next == NULL)/* single path OSPF route */
- action = RTM_CHANGE;
- }
+ kr = kroute_find(&kroute->prefix, kroute->prefixlen, RTP_OSPF);
+ if (kr != NULL && kr->next == NULL && krcount == 1)
+ /* single path OSPF route */
+ action = RTM_CHANGE;
return (kr_change_fib(kr, kroute, krcount, action));
}
@@ -270,14 +251,10 @@ kr_change(struct kroute *kroute, int krc
int
kr_delete_fib(struct kroute_node *kr)
{
- if (!(kr->r.flags & F_OSPFD_INSERTED))
- return 0;
-
- if (kr->r.flags & F_KERNEL) {
- /* remove F_OSPFD_INSERTED flag, route still exists in kernel */
- kr->r.flags &= ~F_OSPFD_INSERTED;
- return (0);
- }
+ if (kr->r.priority != RTP_OSPF)
+ log_warn("kr_delete_fib: %s/%d has wrong priority %d",
+ log_in6addr(&kr->r.prefix), kr->r.prefixlen,
+ kr->r.priority);
if (send_rtmsg(kr_state.fd, RTM_DELETE, &k
multipath / route priority support for ospf6d
Hi,
our QA reports issues with the ospf6d since the kernel uses more multipath
routes.
It exits after certain topology changes with:
rde_send_change_kroute: no valid nexthop found
Since the kernel uses more multipath routes, the lack of multipath support in
ospf6d became a problem.
The attached patch ports the multipath support from ospfd to ospf6d.
It bases on the following ospfd commits:
cvs diff -D "2007-09-24" -D "2007-09-26"
cvs diff -r1.65 -r1.67 kroute.c
A similar patch was suggested by Manues Guesdon a couple of years ago.
This patch doesn't fix all problems I am seeing, but it improves the situation
a lot. A second patch porting priority support will follow and fix further
issues. I decided to split my fixes into two parts to make review easier.
Thanks & Regards
Florian
Index: kroute.c
===
RCS file: /openbsd/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.50
diff -u -p -r1.50 kroute.c
--- kroute.c27 Dec 2016 17:18:56 - 1.50
+++ kroute.c11 May 2017 20:48:49 -
@@ -59,6 +59,8 @@ void kr_redist_remove(struct kroute_node
intkr_redist_eval(struct kroute *, struct kroute *);
void kr_redistribute(struct kroute_node *);
intkroute_compare(struct kroute_node *, struct kroute_node *);
+intkr_change_fib(struct kroute_node *, struct kroute *, int, int);
+intkr_delete_fib(struct kroute_node *);
struct kroute_node *kroute_find(const struct in6_addr *, u_int8_t);
struct kroute_node *kroute_matchgw(struct kroute_node *,
@@ -141,18 +143,105 @@ kr_init(int fs)
}
int
-kr_change(struct kroute *kroute)
+kr_change_fib(struct kroute_node *kr, struct kroute *kroute, int krcount,
+int action)
+{
+ int i;
+ struct kroute_node *kn, *nkn;
+
+ if (action == RTM_ADD) {
+ /*
+* First remove all stale multipath routes.
+* This step must be skipped when the action is RTM_CHANGE
+* because it is already a single path route that will be
+* changed.
+*/
+ for (kn = kr; kn != NULL; kn = nkn) {
+ for (i = 0; i < krcount; i++) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(&kn->r.nexthop,
+ &kroute[i].nexthop))
+ break;
+ }
+ nkn = kn->next;
+ if (i == krcount) {
+ /* stale route */
+ if (kr_delete_fib(kn) == -1)
+ log_warnx("kr_delete_fib failed");
+ /*
+* if head element was removed we need to adjust
+* the head
+*/
+ if (kr == kn)
+ kr = nkn;
+ }
+ }
+ }
+
+ /*
+* now add or change the route
+*/
+ for (i = 0; i < krcount; i++) {
+ /* nexthop ::1 -> ignore silently */
+ if (IN6_IS_ADDR_LOOPBACK(&kroute[i].nexthop))
+ continue;
+
+ if (action == RTM_ADD && kr) {
+ for (kn = kr; kn != NULL; kn = kn->next) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(&kn->r.nexthop,
+ &kroute[i].nexthop))
+ break;
+ }
+
+ if (kn != NULL)
+ /* nexthop already present, skip it */
+ continue;
+ } else
+ /* modify first entry */
+ kn = kr;
+
+ /* send update */
+ if (send_rtmsg(kr_state.fd, action, &kroute[i]) == -1)
+ return (-1);
+
+ /* create new entry unless we are changing the first entry */
+ if (action == RTM_ADD)
+ if ((kn = calloc(1, sizeof(*kn))) == NULL)
+ fatal(NULL);
+
+ kn->r.prefix = kroute[i].prefix;
+ kn->r.prefixlen = kroute[i].prefixlen;
+ kn->r.nexthop = kroute[i].nexthop;
+ kn->r.scope = kroute[i].scope;
+ kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED;
+ kn->r.ext_tag = kroute[i].ext_tag;
+ rtlabel_unref(kn->r.rtlabel);/* for RTM_CHANGE */
+ kn->r.rtlabel = kroute[i].rtlabel;
+
+ if (action == RTM_ADD)
+ if (kroute_insert(kn) == -1) {
+ log_debug("kr_updat
Re: Route priority support for ospf6d
On Wed, May 31, 2017 at 02:29:03PM +0200, Florian Riehm wrote:
> @@ -359,6 +333,7 @@ kr_fib_decouple(void)
> void
> kr_dispatch_msg(int fd, short event, void *bula)
> {
> + /* XXX this is stupid */
> dispatch_rtmsg();
> }
I guess this comment refers to the event_loopexit(NULL) in ospfd code.
So I would not add it here.
> @@ -1377,10 +1376,10 @@ dispatch_rtmsg(void)
> if (rtm->rtm_flags & (RTF_LLINFO|RTF_BROADCAST))
> continue;
>
> -#ifdef RTF_MPATH
> if (rtm->rtm_flags & RTF_MPATH)
> mpath = 1;
> -#endif
> + prio = rtm->rtm_priority;
> +
In ospfd we have at this location
flags = (prio == RTP_OSPF) ?
F_OSPFD_INSERTED : F_KERNEL;
Should we add that here?
At the hackathon consensus was to move forward, so get this in.
OK bluhm@
Re: Route priority support for ospf6d
On Fri, Jun 09, 2017 at 03:28:07PM +0200, Alexander Bluhm wrote:
> On Wed, May 31, 2017 at 02:29:03PM +0200, Florian Riehm wrote:
> > @@ -359,6 +333,7 @@ kr_fib_decouple(void)
> > void
> > kr_dispatch_msg(int fd, short event, void *bula)
> > {
> > + /* XXX this is stupid */
> > dispatch_rtmsg();
> > }
>
> I guess this comment refers to the event_loopexit(NULL) in ospfd code.
> So I would not add it here.
>
This this is from my side, because it is dumb to have a void function that
only does on thing calling another void function.
> > @@ -1377,10 +1376,10 @@ dispatch_rtmsg(void)
> > if (rtm->rtm_flags & (RTF_LLINFO|RTF_BROADCAST))
> > continue;
> >
> > -#ifdef RTF_MPATH
> > if (rtm->rtm_flags & RTF_MPATH)
> > mpath = 1;
> > -#endif
> > + prio = rtm->rtm_priority;
> > +
>
> In ospfd we have at this location
> flags = (prio == RTP_OSPF) ?
> F_OSPFD_INSERTED : F_KERNEL;
> Should we add that here?
Hmmm... this seems to be to detect from where routes are originating.
>
> At the hackathon consensus was to move forward, so get this in.
> OK bluhm@
>
Agreed. OK claudio@
--
:wq Claudio
Re: Route priority support for ospf6d
Thanks,
I commited it to move forward.
On 06/09/17 15:41, Claudio Jeker wrote:
> On Fri, Jun 09, 2017 at 03:28:07PM +0200, Alexander Bluhm wrote:
>> On Wed, May 31, 2017 at 02:29:03PM +0200, Florian Riehm wrote:
>>> @@ -359,6 +333,7 @@ kr_fib_decouple(void)
>>> void
>>> kr_dispatch_msg(int fd, short event, void *bula)
>>> {
>>> + /* XXX this is stupid */
>>> dispatch_rtmsg();
>>> }
>>
>> I guess this comment refers to the event_loopexit(NULL) in ospfd code.
>> So I would not add it here.
>>
>
> This this is from my side, because it is dumb to have a void function that
> only does on thing calling another void function.
I thought it referred to the exit on error approach.
I think it is the right approach (especially for a routing daemon)
but usually our customers tell me "ospf(6)d exited" and there is no
chance for debugging. I am thinking about a solution for this problem.
My idea is do dump 'helpful' data in case of error, but 'helpful'
has to be precised.
>>> @@ -1377,10 +1376,10 @@ dispatch_rtmsg(void)
>>> if (rtm->rtm_flags & (RTF_LLINFO|RTF_BROADCAST))
>>> continue;
>>>
>>> -#ifdef RTF_MPATH
>>> if (rtm->rtm_flags & RTF_MPATH)
>>> mpath = 1;
>>> -#endif
>>> + prio = rtm->rtm_priority;
>>> +
>>
>> In ospfd we have at this location
>> flags = (prio == RTP_OSPF) ?
>> F_OSPFD_INSERTED : F_KERNEL;
>> Should we add that here?
>
> Hmmm... this seems to be to detect from where routes are originating.
Yes, the next change will unify fetchtable and the kroute dispatch handler.
This will fix it.
Re: multipath / route priority support for ospf6d
On 05/12/17 18:07, Florian Riehm wrote:
> Hi,
>
> our QA reports issues with the ospf6d since the kernel uses more multipath
> routes.
> It exits after certain topology changes with:
> rde_send_change_kroute: no valid nexthop found
>
> Since the kernel uses more multipath routes, the lack of multipath support in
> ospf6d became a problem.
>
> The attached patch ports the multipath support from ospfd to ospf6d.
> It bases on the following ospfd commits:
> cvs diff -D "2007-09-24" -D "2007-09-26"
> cvs diff -r1.65 -r1.67 kroute.c
>
> A similar patch was suggested by Manues Guesdon a couple of years ago.
>
> This patch doesn't fix all problems I am seeing, but it improves the situation
> a lot. A second patch porting priority support will follow and fix further
> issues. I decided to split my fixes into two parts to make review easier.
>
Sorry,
Mailclient messed up whitespaces.
Fixed diff below.
friehm
Index: kroute.c
===
RCS file: /openbsd/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.50
diff -u -p -r1.50 kroute.c
--- kroute.c27 Dec 2016 17:18:56 - 1.50
+++ kroute.c11 May 2017 20:48:49 -
@@ -59,6 +59,8 @@ void kr_redist_remove(struct kroute_node
intkr_redist_eval(struct kroute *, struct kroute *);
void kr_redistribute(struct kroute_node *);
intkroute_compare(struct kroute_node *, struct kroute_node *);
+intkr_change_fib(struct kroute_node *, struct kroute *, int, int);
+intkr_delete_fib(struct kroute_node *);
struct kroute_node *kroute_find(const struct in6_addr *, u_int8_t);
struct kroute_node *kroute_matchgw(struct kroute_node *,
@@ -141,18 +143,105 @@ kr_init(int fs)
}
int
-kr_change(struct kroute *kroute)
+kr_change_fib(struct kroute_node *kr, struct kroute *kroute, int krcount,
+int action)
+{
+ int i;
+ struct kroute_node *kn, *nkn;
+
+ if (action == RTM_ADD) {
+ /*
+* First remove all stale multipath routes.
+* This step must be skipped when the action is RTM_CHANGE
+* because it is already a single path route that will be
+* changed.
+*/
+ for (kn = kr; kn != NULL; kn = nkn) {
+ for (i = 0; i < krcount; i++) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(&kn->r.nexthop,
+ &kroute[i].nexthop))
+ break;
+ }
+ nkn = kn->next;
+ if (i == krcount) {
+ /* stale route */
+ if (kr_delete_fib(kn) == -1)
+ log_warnx("kr_delete_fib failed");
+ /*
+* if head element was removed we need to adjust
+* the head
+*/
+ if (kr == kn)
+ kr = nkn;
+ }
+ }
+ }
+
+ /*
+* now add or change the route
+*/
+ for (i = 0; i < krcount; i++) {
+ /* nexthop ::1 -> ignore silently */
+ if (IN6_IS_ADDR_LOOPBACK(&kroute[i].nexthop))
+ continue;
+
+ if (action == RTM_ADD && kr) {
+ for (kn = kr; kn != NULL; kn = kn->next) {
+ if (kn->r.scope == kroute[i].scope &&
+ IN6_ARE_ADDR_EQUAL(&kn->r.nexthop,
+ &kroute[i].nexthop))
+ break;
+ }
+
+ if (kn != NULL)
+ /* nexthop already present, skip it */
+ continue;
+ } else
+ /* modify first entry */
+ kn = kr;
+
+ /* send update */
+ if (send_rtmsg(kr_state.fd, action, &kroute[i]) == -1)
+ return (-1);
+
+ /* create new entry unless we are changing the first entry */
+ if (action == RTM_ADD)
+ if ((kn = calloc(1, sizeof(*kn))) == NULL)
+ fatal(NULL);
+
+ kn->r.prefix = kroute[i].prefix;
+ kn->r.prefixlen = kroute[i].prefixlen;
+ kn->r.nexthop = kroute[i].nexthop;
+ kn->r.scope = kroute[i].scope;
+ kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED;
+ kn->r.ext_tag = kroute[i].ext_tag;
+ rtlabel_unref(kn->r.rtlabel); /* for RTM_CHANGE */
+ kn->r.rtlabel = kroute[i].rtlabel;
+
+ if (acti
Re: multipath / route priority support for ospf6d
On Mon, May 15, 2017 at 08:43:02PM +0200, Florian Riehm wrote:
> > The attached patch ports the multipath support from ospfd to ospf6d.
> @@ -171,48 +260,30 @@ kr_change(struct kroute *kroute)
>* - zero out ifindex (this is no longer relevant)
>*/
> action = RTM_CHANGE;
> - kr->r.flags = kroute->flags | F_OSPFD_INSERTED;
> - kr->r.ifindex = 0;
> - rtlabel_unref(kr->r.rtlabel);
> - kr->r.ext_tag = kroute->ext_tag;
> - kr->r.rtlabel = kroute->rtlabel;
> - }
> + } else if (kr->next == NULL)/* single path OSPF route */
> + action = RTM_CHANGE;
I wonder wether this check should be
} else if (kr->next == NULL && krcount == 1)
At least this is what ospfd is doing.
> RB_FOREACH(kr, kroute_tree, &krt)
> if (!(kr->r.flags & F_KERNEL))
> - send_rtmsg(kr_state.fd, RTM_ADD, &kr->r);
> + for (kn = kr; kn != NULL; kn = kn->next) {
> + send_rtmsg(kr_state.fd, RTM_ADD, &kn->r);
> + }
> - RB_FOREACH(kr, kroute_tree, &krt)
> + RB_FOREACH(kr, kroute_tree, &krt) {
> if (!(kr->r.flags & F_KERNEL))
> - send_rtmsg(kr_state.fd, RTM_DELETE, &kr->r);
> + for (kn = kr; kn != NULL; kn = kn->next) {
> + send_rtmsg(kr_state.fd, RTM_DELETE, &kn->r);
> + }
> + }
The braces { } are used a bit inconsistently here.
Anyway I think this diff should go in as it is.
OK bluhm@
