On Fri, Nov 7, 2025 at 1:48 PM Mark Michelson <[email protected]> wrote:
>
> 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]>
Thank you Mark and Han for the reviews. I applied this series to the
main branch addressing the review comments.
In patch 7 while adding physical flows to the OVS ports in the
provider bridges, I had missed adding a flow to
clear in_port for hairpin traffic i.e both inport and outport the same.
This could happen if CMS adds ARP responder/icmp response logical
flows and sets "outport = inport; output;".
So I also added that. Below are the changes
-------------------------
diff --git a/br-controller/en-pflow.c b/br-controller/en-pflow.c
index 5f4c5b914f..be4bfafaf4 100644
--- a/br-controller/en-pflow.c
+++ b/br-controller/en-pflow.c
@@ -48,6 +48,7 @@ static void physical_flows_add(const struct ovn_bridge *);
static void put_load(uint64_t value, enum mf_field_id dst, int ofs, int n_bits,
struct ofpbuf *);
static void put_resubmit(uint8_t table_id, struct ofpbuf *);
+static void put_stack(enum mf_field_id, struct ofpact_stack *);
static void add_interface_flows(const char *bridge,
const struct ovsrec_interface *iface_rec,
@@ -98,7 +99,7 @@ physical_flows_add(const struct ovn_bridge *br)
struct match match = MATCH_CATCHALL_INITIALIZER;
- /* Table 0 and 65, priority 0, actions=NORMAL
+ /* Table 0 and 121, priority 0, actions=NORMAL
* ===================================
*
*/
@@ -110,6 +111,7 @@ physical_flows_add(const struct ovn_bridge *br)
br->ovs_br->header_.uuid.parts[0],
&match, &ofpacts, &br->ovs_br->header_.uuid);
+ /* Priority-0 action to advance the packet from table 120 to 121. */
ofpbuf_clear(&ofpacts);
put_resubmit(BR_OFTABLE_LOG_TO_PHY, &ofpacts);
br_flow_add_physical_oflow(br->db_br->name, BR_OFTABLE_SAVE_INPORT, 0,
@@ -156,6 +158,14 @@ put_resubmit(uint8_t table_id, struct ofpbuf *ofpacts)
resubmit->table_id = table_id;
}
+static void
+put_stack(enum mf_field_id field, struct ofpact_stack *stack)
+{
+ stack->subfield.field = mf_from_id(field);
+ stack->subfield.ofs = 0;
+ stack->subfield.n_bits = stack->subfield.field->n_bits;
+}
+
static void
add_interface_flows(const char *bridge,
const struct ovsrec_interface *iface_rec,
@@ -189,4 +199,23 @@ add_interface_flows(const char *bridge,
iface_rec->header_.uuid.parts[0],
&match, ofpacts_p,
&iface_rec->header_.uuid);
+
+ /* Priority-100 flow in table BR_OFTABLE_SAVE_INPORT to match on
+ * both inport and outport to the interface's ofport number and
+ * clear the in_port to OFPP_NONE before advancing the packet to
+ * the next table - BR_OFTABLE_LOG_TO_PHY.
+ * This is required for the hairpinned packets.
+ */
+ match_init_catchall(&match);
+ match_set_reg(&match, MFF_LOG_INPORT - MFF_REG0, ofport);
+ match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, ofport);
+
+ ofpbuf_clear(ofpacts_p);
+ put_stack(MFF_IN_PORT, ofpact_put_STACK_PUSH(ofpacts_p));
+ put_load(ofp_to_u16(OFPP_NONE), MFF_IN_PORT, 0, 16, ofpacts_p);
+ put_resubmit(BR_OFTABLE_LOG_TO_PHY, ofpacts_p);
+ put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
+ br_flow_add_physical_oflow(bridge, BR_OFTABLE_SAVE_INPORT, 100,
+ iface_rec->header_.uuid.parts[0],
+ &match, ofpacts_p, &iface_rec->header_.uuid);
}
------------------------------------
Thanks
Numan
>
> >
> > >
> > > 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
