On Thu, Nov 6, 2025 at 8:44 PM Han Zhou <[email protected]> wrote: > > On Thu, Nov 6, 2025 at 8:45 AM Numan Siddique <[email protected]> wrote: > > > > On Wed, Nov 5, 2025 at 7:28 PM Han Zhou <[email protected]> wrote: > > > > > > > > > > > > On Mon, Nov 3, 2025 at 6:17 PM <[email protected]> wrote: > > > > > > > > From: Numan Siddique <[email protected]> > > > > > > > > This patch series adds a new service OVN Bridge Controller > > > > (ovn-br-controller) which can be used to program OpenFlow rules to > > > > OVS bridges using OVN logical flows. > > > > > > > > The syntax and symantics of the logical flows is similar to the > logical > > > > flows generated by ovn-northd. > > > > > > > > Rational for adding this service: > > > > There's a need to configure the provider bridges with specific > OpenFlow > > > > rules after packets leave the OVN pipeline and enters these > provider > > > > bridges via the patch ports. Since OVN has a very good > abstraction > > > > of the OpenFlow rules using OVN logical flows, we can leverage > the same > > > > here so that CMS doesn't have to deal with the specifics of > OpenFlow > > > > protocol. Also if the CMS is written in golang or other > languages, > > > > CMS has to mostly rely on ovs-vsctl/ovs-ofctl to program the > flows. > > > > Adding this support in OVN avoids the CMS to exec these utils to > > > > add/delete and dump the existing OpenFlow rules. > > > > > > > > A user can also use this new service to program and manage any OVS > > > > bridge without using OVN and hence this service is named as > > > > "ovn-br-controller." > > > > > > > > > > Thanks Numan for this nice feature! I think it is very well structured, > and mostly looks good to me. I have sent my feedback and comments for some > of the patches. Feel free to add my Ack for the series when those are > addressed. > > > > Thanks for your comments Han. I'll address your comments for the > > patches and will add your Ack to the series. > > > > At this point, it's a bit hard for me to address the comments on > > moving the common code in ofctrl.c and br-ofctrl.c to a lib file. > > I'll address this as a follow up. Hope that's fine. > > Yes, I think this is fine. > > I replied with one more comment for patch 4. PTAL. > > Best, > Han
I had another look through the code, I agree with Han that we should add TODOs to reduce code duplication. However, I think that with this feature being experimental, it's actually safer to have code that is separate and duplicated, just to ensure that there are no unintended side effects on "standard" br-int bridge programming. I know that the testsuite *should* catch these sorts of things, but we all know that the test coverage isn't 100%. I also mentioned in an earlier review that since all actions are allowed by br-controller, we should have a TODO to restrict the actions usable by ovn-br-controller. Again, since this is experimental, that can be done later. I had one minor nit for patch 7, but other than that, Acked-by: Mark Michelson <[email protected]> > > > > > Thanks > > Numan > > > > > > > > Best regards, > > > Han > > > > > > > > > > > v2 -> v3 > > > > ------- > > > > * Added a new patch - p12 which > > > > - adds a NEWS entry for this new service - ovn-br-controller > > > > - Marks ovn-br-controller as an experimental service. > > > > - Documents the limitations of using OVN logical flow actions > > > > in the Bridge logical flows. > > > > > > > > v1 -> v2 > > > > ------- > > > > * Dropped P12 of v1 and merged it with P1 of v2 to address the CI > > > > failures. > > > > * Addressed review comments from Mark and added his Acked-by tag > > > > for the patches acked in v1. > > > > > > > > > > > > Numan Siddique (12): > > > > Add initial schema and skeleton for OVN bridge controller. > > > > ovn-br-controller: Connect to OVS database and define engine nodes. > > > > ovn-br-controller: Build en_bridge_data engine node. > > > > ovn-br-controller: Add bridge flow table manager. > > > > Move lflow-conj-ids module to lib. > > > > ovn-br-controller: Process logical flows. > > > > ovn-br-controller: Add en_pflow_output engine. > > > > Move ofctrl-seqno module to lib. > > > > ovn-br-controller: Program the openflows to the bridges. > > > > ovn-br-controller: Extend the symbol table to add new fields. > > > > ovn-ctl: Add commands to start OVN bridge controller services. > > > > ovn-br-controller: Add NEWS entry. > > > > > > > > Makefile.am | 5 +- > > > > NEWS | 3 + > > > > automake.mk | 36 + > > > > br-controller/automake.mk | 18 + > > > > br-controller/br-flow-mgr.c | 1263 > +++++++++++++++++ > > > > br-controller/br-flow-mgr.h | 65 + > > > > br-controller/br-ofctrl.c | 730 ++++++++++ > > > > br-controller/br-ofctrl.h | 33 + > > > > br-controller/en-bridge-data.c | 199 +++ > > > > br-controller/en-bridge-data.h | 43 + > > > > br-controller/en-lflow.c | 364 +++++ > > > > br-controller/en-lflow.h | 22 + > > > > br-controller/en-pflow.c | 192 +++ > > > > br-controller/en-pflow.h | 16 + > > > > br-controller/ovn-br-controller.8.xml | 79 ++ > > > > br-controller/ovn-br-controller.c | 561 ++++++++ > > > > controller/automake.mk | 4 - > > > > controller/if-status.c | 2 +- > > > > controller/lflow.h | 2 +- > > > > controller/ovn-controller.c | 4 +- > > > > lib/automake.mk | 21 +- > > > > lib/inc-proc-eng.h | 11 + > > > > {controller => lib}/lflow-conj-ids.c | 0 > > > > {controller => lib}/lflow-conj-ids.h | 2 +- > > > > {controller => lib}/ofctrl-seqno.c | 0 > > > > {controller => lib}/ofctrl-seqno.h | 0 > > > > lib/ovn-br-idl.ann | 9 + > > > > lib/ovn-util.c | 31 + > > > > lib/ovn-util.h | 16 + > > > > {controller => lib}/test-lflow-conj-ids.c | 0 > > > > {controller => lib}/test-ofctrl-seqno.c | 0 > > > > ovn-br.ovsschema | 94 ++ > > > > ovn-br.xml | 470 ++++++ > > > > rhel/automake.mk | 4 +- > > > > rhel/ovn-fedora.spec.in | 22 +- > > > > ...b_systemd_system_ovn-br-controller.service | 35 + > > > > rhel/usr_lib_systemd_system_ovn-br-db.service | 32 + > > > > tests/automake.mk | 11 +- > > > > tests/ovn-br-controller.at | 358 +++++ > > > > tests/testsuite.at | 1 + > > > > tutorial/ovn-sandbox | 24 +- > > > > utilities/automake.mk | 8 + > > > > utilities/ovn-brctl.c | 524 +++++++ > > > > utilities/ovn-ctl | 163 +++ > > > > utilities/ovn-ctl.8.xml | 36 + > > > > utilities/ovn-sbctl.c | 30 - > > > > 46 files changed, 5493 insertions(+), 50 deletions(-) > > > > create mode 100644 br-controller/automake.mk > > > > create mode 100644 br-controller/br-flow-mgr.c > > > > create mode 100644 br-controller/br-flow-mgr.h > > > > create mode 100644 br-controller/br-ofctrl.c > > > > create mode 100644 br-controller/br-ofctrl.h > > > > create mode 100644 br-controller/en-bridge-data.c > > > > create mode 100644 br-controller/en-bridge-data.h > > > > create mode 100644 br-controller/en-lflow.c > > > > create mode 100644 br-controller/en-lflow.h > > > > create mode 100644 br-controller/en-pflow.c > > > > create mode 100644 br-controller/en-pflow.h > > > > create mode 100644 br-controller/ovn-br-controller.8.xml > > > > create mode 100644 br-controller/ovn-br-controller.c > > > > rename {controller => lib}/lflow-conj-ids.c (100%) > > > > rename {controller => lib}/lflow-conj-ids.h (98%) > > > > rename {controller => lib}/ofctrl-seqno.c (100%) > > > > rename {controller => lib}/ofctrl-seqno.h (100%) > > > > create mode 100644 lib/ovn-br-idl.ann > > > > rename {controller => lib}/test-lflow-conj-ids.c (100%) > > > > rename {controller => lib}/test-ofctrl-seqno.c (100%) > > > > create mode 100644 ovn-br.ovsschema > > > > create mode 100644 ovn-br.xml > > > > create mode 100644 > rhel/usr_lib_systemd_system_ovn-br-controller.service > > > > create mode 100644 rhel/usr_lib_systemd_system_ovn-br-db.service > > > > create mode 100644 tests/ovn-br-controller.at > > > > create mode 100644 utilities/ovn-brctl.c > > > > > > > > -- > > > > 2.51.0 > > > > > > > > _______________________________________________ > > > > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
