On 6/19/25 1:52 PM, Gavin McKee wrote: > Hi @Dumitru Ceara, > Hi Gav,
> 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. > Thanks for the additional details. Just in case you missed it we're currently working on extending the dynamic-routing support added in 25.03 to work with EVPN (but via IP and not MPLS). Some details were discussed in the previous technical community meeting: https://mail.openvswitch.org/pipermail/ovs-dev/2025-May/423330.html Back to your patch, I guess it might make sense in general (also for other integration features) to propagate the SB.Port_Binding external_ids (copied from the CMS-provided NB external_ids) to the OVS ports. Maybe even for all switch ports, so not only for localnet patch ports created by ovn-controller. I'm still a bit worried about the potential performance impact though. The problem is that other CMS (e.g., ovn-kubernetes) set Logical_Switch_port.external_ids of their own and, while your patch doesn't do anything with them if they're not on localnet ports, ovn-controller will now receive them all from the SB server (because it now monitors the Port_Binding.external_ids column) and will also trigger all the computations I listed below. In theory we could change all possible SB.Port_Binding handlers in ovn-controller to ignore the external_ids changes if that's the only change that happened to the port binding. Something like: static bool sb_port_binding_only_external_ids_changed(const struct sbrec_port_binding *pb) { if (sbrec_port_binding_is_new(pb) || sbrec_port_binding_is_deleted(pb)) { return false; } for (size_t col = 0; col < SBREC_PORT_BINDING_N_COLUMNS; col++) { if (col != SBREC_PORT_BINDING_COL_EXTERNAL_IDS && sbrec_port_binding_is_updated(pb, col)) { return false; } } return true; } And then sprinkle calls to this function in all SB.Port_Binding handlers. But that's quite error prone, and honestly, ugly. However, I don't have a better idea on how to implement this with external_ids at the moment. CC-ing more maintainers for ideas. OTOH, there might be another way of achieving what you want, with a bit more work on the CMS side. Can you make the CMS component that's adding the OVS.external_ids:ovn-bridge-mappings (required by ovn-controller to create the patch port) aware of the tenant-id information? It could then store that into the patch port external_ids once ovn-controller creates it. Or maybe it could even update the custom OpenFlow pipeline in the provider bridge directly when the patch port is created. But I'm not sure if that's something easy to implement in your CMS. Regards, Dumitru > 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
