On Fri, May 30, 2025 at 4:58 AM 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 changing the external-ids so that the
> northbound UUID and the type correspond to separate keys. The
> external-ids:type corresponds to an enum value for logical routers and
> logical switches. The external-ids:nb_uuid gets the northbound UUID of
> the logical switch or logical router. And for human readability, we also
> are providing a external-ids:type_str that will be "logical-router" for
> logical routers and "logical-switch" for logical switches.
>
> 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.
>
> Signed-off-by: Mark Michelson <mmich...@redhat.com>
> ---
> * v6
> * This is the first version of the series to contain this patch.
> ---
>
Hi Mark,
thank you for the patch, I had something slightly different in mind.
We should use a separate column for type as we do in Port_Binding.
Let's be a bit more proactive and make it also an enum with
supported variants. The downside is that we need to parse it
with strcmp.
It might actually make sense to have a separate column called
nb_uuid, it needs to be set for each row anyway and it would
make parsing easier, WDYT?
ic/ovn-ic.c | 2 +-
> northd/northd.c | 28 ++++++++++++++++------------
> northd/northd.h | 5 +++--
> tests/ovn-northd.at | 4 ++--
> tests/ovn.at | 6 ++----
> utilities/ovn-sbctl.c | 4 ++--
> utilities/ovn-trace.c | 3 +--
> 7 files changed, 27 insertions(+), 25 deletions(-)
>
> diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
> index 0dd079c4e..6b2fd289b 100644
> --- a/ic/ovn-ic.c
> +++ b/ic/ovn-ic.c
> @@ -613,7 +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",
> + return smap_get_uuid(&router_pb->datapath->external_ids, "nb_uuid",
> router_uuid);
> }
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 2da94e7e1..9e9ed5a30 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -580,14 +580,17 @@ 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)) {
> + enum ovn_datapath_type dp_type;
> + dp_type = smap_get_int(&sb->external_ids, "type", DP_MAX);
> + if (dp_type == DP_SWITCH) {
> dps = ls_datapaths;
> - } else if (smap_get_uuid(&sb->external_ids, "logical-router", &key)) {
> + } else if (dp_type == DP_ROUTER) {
> dps = lr_datapaths;
> } else {
> return NULL;
> }
> - if (!dps) {
> +
> + if (!smap_get_uuid(&sb->external_ids, "nb_uuid", &key)) {
> return NULL;
> }
> struct ovn_datapath *od = ovn_datapath_find_(dps, &key);
> @@ -749,11 +752,12 @@ 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";
> + enum ovn_datapath_type dp_type = od->nbs ? DP_SWITCH : DP_ROUTER;
> + const char *dp_type_str =
> + dp_type == DP_SWITCH ? "logical-switch" : "logical-router";
>
> /* Get names to set in external-ids. */
> const char *name = od->nbs ? od->nbs->name : od->nbr->name;
> @@ -765,7 +769,9 @@ 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, "nb_uuid", uuid_s);
> + smap_add_format(&ids, "type", "%d", dp_type);
> + smap_add(&ids, "type_str", dp_type_str);
> smap_add(&ids, "name", name);
> if (name2 && name2[0]) {
> smap_add(&ids, "name2", name2);
> @@ -893,13 +899,11 @@ 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 (!smap_get_uuid(&sb->external_ids, "nb_uuid", &key)) {
> ovsdb_idl_txn_add_comment(
> ovnsb_txn,
> "deleting Datapath_Binding "UUID_FMT" that lacks "
> - "external-ids:logical-switch and "
> - "external-ids:logical-router",
> + "external-ids:nb_uuid",
> UUID_ARGS(&sb->header_.uuid));
> sbrec_datapath_binding_delete(sb);
> continue;
> @@ -909,7 +913,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 external-ids:nb_uuid "UUID_FMT,
> UUID_ARGS(&sb->header_.uuid), UUID_ARGS(&key));
> sbrec_datapath_binding_delete(sb);
> continue;
> diff --git a/northd/northd.h b/northd/northd.h
> index fc138aab5..ebcfdfb90 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->external_ids: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/tests/ovn-northd.at b/tests/ovn-northd.at
> index 4a676e5e4..c2b1c224a 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 . external_ids: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
> external_ids:nb_uuid=$ls1_uuid external_ids:type=0 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 b2ac0e33f..cb650da6b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -22214,8 +22214,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"
>
> @@ -22285,8 +22284,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..6929c2724 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_external_ids, "nb_uuid", NULL},
> + {&sbrec_datapath_binding_col_external_ids, "type_str", 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..2c2c39d3c 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 (!smap_get_uuid(ids, "nb_uuid", &dp->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
