On Wed, Dec 16, 2020 at 8:13 PM Lorenzo Bianconi <[email protected]> wrote: > > Introduce the bfd reference in logical_router_static_router table > in order to check if the next-hop is properly running using the BFD > protocol. The CMS is supposed to populate bfd column with the proper > reference. > Add BFD static routes tests in system-ovn.at/ovn-nbctl.at. > > Signed-off-by: Lorenzo Bianconi <[email protected]>
Hi Lorenzo, Please see below for some comments. Thanks Numan > --- > NEWS | 3 ++- > northd/ovn-northd.c | 10 ++++++++++ > ovn-nb.ovsschema | 6 +++++- > ovn-nb.xml | 7 +++++++ > tests/ovn-nbctl.at | 8 +++++++- > tests/system-ovn.at | 9 ++++++++- > 6 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/NEWS b/NEWS > index 306a7ccda..2f535f9a5 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 ada61275f..ab28b30df 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -7783,6 +7783,16 @@ parsed_routes_add(struct ovs_list *routes, > return NULL; > } > > + if (route->n_bfd > 0) { > + struct nbrec_bfd *nb_bt = route->bfd[0]; > + > + if (!strcmp(nb_bt->dst_ip, route->nexthop) && > + (!strcmp(nb_bt->status, "down") || > + !strcmp(nb_bt->status, "admin_down"))) { > + return NULL; > + } > + } > + > struct parsed_route *pr = xzalloc(sizeof *pr); > pr->prefix = prefix; > pr->plen = plen; > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index a880482a4..62e768e43 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > "version": "5.31.0", > - "cksum": "3663844734 28178", > + "cksum": "2123475422 28416", > "tables": { > "NB_Global": { > "columns": { > @@ -374,6 +374,10 @@ > "min": 0, "max": 1}}, > "nexthop": {"type": "string"}, > "output_port": {"type": {"key": "string", "min": 0, "max": > 1}}, > + "bfd": {"type": {"key": {"type": "uuid", "refTable": "BFD", > + "refType": "weak"}, > + "min": 0, > + "max": "unlimited"}}, We can only have one BFD associated with the next hop right ? I think you can set "max" to 1 instead of unlimited. In v2 I had commented the below -- 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. -- My suggestion is if a BFD row in NB DB is not associated with a logical router static route, then either don't create SB DB BFD rows or create it with "admin_down" status so that ovn-controller doesn't run BFD for that row. I don't see why ovn-controller should run BFD if its not used by the static route entry. Do you know of a use case, where CMS can create BFD entries without associating to a static route and monitor the BFD status on its own ? And take some decision based on the status ? Note: If we decide to run BFD in ovn-controller only if associated to a static route, I think you need to move the system test from patch 3 to patch 5. Thanks Numan > "options": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}, > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 4a28d3f0d..8c0ff0b72 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2644,6 +2644,13 @@ > </p> > </column> > > + <column name="bfd"> > + <p> > + Reference to <ref table="BFD"/> row if the route has associated a > + BFD session > + </p> > + </column> > + > <column name="external_ids" key="ic-learned-route"> > <code>ovn-ic</code> populates this key if the route is learned from the > global <ref db="OVN_IC_Southbound"/> database. In this case the value > diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at > index 01edfcbc1..8bb6ec0b5 100644 > --- a/tests/ovn-nbctl.at > +++ b/tests/ovn-nbctl.at > @@ -1617,7 +1617,13 @@ 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 lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24]) > +bfd_uuid=$(ovn-nbctl create bfd logical_port=lr0-p0 dst_ip=100.0.0.50 > status=down min_tx=250 min_rx=250 detect_mult=10) > +AT_CHECK([ovn-nbctl lr-route-add lr0 100.0.0.0/24 192.168.0.1]) > +route_uuid=$(ovn-nbctl --bare --columns=_uuid find > logical_router_static_route ip_prefix="100.0.0.0/24") Please use the fetch_column helper function to get the route uuid. > +AT_CHECK([ovn-nbctl set logical_router_static_route $route_uuid > bfd=$bfd_uuid])]) > > dnl --------------------------------------------------------------------- > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index d8c21404b..845b3f867 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -5604,7 +5604,7 @@ NS_CHECK_EXEC([server], [bfdd-control allow > 172.16.1.1], [0], [dnl > Allowing connections from 172.16.1.1 > ]) > > -ovn-nbctl create bfd logical_port=rp-public dst_ip=172.16.1.50 status=down > min_tx=250 min_rx=250 detect_mult=10 > +uuid=$(ovn-nbctl create bfd logical_port=rp-public dst_ip=172.16.1.50 > status=down min_tx=250 min_rx=250 detect_mult=10) > ovn-nbctl --wait=hv sync > > OVS_WAIT_UNTIL([test "$(ovn-nbctl list bfd | awk -F: '/status/{print > substr($2,2)}')" = "up"]) > @@ -5612,6 +5612,11 @@ AT_CHECK([ovn-nbctl list bfd | awk -F: '/status/{print > substr($2,2)}'], [0], [dn > up > ]) > > +ovn-nbctl lr-route-add R1 100.0.0.0/8 172.16.1.50 > +route_uuid=$(ovn-nbctl --bare --columns=_uuid find > logical_router_static_route ip_prefix="100.0.0.0/8") Same here. You can use the helper function. > +ovn-nbctl set logical_router_static_route $route_uuid bfd=$uuid > +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 +5626,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 > -- > 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
