On Mon, Nov 3, 2025 at 2:29 PM Mark Michelson <[email protected]> wrote: > > On Thu, Oct 30, 2025 at 3:49 PM Numan Siddique <[email protected]> wrote: > > > > 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 > > > > Yes I'm fine with that. Since this is an experimental feature and it > exists completely separately from "core" OVN bridge logic, I think > it's OK to give the user some loaded foot-guns. We should just make it > clear that the feature is experimental and users are at their own risk > when using it.
Yes I'm fine with that. Since this is an experimental feature and it exists completely separately from "core" OVN bridge logic, I think it's OK to give the user some loaded foot-guns. We should just make it clear that the feature is experimental and users are at their own risk when using it. Thanks Mark. I added a new patch to the series which adds a NEWS entry, sets this service as experimental and documents the limitations of using OVN logical actions in the bridge logical flows - https://patchwork.ozlabs.org/project/ovn/list/?series=480819 Request to take a look. 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
