> Hi Lorenzo, you probably should retitle the commit message as: > > "ovn-nbctl: add --bfd option to lr-route-add"
Hi Mark, thx for the review. > > More down below > > On 1/28/21 6:10 PM, Lorenzo Bianconi wrote: > > Introduce the --bfd option to lr-route-add-bfd. > > If the BFD session UUID is provided, it will be used for the OVN route > > otherwise the next-hop will be used to perform a lookup in the OVN BFD > > table. > > If the lookup fails and outport is specified, a new entry in the BFD table > > will be created using the nexthop as dst_ip and outport as logical_port. > > > > Signed-off-by: Lorenzo Bianconi <[email protected]> > > --- > > Changes since v1: > > - introduce the capability to create the BFD entry if route outport has been > > specified > > --- > > tests/ovn-northd.at | 17 +++++++++++----- > > tests/system-ovn.at | 5 ++--- > > utilities/ovn-nbctl.8.xml | 11 ++++++++++ > > utilities/ovn-nbctl.c | 43 ++++++++++++++++++++++++++++++++++++++- > > 4 files changed, 67 insertions(+), 9 deletions(-) > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 8597ca1b9..c00225e99 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -2342,7 +2342,7 @@ AT_KEYWORDS([northd-bfd]) > > ovn_start > > check ovn-nbctl --wait=sb lr-add r0 > > -for i in $(seq 1 4); do > > +for i in $(seq 1 5); do > > check ovn-nbctl --wait=sb lrp-add r0 r0-sw$i 00:00:00:00:00:0$i > > 192.168.$i.1/24 > > check ovn-nbctl --wait=sb ls-add sw$i > > check ovn-nbctl --wait=sb lsp-add sw$i sw$i-r0 > > @@ -2387,17 +2387,24 @@ check_column 1000 bfd min_tx logical_port=r0-sw1 > > check_column 1000 bfd min_rx logical_port=r0-sw1 > > check_column 100 bfd detect_mult logical_port=r0-sw1 > > -check ovn-nbctl lr-route-add r0 100.0.0.0/8 192.168.10.2 > > -route_uuid=$(fetch_column nb:logical_router_static_route _uuid > > ip_prefix="100.0.0.0/8") > > -check ovn-nbctl set logical_router_static_route $route_uuid bfd=$uuid > > +check ovn-nbctl --bfd=$uuid lr-route-add r0 100.0.0.0/8 192.168.10.2 > > check_column down bfd status logical_port=r0-sw1 > > AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.10.2 | grep -q > > bfd],[0]) > > +check ovn-nbctl --bfd lr-route-add r0 200.0.0.0/8 192.168.20.2 > > +check_column down bfd status logical_port=r0-sw2 > > +AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.20.2 | grep -q > > bfd],[0]) > > + > > +check ovn-nbctl --bfd lr-route-add r0 240.0.0.0/8 192.168.50.2 r0-sw5 > > +check_column down bfd status logical_port=r0-sw5 > > +AT_CHECK([ovn-nbctl lr-route-list r0 | grep 192.168.50.2 | grep -q > > bfd],[0]) > > + > > +route_uuid=$(fetch_column nb:logical_router_static_route _uuid > > ip_prefix="100.0.0.0/8") > > check ovn-nbctl clear logical_router_static_route $route_uuid bfd > > check_column admin_down bfd status logical_port=r0-sw1 > > ovn-nbctl destroy bfd $uuid > > -check_row_count bfd 2 > > +check_row_count bfd 3 > > AT_CLEANUP > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index 4580c05e7..05ccd861a 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -5606,10 +5606,9 @@ NS_CHECK_EXEC([server], [bfdd-control allow > > 172.16.1.1], [0], [dnl > > Allowing connections from 172.16.1.1 > > ]) > > -uuid=$(ovn-nbctl create bfd logical_port=rp-public dst_ip=172.16.1.50 > > min_tx=250 min_rx=250 detect_mult=10) > > -check ovn-nbctl lr-route-add R1 100.0.0.0/8 172.16.1.50 > > +check ovn-nbctl --bfd lr-route-add R1 100.0.0.0/8 172.16.1.50 rp-public > > +uuid=$(fetch_column nb:bfd _uuid logical_port="rp-public") > > route_uuid=$(fetch_column nb:logical_router_static_route _uuid > > ip_prefix="100.0.0.0/8") > > -check ovn-nbctl set logical_router_static_route $route_uuid bfd=$uuid > > check ovn-nbctl --wait=hv sync > > wait_column "up" nb:bfd status logical_port=rp-public > > diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml > > index 6ed8bcb75..249a17344 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=<var>UUID</var></code>] > > To better indicate the UUID is optional, would it make more sense to make > this: > > [<code>--bfd[=<var>UUID</var></code>]] ack, I will fix it in v3. > > ? > > > <code>lr-route-add</code> <var>router</var> > > <var>prefix</var> <var>nexthop</var> [<var>port</var>]</dt> > > <dd> > > @@ -695,6 +696,16 @@ > > it is not necessary to set both. > > </p> > > + <p> > > + <code>--bfd</code> option is used to link a BFD session to the > > + OVN route. If the BFD session UUID is provided, it will be used > > + for the OVN route otherwise the next-hop will be used to perform > > + a lookup in the OVN BFD table. > > + If the lookup fails and <var>port</var> is specified, a new entry > > + in the BFD table will be created using the <var>nexthop</var> as > > + <var>dst_ip</var> and <var>port</var> as <var>logical_port</var>. > > + </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 1b7be1ef9..b5bf59e0b 100644 > > --- a/utilities/ovn-nbctl.c > > +++ b/utilities/ovn-nbctl.c > > @@ -3960,6 +3960,41 @@ nbctl_lr_route_add(struct ctl_context *ctx) > > goto cleanup; > > } > > + struct shash_node *bfd = shash_find(&ctx->options, "--bfd"); > > + const struct nbrec_bfd *nb_bt = NULL; > > + if (bfd) { > > + if (bfd->data) { > > + char *data = xstrdup(bfd->data); > > + struct uuid bfd_uuid; > > + if (uuid_from_string(&bfd_uuid, data)) { > > + nb_bt = nbrec_bfd_get_for_uuid(ctx->idl, &bfd_uuid); > > + } > > + free(data); > > + if (!nb_bt) { > > + ctl_error(ctx, "no entry found in the BFD table"); > > + goto cleanup; > > + } > > + } else { > > + const struct nbrec_bfd *iter; > > + NBREC_BFD_FOR_EACH (iter, ctx->idl) { > > + if (!strcmp(iter->dst_ip, next_hop)) { > > + nb_bt = iter; > > + break; > > + } > > + } > > + if (!nb_bt) { > > + if (ctx->argc != 5) { > > + ctl_error(ctx, > > + "not able to create an entry in the BFD > > table"); > > + goto cleanup; > > + } > > + nb_bt = nbrec_bfd_insert(ctx->txn); > > + nbrec_bfd_set_dst_ip(nb_bt, next_hop); > > + nbrec_bfd_set_logical_port(nb_bt, ctx->argv[4]); > > The problem with this block is that it is possible for us to insert a new > BFD row, then have an error occur lower down, resulting in the BFD row not > being used by a static route. > > For instance, let's say that someone intended to add a new ECMP route but > forgot to specify "--ecmp". In the code block below, the user would hit the > "duplicate prefix" error, and the route would not be created. However, the > BFD row would still be created. I think it's a better idea to ensure that we > only add the BFD row if the static route is successfully created/updated. I guess it is not a big deal since even if the configuration of the static route fails and the BFD entry is already created, it will be used next time the user tries to insert the route. Anyway I agree it is better to move this block below. I will fix it in v3. Regards, Lorenzo > > > + } > > + } > > + } > > + > > bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL; > > bool ecmp_symmetric_reply = shash_find(&ctx->options, > > "--ecmp-symmetric-reply") != > > NULL; > > @@ -4007,6 +4042,9 @@ nbctl_lr_route_add(struct ctl_context *ctx) > > nbrec_logical_router_static_route_verify_nexthop(route); > > nbrec_logical_router_static_route_set_ip_prefix(route, > > prefix); > > nbrec_logical_router_static_route_set_nexthop(route, > > next_hop); > > + if (nb_bt) { > > + nbrec_logical_router_static_route_set_bfd(route, nb_bt); > > + } > > if (ctx->argc == 5) { > > nbrec_logical_router_static_route_set_output_port( > > route, ctx->argv[4]); > > @@ -4038,6 +4076,9 @@ nbctl_lr_route_add(struct ctl_context *ctx) > > } > > nbrec_logical_router_update_static_routes_addvalue(lr, route); > > + if (nb_bt) { > > + nbrec_logical_router_static_route_set_bfd(route, nb_bt); > > + } > > cleanup: > > free(next_hop); > > @@ -6551,7 +6592,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 }, > > + "--policy=,--bfd?", 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, > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
