On Thu, Oct 23, 2025 at 1:38 PM Numan Siddique <[email protected]> wrote: > > 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 ?
Hi Mark, Just checking if you're ok with the reply ? If you've any concerns or comments, please let me know. Thanks Numan > > 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
