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
