> > On Wed, Nov 25, 2020 at 3:30 PM Lorenzo Bianconi > <lorenzo.bianc...@redhat.com> wrote: > > > > Introduce BFD protocol in ovn-controller according to RFC5880 [0] > > We added BFD implementation in ovn since layered protocols usually request > > to > > enable it on ovn entities (e.g. logical router ports) while ovs > > implementation > > relies on physical entities (e.g. ovs interfaces). > > Moreover we would establish a BFD session between a given ovn-port and > > multiple peers (1:n relation). A typical use-case is reported in [1]. > > In the current implementation Asynchronous mode is fully supported, while > > Demand > > mode is supported only on rx side. > > > > [0] - https://tools.ietf.org/html/rfc5880 > > [1] - https://bugzilla.redhat.com/show_bug.cgi?id=1847570 > > Hi Lorenzo, >
Hi Numan, thx for the review :) > Thanks for working on this feature. > > I quickly went through the patch series. I have few comments and questions. > > 1. From the commit messages, it is not very clear to me about the use case > and why BFD support is required. Can you please add more information > in the commit messages. > ack, will do in v2 > 2. From what I understand, CMS is supposed to create BFD rows and ovn-northd > will eventually run BFD on the destination IP specified and update > the status. > So is it upto the CMS to make use of this information ? If so, I'm > not sure that > is the right approach. Ideally I would expect BFD to be internal to OVN. > If I understand correctly, the use case is to add BFD support for > the ECMP routes. > So I'd expect the CMS to do something like below > > ovn-nbctl lr-route-add --ecmp lr0 10.0.0.6/32 172.168.0.201 bfd=true > ovn-nbctl lr-route-add --ecmp lr0 10.0.0.6/32 172.168.0.202 bfd=true > ovn-nbctl lr-route-add --ecmp lr0 10.0.0.6/32 172.168.0.203 bfd=true > > And ovn-northd will create BFD rows in the SB DB (just like how it does for > load balancer health check). > For the above routes, ovn-northd will add the lflows like below > IIUC what you mean here is we do not need the BFD table in ovn-nd db, right? The main reason of nb db BFD table is to provide the configuration parameter of bfd protocol (e.g min_tx, multiplier, ...) that the CMS needs provide. Please note I will add the integration with ovn routing (as you indicated below), it is just not part of the series since it just aims to add bfd support. > **** > table=10(lr_in_ip_routing ), priority=65 , match=(ip4.dst == > 10.0.0.6/32), action=(ip.ttl--; flags.loopback = 1; reg8[0..15] = 1; > reg8[16..31] = select(1, 2, 3);) > table=11(lr_in_ip_routing_ecmp), priority=100 , > match=(reg8[0..15] == 1 && reg8[16..31] == 1), action=(reg0 = > 172.168.0.202; reg1 = 172.168.0.100; eth.src = 00:00:20:20:12:13; > outport = "lr0-public"; next;) > table=11(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[0..15] > == 1 && reg8[16..31] == 2), action=(reg0 = 172.168.0.201; reg1 = > 172.168.0.100; eth.src = 00:00:20:20:12:13; outport = "lr0-public"; > next;) > table=11(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[0..15] > == 1 && reg8[16..31] == 3), action=(reg0 = 172.168.0.203; reg1 = > 172.168.0.100; eth.src = 00:00:20:20:12:13; outport = "lr0-public"; > next;) > ******* > > Suppose if the BFD session to 172.168.0.203 is down, then I'd > expect ovn-northd to update the lflows accordingly. > Like: > **** > table=10(lr_in_ip_routing ), priority=65 , match=(ip4.dst == > 10.0.0.6/32), action=(ip.ttl--; flags.loopback = 1; reg8[0..15] = 1; > reg8[16..31] = select(1, 2);) > table=11(lr_in_ip_routing_ecmp), priority=100 , > match=(reg8[0..15] == 1 && reg8[16..31] == 1), action=(reg0 = > 172.168.0.202; reg1 = 172.168.0.100; eth.src = 00:00:20:20:12:13; > outport = "lr0-public"; next;) > table=11(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[0..15] > == 1 && reg8[16..31] == 2), action=(reg0 = 172.168.0.201; reg1 = > 172.168.0.100; eth.src = 00:00:20:20:12:13; outport = "lr0-public"; > next;) > ******* > > With your present approach, suppose BFD session with 172.168.0.203 > is down, then CMS should monitor the status and remove the logical > router static route - (10.0.0.6/32 172.168.0.203) if status is down. > And if the status is up, it should add back again. > > I think all this should be transparent to the CMS. What do you think ? > please see my comment above > 3. I don't think there is a need to support global BFD config when a > user can create BFD NB rows (or bfd=true in static routes). > ack, will fix in v2 > 4. Can you please add some tests in ovn-northd.at to make sure that > ovn-northd generates logical flows as expected. > ack, will fix in v2 > 5. Can you please change the new action handle_bfd_msg to handle_bfd_msg(). > ack, will fix in v2 Regards, Lorenzo > Thanks > Numan > > > > > Lorenzo Bianconi (4): > > controller: introduce BFD tx path in ovn-controller > > action: introduce handle_bfd_msg() action > > controller: bfd: introduce BFD state machine > > bfp: support demand mode on rx side > > > > controller/ovn-controller.c | 1 + > > controller/pinctrl.c | 600 +++++++++++++++++++++++++++++++++++- > > controller/pinctrl.h | 2 + > > include/ovn/actions.h | 7 + > > lib/actions.c | 16 + > > lib/ovn-l7.h | 19 ++ > > northd/ovn-northd.8.xml | 21 ++ > > northd/ovn-northd.c | 198 +++++++++++- > > ovn-nb.ovsschema | 19 +- > > ovn-nb.xml | 62 ++++ > > ovn-sb.ovsschema | 23 +- > > ovn-sb.xml | 74 +++++ > > tests/atlocal.in | 3 + > > tests/ovn.at | 4 + > > tests/system-ovn.at | 106 +++++++ > > utilities/ovn-trace.c | 2 + > > 16 files changed, 1146 insertions(+), 11 deletions(-) > > > > -- > > 2.28.0 > > > > _______________________________________________ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
