Commit 6919992d8781 changed ovn-northd to sync the NB logical
datapath UUID with the corresponding SB Datapath_Binding. They thus
would have the same UUID. This causes issues on upgrades since it
involves having to essentially recreate all southbound datapath
bindings, as well as all records that refer to those southbound datapath
bindings.
This commit changes so that instead of having the same UUID, the
southbound datapath_binding has a new nb_uuid column that contains the
UUID of the northbound logical datapath. This way, there is still a
simple way to link the Datapath_Binding to the northbound logical
datapath, but there is no need to have to update pre-existing
Datapath_Bindings to have new UUIDs.
Commit 6919992d8781 also added a new "type" field to the
Datapath_Binding table. This commit also updates the "type" field
to be optional. This is to assist with databases that are in
active-backup mode. Databases with "old" style Datapath_Binding records
would not have a "type" set. This would cause schema constraint issues
with the backup database:
unexpected ovsdb error: constraint violation: ""
is not one of the allowed values ([logical-router, logical-switch])
This commit changes the "type" field to be optional.
Fixes: 6919992d8781 ("Datapath_Binding: Separate type column and sync NB.UUID
to SB.")
Co-authored-by: Dumitru Ceara <[email protected]>
Signed-off-by: Mark Michelson <[email protected]>
---
ic/ovn-ic.c | 2 ++
lib/ovn-util.c | 8 ++++----
northd/en-datapath-sync.c | 28 +++++++++++++++++++---------
ovn-sb.ovsschema | 8 ++++++--
ovn-sb.xml | 5 +++++
tests/ovn-northd.at | 22 +++++++++++++++++++++-
tests/ovn.at | 6 ++----
utilities/ovn-sbctl.c | 33 ++++++++++++---------------------
8 files changed, 71 insertions(+), 41 deletions(-)
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c
index fa4a6b118..ac8eb200f 100644
--- a/ic/ovn-ic.c
+++ b/ic/ovn-ic.c
@@ -3021,6 +3021,8 @@ main(int argc, char *argv[])
ovsdb_idl_add_column(ovnsb_idl_loop.idl, &sbrec_datapath_binding_col_type);
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 8b583fa6d..da8a4e4c6 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -1522,10 +1522,10 @@ bool
datapath_get_nb_uuid_and_type(const struct sbrec_datapath_binding *sb,
struct uuid *nb_uuid, const char **type)
{
- if (sb->type && *sb->type) {
- /* New style. The UUID is propagated from the NB logical datapath
- * and the type is a direct column, so use those. */
- *nb_uuid = sb->header_.uuid;
+ if (sb->nb_uuid && sb->type && *sb->type) {
+ /* New style. The UUID and the type are direct columns,
+ * so use those. */
+ *nb_uuid = *sb->nb_uuid;
*type = sb->type;
return true;
}
diff --git a/northd/en-datapath-sync.c b/northd/en-datapath-sync.c
index 3044f695d..9c7dcd402 100644
--- a/northd/en-datapath-sync.c
+++ b/northd/en-datapath-sync.c
@@ -95,9 +95,20 @@ find_synced_datapath_from_sb(const struct hmap *datapaths,
const struct sbrec_datapath_binding *sb_dp)
{
struct ovn_synced_datapath *sdp;
- uint32_t hash = uuid_hash(&sb_dp->header_.uuid);
+ struct uuid nb_uuid;
+
+ /* Don't reference sb_dp->nb_uuid directly here. The nb_uuid column was
+ * a later addition to the Datapath_Binding table. We might be
+ * referencing a record that does not have this column set, so use
+ * the helper function instead.
+ */
+ if (!datapath_get_nb_uuid(sb_dp, &nb_uuid)) {
+ return NULL;
+ }
+
+ uint32_t hash = uuid_hash(&nb_uuid);
HMAP_FOR_EACH_WITH_HASH (sdp, hmap_node, hash, datapaths) {
- if (uuid_equals(&sdp->nb_row->uuid, &sb_dp->header_.uuid)) {
+ if (uuid_equals(&sdp->nb_row->uuid, sb_dp->nb_uuid)) {
return sdp;
}
}
@@ -134,6 +145,7 @@ synced_datapath_set_sb_fields(const struct
sbrec_datapath_binding *sb_dp,
sbrec_datapath_binding_set_external_ids(sb_dp, &udp->external_ids);
sbrec_datapath_binding_set_type(sb_dp,
ovn_datapath_type_to_string(udp->type));
+ sbrec_datapath_binding_set_nb_uuid(sb_dp, &udp->nb_row->uuid, 1);
}
static void
@@ -214,8 +226,7 @@ create_synced_datapath_candidates_from_nb(
continue;
}
struct sbrec_datapath_binding *sb_dp;
- sb_dp = sbrec_datapath_binding_insert_persist_uuid(
- ovnsb_idl_txn, &udp->nb_row->uuid);
+ sb_dp = sbrec_datapath_binding_insert(ovnsb_idl_txn);
struct candidate_sdp candidate = {
.sdp = synced_datapath_alloc(udp, sb_dp, true),
.requested_tunnel_key = udp->requested_tunnel_key,
@@ -248,7 +259,7 @@ assign_requested_tunnel_keys(struct vector *candidate_sdps,
sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp,
candidate->requested_tunnel_key);
hmap_insert(&synced_datapaths->synced_dps, &candidate->sdp->hmap_node,
- uuid_hash(&candidate->sdp->sb_dp->header_.uuid));
+ uuid_hash(candidate->sdp->sb_dp->nb_uuid));
candidate->tunnel_key_assigned = true;
}
}
@@ -270,7 +281,7 @@ assign_existing_tunnel_keys(struct vector *candidate_sdps,
candidate->existing_tunnel_key)) {
hmap_insert(&synced_datapaths->synced_dps,
&candidate->sdp->hmap_node,
- uuid_hash(&candidate->sdp->sb_dp->header_.uuid));
+ uuid_hash(candidate->sdp->sb_dp->nb_uuid));
candidate->tunnel_key_assigned = true;
}
}
@@ -297,7 +308,7 @@ allocate_tunnel_keys(struct vector *candidate_sdps,
sbrec_datapath_binding_set_tunnel_key(candidate->sdp->sb_dp,
tunnel_key);
hmap_insert(&synced_datapaths->synced_dps, &candidate->sdp->hmap_node,
- uuid_hash(&candidate->sdp->sb_dp->header_.uuid));
+ uuid_hash(candidate->sdp->sb_dp->nb_uuid));
candidate->tunnel_key_assigned = true;
}
}
@@ -372,8 +383,7 @@ datapath_sync_unsynced_datapath_handler(
}
struct sbrec_datapath_binding *sb_dp =
- sbrec_datapath_binding_insert_persist_uuid(ovnsb_idl_txn,
- &udp->nb_row->uuid);
+ sbrec_datapath_binding_insert(ovnsb_idl_txn);
sbrec_datapath_binding_set_tunnel_key(sb_dp, tunnel_key);
sdp = synced_datapath_alloc(udp, sb_dp, true);
synced_datapath_set_sb_fields(sb_dp, udp);
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index f64cb99dd..8a890b27b 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@
{
"name": "OVN_Southbound",
"version": "21.4.0",
- "cksum": "812831561 35225",
+ "cksum": "317670898 35437",
"tables": {
"SB_Global": {
"columns": {
@@ -199,7 +199,11 @@
"type": {"type": {"key": {"type": "string",
"enum": ["set",
["logical-switch",
- "logical-router"]]}}},
+ "logical-router"]]},
+ "min": 0, "max": 1}},
+ "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 4b563c5f1..ab647f522 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -3246,6 +3246,11 @@ tcp.flags = RST;
</ul>
</column>
+ <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"}'>
For a logical datapath that represents a logical switch that represents
a transit switch for interconnection, <code>ovn-northd</code> stores in
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index b2b9f092c..bbcf547f9 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -1893,7 +1893,7 @@ check ovn-nbctl --wait=sb \
check_row_count Datapath_Binding 1
-nb_uuid=$(ovn-sbctl get Datapath_Binding . _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}"])
@@ -7583,6 +7583,26 @@ AT_CHECK([grep -E "ls_.*fdb.*S1-" S1flows |
ovn_strip_lflows], [0], [dnl
AT_CLEANUP
])
+# Duplicated datapaths shouldn't be created, but in case it is created because
+# of bug or dirty data, it should be properly deleted instead of causing
+# permanent failure in northd.
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([handling duplicated datapaths])
+ovn_start
+
+check ovn-nbctl --wait=sb ls-add ls1
+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 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
+
+AT_CLEANUP
+])
+
OVN_FOR_EACH_NORTHD_NO_HV([
AT_SETUP([conntrack nat implies conntrack])
ovn_start
diff --git a/tests/ovn.at b/tests/ovn.at
index 81fbfc629..6b49f0583 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -22239,8 +22239,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"
@@ -22310,8 +22309,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 cc3151e78..0bba8f688 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);
@@ -1090,27 +1091,15 @@ cmd_lflow_list(struct ctl_context *ctx)
if (ctx->argc > 1) {
const struct ovsdb_idl_row *row;
- struct uuid nb_uuid;
- bool is_uuid = uuid_from_string(&nb_uuid, ctx->argv[1]);
- if (is_uuid) {
- datapath = sbrec_datapath_binding_get_for_uuid(ctx->idl, &nb_uuid);
- if (!datapath) {
- ctl_error(ctx, "No such logical datapath with UUID "
- UUID_FMT".",
- UUID_ARGS(&nb_uuid));
- return;
- }
- } else {
- char *error = ctl_get_row(ctx, &sbrec_table_datapath_binding,
- ctx->argv[1], false, &row);
- if (error) {
- ctl_error(ctx, "%s", error);
- free(error);
- return;
- }
-
- datapath = (const struct sbrec_datapath_binding *) row;
+ char *error = ctl_get_row(ctx, &sbrec_table_datapath_binding,
+ ctx->argv[1], false, &row);
+ if (error) {
+ ctl_error(ctx, "%s", error);
+ free(error);
+ return;
}
+
+ datapath = (const struct sbrec_datapath_binding *) row;
if (datapath) {
ctx->argc--;
ctx->argv++;
@@ -1504,7 +1493,9 @@ 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, "name2", NULL},
+ {&sbrec_datapath_binding_col_nb_uuid, NULL, NULL}},
+
[SBREC_TABLE_PORT_BINDING].row_ids
= {{&sbrec_port_binding_col_logical_port, NULL, NULL},
--
2.50.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
