On Thu, Jun 19, 2025 at 11:20 AM Dumitru Ceara via dev < ovs-dev@openvswitch.org> 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? > Sounds good to me. @Gavin McKee <gmc...@crusoe.ai> What do you think ? Numan 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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
