diff mbox series

[ovs-dev] ovsdb-idl: Preserve references for rows deleted in same IDL as their insertion.

Message ID 20220916084006.884447-1-xsimonar@redhat.com
State Accepted
Headers show
Series [ovs-dev] ovsdb-idl: Preserve references for rows deleted in same IDL as their insertion. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Xavier Simonart Sept. 16, 2022, 8:40 a.m. UTC 
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 inserted in the IDL client's in-memory view of
the database, if row 'b' is also deleted in the same transaction, it should
generate the following tracked changes:

- for table A:
  - inserted records: a = {A._uuid=<U1>}
- for table B:
  - inserted records: b = {B._uuid=<U2>, B.ref_a=<U1>}
  - deleted records: b = {B._uuid=<U2>, B.ref_a=<U1>}

Before this patch, inserted and deleted records in table B
would (in some cases [0]) be b = {B._uuid=<U2>, B.ref_a=[]}.
Having B.ref_a=[] would violate the integrity of the database from client
perspective.

test-ovsdb has also been updated to show that one row can be
both inserted and deleted within one IDL.

[0] In ovn-controller the fact that the reference is NULL caused a crash
    in the following case, when both commands were handled by ovn-controller
    within the same loop:
    $ ovn-nbctl ls-add sw0 -- lsp-add sw0 sw0-port1 -- lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2"
    $ ovn-nbctl lsp-del sw0-port1

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2126450
Fixes: 91e1ff5dde39 ("ovsdb-idl: Don't reparse orphaned rows.")
Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
---
 lib/ovsdb-idl.c    |  4 +++
 tests/ovsdb-idl.at | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/test-ovsdb.c | 13 ++++---
 3 files changed, 101 insertions(+), 5 deletions(-)

Comments

Ilya Maximets Sept. 26, 2022, 11:59 p.m. UTC | #1 
On 9/16/22 10:40, Xavier Simonart wrote:
> 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 inserted in the IDL client's in-memory view of
> the database, if row 'b' is also deleted in the same transaction, it should
> generate the following tracked changes:
> 
> - for table A:
>   - inserted records: a = {A._uuid=<U1>}
> - for table B:
>   - inserted records: b = {B._uuid=<U2>, B.ref_a=<U1>}
>   - deleted records: b = {B._uuid=<U2>, B.ref_a=<U1>}
> 
> Before this patch, inserted and deleted records in table B
> would (in some cases [0]) be b = {B._uuid=<U2>, B.ref_a=[]}.
> Having B.ref_a=[] would violate the integrity of the database from client
> perspective.
> 
> test-ovsdb has also been updated to show that one row can be
> both inserted and deleted within one IDL.
> 
> [0] In ovn-controller the fact that the reference is NULL caused a crash
>     in the following case, when both commands were handled by ovn-controller
>     within the same loop:
>     $ ovn-nbctl ls-add sw0 -- lsp-add sw0 sw0-port1 -- lsp-set-addresses sw0-port1 "50:54:00:00:00:01 192.168.0.2"
>     $ ovn-nbctl lsp-del sw0-port1
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2126450
> Fixes: 91e1ff5dde39 ("ovsdb-idl: Don't reparse orphaned rows.")
> Signed-off-by: Xavier Simonart <xsimonar@redhat.com>
> ---
>  lib/ovsdb-idl.c    |  4 +++
>  tests/ovsdb-idl.at | 89 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/test-ovsdb.c | 13 ++++---
>  3 files changed, 101 insertions(+), 5 deletions(-)

