On Fri, Jul 18, 2025 at 6:15 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" column accidentally. The
> change to the test uses `fetch_column` since it is more precise.
>
> Signed-off-by: Mark Michelson <mmich...@redhat.com>
> ---
> v11
> * Rebased.
>
> v10
> * Rebased.
> * Maintained backwards-compatibility with external_ids method of
> storing NB UUID and type. It was recommended to continue setting
> the external-ids in addition to the new fields. This patch does not
> do that because that code would get immediately wiped out by the
> next patch in the series. The next patch in the series does write
> the old external_ids values, though.
>
> v9
> * Rebased.
>
> v8
> * Rebased.
> * Changed the types of the new columns. The nb_uuid is type "uuid" and
> the type is an enum.
>
> 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.
> ---
> controller/local_data.c | 2 +-
> ic/ovn-ic.c | 5 +++--
> lib/ovn-util.c | 45 +++++++++++++++++++++++++++++++++++++++++
> lib/ovn-util.h | 8 ++++++++
> northd/northd.c | 38 +++++++++++++++++-----------------
> northd/northd.h | 5 +++--
> ovn-sb.ovsschema | 11 ++++++++--
> ovn-sb.xml | 21 ++++++++++---------
> tests/ovn-controller.at | 4 ++++
> tests/ovn-northd.at | 6 +++---
> tests/ovn.at | 6 ++----
> utilities/ovn-sbctl.c | 4 ++--
> utilities/ovn-trace.c | 3 +--
> 13 files changed, 112 insertions(+), 46 deletions(-)
>
> diff --git a/controller/local_data.c b/controller/local_data.c
> index 00383d091..a35d3fa5a 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -708,7 +708,7 @@ local_datapath_peer_port_add(struct local_datapath *ld,
> static bool
> datapath_is_switch(const struct sbrec_datapath_binding *ldp)
> {
> - return smap_get(&ldp->external_ids, "logical-switch") != NULL;
> + return strcmp(datapath_get_nb_type(ldp), "logical-switch") == 0;
> }
>
> static bool
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index caffa6fe0..0cd78e720 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 datapath_get_nb_uuid(router_pb->datapath, router_uuid);
> }
>
> static void
> @@ -2582,6 +2581,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/lib/ovn-util.c b/lib/ovn-util.c
> index 9f0bc99ba..abfff0890 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -1484,3 +1484,48 @@ ovn_mirror_port_name(const char *datapath_name,
> {
> return xasprintf("mp-%s-%s", datapath_name, port_name);
> }
> +
> +bool
> +datapath_get_nb_uuid_and_type(const struct sbrec_datapath_binding *sb,
> + struct uuid *nb_uuid, const char **type)
> +{
> + if (sb->nb_uuid) {
> + /* New style. The UUID and type are direct columns, so use those.
> */
> + *nb_uuid = *sb->nb_uuid;
> + *type = sb->type;
> + return true;
> + }
> +
> + /* Old style. The UUID is stored in external_ids, and the key
> + * corresponds to the datapath type. This only works with
> + * logical switches and logical routers.
> + */
> + *type = "logical-switch";
> + if (smap_get_uuid(&sb->external_ids, *type, nb_uuid)) {
> + return true;
> + }
> + *type = "logical-router";
> + if (smap_get_uuid(&sb->external_ids, *type, nb_uuid)) {
> + return true;
> + }
> + *type = "";
> + *nb_uuid = UUID_ZERO;
> + return false;
> +}
> +
> +bool
> +datapath_get_nb_uuid(const struct sbrec_datapath_binding *sb,
> + struct uuid *nb_uuid)
> +{
> + const char *type;
> + return datapath_get_nb_uuid_and_type(sb, nb_uuid, &type);
> +}
> +
> +const char *
> +datapath_get_nb_type(const struct sbrec_datapath_binding *sb)
> +{
> + const char *type;
> + struct uuid nb_uuid;
> + datapath_get_nb_uuid_and_type(sb, &nb_uuid, &type);
> + return type;
> +}
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 2468a6de4..695058f90 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -540,4 +540,12 @@ const struct sbrec_port_binding *lport_lookup_by_name(
> #define _VFUNC(name, n) _VFUNC_(name, n)
> #define VFUNC(func, ...) _VFUNC(func, __NARG__(__VA_ARGS__)) (__VA_ARGS__)
>
> +bool datapath_get_nb_uuid_and_type(const struct sbrec_datapath_binding
> *sb,
> + struct uuid *nb_uuid, const char
> **type);
> +
> +bool datapath_get_nb_uuid(const struct sbrec_datapath_binding *sb,
> + struct uuid *nb_uuid);
> +
> +const char *datapath_get_nb_type(const struct sbrec_datapath_binding *sb);
> +
> #endif /* OVN_UTIL_H */
> diff --git a/northd/northd.c b/northd/northd.c
> index b8ca445bc..f39ff77ca 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -577,19 +577,21 @@ ovn_datapath_from_sbrec(const struct hmap
> *ls_datapaths,
> const struct hmap *lr_datapaths,
> const struct sbrec_datapath_binding *sb)
> {
> - struct uuid key;
> const struct hmap *dps;
> + struct uuid key;
> + const char *type;
> + if (!datapath_get_nb_uuid_and_type(sb, &key, &type)) {
> + return NULL;
> + }
>
> - if (smap_get_uuid(&sb->external_ids, "logical-switch", &key)) {
> + if (!strcmp(type, "logical-switch")) {
> dps = ls_datapaths;
> - } else if (smap_get_uuid(&sb->external_ids, "logical-router", &key)) {
> + } else if (!strcmp(type, "logical-router")) {
> dps = lr_datapaths;
> } else {
> return NULL;
> }
> - if (!dps) {
> - return NULL;
> - }
> +
> struct ovn_datapath *od = ovn_datapath_find_(dps, &key);
> if (od && (od->sb == sb)) {
> return od;
> @@ -749,11 +751,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));
> - 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 +765,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 +892,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 (!datapath_get_nb_uuid(sb, &key)) {
> 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,13 +905,13 @@ 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,
> - UUID_ARGS(&sb->header_.uuid), UUID_ARGS(&key));
> + "duplicate nb_uuid "UUID_FMT,
> + UUID_ARGS(&sb->header_.uuid), UUID_ARGS(sb->nb_uuid));
> sbrec_datapath_binding_delete(sb);
> continue;
> }
>
> - struct ovn_datapath *od = ovn_datapath_create(datapaths, &key,
> + struct ovn_datapath *od = ovn_datapath_create(datapaths,
> sb->nb_uuid,
> NULL, NULL, sb);
> ovs_list_push_back(sb_only, &od->list);
> }
> @@ -1117,11 +1113,17 @@ 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);
> + sbrec_datapath_binding_set_nb_uuid(od->sb, &od->key, 1);
> + 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);
> + sbrec_datapath_binding_set_nb_uuid(od->sb, &od->key, 1);
> + 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 eacff73eb..2b2fedec5 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -347,7 +347,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. */
> @@ -449,7 +449,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..8bc7bbba3 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": "3179330761 35286",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -196,6 +196,13 @@
> "load_balancers": {"type": {"key": {"type": "uuid"},
> "min": 0,
> "max": "unlimited"}},
> + "type": {"type": {"key": {"type": "string",
> + "enum": ["set",
> + ["logical-switch",
> +
> "logical-router"]]}}},
> + "nb_uuid": {"type": {"key": {"type": "uuid"},
> + "min": 0,
> + "max": 1}},
> "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 6007a7454..4e73dcdcf 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 8204a5cc3..58d8e66e9 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -1893,8 +1893,8 @@ check ovn-nbctl --wait=sb \
>
> check_row_count Datapath_Binding 1
>
> -nb_uuid=$(ovn-sbctl get Datapath_Binding . external_ids:logical-router)
> -lr_uuid=\"$(ovn-nbctl get Logical_Router . _uuid)\"
> +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}"])
>
> @@ -7582,7 +7582,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 108de260e..a6c7ae359 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -22194,8 +22194,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"
>
> @@ -22265,8 +22264,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..74f759699 100644
> --- a/utilities/ovn-sbctl.c
> +++ b/utilities/ovn-sbctl.c
> @@ -382,6 +382,7 @@ pre_get_info(struct ctl_context *ctx)
> ovsdb_idl_add_column(ctx->idl, &sbrec_logical_dp_group_col_datapaths);
>
> ovsdb_idl_add_column(ctx->idl,
> &sbrec_datapath_binding_col_external_ids);
> + ovsdb_idl_add_column(ctx->idl, &sbrec_datapath_binding_col_nb_uuid);
>
> ovsdb_idl_add_column(ctx->idl, &sbrec_ip_multicast_col_datapath);
> ovsdb_idl_add_column(ctx->idl, &sbrec_ip_multicast_col_seq_no);
> @@ -1493,8 +1494,7 @@ 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_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..40acf4a64 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 (!datapath_get_nb_uuid(sbdb, &dp->nb_uuid)) {
> dp->nb_uuid = dp->sb_uuid;
> }
>
> --
> 2.49.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.
Acked-by: Ales Musil <amu...@redhat.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
