Hi Lorenzo, you probably should retitle the commit message as:
"ovn-nbctl: add --bfd option to lr-route-add"
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>]]
?
<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.
+ }
+ }
+ }
+
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