Thanks!  Applied and backported down to 2.17.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 576979f9e..e33d2a5c5 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2419,6 +2419,10 @@  ovsdb_idl_insert_row(struct ovsdb_idl_row *row, const struct shash *data)
 static void
 ovsdb_idl_delete_row(struct ovsdb_idl_row *row)
 {
+    /* If row has to be reparsed, reparse it before it's deleted */
+    if (!ovs_list_is_empty(&row->reparse_node)) {
+        ovsdb_idl_row_parse(row);
+    }
     ovsdb_idl_remove_from_indexes(row);
     ovsdb_idl_row_clear_arcs(row, true);
     ovsdb_idl_row_destroy(row);
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index d1cfa59c5..8e75d00d7 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -2466,3 +2466,92 @@  unix:socket2 remote has col id in table simple7
 
 OVSDB_SERVER_SHUTDOWN
 AT_CLEANUP
+
+dnl This test checks that inserting and deleting the source of a reference
+dnl doesn't remove the reference in the (deleted) source tracked record.
+OVSDB_CHECK_IDL_TRACK([track, insert and delete, refs to link1],
+  [],
+  [['["idltest",
+      {"op": "insert",
+       "table": "link2",
+       "uuid-name": "l2row0",
+       "row": {"i": 1, "l1": ["set", [["named-uuid", "l1row0"]]]}
+      },
+      {"op": "insert",
+       "table": "link1",
+       "uuid-name": "l1row0",
+       "row": {"i": 1, "k": ["named-uuid", "l1row0"]}
+      },
+      {"op": "insert",
+       "table": "link2",
+       "uuid-name": "l2row1",
+       "row": {"i": 2, "l1": ["set", [["named-uuid", "l1row0"]]]}
+      }
+    ]' \
+    '+["idltest",
+      {"op": "delete",
+       "table": "link2",
+       "where": [["i", "==", 2]]}
+       ]' \
+    '["idltest",
+      {"op": "delete",
+       "table": "link2",
+       "where": [["i", "==", 1]]}
+       ]'
+  ]],
+  [[000: empty
+001: {"error":null,"result":[{"uuid":["uuid","<0>"]},{"uuid":["uuid","<1>"]},{"uuid":["uuid","<2>"]}]}
+002: {"error":null,"result":[{"count":1}]}
+003: table link1: inserted row: i=1 k=1 ka=[] l2= uuid=<1>
+003: table link1: updated columns: i k
+003: table link2: inserted row: i=1 l1=1 uuid=<0>
+003: table link2: inserted/deleted row: i=2 l1=1 uuid=<2>
+003: table link2: updated columns: i l1
+003: table link2: updated columns: i l1
+004: {"error":null,"result":[{"count":1}]}
+005: table link1: i=1 k=1 ka=[] l2= uuid=<1>
+006: done
+]])
+OVSDB_CHECK_IDL_TRACK([track, insert and delete, refs to link2],
+  [],
+  [['["idltest",
+      {"op": "insert",
+       "table": "link1",
+       "uuid-name": "l1row0",
+       "row": {"i": 1, "k": ["named-uuid", "l1row0"], "l2": ["set", [["named-uuid", "l2row0"]]]}
+      },
+      {"op": "insert",
+       "table": "link2",
+       "uuid-name": "l2row0",
+       "row": {"i": 1}
+      },
+      {"op": "insert",
+       "table": "link1",
+       "uuid-name": "l1row1",
+       "row": {"i": 2, "k": ["named-uuid", "l1row0"], "l2": ["set", [["named-uuid", "l2row0"]]]}
+      }
+    ]' \
+    '+["idltest",
+      {"op": "delete",
+       "table": "link1",
+       "where": [["i", "==", 2]]}
+       ]' \
+    '["idltest",
+      {"op": "delete",
+       "table": "link1",
+       "where": [["i", "==", 1]]}
+       ]'
+  ]],
+  [[000: empty
+001: {"error":null,"result":[{"uuid":["uuid","<0>"]},{"uuid":["uuid","<1>"]},{"uuid":["uuid","<2>"]}]}
+002: {"error":null,"result":[{"count":1}]}
+003: table link1: inserted row: i=1 k=1 ka=[] l2=1 uuid=<0>
+003: table link1: inserted/deleted row: i=2 k=1 ka=[] l2=1 uuid=<2>
+003: table link1: updated columns: i k l2
+003: table link1: updated columns: i k l2
+003: table link2: inserted row: i=1 l1= uuid=<1>
+003: table link2: updated columns: i
+004: {"error":null,"result":[{"count":1}]}
+005: table link2: i=1 l1= uuid=<1>
+006: done
+]])
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 965b0f913..5f7110f41 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -1953,11 +1953,14 @@  format_idl_row(const struct ovsdb_idl_row *row, int step, const char *contents,
     const char *change_str =
         !ovsdb_idl_track_is_set(row->table)
         ? ""
-        : ovsdb_idl_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0
-          ? "inserted row: "
-          : ovsdb_idl_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0
-            ? "deleted row: "
-            : "";
+        : ovsdb_idl_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0 &&
+            ovsdb_idl_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0
+          ? "inserted/deleted row: "
+          : ovsdb_idl_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0
+            ? "inserted row: "
+            : ovsdb_idl_row_get_seqno(row, OVSDB_IDL_CHANGE_DELETE) > 0
+              ? "deleted row: "
+              : "";
 
     if (terse) {
         return xasprintf("%03d: table %s", step, row->table->class_->name);