Hi @Dumitru Ceara,
Thanks for taking the time to review the patch.
> It would be great if you could elaborate more on why copying the
> external_ids to the patch port is needed.
The motivation for this is driven simply to allow the assignment of a
localnet port (and the associated patch port) to a specific tenant in
the provider bridge.
I am implementing a custom Openflow pipeline in the provider bridge to
enable routing to MPLS edge nodes via EVPN Type 5.
The customer logical router port will have a tenant specific gateway
implemented in the provider bridge and I'd like to use a metadata
field to carry that context by setting an external_id:tenant_id=XXXXX
. Any packets received on that patch port I can
1. set the metadata to tenant_id
2. carry that through the match action conditions. This is why localnet only.
I hope that makes sense . I'm an amature here so if you've an
approach that would work I'd appreciate the guidance.
Regards
Gav
On Thu, 19 Jun 2025 at 04:19, Dumitru Ceara <dce...@redhat.com> wrote:
>
> Hi Gavin,
>
> Thanks for the patch!
>
> It would be great if you could elaborate more on why copying the
> external_ids to the patch port is needed.
>
> I'm asking because your code is now changing ovn-controller to also
> monitor SB.Port_Binding.external_ids. That means that every time a port
> binding's external_ids get updated (i.e., every time the
> NB.Logical_Switch_Port.external_ids are updated) we end up doing quite a
> lot of compute work in ovn-controller.
>
> That is (probably not complete) the following I-P handlers are called:
> - pflow_output_sb_port_binding_handler()
>
> - lflow_output_sb_port_binding_handler()
>
> - runtime_data_sb_port_binding_handler() -> which marks en_runtime_data
> as updated and in turn triggers the following I-P nodes to be computed:
> a. en_pflow_output (pflow_output_runtime_data_handler())
> b. en_lflow_output (lflow_output_runtime_data_handler())
> c. en_ct_zones (ct_zones_runtime_data_handler())
> d. en_mac_cache (mac_cache_runtime_data_handler())
> e. en_garp_rarp (garp_rarp_runtime_data_handler() - this will actually
> fail to incrementally process changes for switches with localnet ports)
> f. en_lb_data (lb_data_runtime_data_handler())
>
> - garp_rarp_sb_port_binding_handler() - this will actually fail to
> incrementally process changes for switches with localnet ports)
>
> So, I guess, based on how often the NB.Logical_Switch_Port external_ids
> change we might see some signficant performance implications on the
> ovn-controller side.
>
> Please see some more comments inline.
>
> On 6/18/25 5:57 PM, Gavin McKee via dev wrote:
> > Signed-off-by: Gavin McKee <gavmcke...@googlemail.com>
> > ---
> > controller/ovn-controller.8.xml | 5 +++
> > controller/ovn-controller.c | 1 -
> > controller/patch.c | 44 +++++++++++++++++---
> > tests/ovn-controller.at | 72 +++++++++++++++++++++++++++++++++
> > 4 files changed, 115 insertions(+), 7 deletions(-)
> >
> > diff --git a/controller/ovn-controller.8.xml
> > b/controller/ovn-controller.8.xml
> > index 5007f5f80..422eb2cf7 100644
> > --- a/controller/ovn-controller.8.xml
> > +++ b/controller/ovn-controller.8.xml
> > @@ -554,6 +554,11 @@
> > bridge, with the same <code>external_ids:ovn-localnet-port</code>
> > value.
> > </p>
> > + <p>
> > + When a <code>localnet</code> port has additional values in its
> > + <code>external_ids</code> column, these key-value pairs are
> > copied to
> > + the corresponding patch ports.
> > + </p>
> > </dd>
> >
> > <dt>
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 7940091da..8c8d47fe3 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -5857,7 +5857,6 @@ main(int argc, char *argv[])
> >
> > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_sb_global_col_external_ids);
> > ovsdb_idl_omit(ovnsb_idl_loop.idl,
> > &sbrec_logical_flow_col_external_ids);
> > - ovsdb_idl_omit(ovnsb_idl_loop.idl,
> > &sbrec_port_binding_col_external_ids);
> > ovsdb_idl_omit(ovnsb_idl_loop.idl, &sbrec_ssl_col_external_ids);
> > ovsdb_idl_omit(ovnsb_idl_loop.idl,
> > &sbrec_gateway_chassis_col_external_ids);
> > diff --git a/controller/patch.c b/controller/patch.c
> > index 4fed6e375..5ba1466e0 100644
> > --- a/controller/patch.c
> > +++ b/controller/patch.c
> > @@ -73,18 +73,38 @@ match_patch_port(const struct ovsrec_port *port, const
> > char *peer)
> > * 'dst_name' in bridge 'dst'. Initializes the patch port's
> > external-ids:'key'
> > * to 'key'.
> > *
> > - * If such a patch port already exists, removes it from 'existing_ports'.
> > */
> > + * If such a patch port already exists, removes it from 'existing_ports'.
> > Any
> > + * key-value pairs in 'extra_ids' are merged into the port's external IDs.
> > If
> > + * the port already exists, its external IDs are updated accordingly.
> > + */
> > static void
> > create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn,
> > const char *key, const char *value,
> > const struct ovsrec_bridge *src, const char *src_name,
> > const struct ovsrec_bridge *dst, const char *dst_name,
> > - struct shash *existing_ports)
> > + struct shash *existing_ports,
> > + const struct smap *extra_ids)
> > {
> > for (size_t i = 0; i < src->n_ports; i++) {
> > if (match_patch_port(src->ports[i], dst_name)) {
> > /* Patch port already exists on 'src'. */
> > - shash_find_and_delete(existing_ports, src->ports[i]->name);
> > + const struct ovsrec_port *port = src->ports[i];
> > + shash_find_and_delete(existing_ports, port->name);
> > + if (extra_ids) {
> > + const struct smap *ids = &port->external_ids;
> > + struct smap merged = SMAP_INITIALIZER(&merged);
> > + smap_clone(&merged, ids);
>
> We can avoid the local variable and just use "&port->external_ids" here.
>
> > + struct smap_node *node;
> > + SMAP_FOR_EACH (node, extra_ids) {
> > + const char *cur = smap_get(&merged, node->key);
> > + if (!cur || strcmp(cur, node->value)) {
> > + ovsrec_port_update_external_ids_setkey(port,
> > + node->key,
> > + node->value);
> > + }
> > + }
>
> It might be faster to just merge &port->external_ids and extra_ids and
> unconditionally call ovsrec_port_update_external_ids() and let the IDL
> layer take care of the diffs.
>
> Also, "merged" is a confusing name because it doesn't really contain any
> merged sets of values.
>
> > + smap_destroy(&merged);
> > + }
> > return;
> > }
> > }
> > @@ -94,9 +114,16 @@ create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn,
> > src_name, src->name, dst->name);
> >
> > const struct smap if_options = SMAP_CONST1(&if_options, "peer",
> > dst_name);
> > - const struct smap port_ids = SMAP_CONST1(&port_ids, key, value);
> > +
> > + struct smap port_ids = SMAP_INITIALIZER(&port_ids);
> > + if (extra_ids) {
> > + smap_clone(&port_ids, extra_ids);
> > + }
> > + smap_replace(&port_ids, key, value);
> > +
> > ovsport_create(ovs_idl_txn, src, src_name, "patch", &port_ids, NULL,
> > &if_options, 0);
> > + smap_destroy(&port_ids);
> > }
> >
> > static void
> > @@ -223,12 +250,17 @@ add_bridge_mappings_by_type(struct ovsdb_idl_txn
> > *ovs_idl_txn,
> > sset_find_and_delete(&missed_bridges, msg_key);
> > free(msg_key);
> >
> > + const struct smap *extra_ids =
> > + !strcmp(pb_type, "localnet") ? &binding->external_ids : NULL;
> > +
>
> Why only for localnet? But I guess that information would be part of
> the answer to the question I had on the commit message (about the actual
> use case).
>
> > char *name1 = patch_port_name(br_int->name, binding->logical_port);
> > char *name2 = patch_port_name(binding->logical_port, br_int->name);
> > create_patch_port(ovs_idl_txn, patch_port_id,
> > binding->logical_port,
> > - br_int, name1, br_ln, name2, existing_ports);
> > + br_int, name1, br_ln, name2, existing_ports,
> > + extra_ids);
> > create_patch_port(ovs_idl_txn, patch_port_id,
> > binding->logical_port,
> > - br_ln, name2, br_int, name1, existing_ports);
> > + br_ln, name2, br_int, name1, existing_ports,
> > + extra_ids);
> > free(name1);
> > free(name2);
> > }
> > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> > index ae7eb6f31..6ae97e87d 100644
> > --- a/tests/ovn-controller.at
> > +++ b/tests/ovn-controller.at
> > @@ -139,6 +139,7 @@ check_patches \
> > 'br-int patch-br-int-to-localnet2 patch-localnet2-to-br-int' \
> > 'br-eth0 patch-localnet2-to-br-int patch-br-int-to-localnet2'
> >
> > +
>
> Nit: unrelated change.
>
> > # Add logical patch ports to connect new logical datapath.
> > #
> > # OVN no longer uses OVS patch ports to implement logical patch ports, so
> > @@ -174,6 +175,77 @@ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> > AT_CLEANUP
> > ])
> >
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([Localnet port external ids])
> > +AT_KEYWORDS([lb])
> > +ovn_start
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lsp-add sw0 sw0-port1
> > +check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 10.0.0.3
> > 1000::3"
> > +check ovn-nbctl lsp-add sw0 sw0-port2
> > +check ovn-nbctl lsp-set-addresses sw0-port2 "50:54:00:00:00:02 10.0.0.4
> > 1000::4"
> > +
> > +check ovn-nbctl lr-add lr0
> > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 10.0.0.1/24
> > 1000::1/64
> > +check ovn-nbctl lsp-add sw0 sw0-lr0
> > +check ovn-nbctl lsp-set-type sw0-lr0 router
> > +check ovn-nbctl lsp-set-addresses sw0-lr0 router
> > +check ovn-nbctl lsp-set-options sw0-lr0 router-port=lr0-sw0
> > +
> > +check ovn-nbctl ls-add public
> > +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 172.168.0.100/24
> > 2000::1/64
> > +check ovn-nbctl lsp-add public public-lr0
> > +check ovn-nbctl lsp-set-type public-lr0 router
> > +check ovn-nbctl lsp-set-addresses public-lr0 router
> > +check ovn-nbctl lsp-set-options public-lr0 router-port=lr0-public
> > +
> > +# localnet port
> > +check ovn-nbctl lsp-add public ln-public
> > +check ovn-nbctl lsp-set-type ln-public localnet
> > +check ovn-nbctl lsp-set-addresses ln-public unknown
> > +check ovn-nbctl lsp-set-options ln-public network_name=phys
> > +
> > +check ovn-nbctl lrp-set-gateway-chassis lr0-public hv1 20
> > +
> > +net_add n1
> > +
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
> > +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> > + set interface hv1-vif1 external-ids:iface-id=sw0-port1 \
> > + options:tx_pcap=hv1/vif1-tx.pcap \
> > + options:rxq_pcap=hv1/vif1-rx.pcap \
> > + ofport-request=1
> > +
> > +check ovn-nbctl --wait=hv set logical_switch_port ln-public
> > external_ids:foo=bar
> > +wait_column "foo=bar" Port_Binding external_ids logical_port=ln-public
> > +
> > +AT_CHECK([as hv1 ovs-vsctl get port patch-br-int-to-ln-public
> > external_ids:foo], [0], [dnl
> > +bar
> > +])
> > +
> > +AT_CHECK([as hv1 ovs-vsctl get port patch-ln-public-to-br-int
> > external_ids:foo], [0], [dnl
> > +bar
> > +])
> > +
> > +check ovn-nbctl --wait=hv set logical_switch_port ln-public
> > external_ids:foo=foobar
> > +wait_column "foo=foobar" Port_Binding external_ids logical_port=ln-public
> > +
> > +AT_CHECK([as hv1 ovs-vsctl get port patch-br-int-to-ln-public
> > external_ids:foo], [0], [dnl
> > +foobar
> > +])
> > +
> > +AT_CHECK([as hv1 ovs-vsctl get port patch-ln-public-to-br-int
> > external_ids:foo], [0], [dnl
> > +foobar
> > +])
> > +
> > +AT_CLEANUP
> > +])
> > +
> > # Checks that ovn-controller populates datapath-type and iface-types
> > # correctly in the Chassis other_config column.
> > OVN_FOR_EACH_NORTHD([
>
> Regards,
> Dumitru
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
