[ovs-dev] [PATCH v2 ovn 5/5] ovn: integrate bfd for static routes
Numan Siddique
numans at ovn.org
Tue Dec 15 14:36:24 UTC 2020
On Fri, Dec 11, 2020 at 5:56 PM Lorenzo Bianconi
<lorenzo.bianconi at redhat.com> 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 <lorenzo.bianconi at redhat.com>
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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list