[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