On 6/19/25 6:07 PM, Gavin McKee wrote: > I’m cool with anything that doesn’t compromise the performance of the > controller . > > If an option on the LSP would do it then I’d be more than happy. As long as > the end state is something I can get from the OVS patch port on the > provider bridge then it’s a win . >
Sounds good to me, looking forward to a new version of the patch! > I went with the external_ids approach as I thought it was the CMS > integration preferred approach . > Thanks for the discussion, everyone! Regards, Dumitru > Gav > > On Thu, Jun 19, 2025 at 16:20 Dumitru Ceara <dce...@redhat.com> wrote: > >> Hi Numan, Gavin, >> >> On 6/19/25 5:09 PM, Numan Siddique wrote: >>> On Thu, Jun 19, 2025 at 9:21 AM Dumitru Ceara <dce...@redhat.com> wrote: >>>> >>>> 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. >>>> >>> >>> >>> Thanks for the patch Gavin. >>> >>> @Dumitru Ceara Eventually we will be using the ovn bridge controller >>> (or ovn-pr-controller) >>> to control the provider bridge. Maybe we can support adding an OVN >>> bridge port in the "Bridge" table >>> here - >> https://github.com/numansiddique/ovnbr/blob/initial_commits/ovn-br.ovsschema#L69 >>> >>> And CMS can add a Bridge port in the ovnbr database which maps to the >>> localnet port / patch port >>> and it can set the tenant specific metadata. This way we probably >>> don't need this patch ? >>> >> >> Interesting, I hadn't made the link between the "ovn bridge controller" >> proposal and this patch until now. :) >> >> I guess that could work. >> >> Alternatively (or maybe in conjunction), we could create a new explicit >> well documented OVN.NB.Logical_Switch_Port.option that northd would >> propagate to OVN.SB.Port_Binding. >> >> ovn-controller could then reflect that (and only that) in the OVS >> (patch) port associated to the Port_binding. >> >> What do you think? >> >> Thanks, >> Dumitru >> >>> Thanks >>> Numan >>> >>> >>>> 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
