[ovs-dev] [PATCH v2 2/2] ovsdb-idl: Preserve references for deleted rows.
Dumitru Ceara
dceara at redhat.com
Wed Mar 3 14:40:00 UTC 2021
Considering two DB rows, 'a' from table A and 'b' from table B (with
column 'ref_a' a reference to table A):
a = {A._uuid=<U1>}
b = {B._uuid=<U2>, B.ref_a=<U1>}
Assuming both records are present in the IDL client's in-memory view of
the database, depending whether row 'b' is also deleted in the same
transaction or not, deletion of row 'a' should generate the following
tracked changes:
1. only row 'a' is deleted:
- for table A:
- deleted records: a = {A._uuid=<U1>}
- for table B:
- updated records: b = {B._uuid=<U2>, B.ref_a=[]}
2. row 'a' and row 'b' are deleted in the same update:
- for table A:
- deleted records: a = {A._uuid=<U1>}
- for table B:
- deleted records: b = {B._uuid=<U2>, B.ref_a=<U1>}
To ensure this, we now delay reparsing row backrefs until the row has
been removed from the table's hmap and until the IDL client has
processed all tracked changes (ovsdb_idl_track_clear() was called).
Without this change, in scenario 2 above, the tracked changes for table
B would be:
- deleted records: b = {B._uuid=<U2>, B.ref_a=[]}
In particular, for strong references, row 'a' can never be deleted in
a transaction that happens strictly before row 'b' is deleted. In some
cases [0] both rows are deleted in the same transaction and having
B.ref_a=[] would violate the integrity of the database from client
perspective. This would force the client to always validate that
strong reference fields are non-NULL. This is not really an option
because the information in the original reference is required for
incrementally processing the record deletion.
[0] with ovn-monitor-all=true, the following command triggers a crash
in ovn-controller because a strong reference field becomes NULL:
$ ovn-nbctl --wait=hv -- lr-add r -- lrp-add r rp 00:00:00:00:00:01 1.0.0.1/24
$ ovn-nbctl lr-del r
Reported-at: https://bugzilla.redhat.com/1932642
Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.")
Signed-off-by: Dumitru Ceara <dceara at redhat.com>
---
v2:
- Added ovsdb-idl.at test for strong references.
---
lib/ovsdb-idl.c | 6 ++-
tests/ovsdb-idl.at | 118 ++++++++++++++++++++++++++++++++++++++++++++++++++++
tests/test-ovsdb.c | 45 ++++++++++++++++++++
3 files changed, 167 insertions(+), 2 deletions(-)
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 9e1e787..ecd4924 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -163,6 +163,7 @@ static void ovsdb_idl_row_unparse(struct ovsdb_idl_row *);
static void ovsdb_idl_row_clear_old(struct ovsdb_idl_row *);
static void ovsdb_idl_row_clear_new(struct ovsdb_idl_row *);
static void ovsdb_idl_row_clear_arcs(struct ovsdb_idl_row *, bool destroy_dsts);
+static void ovsdb_idl_row_reparse_backrefs(struct ovsdb_idl_row *row);
static void ovsdb_idl_txn_abort_all(struct ovsdb_idl *);
static bool ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row *,
@@ -367,6 +368,8 @@ ovsdb_idl_clear(struct ovsdb_idl *db)
ovsdb_idl_row_unparse(row);
}
LIST_FOR_EACH_SAFE (arc, next_arc, src_node, &row->src_arcs) {
+ ovs_list_remove(&arc->src_node);
+ ovs_list_remove(&arc->dst_node);
free(arc);
}
/* No need to do anything with dst_arcs: some node has those arcs
@@ -1234,6 +1237,7 @@ ovsdb_idl_track_clear__(struct ovsdb_idl *idl, bool flush_all)
ovs_list_remove(&row->track_node);
ovs_list_init(&row->track_node);
if (ovsdb_idl_row_is_orphan(row)) {
+ ovsdb_idl_row_reparse_backrefs(row);
ovsdb_idl_row_unparse(row);
if (row->tracked_old_datum) {
const struct ovsdb_idl_table_class *class =
@@ -2156,8 +2160,6 @@ ovsdb_idl_delete_row(struct ovsdb_idl_row *row)
ovsdb_idl_row_clear_old(row);
if (ovs_list_is_empty(&row->dst_arcs)) {
ovsdb_idl_row_destroy(row);
- } else {
- ovsdb_idl_row_reparse_backrefs(row);
}
}
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index 610680c..7025022 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1269,6 +1269,124 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan rows, cond
010: done
]])
+dnl This test checks that deleting the destination of a reference updates the
+dnl source tracked record.
+OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, references, single delete],
+ [['["idltest",
+ {"op": "insert",
+ "table": "simple",
+ "row": {"s": "row0_s"},
+ "uuid-name": "uuid_row0_s"},
+ {"op": "insert",
+ "table": "simple6",
+ "row": {"name": "row0_s6",
+ "weak_ref": ["set",
+ [["named-uuid", "uuid_row0_s"]]
+ ]}}]']],
+ [['condition simple [true]; simple6 [true]' \
+ '["idltest",
+ {"op": "delete",
+ "table": "simple",
+ "where": []}]' \
+ '["idltest",
+ {"op": "insert",
+ "table": "simple",
+ "row": {"s": "row0_s"}}]']],
+ [[000: change conditions
+001: table simple6: inserted row: name=row0_s6 weak_ref=[<0>] uuid=<1>
+001: table simple6: updated columns: name weak_ref
+001: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<0>
+001: table simple: updated columns: s
+002: {"error":null,"result":[{"count":1}]}
+003: table simple6: name=row0_s6 weak_ref=[] uuid=<1>
+003: table simple6: updated columns: weak_ref
+003: table simple: deleted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<0>
+004: {"error":null,"result":[{"uuid":["uuid","<3>"]}]}
+005: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<3>
+005: table simple: updated columns: s
+006: done
+]])
+
+dnl This test checks that deleting both the destination and source of the
+dnl reference doesn't remove the reference the source tracked record.
+OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, weak references, multiple deletes],
+ [['["idltest",
+ {"op": "insert",
+ "table": "simple",
+ "row": {"s": "row0_s"},
+ "uuid-name": "uuid_row0_s"},
+ {"op": "insert",
+ "table": "simple6",
+ "row": {"name": "row0_s6",
+ "weak_ref": ["set",
+ [["named-uuid", "uuid_row0_s"]]
+ ]}}]']],
+ [['condition simple [true]; simple6 [true]' \
+ '["idltest",
+ {"op": "delete",
+ "table": "simple",
+ "where": []},
+ {"op": "delete",
+ "table": "simple6",
+ "where": []}]' \
+ '["idltest",
+ {"op": "insert",
+ "table": "simple",
+ "row": {"s": "row0_s"}}]']],
+ [[000: change conditions
+001: table simple6: inserted row: name=row0_s6 weak_ref=[<0>] uuid=<1>
+001: table simple6: updated columns: name weak_ref
+001: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<0>
+001: table simple: updated columns: s
+002: {"error":null,"result":[{"count":1},{"count":1}]}
+003: table simple6: deleted row: name=row0_s6 weak_ref=[<0>] uuid=<1>
+003: table simple: deleted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<0>
+004: {"error":null,"result":[{"uuid":["uuid","<3>"]}]}
+005: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<2> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<3>
+005: table simple: updated columns: s
+006: done
+]])
+
+dnl This test checks that deleting both the destination and source of the
+dnl reference doesn't remove the reference the source tracked record.
+OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, strong references, multiple deletes],
+ [['["idltest",
+ {"op": "insert",
+ "table": "simple4",
+ "row": {"name": "row0_s4"},
+ "uuid-name": "uuid_row0_s4"},
+ {"op": "insert",
+ "table": "simple3",
+ "row": {"name": "row0_s3",
+ "uref": ["set",
+ [["named-uuid", "uuid_row0_s4"]]
+ ]}}]']],
+ [['condition simple [true]; simple3 [true]; simple4 [true]' \
+ '["idltest",
+ {"op": "delete",
+ "table": "simple3",
+ "where": []},
+ {"op": "delete",
+ "table": "simple4",
+ "where": []}]' \
+ '["idltest",
+ {"op": "insert",
+ "table": "simple",
+ "row": {"s": "row0_s"}}]']],
+ [[000: change conditions
+001: table simple3: inserted row: name=row0_s3 uset=[] uref=[[<0>]] uuid=<1>
+001: table simple3: updated columns: name uref
+001: table simple4: inserted row: name=row0_s4 uuid=<0>
+001: table simple4: updated columns: name
+002: {"error":null,"result":[{"count":1},{"count":1}]}
+003: table simple3: deleted row: name=row0_s3 uset=[] uref=[[<0>]] uuid=<1>
+003: table simple4: deleted row: name=row0_s4 uuid=<0>
+004: {"error":null,"result":[{"uuid":["uuid","<2>"]}]}
+005: table simple: inserted row: i=0 r=0 b=false s=row0_s u=<3> ia=[] ra=[] ba=[] sa=[] ua=[] uuid=<2>
+005: table simple: updated columns: s
+006: done
+]])
+
OVSDB_CHECK_IDL_TRACK([track, simple idl, initially empty, various ops],
[],
[['["idltest",
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index f50e88d..36fb9cc 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -1947,6 +1947,23 @@ print_idl_row_updated_simple3(const struct idltest_simple3 *s3, int step)
}
static void
+print_idl_row_updated_simple4(const struct idltest_simple4 *s4, int step)
+{
+ struct ds updates = DS_EMPTY_INITIALIZER;
+ for (size_t i = 0; i < IDLTEST_SIMPLE4_N_COLUMNS; i++) {
+ if (idltest_simple4_is_updated(s4, i)) {
+ ds_put_format(&updates, " %s", idltest_simple4_columns[i].name);
+ }
+ }
+ if (updates.length) {
+ print_and_log("%03d: table %s: updated columns:%s",
+ step, s4->header_.table->class_->name,
+ ds_cstr(&updates));
+ ds_destroy(&updates);
+ }
+}
+
+static void
print_idl_row_updated_simple6(const struct idltest_simple6 *s6, int step)
{
struct ds updates = DS_EMPTY_INITIALIZER;
@@ -2090,6 +2107,20 @@ print_idl_row_simple3(const struct idltest_simple3 *s3, int step)
}
static void
+print_idl_row_simple4(const struct idltest_simple4 *s4, int step)
+{
+ struct ds msg = DS_EMPTY_INITIALIZER;
+ ds_put_format(&msg, "name=%s", s4->name);
+
+ char *row_msg = format_idl_row(&s4->header_, step, ds_cstr(&msg));
+ print_and_log("%s", row_msg);
+ ds_destroy(&msg);
+ free(row_msg);
+
+ print_idl_row_updated_simple4(s4, step);
+}
+
+static void
print_idl_row_simple6(const struct idltest_simple6 *s6, int step)
{
struct ds msg = DS_EMPTY_INITIALIZER;
@@ -2156,6 +2187,8 @@ print_idl(struct ovsdb_idl *idl, int step)
static void
print_idl_track(struct ovsdb_idl *idl, int step)
{
+ const struct idltest_simple3 *s3;
+ const struct idltest_simple4 *s4;
const struct idltest_simple6 *s6;
const struct idltest_simple *s;
const struct idltest_link1 *l1;
@@ -2174,6 +2207,14 @@ print_idl_track(struct ovsdb_idl *idl, int step)
print_idl_row_link2(l2, step);
n++;
}
+ IDLTEST_SIMPLE3_FOR_EACH_TRACKED (s3, idl) {
+ print_idl_row_simple3(s3, step);
+ n++;
+ }
+ IDLTEST_SIMPLE4_FOR_EACH_TRACKED (s4, idl) {
+ print_idl_row_simple4(s4, step);
+ n++;
+ }
IDLTEST_SIMPLE6_FOR_EACH_TRACKED (s6, idl) {
print_idl_row_simple6(s6, step);
n++;
@@ -2404,6 +2445,10 @@ find_table_class(const char *name)
return &idltest_table_link1;
} else if (!strcmp(name, "link2")) {
return &idltest_table_link2;
+ } else if (!strcmp(name, "simple3")) {
+ return &idltest_table_simple3;
+ } else if (!strcmp(name, "simple4")) {
+ return &idltest_table_simple4;
} else if (!strcmp(name, "simple6")) {
return &idltest_table_simple6;
}
More information about the dev
mailing list