On 8/13/25 10:27 AM, Dumitru Ceara wrote:
> Hi Mark,
>
> On 8/12/25 8:33 PM, Mark Michelson via dev wrote:
>> On 8/11/25 12:00 PM, Numan Siddique wrote:
>>> On Mon, Aug 11, 2025 at 8:29 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>
>>>> Acked-by: Ales Musil <amu...@redhat.com>
>>>
>>> Hi Mark,
>>>
>>> Thanks for this series. I had a very quick look into some of the
>>> patches of this series.
>>> I like the approach taken in this series.
>>>
>>> I've a couple of small comments in this first patch.
>>>
>>> 1. Is it possible to add a test case in ovn-northd.at which checks
>>> that the datapath type and uuid
>>> in the SB DB is created properly by ovn-northd when the NB logical
>>> switches and routers are created ?
>>>
>>> (Please see below for the 2nd comment)
>>>
>>>
>>>> ---
>>>> v14
>>>> * Rebased.
>>>>
>>>> v13
>>>> * Rebased.
>>>>
>>>> v12
>>>> * Rebased.
>>>> * Added Ales Musil's ack.
>>>>
>>>> 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 ab5c4fc47..44a1082d4 100644
>>>> --- a/lib/ovn-util.c
>>>> +++ b/lib/ovn-util.c
>>>> @@ -1503,3 +1503,48 @@ put_load(uint64_t value, enum mf_field_id dst,
>>>> size_t ofs, size_t n_bits,
>>>> ovs_be64 n_value = htonll(value);
>>>> put_load_bytes(&n_value, 8, dst, ofs, n_bits, ofpacts);
>>>> }
>>>> +
>>>> +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 bde40666a..18ebea912 100644
>>>> --- a/lib/ovn-util.h
>>>> +++ b/lib/ovn-util.h
>>>> @@ -546,4 +546,12 @@ void put_load(uint64_t value, enum mf_field_id
>>>> dst, size_t ofs, size_t n_bits,
>>>> #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 2cb69f9aa..241af1d3c 100644
>>>> --- a/northd/northd.c
>>>> +++ b/northd/northd.c
>>>> @@ -608,19 +608,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;
>>>> @@ -768,11 +770,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;
>>>> @@ -784,7 +784,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);
>>>> @@ -912,13 +911,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;
>>>> @@ -928,13 +924,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);
>>>> }
>>>> @@ -1140,11 +1136,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 1108793d7..d3b268129 100644
>>>> --- a/northd/northd.h
>>>> +++ b/northd/northd.h
>>>> @@ -351,7 +351,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. */
>>>> @@ -454,7 +454,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"]]}}},
>>>
>>> 2. Instead of using type string here for the "type", how about we
>>> use "integer" something like ?
>>>
>>> ---
>>> "type": {"type": {"key": {"type": "integer",
>>> "enum": ["set",
>>> [1, 2]]}}},
>>>
>>> ----
>>>
>>> This way we can avoid using "strcmp" in the code base. type = 1 is
>>> for logical switch and 2 for router.
>>>
>>> What do you think ?
>>>
>>> Thanks
>>> Numan
>>
>> I like that. What I'd probably do is align this with the existing enum
>> ovn_datapath_type.
>>
>> enum ovn_datapath_type {
>> DP_SWITCH, /* OVN logical switch. */
>> DP_ROUTER /* OVN logical router. */
>> };
>>
>> Therefore, logical switch would be 0, and logical router would be 1.
>>
>
> This is a bit unfortunate, but the change to '"enum": ["set", [1, 2]]'
> breaks ovn-kubernetes builds:
>
> https://patchwork.ozlabs.org/project/ovn/patch/20250812230233.63769-9-mmich...@redhat.com/
>
> https://mail.openvswitch.org/pipermail/ovs-build/2025-August/048140.html
>
> https://github.com/ovsrobot/ovn/actions/runs/16923415749/job/47954327716#step:7:9386
>
> modelgen: template: :174:27: executing "enums" at <.>: wrong type for
> value; expected string; got float64
>
> This is a bug in ovn-kubernetes' modelgen but has the annoying side
> effect that we cannot run any ovn-kubernetes CI tests with ovn code
> until that's fixed.
>
I opened a new usptream libovsdb issue for this:
https://github.com/ovn-kubernetes/libovsdb/issues/436
> IMO it's fine to leave enum value as "string" as it was in v14, the
> strcmps we do in northd should not matter too much performance-wise. In
> my eyes, another advantage of using strings is that it makes it easier
> for humans looking at the database contents directly to figure out what
> that value means.
>
> Regards,
> Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
