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.
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
