On Thu, Oct 23, 2025 at 11:57 AM Mark Michelson <[email protected]> wrote: > > Hi Numan, > > I had a second look through this, and only found some minor nits. On a > higher level, I want to double check about some potential unsafe > behavior this allows. > > For instance, the entire library of logical flow actions is available > for users when using ovn bridge controller. Actions that translate to > controller() actions in OVS, such as dns_lookup(), can be programmed. > Since there is no controller to handle packet-ins, these actions won't > work properly. Also, actions like lookup_arp() or lookup_fdb() that > resubmit to a particular table can also be used, but they are unlikely > to be useful since those tables are now open to having custom flows > programmed on them, and there is not necessarily anything guaranteed > to be calling put_arp() or put_fdb() to populate those pre-determined > tables. Actions like lookup_arp() and lookup_fdb() also are programmed > to set specific register bits, which could cause conflict if an admin > is using those register bits for some other purpose. > > There are a couple of attitudes we can have about this. One is that > this is an experimental feature and so users are at their own risk > when they use it. If they try something that is guaranteed to fail, > then it's up to them to also come up with an alternative when that > inevitably fails. The other attitude we can have is that only logical > flow actions that are guaranteed to be useful should be available to > users of ovn-br-controller. So in that case, we probably should have a > way of disabling certain actions in ovn-br-controller. > > What do you think about this?
Hi Mark, Thanks for the comments. In the proposal discussion, there were suggestions to have this service as a separate repo to address the exact same concerns you had. But then it was decided to have it part of OVN to make it more manageable. I do agree with your comments. I think to start with we can have the first attitude you mentioned. I'll work on restricting the actions which are not relevant to bridge controller or strip them off from the symbol table. I haven't thought of how it can be done, but I think it should be possible. My proposal would be to add the responsibility of using the actions to the user and move towards making it more restricted. Does it sound good to you ? Also I've missed out on adding a NEWS entry. I'll add it in the next version (if required) or while merging. Numan > > On Thu, Oct 9, 2025 at 12:54 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." > > > > > > 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 (11): > > 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. > > > > Makefile.am | 5 +- > > 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 | 560 ++++++++ > > 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 | 452 ++++++ > > 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 - > > 45 files changed, 5471 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
