On 6/11/25 02:53, Ales Musil wrote:
On Fri, Jun 6, 2025 at 8:14 PM Mark Michelson via dev
<ovs-dev@openvswitch.org <mailto: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 <http://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
<mailto: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?
It looks like I did not re-run system tests when I made the latest
revision. I'll get those straightened out.
The other failure I noticed was the "neighbor update on same HV" . This
test has been flaky for me lately. I find that there's a about a 50-50
shot the test will succeed or fail.
* 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 <http://ovn-controller.at> | 4 ++++
tests/ovn-northd.at <http://ovn-northd.at> | 4 ++--
tests/ovn.at <http://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.
This is addressed in the commit message "note 2" section, but I can go
into a bit more detail here.
I tried to make the type UUID in the following way:
"nb_uuid": {"type": "uuid"},
I did it this way since the nb_uuid is required and there can be at most
one.
This made two tests in ovn-sbctl.at cause crashes due to ovs_asserts().
The two tests each attempt to feed invalid input into `ovn-sbctl
lflow-list`. As an example, the "invalid 0x flow" test tries `ovn-sbctl
lflow-list 0x12345678` . This is a nonsense hex value that is expected
to fail.
ovn-sbctl calls ctl_get_row() in OVS's db-ctl-base.c using "0x12345678"
as the potential record_id. ctl_get_row() iterates over the rows of
Datapath_Binding to try to find a row that matches. When it reaches the
"nb_uuid" row, it calls get_row_by_id(). This function then asserts
because the "name_type" variable is OVSDB_TYPE_UUID, but the assertion
assumes it is OVSDB_TYPE_STRING. The comment above the block where the
code asserts even says:
/* We only support integer and string names (so far). */
So to be able to use a UUID here would require updating OVS to support
UUID types in get_row_by_id(). Or it may be possible to get around the
assertions using some alternate means of specifying the column, where we
use "key" instead of having the type be "uuid" directly. I could
experiment with that option to see if it works without causing crashes.
"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 <http://ovn-controller.at>
b/tests/ovn-controller.at <http://ovn-controller.at>
index ae7eb6f31..137442262 100644
--- a/tests/ovn-controller.at <http://ovn-controller.at>
+++ b/tests/ovn-controller.at <http://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 <http://ovn-northd.at>
b/tests/ovn-northd.at <http://ovn-northd.at>
index 97eaa6a40..f334698dd 100644
--- a/tests/ovn-northd.at <http://ovn-northd.at>
+++ b/tests/ovn-northd.at <http://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 <http://ovn.at> b/tests/ovn.at <http://ovn.at>
index b561c69aa..f11dcd1f8 100644
--- a/tests/ovn.at <http://ovn.at>
+++ b/tests/ovn.at <http://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 <http://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 <mailto:d...@openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
