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@