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. 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. 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 ? 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
