On Fri, Jun 6, 2025 at 8:14 PM Mark Michelson via dev <
ovs-dev@openvswitch.org> wrote:
> Prior to this commit, if you wanted to find the corresponding northbound
> UUID for a southbound Logical Datapath, you could find it in one of two
> places:
>
> For logical switches, it was in external-ids:logical-switch.
> For logical routers, it was in external-ids:logical-router.
>
> With this commit, we are separating the type and UUID into separate
> fields. This way, no matter the type of the datapath, you can find the
> UUID. And you can find the type of the datapath without having to
> potentially check multiple external-ids keys to do so.
>
> These fields are going to be used pretty heavily by northd in upcoming
> patches, so instead of making them external-ids, these are now
> full-fledged columns on the southbound Datapath_Binding. The UUID of the
> northbound logical datapath is in a column called "nb_uuid" and the type
> of the logical datapath is stored in an enumerated column called "type".
>
> Note that there is a seemingly-unrelated change in the check packet
> length test in tests/ovn.at. This test started failing because its use
> of `grep` was picking up the new "nb_uuid" external-id accidentally. The
> change to the test uses `fetch_column` since it is more precise.
>
> Note 2: I attempted to make the new "nb_uuid" column type "UUID" but
> this caused an assertion in ovs's db-ctl-base.c file when running the
> ovn-sbctl "invalid 0x flow" and "count-flows wrongDatapath" tests. This
> is because db-ctl-base.c is incapable of handling records other than of
> type "int" or "string". The downside is that we have to use
> uuid_to_string() in order to ensure the UUID is in proper form.
>
> Signed-off-by: Mark Michelson <mmich...@redhat.com>
> ---
>
Hi Mark,
thank you for v7. I have a few more comments down below.
Also there is a lot of CI failures, could you please take a look
at it?
> * v7
> * Rebased.
> * Changed from using external-ids for the nb_uuid and type to using
> new columns in the Datapath_Binding table.
>
> * v6
> * This is the first version of the series to contain this patch.
> ic/ovn-ic.c | 5 +++--
> northd/northd.c | 31 ++++++++++++++++++-------------
> northd/northd.h | 5 +++--
> ovn-sb.ovsschema | 7 +++++--
> ovn-sb.xml | 21 +++++++++++----------
> tests/ovn-controller.at | 4 ++++
> tests/ovn-northd.at | 4 ++--
> tests/ovn.at | 6 ++----
> utilities/ovn-sbctl.c | 4 ++--
> utilities/ovn-trace.c | 3 +--
> 10 files changed, 51 insertions(+), 39 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 0dd079c4e..de5a91b23 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -613,8 +613,7 @@ get_router_uuid_by_sb_pb(struct ic_context *ctx,
> return NULL;
> }
>
> - return smap_get_uuid(&router_pb->datapath->external_ids,
> "logical-router",
> - router_uuid);
> + return uuid_from_string(router_uuid, router_pb->datapath->nb_uuid);
> }
>
> static void
> @@ -2391,6 +2390,8 @@ main(int argc, char *argv[])
> ovsdb_idl_add_table(ovnsb_idl_loop.idl,
> &sbrec_table_datapath_binding);
> ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> &sbrec_datapath_binding_col_external_ids);
> + ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> + &sbrec_datapath_binding_col_nb_uuid);
>
> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_port_binding);
> ovsdb_idl_add_column(ovnsb_idl_loop.idl,
> diff --git a/northd/northd.c b/northd/northd.c
> index 2da94e7e1..c3fa5d3eb 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -580,14 +580,15 @@ ovn_datapath_from_sbrec(const struct hmap
> *ls_datapaths,
> struct uuid key;
> const struct hmap *dps;
>
> - if (smap_get_uuid(&sb->external_ids, "logical-switch", &key)) {
> + if (!strcmp(sb->type, "logical-switch")) {
> dps = ls_datapaths;
> - } else if (smap_get_uuid(&sb->external_ids, "logical-router", &key)) {
> + } else if (!strcmp(sb->type, "logical-router")) {
> dps = lr_datapaths;
> } else {
> return NULL;
> }
> - if (!dps) {
> +
> + if (!uuid_from_string(&key, sb->nb_uuid)) {
> return NULL;
> }
> struct ovn_datapath *od = ovn_datapath_find_(dps, &key);
> @@ -749,11 +750,9 @@ store_mcast_info_for_switch_datapath(const struct
> sbrec_ip_multicast *sb,
> static void
> ovn_datapath_update_external_ids(struct ovn_datapath *od)
> {
> - /* Get the logical-switch or logical-router UUID to set in
> - * external-ids. */
> + /* Get the NB UUID to set in external-ids. */
> char uuid_s[UUID_LEN + 1];
> sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key));
>
The uuid_s isn't actually used for anything.
> - const char *key = od->nbs ? "logical-switch" : "logical-router";
>
> /* Get names to set in external-ids. */
> const char *name = od->nbs ? od->nbs->name : od->nbr->name;
> @@ -765,7 +764,6 @@ ovn_datapath_update_external_ids(struct ovn_datapath
> *od)
>
> /* Set external-ids. */
> struct smap ids = SMAP_INITIALIZER(&ids);
> - smap_add(&ids, key, uuid_s);
> smap_add(&ids, "name", name);
> if (name2 && name2[0]) {
> smap_add(&ids, "name2", name2);
> @@ -893,13 +891,10 @@ join_datapaths(const struct
> nbrec_logical_switch_table *nbrec_ls_table,
> const struct sbrec_datapath_binding *sb;
> SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_SAFE (sb, sbrec_dp_table) {
> struct uuid key;
> - if (!smap_get_uuid(&sb->external_ids, "logical-switch", &key) &&
> - !smap_get_uuid(&sb->external_ids, "logical-router", &key)) {
> + if (!uuid_from_string(&key, sb->nb_uuid)) {
> ovsdb_idl_txn_add_comment(
> ovnsb_txn,
> - "deleting Datapath_Binding "UUID_FMT" that lacks "
> - "external-ids:logical-switch and "
> - "external-ids:logical-router",
> + "deleting Datapath_Binding "UUID_FMT" that lacks nb_uuid",
> UUID_ARGS(&sb->header_.uuid));
> sbrec_datapath_binding_delete(sb);
> continue;
> @@ -909,7 +904,7 @@ join_datapaths(const struct nbrec_logical_switch_table
> *nbrec_ls_table,
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> VLOG_INFO_RL(
> &rl, "deleting Datapath_Binding "UUID_FMT" with "
> - "duplicate external-ids:logical-switch/router "UUID_FMT,
> + "duplicate nb_uuid "UUID_FMT,
> UUID_ARGS(&sb->header_.uuid), UUID_ARGS(&key));
> sbrec_datapath_binding_delete(sb);
> continue;
> @@ -1117,11 +1112,21 @@ build_datapaths(struct ovsdb_idl_txn *ovnsb_txn,
> sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key);
> }
> ovn_datapath_update_external_ids(od);
> + char uuid_s[UUID_LEN + 1];
> + sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key));
> + sbrec_datapath_binding_set_nb_uuid(od->sb, uuid_s);
> + sbrec_datapath_binding_set_type(od->sb, od->nbs ?
> "logical-switch" :
> + "logical-router");
> }
> LIST_FOR_EACH (od, list, &nb_only) {
> od->sb = sbrec_datapath_binding_insert(ovnsb_txn);
> ovn_datapath_update_external_ids(od);
> sbrec_datapath_binding_set_tunnel_key(od->sb, od->tunnel_key);
> + char uuid_s[UUID_LEN + 1];
> + sprintf(uuid_s, UUID_FMT, UUID_ARGS(&od->key));
> + sbrec_datapath_binding_set_nb_uuid(od->sb, uuid_s);
> + sbrec_datapath_binding_set_type(od->sb, od->nbs ?
> "logical-switch" :
> + "logical-router");
> }
> ovn_destroy_tnlids(&dp_tnlids);
>
> diff --git a/northd/northd.h b/northd/northd.h
> index fc138aab5..998ee8090 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -346,7 +346,7 @@ DRR_MODES
> #undef DRR_MODE
>
> /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or
> - * sb->external_ids:logical-switch. */
> + * sb->nb_uuid. */
> struct ovn_datapath {
> struct hmap_node key_node; /* Index on 'key'. */
> struct uuid key; /* (nbs/nbr)->header_.uuid. */
> @@ -448,7 +448,8 @@ ovn_datapath_is_stale(const struct ovn_datapath *od)
> /* The two purposes for which ovn-northd uses OVN logical datapaths. */
> enum ovn_datapath_type {
> DP_SWITCH, /* OVN logical switch. */
> - DP_ROUTER /* OVN logical router. */
> + DP_ROUTER, /* OVN logical router. */
> + DP_MAX,
> };
>
> /* Returns an "enum ovn_stage" built from the arguments.
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index 4c24f5b51..a020a5c1c 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> - "version": "21.2.0",
> - "cksum": "29145795 34859",
> + "version": "21.3.0",
> + "cksum": "3112866017 35066",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -196,6 +196,9 @@
> "load_balancers": {"type": {"key": {"type": "uuid"},
> "min": 0,
> "max": "unlimited"}},
> + "type": {"type": {"key": {"type": "string",
> + "enum": ["set",
> ["logical-switch", "logical-router"]]}}},
> + "nb_uuid": {"type": "string"},
>
Why not use the type "uuid" directly? We could avoid the extra parsing.
"external_ids": {
> "type": {"key": "string", "value": "string",
> "min": 0, "max": "unlimited"}}},
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index db5faac66..ad09a472e 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -3236,18 +3236,19 @@ tcp.flags = RST;
> db="OVN_Northbound"/> database.
> </p>
>
> - <column name="external_ids" key="logical-switch" type='{"type":
> "uuid"}'>
> - For a logical datapath that represents a logical switch,
> - <code>ovn-northd</code> stores in this key the UUID of the
> - corresponding <ref table="Logical_Switch" db="OVN_Northbound"/>
> row in
> - the <ref db="OVN_Northbound"/> database.
> + <column name="type" type='{"type": "string"}'>
> + This represents the type of the northbound logical datapath that
> is
> + represented by this southbound <code>Datapath_Binding</code>. The
> + possible values for this are:
> + <ul>
> + <li><code>logical-switch</code></li>
> + <li><code>logical-router</code></li>
> + </ul>
> </column>
>
> - <column name="external_ids" key="logical-router" type='{"type":
> "uuid"}'>
> - For a logical datapath that represents a logical router,
> - <code>ovn-northd</code> stores in this key the UUID of the
> - corresponding <ref table="Logical_Router" db="OVN_Northbound"/>
> row in
> - the <ref db="OVN_Northbound"/> database.
> + <column name="nb_uuid" type='{"type": "string"}'>
> + This is the UUID of the corresponding northbound logical datapath
> + represented by this southbound <code>Datapath_Binding</code>.
> </column>
>
> <column name="external_ids" key="interconn-ts" type='{"type":
> "string"}'>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index ae7eb6f31..137442262 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -123,6 +123,7 @@ check_patches
> # another logical port bound to this chassis.
> check_uuid ovn-sbctl \
> -- --id=@dp101 create Datapath_Binding tunnel_key=101
> external_ids:name=dp101 \
> + type=logical-switch \
> -- create Port_Binding datapath=@dp101 logical_port=localnet1
> tunnel_key=1 \
> type=localnet options:network_name=physnet1
> check_patches
> @@ -131,6 +132,7 @@ check_patches
> # Now we should get some patch ports created.
> check_uuid ovn-sbctl \
> -- --id=@dp102 create Datapath_Binding tunnel_key=102
> external_ids:name=dp102 \
> + type=logical-switch \
> -- create Port_Binding datapath=@dp102 logical_port=localnet2
> tunnel_key=1 \
> type=localnet options:network_name=physnet1 \
> -- create Port_Binding datapath=@dp102 logical_port=localvif2
> tunnel_key=2
> @@ -145,7 +147,9 @@ check_patches \
> # the set of OVS patch ports doesn't change.
> AT_CHECK([ovn-sbctl \
> -- --id=@dp1 create Datapath_Binding tunnel_key=1
> external_ids:name=dp1 \
> + type=logical-switch \
> -- --id=@dp2 create Datapath_Binding tunnel_key=2
> external_ids:name=dp2 \
> + type=logical-switch \
> -- create Port_Binding datapath=@dp1 logical_port=foo tunnel_key=1
> type=patch options:peer=bar \
> -- create Port_Binding datapath=@dp2 logical_port=bar tunnel_key=2
> type=patch options:peer=foo \
> -- create Port_Binding datapath=@dp1 logical_port=dp1vif tunnel_key=3
> \
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 97eaa6a40..f334698dd 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1887,7 +1887,7 @@ check ovn-nbctl --wait=sb \
>
> check_row_count Datapath_Binding 1
>
> -nb_uuid=$(ovn-sbctl get Datapath_Binding . external_ids:logical-router)
> +nb_uuid=$(ovn-sbctl get Datapath_Binding . nb_uuid)
> lr_uuid=\"$(ovn-nbctl get Logical_Router . _uuid)\"
> echo nb_uuid="$nb_uuid" lr_uuid="$lr_uuid"
> AT_CHECK([test "${nb_uuid}" = "${lr_uuid}"])
> @@ -7574,7 +7574,7 @@ ls1_uuid=$(fetch_column nb:Logical_Switch _uuid)
>
> # create a duplicated sb datapath (and an IP_Mulicast record that
> references
> # it) on purpose.
> -AT_CHECK([ovn-sbctl --id=@dp create Datapath_Binding
> external_ids:logical-switch=$ls1_uuid external_ids:name=ls1 tunnel_key=123
> -- create IP_Multicast datapath=@dp], [0], [ignore])
> +AT_CHECK([ovn-sbctl --id=@dp create Datapath_Binding nb_uuid=$ls1_uuid
> type=logical-switch external_ids:name=ls1 tunnel_key=123 -- create
> IP_Multicast datapath=@dp], [0], [ignore])
>
> # northd should delete one of the datapaths in the end
> wait_row_count Datapath_Binding 1
> diff --git a/tests/ovn.at b/tests/ovn.at
> index b561c69aa..f11dcd1f8 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -22193,8 +22193,7 @@ AT_CAPTURE_FILE([sbflows])
> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int \
> | grep "check_pkt_larger" | wc -l], [0], [[0
> ]])
> -dp_uuid=$(ovn-sbctl find datapath_binding | grep lr0 -B2 | grep _uuid | \
> -awk '{print $3}')
> +dp_uuid=$(fetch_column Datapath_Binding _uuid external-ids:name=lr0)
> check_uuid ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \
> logical_port=lr0-public mac="00\:00\:00\:12\:af\:11"
>
> @@ -22264,8 +22263,7 @@ check ovn-nbctl lr-nat-add lr1 snat 172.168.0.100
> 10.0.0.0/24
> check ovn-nbctl lr-nat-add lr1 snat 2000::1 1000::/64
> check ovn-nbctl --wait=sb sync
>
> -dp_uuid=$(ovn-sbctl find datapath_binding | grep lr1 -B2 | grep _uuid | \
> -awk '{print $3}')
> +dp_uuid=$(fetch_column Datapath_Binding _uuid external-ids:name=lr1)
> check_uuid ovn-sbctl create MAC_Binding ip=172.168.0.3 datapath=$dp_uuid \
> logical_port=lr1-public mac="00\:00\:00\:12\:af\:11"
>
> diff --git a/utilities/ovn-sbctl.c b/utilities/ovn-sbctl.c
> index 60cc39149..9c7be5e57 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -1493,8 +1493,8 @@ static const struct ctl_table_class
> tables[SBREC_N_TABLES] = {
> [SBREC_TABLE_DATAPATH_BINDING].row_ids
> = {{&sbrec_datapath_binding_col_external_ids, "name", NULL},
> {&sbrec_datapath_binding_col_external_ids, "name2", NULL},
> - {&sbrec_datapath_binding_col_external_ids, "logical-switch",
> NULL},
> - {&sbrec_datapath_binding_col_external_ids, "logical-router",
> NULL}},
> + {&sbrec_datapath_binding_col_nb_uuid, NULL, NULL},
> + {&sbrec_datapath_binding_col_type, NULL, NULL}},
>
> [SBREC_TABLE_PORT_BINDING].row_ids
> = {{&sbrec_port_binding_col_logical_port, NULL, NULL},
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 0331eac21..648a1d43e 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -703,8 +703,7 @@ read_datapaths(void)
> const struct smap *ids = &sbdb->external_ids;
>
> dp->sb_uuid = sbdb->header_.uuid;
> - if (!smap_get_uuid(ids, "logical-switch", &dp->nb_uuid) &&
> - !smap_get_uuid(ids, "logical-router", &dp->nb_uuid)) {
> + if (!uuid_from_string(&dp->nb_uuid, sbdb->nb_uuid)) {
> dp->nb_uuid = dp->sb_uuid;
> }
>
> --
> 2.47.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
