> On Fri, Dec 11, 2020 at 5:56 PM Lorenzo Bianconi > <[email protected]> wrote: > > > > Introduce the --bfd option for ovn static routes in order to > > check if the next-hop is properly running using the BFD > > protocol. E.g: > > > > $ovn-nbctl --bfd lr-route-add lr0 10.0.0.0/8 172.16.0.50 > > > > Add BFD static routes tests in system-ovn.at/ovn-nbctl.at. > > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > Hi Lorenzo, > > Please see a few comments below.
Hi Numan, thx for the review. > > Numan > > > --- > > NEWS | 3 ++- > > northd/ovn-northd.c | 41 +++++++++++++++++++++++++++++---------- > > tests/ovn-nbctl.at | 5 ++++- > > tests/system-ovn.at | 5 +++++ > > utilities/ovn-nbctl.8.xml | 6 ++++++ > > utilities/ovn-nbctl.c | 18 +++++++++++------ > > 6 files changed, 60 insertions(+), 18 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index 178766f7a..769e52366 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -24,7 +24,8 @@ OVN v20.12.0 - xx xxx xxxx > > significantly decrease size of a Southbound DB. However, in some > > cases, > > it could have performance penalty for ovn-controller. Disabled by > > default. > > - - BFD protocol support according to RFC880 [0] > > + - BFD protocol support according to RFC880 [0]. Introduce next-hop BFD > > + availability check for OVN static routes. > > [0] https://tools.ietf.org/html/rfc5880) > > > > OVN v20.09.0 - 28 Sep 2020 > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index 21e04cdfe..83a86aaab 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -7628,7 +7628,7 @@ route_hash(struct parsed_route *route) > > /* Parse and validate the route. Return the parsed route if successful. > > * Otherwise return NULL. */ > > static struct parsed_route * > > -parsed_routes_add(struct ovs_list *routes, > > +parsed_routes_add(struct northd_context *ctx, struct ovs_list *routes, > > const struct nbrec_logical_router_static_route *route) > > { > > /* Verify that the next hop is an IP address with an all-ones mask. */ > > @@ -7670,6 +7670,23 @@ parsed_routes_add(struct ovs_list *routes, > > return NULL; > > } > > > > + if (smap_get_bool(&route->options, "bfd", false)) { > > + const struct nbrec_bfd *nb_bt; > > + int online = -1; > > + > > + NBREC_BFD_FOR_EACH (nb_bt, ctx->ovnnb_idl) { > > + if (!strcmp(nb_bt->dst_ip, route->nexthop)) { > > + online = strcmp(nb_bt->status, "down"); > > + if (online) { > > + break; > > + } > > + } > > I think running a BFD_FOR_EACH loop can be avoided. > > I would suggest to add a column - 'bfd' in the Logical_Router_Static_Route > which will weakly refer to the BFD table. > > If a user wants to enable BFD on a static router, the user needs to create a > BFD > row and then associate with the static route. ack, I will fix it in v3. > > In my opinion if a user creates BFD rows but doesn't associate to any > static routes > then ovn-controller should not run the BFD protocol at all. I don't > see a point in that. > > What do you think ? May be ovn-northd can set the state as > "admin_down" in SB DB > if there are no users for it ? I agree, but if we do not have the reference here we will need to perform a full lookup in bfd table, do you think it is ok? Regards, Lorenzo > > Thanks > Numan > > > + } > > + if (!online) { > > + return NULL; > > + } > > + } > > + > > struct parsed_route *pr = xzalloc(sizeof *pr); > > pr->prefix = prefix; > > pr->plen = plen; > > @@ -10225,9 +10242,10 @@ build_ip_routing_flows_for_lrouter_port( > > } > > > > static void > > -build_static_route_flows_for_lrouter( > > - struct ovn_datapath *od, struct hmap *lflows, > > - struct hmap *ports) > > +build_static_route_flows_for_lrouter(struct northd_context *ctx, > > + struct ovn_datapath *od, > > + struct hmap *lflows, > > + struct hmap *ports) > > { > > if (od->nbr) { > > ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING_ECMP, 150, > > @@ -10239,7 +10257,8 @@ build_static_route_flows_for_lrouter( > > struct ecmp_groups_node *group; > > for (int i = 0; i < od->nbr->n_static_routes; i++) { > > struct parsed_route *route = > > - parsed_routes_add(&parsed_routes, > > od->nbr->static_routes[i]); > > + parsed_routes_add(ctx, &parsed_routes, > > + od->nbr->static_routes[i]); > > if (!route) { > > continue; > > } > > @@ -11244,7 +11263,8 @@ struct lswitch_flow_build_info { > > */ > > > > static void > > -build_lswitch_and_lrouter_iterate_by_od(struct ovn_datapath *od, > > +build_lswitch_and_lrouter_iterate_by_od(struct northd_context *ctx, > > + struct ovn_datapath *od, > > struct lswitch_flow_build_info > > *lsi) > > { > > /* Build Logical Switch Flows. */ > > @@ -11260,7 +11280,7 @@ build_lswitch_and_lrouter_iterate_by_od(struct > > ovn_datapath *od, > > build_neigh_learning_flows_for_lrouter(od, lsi->lflows, &lsi->match, > > &lsi->actions); > > build_ND_RA_flows_for_lrouter(od, lsi->lflows); > > - build_static_route_flows_for_lrouter(od, lsi->lflows, lsi->ports); > > + build_static_route_flows_for_lrouter(ctx, od, lsi->lflows, lsi->ports); > > build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match, > > &lsi->actions); > > build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->ports); > > @@ -11303,7 +11323,8 @@ build_lswitch_and_lrouter_iterate_by_op(struct > > ovn_port *op, > > } > > > > static void > > -build_lswitch_and_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > > +build_lswitch_and_lrouter_flows(struct northd_context *ctx, > > + struct hmap *datapaths, struct hmap *ports, > > struct hmap *port_groups, struct hmap > > *lflows, > > struct hmap *mcgroups, > > struct hmap *igmp_groups, > > @@ -11332,7 +11353,7 @@ build_lswitch_and_lrouter_flows(struct hmap > > *datapaths, struct hmap *ports, > > * will move here and will be reogranized by iterator type. > > */ > > HMAP_FOR_EACH (od, key_node, datapaths) { > > - build_lswitch_and_lrouter_iterate_by_od(od, &lsi); > > + build_lswitch_and_lrouter_iterate_by_od(ctx, od, &lsi); > > } > > HMAP_FOR_EACH (op, key_node, ports) { > > build_lswitch_and_lrouter_iterate_by_op(op, &lsi); > > @@ -11433,7 +11454,7 @@ build_lflows(struct northd_context *ctx, struct > > hmap *datapaths, > > { > > struct hmap lflows = HMAP_INITIALIZER(&lflows); > > > > - build_lswitch_and_lrouter_flows(datapaths, ports, > > + build_lswitch_and_lrouter_flows(ctx, datapaths, ports, > > port_groups, &lflows, mcgroups, > > igmp_groups, meter_groups, lbs); > > > > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > > index 01edfcbc1..46366500e 100644 > > --- a/tests/ovn-nbctl.at > > +++ b/tests/ovn-nbctl.at > > @@ -1617,7 +1617,10 @@ IPv6 Routes > > 2001:db8::/64 2001:db8:0:f102::1 dst-ip lp0 > > 2001:db8:1::/64 2001:db8:0:f103::1 dst-ip > > ::/0 2001:db8:0:f101::1 dst-ip > > -])]) > > +]) > > + > > +AT_CHECK([ovn-nbctl --bfd lr-route-add lr0 100.0.0.1/24 192.168.0.1])]) > > + > > > > dnl --------------------------------------------------------------------- > > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index d8c21404b..6bc1988e1 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -5612,6 +5612,9 @@ AT_CHECK([ovn-nbctl list bfd | awk -F: > > '/status/{print substr($2,2)}'], [0], [dn > > up > > ]) > > > > +ovn-nbctl --bfd lr-route-add R1 100.0.0.0/8 172.16.1.50 > > +OVS_WAIT_UNTIL([ovn-sbctl dump-flows R1 | grep 'match=(ip4.dst == > > 100.0.0.0/8)' | grep -q 172.16.1.50]) > > + > > NS_CHECK_EXEC([server], [bfdd-control stop], [0], [dnl > > stopping > > ]) > > @@ -5621,6 +5624,8 @@ AT_CHECK([ovn-nbctl list bfd | awk -F: > > '/status/{print substr($2,2)}'], [0], [dn > > down > > ]) > > > > +OVS_WAIT_UNTIL([test "$(ovn-sbctl dump-flows R1 | grep 'match=(ip4.dst == > > 100.0.0.0/8)' | grep 172.16.1.50)" = ""]) > > + > > kill $(pidof ovn-controller) > > > > as ovn-sb > > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > > index e5a35f307..3410c6137 100644 > > --- a/utilities/ovn-nbctl.8.xml > > +++ b/utilities/ovn-nbctl.8.xml > > @@ -659,6 +659,7 @@ > > <dl> > > <dt>[<code>--may-exist</code>] > > [<code>--policy</code>=<var>POLICY</var>] > > [<code>--ecmp</code>] [<code>--ecmp-symmetric-reply</code>] > > + [<code>--bfd</code>] > > <code>lr-route-add</code> <var>router</var> > > <var>prefix</var> <var>nexthop</var> [<var>port</var>]</dt> > > <dd> > > @@ -695,6 +696,11 @@ > > it is not necessary to set both. > > </p> > > > > + <p> > > + The <code>--bfd</code> option allows to check if the next-hop is > > + running using BFD protocol > > + </p> > > + > > <p> > > It is an error if a route with <var>prefix</var> and > > <var>POLICY</var> already exists, unless > > <code>--may-exist</code>, > > diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c > > index 3a95f6b1f..9d82e2cfa 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -699,7 +699,8 @@ Logical router port commands:\n\ > > ('overlay' or 'bridged')\n\ > > \n\ > > Route commands:\n\ > > - [--policy=POLICY] [--ecmp] [--ecmp-symmetric-reply] lr-route-add ROUTER > > \n\ > > + [--policy=POLICY] [--ecmp] [--ecmp-symmetric-reply] [--bfd] \ > > + lr-route-add ROUTER \n\ > > PREFIX NEXTHOP [PORT]\n\ > > add a route to ROUTER\n\ > > [--policy=POLICY] lr-route-del ROUTER [PREFIX [NEXTHOP [PORT]]]\n\ > > @@ -3932,6 +3933,7 @@ nbctl_lr_route_add(struct ctl_context *ctx) > > goto cleanup; > > } > > > > + bool bfd = shash_find(&ctx->options, "--bfd") != NULL; > > bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL; > > bool ecmp_symmetric_reply = shash_find(&ctx->options, > > "--ecmp-symmetric-reply") != > > NULL; > > @@ -4002,12 +4004,16 @@ nbctl_lr_route_add(struct ctl_context *ctx) > > nbrec_logical_router_static_route_set_policy(route, policy); > > } > > > > + struct smap options; > > + smap_init(&options); > > if (ecmp_symmetric_reply) { > > - const struct smap options = SMAP_CONST1(&options, > > - "ecmp_symmetric_reply", > > - "true"); > > - nbrec_logical_router_static_route_set_options(route, &options); > > + smap_add(&options, "ecmp_symmetric_reply", "true"); > > } > > + if (bfd) { > > + smap_add(&options, "bfd", "true"); > > + } > > + nbrec_logical_router_static_route_set_options(route, &options); > > + smap_destroy(&options); > > > > nbrec_logical_router_verify_static_routes(lr); > > struct nbrec_logical_router_static_route **new_routes > > @@ -6566,7 +6572,7 @@ static const struct ctl_command_syntax > > nbctl_commands[] = { > > /* logical router route commands. */ > > { "lr-route-add", 3, 4, "ROUTER PREFIX NEXTHOP [PORT]", NULL, > > nbctl_lr_route_add, NULL, > > "--may-exist,--ecmp,--ecmp-symmetric-reply," > > - "--policy=", RW }, > > + "--bfd,--policy=", RW }, > > { "lr-route-del", 1, 4, "ROUTER [PREFIX [NEXTHOP [PORT]]]", NULL, > > nbctl_lr_route_del, NULL, "--if-exists,--policy=", RW }, > > { "lr-route-list", 1, 1, "ROUTER", NULL, nbctl_lr_route_list, NULL, > > -- > > 2.29.2 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
