[ovs-dev,v2,2/2] ovsdb-idl: Preserve references for deleted rows.

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@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(-)

On 3/3/21 3:40 PM, Dumitru Ceara 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 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@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);
>      }

By removing the ovsdb_idl_row_reparse_backrefs() you're making an assumption
that we always have a modification event for all rows that are referencing
this one.  But that is not always the case.

There is scenario where this re-parsing is needed.  In short, if you're
reducing the scope of conditional monitoring and some rows becomes
orphan rows.  In this case ovsdb-server will not generate 'modify' event
for the row that references our newly orphan row, so it will not be
re-parsed and these orphan rows will be accessible the row that references
them.

Here is a quick'n'dirty testcase, how to reproduce:
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index 702502280..c6bb3f348 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated],
 dnl This test creates database with weak references and checks that orphan
 dnl rows created for weak references are not available for iteration via
 dnl list of tracked changes.
-OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references],
+OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references quickndirty],
   [['["idltest",
       {"op": "insert",
        "table": "simple",
@@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak refer
                               ["named-uuid", "weak_row1"],
                               ["named-uuid", "weak_row2"]]
                            ]}}]']],
-  [['condition simple []' \
+  [['condition simple [true]' \
     'condition simple [["s","==","row1_s"]]' \
     '["idltest",
       {"op": "update",
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 36fb9cc08..ace87b13d 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct idltest_singleton *sng, int step)
 static void
 print_idl(struct ovsdb_idl *idl, int step)
 {
+    const struct idltest_simple6 *s6;
     const struct idltest_simple *s;
     const struct idltest_link1 *l1;
     const struct idltest_link2 *l2;
@@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
         print_idl_row_link2(l2, step);
         n++;
     }
+    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
+        print_idl_row_simple6(s6, step);
+        n++;
+    }
     IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
         print_idl_row_singleton(sng, step);
         n++;
@@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
 
             /* Print update. */
             if (track) {
-                print_idl_track(idl, step++);
+                print_idl_track(idl, step);
+                print_and_log("%03d: print_idl[1]", step);
+                print_idl(idl, step);
                 ovsdb_idl_track_clear(idl);
+                print_and_log("%03d: print_idl[2]", step);
+                print_idl(idl, step++);
             } else {
                 print_idl(idl, step++);
             }
---

By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', you will have
some output for the commands in the log.


The first condition for a 'simple' table is 'true', so we will receive all
3 rows from this table and also row from the 'simple6'.
All 3 rows are in a weak references:

test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
test-ovsdb|test_ovsdb|001: table simple: updated columns: s
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
test-ovsdb|test_ovsdb|001: table simple: updated columns: s
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
test-ovsdb|test_ovsdb|001: table simple: updated columns: s
test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref


All of them are accessible for a user before the track_clear():

test-ovsdb|test_ovsdb|001: print_idl[1]
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
test-ovsdb|test_ovsdb|001: table simple: updated columns: s
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
test-ovsdb|test_ovsdb|001: table simple: updated columns: s
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
test-ovsdb|test_ovsdb|001: table simple: updated columns: s
test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref

And after:

test-ovsdb|test_ovsdb|001: print_idl[2]
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
test-ovsdb|test_ovsdb|001: table simple6: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]


Now condition is changed.  This makes 2 of 3 rows to become orphan rows.
Note that the update notification only removes orphan rows, but doesn't
modify the row in simple6, because rows wasn't actually removed, just
not needed anymore:

test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5

Track list is empty, but 2 orphan rows should be on a track list as deleted:

test-ovsdb|test_ovsdb|003: empty

But all 3 rows are still accessible for a user from the row in 'simple6' even
though table 'simple' has only 1 row accessible:

test-ovsdb|test_ovsdb|003: print_idl[1]
test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]

And they are still here after the track_clear():

test-ovsdb|test_ovsdb|003: print_idl[2]
test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]


When we have unrelated update on a row in 'simple6' it finally gets
re-parsed and we have tracked orphan rows as deleted and they are
no longer accessible for a user from a row in 'simple6':

test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]

Tracked:

test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s <> uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s <> uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
test-ovsdb|test_ovsdb|005: table simple6: updated columns: name

Before track_clear():

test-ovsdb|test_ovsdb|005: print_idl[1]
test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
test-ovsdb|test_ovsdb|005: table simple6: updated columns: name

After:

test-ovsdb|test_ovsdb|005: print_idl[2]
test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]


Best regards, Ilya Maximets.

On 3/9/21 4:07 PM, Ilya Maximets wrote:
> On 3/3/21 3:40 PM, Dumitru Ceara 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 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@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);
>>       }
> 
> By removing the ovsdb_idl_row_reparse_backrefs() you're making an assumption
> that we always have a modification event for all rows that are referencing
> this one.  But that is not always the case.
> 
> There is scenario where this re-parsing is needed.  In short, if you're
> reducing the scope of conditional monitoring and some rows becomes
> orphan rows.  In this case ovsdb-server will not generate 'modify' event
> for the row that references our newly orphan row, so it will not be
> re-parsed and these orphan rows will be accessible the row that references
> them.

You're right, thanks for spotting this and for the test case.

> 
> Here is a quick'n'dirty testcase, how to reproduce:
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 702502280..c6bb3f348 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated],
>   dnl This test creates database with weak references and checks that orphan
>   dnl rows created for weak references are not available for iteration via
>   dnl list of tracked changes.
> -OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references],
> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references quickndirty],
>     [['["idltest",
>         {"op": "insert",
>          "table": "simple",
> @@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak refer
>                                 ["named-uuid", "weak_row1"],
>                                 ["named-uuid", "weak_row2"]]
>                              ]}}]']],
> -  [['condition simple []' \
> +  [['condition simple [true]' \
>       'condition simple [["s","==","row1_s"]]' \
>       '["idltest",
>         {"op": "update",
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 36fb9cc08..ace87b13d 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct idltest_singleton *sng, int step)
>   static void
>   print_idl(struct ovsdb_idl *idl, int step)
>   {
> +    const struct idltest_simple6 *s6;
>       const struct idltest_simple *s;
>       const struct idltest_link1 *l1;
>       const struct idltest_link2 *l2;
> @@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
>           print_idl_row_link2(l2, step);
>           n++;
>       }
> +    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
> +        print_idl_row_simple6(s6, step);
> +        n++;
> +    }
>       IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
>           print_idl_row_singleton(sng, step);
>           n++;
> @@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
>   
>               /* Print update. */
>               if (track) {
> -                print_idl_track(idl, step++);
> +                print_idl_track(idl, step);
> +                print_and_log("%03d: print_idl[1]", step);
> +                print_idl(idl, step);
>                   ovsdb_idl_track_clear(idl);
> +                print_and_log("%03d: print_idl[2]", step);
> +                print_idl(idl, step++);
>               } else {
>                   print_idl(idl, step++);
>               }
> ---
> 
> By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', you will have
> some output for the commands in the log.
> 
> 
> The first condition for a 'simple' table is 'true', so we will receive all
> 3 rows from this table and also row from the 'simple6'.
> All 3 rows are in a weak references:
> 
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
> 
> 
> All of them are accessible for a user before the track_clear():
> 
> test-ovsdb|test_ovsdb|001: print_idl[1]
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
> 
> And after:
> 
> test-ovsdb|test_ovsdb|001: print_idl[2]
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> test-ovsdb|test_ovsdb|001: table simple6: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> 
> 
> Now condition is changed.  This makes 2 of 3 rows to become orphan rows.
> Note that the update notification only removes orphan rows, but doesn't
> modify the row in simple6, because rows wasn't actually removed, just
> not needed anymore:
> 
> test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
> 
> Track list is empty, but 2 orphan rows should be on a track list as deleted:

This actually uncovers a new potential issue: the 2 orphan rows are not 
on the tracked list even without my change.  That's because 
ovsdb_idl_row_destroy() doesn't get called if the row becomes orphan so 
it won't be added to 'row->table->track_list'.

> 
> test-ovsdb|test_ovsdb|003: empty
> 
> But all 3 rows are still accessible for a user from the row in 'simple6' even
> though table 'simple' has only 1 row accessible:
> 
> test-ovsdb|test_ovsdb|003: print_idl[1]
> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
> 
> And they are still here after the track_clear():
> 
> test-ovsdb|test_ovsdb|003: print_idl[2]
> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
> 
> 
> When we have unrelated update on a row in 'simple6' it finally gets
> re-parsed and we have tracked orphan rows as deleted and they are
> no longer accessible for a user from a row in 'simple6':
> 
> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]
> 
> Tracked:
> 
> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s <> uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s <> uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> 
> Before track_clear():
> 
> test-ovsdb|test_ovsdb|005: print_idl[1]
> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> 
> After:
> 
> test-ovsdb|test_ovsdb|005: print_idl[2]
> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> 

To cover all cases above I'm thinking of the following approach:

1. Delay ovsdb_idl_row_reparse_backrefs() for deleted rows until 
ovsdb_idl_parse_update() has finished.

2. After ovsdb_idl_parse_update(), walk all deleted rows and reparse 
backrefs but only for src rows that have not been deleted.

3. IDL user (e.g., ovn-controller) processes tracked changes.  At this 
point:
- non-deleted src rows with a weak reference to orphaned dst rows have 
ref == NULL.
- deleted src rows with a weak/strong reference to orphaned dst rows 
have ref != NULL but that's fine because they'll both be cleaned up at 
the next step below.

4. ovsdb_idl_track_clear()

What do you think?

> 
> Best regards, Ilya Maximets.
> 

Thanks,
Dumitru

On 3/9/21 5:26 PM, Dumitru Ceara wrote:
> On 3/9/21 4:07 PM, Ilya Maximets wrote:
>> On 3/3/21 3:40 PM, Dumitru Ceara 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 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@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);
>>>       }
>>
>> By removing the ovsdb_idl_row_reparse_backrefs() you're making an assumption
>> that we always have a modification event for all rows that are referencing
>> this one.  But that is not always the case.
>>
>> There is scenario where this re-parsing is needed.  In short, if you're
>> reducing the scope of conditional monitoring and some rows becomes
>> orphan rows.  In this case ovsdb-server will not generate 'modify' event
>> for the row that references our newly orphan row, so it will not be
>> re-parsed and these orphan rows will be accessible the row that references
>> them.
> 
> You're right, thanks for spotting this and for the test case.
> 
>>
>> Here is a quick'n'dirty testcase, how to reproduce:
>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
>> index 702502280..c6bb3f348 100644
>> --- a/tests/ovsdb-idl.at
>> +++ b/tests/ovsdb-idl.at
>> @@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated],
>>   dnl This test creates database with weak references and checks that orphan
>>   dnl rows created for weak references are not available for iteration via
>>   dnl list of tracked changes.
>> -OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references],
>> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references quickndirty],
>>     [['["idltest",
>>         {"op": "insert",
>>          "table": "simple",
>> @@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak refer
>>                                 ["named-uuid", "weak_row1"],
>>                                 ["named-uuid", "weak_row2"]]
>>                              ]}}]']],
>> -  [['condition simple []' \
>> +  [['condition simple [true]' \
>>       'condition simple [["s","==","row1_s"]]' \
>>       '["idltest",
>>         {"op": "update",
>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
>> index 36fb9cc08..ace87b13d 100644
>> --- a/tests/test-ovsdb.c
>> +++ b/tests/test-ovsdb.c
>> @@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct idltest_singleton *sng, int step)
>>   static void
>>   print_idl(struct ovsdb_idl *idl, int step)
>>   {
>> +    const struct idltest_simple6 *s6;
>>       const struct idltest_simple *s;
>>       const struct idltest_link1 *l1;
>>       const struct idltest_link2 *l2;
>> @@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
>>           print_idl_row_link2(l2, step);
>>           n++;
>>       }
>> +    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
>> +        print_idl_row_simple6(s6, step);
>> +        n++;
>> +    }
>>       IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
>>           print_idl_row_singleton(sng, step);
>>           n++;
>> @@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
>>                 /* Print update. */
>>               if (track) {
>> -                print_idl_track(idl, step++);
>> +                print_idl_track(idl, step);
>> +                print_and_log("%03d: print_idl[1]", step);
>> +                print_idl(idl, step);
>>                   ovsdb_idl_track_clear(idl);
>> +                print_and_log("%03d: print_idl[2]", step);
>> +                print_idl(idl, step++);
>>               } else {
>>                   print_idl(idl, step++);
>>               }
>> ---
>>
>> By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', you will have
>> some output for the commands in the log.
>>
>>
>> The first condition for a 'simple' table is 'true', so we will receive all
>> 3 rows from this table and also row from the 'simple6'.
>> All 3 rows are in a weak references:
>>
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
>>
>>
>> All of them are accessible for a user before the track_clear():
>>
>> test-ovsdb|test_ovsdb|001: print_idl[1]
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
>>
>> And after:
>>
>> test-ovsdb|test_ovsdb|001: print_idl[2]
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>> test-ovsdb|test_ovsdb|001: table simple6: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>>
>>
>> Now condition is changed.  This makes 2 of 3 rows to become orphan rows.
>> Note that the update notification only removes orphan rows, but doesn't
>> modify the row in simple6, because rows wasn't actually removed, just
>> not needed anymore:
>>
>> test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
>> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
>>
>> Track list is empty, but 2 orphan rows should be on a track list as deleted:
> 
> This actually uncovers a new potential issue: the 2 orphan rows are not on the tracked list even without my change.  That's because ovsdb_idl_row_destroy() doesn't get called if the row becomes orphan so it won't be added to 'row->table->track_list'.

Hmm.  Thanks for sponning that.

On current master these 2 rows appears on a tracked list only after
occasional unrelated modification of a row in 'simple6' and that is
not good:

test-ovsdb|test_ovsdb|002: change conditions
test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78":{"delete":null},"5a1d0257-fc27-4046-9e18-25539f122052":{"delete":null}}}]
test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
test-ovsdb|test_ovsdb|003: empty
test-ovsdb|test_ovsdb|003: print_idl[1]
test-ovsdb|test_ovsdb|003: table simple: s=row1_s uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
test-ovsdb|test_ovsdb|003: print_idl[2]
test-ovsdb|test_ovsdb|003: table simple: s=row1_s uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]

test-ovsdb|jsonrpc|unix:socket: send request, method="transact", params=["idltest",{"where":[],"row":{"name":"new_name"},"op":"update","table":"simple6"}], id=6
test-ovsdb|jsonrpc|unix:socket: received reply, result=[{"count":1}], id=6
test-ovsdb|test_ovsdb|004: {"error":null,"result":[{"count":1}]}
test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"618cef34-e635-4841-9dc7-a14eb2b9627e":{"modify":{"name":"new_name"}}}}]
test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s uuid=5a1d0257-fc27-4046-9e18-25539f122052
test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s uuid=0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78
test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
test-ovsdb|test_ovsdb|005: table simple6: updated columns: name


It's probably because ovsdb_idl_row_reparse_backrefs() calls
ovsdb_idl_row_clear_arcs() with destroy_dsts=false.  So, re-parsing
invoked for two deleted rows doesn't actually destroy them.

> 
>>
>> test-ovsdb|test_ovsdb|003: empty
>>
>> But all 3 rows are still accessible for a user from the row in 'simple6' even
>> though table 'simple' has only 1 row accessible:
>>
>> test-ovsdb|test_ovsdb|003: print_idl[1]
>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>>
>> And they are still here after the track_clear():
>>
>> test-ovsdb|test_ovsdb|003: print_idl[2]
>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>>
>>
>> When we have unrelated update on a row in 'simple6' it finally gets
>> re-parsed and we have tracked orphan rows as deleted and they are
>> no longer accessible for a user from a row in 'simple6':
>>
>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]
>>
>> Tracked:
>>
>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s <> uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s <> uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>>
>> Before track_clear():
>>
>> test-ovsdb|test_ovsdb|005: print_idl[1]
>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>>
>> After:
>>
>> test-ovsdb|test_ovsdb|005: print_idl[2]
>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>>
> 
> To cover all cases above I'm thinking of the following approach:
> 
> 1. Delay ovsdb_idl_row_reparse_backrefs() for deleted rows until ovsdb_idl_parse_update() has finished.
> 
> 2. After ovsdb_idl_parse_update(), walk all deleted rows and reparse backrefs but only for src rows that have not been deleted.
> 
> 3. IDL user (e.g., ovn-controller) processes tracked changes.  At this point:
> - non-deleted src rows with a weak reference to orphaned dst rows have ref == NULL.
> - deleted src rows with a weak/strong reference to orphaned dst rows have ref != NULL but that's fine because they'll both be cleaned up at the next step below.
> 
> 4. ovsdb_idl_track_clear()
> 
> What do you think?

Sounds OK.  I'm not sure how the implementation should look like, though.
Will this also fix the issue with not reacking deleted orphaned rows right away?

> 
>>
>> Best regards, Ilya Maximets.
>>
> 
> Thanks,
> Dumitru
>

On 3/9/21 5:54 PM, Ilya Maximets wrote:
> On 3/9/21 5:26 PM, Dumitru Ceara wrote:
>> On 3/9/21 4:07 PM, Ilya Maximets wrote:
>>> On 3/3/21 3:40 PM, Dumitru Ceara 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 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@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);
>>>>       }
>>>
>>> By removing the ovsdb_idl_row_reparse_backrefs() you're making an assumption
>>> that we always have a modification event for all rows that are referencing
>>> this one.  But that is not always the case.
>>>
>>> There is scenario where this re-parsing is needed.  In short, if you're
>>> reducing the scope of conditional monitoring and some rows becomes
>>> orphan rows.  In this case ovsdb-server will not generate 'modify' event
>>> for the row that references our newly orphan row, so it will not be
>>> re-parsed and these orphan rows will be accessible the row that references
>>> them.
>>
>> You're right, thanks for spotting this and for the test case.
>>
>>>
>>> Here is a quick'n'dirty testcase, how to reproduce:
>>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
>>> index 702502280..c6bb3f348 100644
>>> --- a/tests/ovsdb-idl.at
>>> +++ b/tests/ovsdb-idl.at
>>> @@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated],
>>>   dnl This test creates database with weak references and checks that orphan
>>>   dnl rows created for weak references are not available for iteration via
>>>   dnl list of tracked changes.
>>> -OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references],
>>> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references quickndirty],
>>>     [['["idltest",
>>>         {"op": "insert",
>>>          "table": "simple",
>>> @@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak refer
>>>                                 ["named-uuid", "weak_row1"],
>>>                                 ["named-uuid", "weak_row2"]]
>>>                              ]}}]']],
>>> -  [['condition simple []' \
>>> +  [['condition simple [true]' \
>>>       'condition simple [["s","==","row1_s"]]' \
>>>       '["idltest",
>>>         {"op": "update",
>>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
>>> index 36fb9cc08..ace87b13d 100644
>>> --- a/tests/test-ovsdb.c
>>> +++ b/tests/test-ovsdb.c
>>> @@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct idltest_singleton *sng, int step)
>>>   static void
>>>   print_idl(struct ovsdb_idl *idl, int step)
>>>   {
>>> +    const struct idltest_simple6 *s6;
>>>       const struct idltest_simple *s;
>>>       const struct idltest_link1 *l1;
>>>       const struct idltest_link2 *l2;
>>> @@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
>>>           print_idl_row_link2(l2, step);
>>>           n++;
>>>       }
>>> +    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
>>> +        print_idl_row_simple6(s6, step);
>>> +        n++;
>>> +    }
>>>       IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
>>>           print_idl_row_singleton(sng, step);
>>>           n++;
>>> @@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
>>>                 /* Print update. */
>>>               if (track) {
>>> -                print_idl_track(idl, step++);
>>> +                print_idl_track(idl, step);
>>> +                print_and_log("%03d: print_idl[1]", step);
>>> +                print_idl(idl, step);
>>>                   ovsdb_idl_track_clear(idl);
>>> +                print_and_log("%03d: print_idl[2]", step);
>>> +                print_idl(idl, step++);
>>>               } else {
>>>                   print_idl(idl, step++);
>>>               }
>>> ---
>>>
>>> By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', you will have
>>> some output for the commands in the log.
>>>
>>>
>>> The first condition for a 'simple' table is 'true', so we will receive all
>>> 3 rows from this table and also row from the 'simple6'.
>>> All 3 rows are in a weak references:
>>>
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
>>>
>>>
>>> All of them are accessible for a user before the track_clear():
>>>
>>> test-ovsdb|test_ovsdb|001: print_idl[1]
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
>>>
>>> And after:
>>>
>>> test-ovsdb|test_ovsdb|001: print_idl[2]
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>>> test-ovsdb|test_ovsdb|001: table simple6: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>>>
>>>
>>> Now condition is changed.  This makes 2 of 3 rows to become orphan rows.
>>> Note that the update notification only removes orphan rows, but doesn't
>>> modify the row in simple6, because rows wasn't actually removed, just
>>> not needed anymore:
>>>
>>> test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
>>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
>>> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
>>>
>>> Track list is empty, but 2 orphan rows should be on a track list as deleted:
>>
>> This actually uncovers a new potential issue: the 2 orphan rows are not on the tracked list even without my change.  That's because ovsdb_idl_row_destroy() doesn't get called if the row becomes orphan so it won't be added to 'row->table->track_list'.
> 
> Hmm.  Thanks for sponning that.

*spotting

> 
> On current master these 2 rows appears on a tracked list only after
> occasional unrelated modification of a row in 'simple6' and that is
> not good:
> 
> test-ovsdb|test_ovsdb|002: change conditions
> test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78":{"delete":null},"5a1d0257-fc27-4046-9e18-25539f122052":{"delete":null}}}]
> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
> test-ovsdb|test_ovsdb|003: empty
> test-ovsdb|test_ovsdb|003: print_idl[1]
> test-ovsdb|test_ovsdb|003: table simple: s=row1_s uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> test-ovsdb|test_ovsdb|003: print_idl[2]
> test-ovsdb|test_ovsdb|003: table simple: s=row1_s uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> 
> test-ovsdb|jsonrpc|unix:socket: send request, method="transact", params=["idltest",{"where":[],"row":{"name":"new_name"},"op":"update","table":"simple6"}], id=6
> test-ovsdb|jsonrpc|unix:socket: received reply, result=[{"count":1}], id=6
> test-ovsdb|test_ovsdb|004: {"error":null,"result":[{"count":1}]}
> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"618cef34-e635-4841-9dc7-a14eb2b9627e":{"modify":{"name":"new_name"}}}}]
> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s uuid=5a1d0257-fc27-4046-9e18-25539f122052
> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s uuid=0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78
> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> 
> 
> It's probably because ovsdb_idl_row_reparse_backrefs() calls
> ovsdb_idl_row_clear_arcs() with destroy_dsts=false.  So, re-parsing
> invoked for two deleted rows doesn't actually destroy them.
> 
>>
>>>
>>> test-ovsdb|test_ovsdb|003: empty
>>>
>>> But all 3 rows are still accessible for a user from the row in 'simple6' even
>>> though table 'simple' has only 1 row accessible:
>>>
>>> test-ovsdb|test_ovsdb|003: print_idl[1]
>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>>>
>>> And they are still here after the track_clear():
>>>
>>> test-ovsdb|test_ovsdb|003: print_idl[2]
>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>>>
>>>
>>> When we have unrelated update on a row in 'simple6' it finally gets
>>> re-parsed and we have tracked orphan rows as deleted and they are
>>> no longer accessible for a user from a row in 'simple6':
>>>
>>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]
>>>
>>> Tracked:
>>>
>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s <> uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s <> uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>>>
>>> Before track_clear():
>>>
>>> test-ovsdb|test_ovsdb|005: print_idl[1]
>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>>>
>>> After:
>>>
>>> test-ovsdb|test_ovsdb|005: print_idl[2]
>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>>>
>>
>> To cover all cases above I'm thinking of the following approach:
>>
>> 1. Delay ovsdb_idl_row_reparse_backrefs() for deleted rows until ovsdb_idl_parse_update() has finished.
>>
>> 2. After ovsdb_idl_parse_update(), walk all deleted rows and reparse backrefs but only for src rows that have not been deleted.
>>
>> 3. IDL user (e.g., ovn-controller) processes tracked changes.  At this point:
>> - non-deleted src rows with a weak reference to orphaned dst rows have ref == NULL.
>> - deleted src rows with a weak/strong reference to orphaned dst rows have ref != NULL but that's fine because they'll both be cleaned up at the next step below.
>>
>> 4. ovsdb_idl_track_clear()
>>
>> What do you think?
> 
> Sounds OK.  I'm not sure how the implementation should look like, though.
> Will this also fix the issue with not reacking deleted orphaned rows right away?
> 
>>
>>>
>>> Best regards, Ilya Maximets.
>>>
>>
>> Thanks,
>> Dumitru
>>
>

On 3/9/21 5:56 PM, Ilya Maximets wrote:
> On 3/9/21 5:54 PM, Ilya Maximets wrote:
>> On 3/9/21 5:26 PM, Dumitru Ceara wrote:
>>> On 3/9/21 4:07 PM, Ilya Maximets wrote:
>>>> On 3/3/21 3:40 PM, Dumitru Ceara 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 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@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);
>>>>>        }
>>>>
>>>> By removing the ovsdb_idl_row_reparse_backrefs() you're making an assumption
>>>> that we always have a modification event for all rows that are referencing
>>>> this one.  But that is not always the case.
>>>>
>>>> There is scenario where this re-parsing is needed.  In short, if you're
>>>> reducing the scope of conditional monitoring and some rows becomes
>>>> orphan rows.  In this case ovsdb-server will not generate 'modify' event
>>>> for the row that references our newly orphan row, so it will not be
>>>> re-parsed and these orphan rows will be accessible the row that references
>>>> them.
>>>
>>> You're right, thanks for spotting this and for the test case.
>>>
>>>>
>>>> Here is a quick'n'dirty testcase, how to reproduce:
>>>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
>>>> index 702502280..c6bb3f348 100644
>>>> --- a/tests/ovsdb-idl.at
>>>> +++ b/tests/ovsdb-idl.at
>>>> @@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated],
>>>>    dnl This test creates database with weak references and checks that orphan
>>>>    dnl rows created for weak references are not available for iteration via
>>>>    dnl list of tracked changes.
>>>> -OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references],
>>>> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak references quickndirty],
>>>>      [['["idltest",
>>>>          {"op": "insert",
>>>>           "table": "simple",
>>>> @@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, orphan weak refer
>>>>                                  ["named-uuid", "weak_row1"],
>>>>                                  ["named-uuid", "weak_row2"]]
>>>>                               ]}}]']],
>>>> -  [['condition simple []' \
>>>> +  [['condition simple [true]' \
>>>>        'condition simple [["s","==","row1_s"]]' \
>>>>        '["idltest",
>>>>          {"op": "update",
>>>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
>>>> index 36fb9cc08..ace87b13d 100644
>>>> --- a/tests/test-ovsdb.c
>>>> +++ b/tests/test-ovsdb.c
>>>> @@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct idltest_singleton *sng, int step)
>>>>    static void
>>>>    print_idl(struct ovsdb_idl *idl, int step)
>>>>    {
>>>> +    const struct idltest_simple6 *s6;
>>>>        const struct idltest_simple *s;
>>>>        const struct idltest_link1 *l1;
>>>>        const struct idltest_link2 *l2;
>>>> @@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
>>>>            print_idl_row_link2(l2, step);
>>>>            n++;
>>>>        }
>>>> +    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
>>>> +        print_idl_row_simple6(s6, step);
>>>> +        n++;
>>>> +    }
>>>>        IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
>>>>            print_idl_row_singleton(sng, step);
>>>>            n++;
>>>> @@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
>>>>                  /* Print update. */
>>>>                if (track) {
>>>> -                print_idl_track(idl, step++);
>>>> +                print_idl_track(idl, step);
>>>> +                print_and_log("%03d: print_idl[1]", step);
>>>> +                print_idl(idl, step);
>>>>                    ovsdb_idl_track_clear(idl);
>>>> +                print_and_log("%03d: print_idl[2]", step);
>>>> +                print_idl(idl, step++);
>>>>                } else {
>>>>                    print_idl(idl, step++);
>>>>                }
>>>> ---
>>>>
>>>> By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', you will have
>>>> some output for the commands in the log.
>>>>
>>>>
>>>> The first condition for a 'simple' table is 'true', so we will receive all
>>>> 3 rows from this table and also row from the 'simple6'.
>>>> All 3 rows are in a weak references:
>>>>
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
>>>>
>>>>
>>>> All of them are accessible for a user before the track_clear():
>>>>
>>>> test-ovsdb|test_ovsdb|001: print_idl[1]
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name weak_ref
>>>>
>>>> And after:
>>>>
>>>> test-ovsdb|test_ovsdb|001: print_idl[2]
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>>>> test-ovsdb|test_ovsdb|001: table simple6: name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>>>>
>>>>
>>>> Now condition is changed.  This makes 2 of 3 rows to become orphan rows.
>>>> Note that the update notification only removes orphan rows, but doesn't
>>>> modify the row in simple6, because rows wasn't actually removed, just
>>>> not needed anymore:
>>>>
>>>> test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
>>>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
>>>> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
>>>>
>>>> Track list is empty, but 2 orphan rows should be on a track list as deleted:
>>>
>>> This actually uncovers a new potential issue: the 2 orphan rows are not on the tracked list even without my change.  That's because ovsdb_idl_row_destroy() doesn't get called if the row becomes orphan so it won't be added to 'row->table->track_list'.
>>
>> Hmm.  Thanks for sponning that.
> 
> *spotting
> 
>>
>> On current master these 2 rows appears on a tracked list only after
>> occasional unrelated modification of a row in 'simple6' and that is
>> not good:
>>
>> test-ovsdb|test_ovsdb|002: change conditions
>> test-ovsdb|jsonrpc|unix:socket: send request, method="monitor_cond_change", params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], id=5
>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78":{"delete":null},"5a1d0257-fc27-4046-9e18-25539f122052":{"delete":null}}}]
>> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
>> test-ovsdb|test_ovsdb|003: empty
>> test-ovsdb|test_ovsdb|003: print_idl[1]
>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
>> test-ovsdb|test_ovsdb|003: print_idl[2]
>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
>>
>> test-ovsdb|jsonrpc|unix:socket: send request, method="transact", params=["idltest",{"where":[],"row":{"name":"new_name"},"op":"update","table":"simple6"}], id=6
>> test-ovsdb|jsonrpc|unix:socket: received reply, result=[{"count":1}], id=6
>> test-ovsdb|test_ovsdb|004: {"error":null,"result":[{"count":1}]}
>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"618cef34-e635-4841-9dc7-a14eb2b9627e":{"modify":{"name":"new_name"}}}}]
>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s uuid=5a1d0257-fc27-4046-9e18-25539f122052
>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s uuid=0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78
>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>>
>>
>> It's probably because ovsdb_idl_row_reparse_backrefs() calls
>> ovsdb_idl_row_clear_arcs() with destroy_dsts=false.  So, re-parsing
>> invoked for two deleted rows doesn't actually destroy them.
>>
>>>
>>>>
>>>> test-ovsdb|test_ovsdb|003: empty
>>>>
>>>> But all 3 rows are still accessible for a user from the row in 'simple6' even
>>>> though table 'simple' has only 1 row accessible:
>>>>
>>>> test-ovsdb|test_ovsdb|003: print_idl[1]
>>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>>>>
>>>> And they are still here after the track_clear():
>>>>
>>>> test-ovsdb|test_ovsdb|003: print_idl[2]
>>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>>>>
>>>>
>>>> When we have unrelated update on a row in 'simple6' it finally gets
>>>> re-parsed and we have tracked orphan rows as deleted and they are
>>>> no longer accessible for a user from a row in 'simple6':
>>>>
>>>> test-ovsdb|jsonrpc|unix:socket: received notification, method="update3", params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]
>>>>
>>>> Tracked:
>>>>
>>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s <> uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
>>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s <> uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
>>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>>>>
>>>> Before track_clear():
>>>>
>>>> test-ovsdb|test_ovsdb|005: print_idl[1]
>>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>>>>
>>>> After:
>>>>
>>>> test-ovsdb|test_ovsdb|005: print_idl[2]
>>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>>>>
>>>
>>> To cover all cases above I'm thinking of the following approach:
>>>
>>> 1. Delay ovsdb_idl_row_reparse_backrefs() for deleted rows until ovsdb_idl_parse_update() has finished.
>>>
>>> 2. After ovsdb_idl_parse_update(), walk all deleted rows and reparse backrefs but only for src rows that have not been deleted.
>>>
>>> 3. IDL user (e.g., ovn-controller) processes tracked changes.  At this point:
>>> - non-deleted src rows with a weak reference to orphaned dst rows have ref == NULL.
>>> - deleted src rows with a weak/strong reference to orphaned dst rows have ref != NULL but that's fine because they'll both be cleaned up at the next step below.
>>>
>>> 4. ovsdb_idl_track_clear()
>>>
>>> What do you think?
>>
>> Sounds OK.  I'm not sure how the implementation should look like, though.
>> Will this also fix the issue with not reacking deleted orphaned rows right away?

I think so.

If not, I'll add it as a third patch in the series.

Thanks,
Dumitru

On Tue, Mar 9, 2021 at 9:03 AM Dumitru Ceara <dceara@redhat.com> wrote:
>
> On 3/9/21 5:56 PM, Ilya Maximets wrote:
> > On 3/9/21 5:54 PM, Ilya Maximets wrote:
> >> On 3/9/21 5:26 PM, Dumitru Ceara wrote:
> >>> On 3/9/21 4:07 PM, Ilya Maximets wrote:
> >>>> On 3/3/21 3:40 PM, Dumitru Ceara 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 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@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);
> >>>>>        }
> >>>>
> >>>> By removing the ovsdb_idl_row_reparse_backrefs() you're making an
assumption
> >>>> that we always have a modification event for all rows that are
referencing
> >>>> this one.  But that is not always the case.
> >>>>
> >>>> There is scenario where this re-parsing is needed.  In short, if
you're
> >>>> reducing the scope of conditional monitoring and some rows becomes
> >>>> orphan rows.  In this case ovsdb-server will not generate 'modify'
event
> >>>> for the row that references our newly orphan row, so it will not be
> >>>> re-parsed and these orphan rows will be accessible the row that
references
> >>>> them.
> >>>
> >>> You're right, thanks for spotting this and for the test case.
> >>>
> >>>>
> >>>> Here is a quick'n'dirty testcase, how to reproduce:
> >>>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> >>>> index 702502280..c6bb3f348 100644
> >>>> --- a/tests/ovsdb-idl.at
> >>>> +++ b/tests/ovsdb-idl.at
> >>>> @@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl,
initially populated],
> >>>>    dnl This test creates database with weak references and checks
that orphan
> >>>>    dnl rows created for weak references are not available for
iteration via
> >>>>    dnl list of tracked changes.
> >>>> -OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated,
orphan weak references],
> >>>> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated,
orphan weak references quickndirty],
> >>>>      [['["idltest",
> >>>>          {"op": "insert",
> >>>>           "table": "simple",
> >>>> @@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl,
initially populated, orphan weak refer
> >>>>                                  ["named-uuid", "weak_row1"],
> >>>>                                  ["named-uuid", "weak_row2"]]
> >>>>                               ]}}]']],
> >>>> -  [['condition simple []' \
> >>>> +  [['condition simple [true]' \
> >>>>        'condition simple [["s","==","row1_s"]]' \
> >>>>        '["idltest",
> >>>>          {"op": "update",
> >>>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> >>>> index 36fb9cc08..ace87b13d 100644
> >>>> --- a/tests/test-ovsdb.c
> >>>> +++ b/tests/test-ovsdb.c
> >>>> @@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct
idltest_singleton *sng, int step)
> >>>>    static void
> >>>>    print_idl(struct ovsdb_idl *idl, int step)
> >>>>    {
> >>>> +    const struct idltest_simple6 *s6;
> >>>>        const struct idltest_simple *s;
> >>>>        const struct idltest_link1 *l1;
> >>>>        const struct idltest_link2 *l2;
> >>>> @@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
> >>>>            print_idl_row_link2(l2, step);
> >>>>            n++;
> >>>>        }
> >>>> +    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
> >>>> +        print_idl_row_simple6(s6, step);
> >>>> +        n++;
> >>>> +    }
> >>>>        IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
> >>>>            print_idl_row_singleton(sng, step);
> >>>>            n++;
> >>>> @@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
> >>>>                  /* Print update. */
> >>>>                if (track) {
> >>>> -                print_idl_track(idl, step++);
> >>>> +                print_idl_track(idl, step);
> >>>> +                print_and_log("%03d: print_idl[1]", step);
> >>>> +                print_idl(idl, step);
> >>>>                    ovsdb_idl_track_clear(idl);
> >>>> +                print_and_log("%03d: print_idl[2]", step);
> >>>> +                print_idl(idl, step++);
> >>>>                } else {
> >>>>                    print_idl(idl, step++);
> >>>>                }
> >>>> ---
> >>>>
> >>>> By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', you
will have
> >>>> some output for the commands in the log.
> >>>>
> >>>>
> >>>> The first condition for a 'simple' table is 'true', so we will
receive all
> >>>> 3 rows from this table and also row from the 'simple6'.
> >>>> All 3 rows are in a weak references:
> >>>>
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <>
uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <>
uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <>
uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row:
name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada
1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> >>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name
weak_ref
> >>>>
> >>>>
> >>>> All of them are accessible for a user before the track_clear():
> >>>>
> >>>> test-ovsdb|test_ovsdb|001: print_idl[1]
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <>
uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <>
uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <>
uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row:
name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada
1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> >>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name
weak_ref
> >>>>
> >>>> And after:
> >>>>
> >>>> test-ovsdb|test_ovsdb|001: print_idl[2]
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <>
uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <>
uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <>
uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> >>>> test-ovsdb|test_ovsdb|001: table simple6: name=first_row
weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada
1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> >>>>
> >>>>
> >>>> Now condition is changed.  This makes 2 of 3 rows to become orphan
rows.
> >>>> Note that the update notification only removes orphan rows, but
doesn't
> >>>> modify the row in simple6, because rows wasn't actually removed, just
> >>>> not needed anymore:
> >>>>
> >>>> test-ovsdb|jsonrpc|unix:socket: send request,
method="monitor_cond_change",
params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}],
id=5
> >>>> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
> >>>> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
> >>>>
> >>>> Track list is empty, but 2 orphan rows should be on a track list as
deleted:
> >>>
> >>> This actually uncovers a new potential issue: the 2 orphan rows are
not on the tracked list even without my change.  That's because
ovsdb_idl_row_destroy() doesn't get called if the row becomes orphan so it
won't be added to 'row->table->track_list'.
> >>
> >> Hmm.  Thanks for sponning that.
> >
> > *spotting
> >
> >>
> >> On current master these 2 rows appears on a tracked list only after
> >> occasional unrelated modification of a row in 'simple6' and that is
> >> not good:
> >>
> >> test-ovsdb|test_ovsdb|002: change conditions
> >> test-ovsdb|jsonrpc|unix:socket: send request,
method="monitor_cond_change",
params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}],
id=5
> >> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78":{"delete":null},"5a1d0257-fc27-4046-9e18-25539f122052":{"delete":null}}}]
> >> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
> >> test-ovsdb|test_ovsdb|003: empty
> >> test-ovsdb|test_ovsdb|003: print_idl[1]
> >> test-ovsdb|test_ovsdb|003: table simple: s=row1_s
uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
> >> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> >> test-ovsdb|test_ovsdb|003: print_idl[2]
> >> test-ovsdb|test_ovsdb|003: table simple: s=row1_s
uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
> >> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> >>
> >> test-ovsdb|jsonrpc|unix:socket: send request, method="transact",
params=["idltest",{"where":[],"row":{"name":"new_name"},"op":"update","table":"simple6"}],
id=6
> >> test-ovsdb|jsonrpc|unix:socket: received reply, result=[{"count":1}],
id=6
> >> test-ovsdb|test_ovsdb|004: {"error":null,"result":[{"count":1}]}
> >> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"618cef34-e635-4841-9dc7-a14eb2b9627e":{"modify":{"name":"new_name"}}}}]
> >> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s
uuid=5a1d0257-fc27-4046-9e18-25539f122052
> >> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s
uuid=0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78
> >> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> >> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> >>
> >>
> >> It's probably because ovsdb_idl_row_reparse_backrefs() calls
> >> ovsdb_idl_row_clear_arcs() with destroy_dsts=false.  So, re-parsing
> >> invoked for two deleted rows doesn't actually destroy them.
> >>
> >>>
> >>>>
> >>>> test-ovsdb|test_ovsdb|003: empty
> >>>>
> >>>> But all 3 rows are still accessible for a user from the row in
'simple6' even
> >>>> though table 'simple' has only 1 row accessible:
> >>>>
> >>>> test-ovsdb|test_ovsdb|003: print_idl[1]
> >>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8
b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
> >>>>
> >>>> And they are still here after the track_clear():
> >>>>
> >>>> test-ovsdb|test_ovsdb|003: print_idl[2]
> >>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8
b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
> >>>>
> >>>>
> >>>> When we have unrelated update on a row in 'simple6' it finally gets
> >>>> re-parsed and we have tracked orphan rows as deleted and they are
> >>>> no longer accessible for a user from a row in 'simple6':
> >>>>
> >>>> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]
> >>>>
> >>>> Tracked:
> >>>>
> >>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s <>
uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
> >>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s <>
uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
> >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> >>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> >>>>
> >>>> Before track_clear():
> >>>>
> >>>> test-ovsdb|test_ovsdb|005: print_idl[1]
> >>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> >>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> >>>>
> >>>> After:
> >>>>
> >>>> test-ovsdb|test_ovsdb|005: print_idl[2]
> >>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> >>>>
> >>>
> >>> To cover all cases above I'm thinking of the following approach:
> >>>
> >>> 1. Delay ovsdb_idl_row_reparse_backrefs() for deleted rows until
ovsdb_idl_parse_update() has finished.
> >>>
> >>> 2. After ovsdb_idl_parse_update(), walk all deleted rows and reparse
backrefs but only for src rows that have not been deleted.

Sounds good. This step seems natural because in step 1)
ovsdb_idl_delete_row() will clear forward arcs for deleted rows, so in this
step 2) it won't reparse for deleted src rows.
I *hope* it covers all cases. This part of code becomes really tricky. I
think maybe the tracking code needs a refactor, but I am also worried about
creating even more problems. (I haven't checked yet how ddlog change
tracking is done).

> >>>
> >>> 3. IDL user (e.g., ovn-controller) processes tracked changes.  At
this point:
> >>> - non-deleted src rows with a weak reference to orphaned dst rows
have ref == NULL.
> >>> - deleted src rows with a weak/strong reference to orphaned dst rows
have ref != NULL but that's fine because they'll both be cleaned up at the
next step below.
> >>>
> >>> 4. ovsdb_idl_track_clear()
> >>>
> >>> What do you think?
> >>
> >> Sounds OK.  I'm not sure how the implementation should look like,
though.
> >> Will this also fix the issue with not reacking deleted orphaned rows
right away?
>
> I think so.
>
> If not, I'll add it as a third patch in the series.
>
> Thanks,
> Dumitru
>

On 3/10/21 9:33 AM, Han Zhou wrote:
> 
> 
> On Tue, Mar 9, 2021 at 9:03 AM Dumitru Ceara <dceara@redhat.com 
> <mailto:dceara@redhat.com>> wrote:
>  >
>  > On 3/9/21 5:56 PM, Ilya Maximets wrote:
>  > > On 3/9/21 5:54 PM, Ilya Maximets wrote:
>  > >> On 3/9/21 5:26 PM, Dumitru Ceara wrote:
>  > >>> On 3/9/21 4:07 PM, Ilya Maximets wrote:
>  > >>>> On 3/3/21 3:40 PM, Dumitru Ceara 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 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 <http://1.0.0.1/24>
>  > >>>>>       $ ovn-nbctl lr-del r
>  > >>>>>
>  > >>>>> Reported-at: https://bugzilla.redhat.com/1932642 
> <https://bugzilla.redhat.com/1932642>
>  > >>>>> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for 
> deleted rows.")
>  > >>>>> Signed-off-by: Dumitru Ceara <dceara@redhat.com 
> <mailto:dceara@redhat.com>>
>  > >>>>> ---
>  > >>>>> v2:
>  > >>>>>     - Added ovsdb-idl.at <http://ovsdb-idl.at> test for strong 
> references.
>  > >>>>> ---
>  > >>>>>    lib/ovsdb-idl.c    |    6 ++-
>  > >>>>>    tests/ovsdb-idl.at <http://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);
>  > >>>>>        }
>  > >>>>
>  > >>>> By removing the ovsdb_idl_row_reparse_backrefs() you're making 
> an assumption
>  > >>>> that we always have a modification event for all rows that are 
> referencing
>  > >>>> this one.  But that is not always the case.
>  > >>>>
>  > >>>> There is scenario where this re-parsing is needed.  In short, if 
> you're
>  > >>>> reducing the scope of conditional monitoring and some rows becomes
>  > >>>> orphan rows.  In this case ovsdb-server will not generate 
> 'modify' event
>  > >>>> for the row that references our newly orphan row, so it will not be
>  > >>>> re-parsed and these orphan rows will be accessible the row that 
> references
>  > >>>> them.
>  > >>>
>  > >>> You're right, thanks for spotting this and for the test case.
>  > >>>
>  > >>>>
>  > >>>> Here is a quick'n'dirty testcase, how to reproduce:
>  > >>>> diff --git a/tests/ovsdb-idl.at <http://ovsdb-idl.at> 
> b/tests/ovsdb-idl.at <http://ovsdb-idl.at>
>  > >>>> index 702502280..c6bb3f348 100644
>  > >>>> --- a/tests/ovsdb-idl.at <http://ovsdb-idl.at>
>  > >>>> +++ b/tests/ovsdb-idl.at <http://ovsdb-idl.at>
>  > >>>> @@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, 
> initially populated],
>  > >>>>    dnl This test creates database with weak references and 
> checks that orphan
>  > >>>>    dnl rows created for weak references are not available for 
> iteration via
>  > >>>>    dnl list of tracked changes.
>  > >>>> -OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, 
> orphan weak references],
>  > >>>> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated, 
> orphan weak references quickndirty],
>  > >>>>      [['["idltest",
>  > >>>>          {"op": "insert",
>  > >>>>           "table": "simple",
>  > >>>> @@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl, 
> initially populated, orphan weak refer
>  > >>>>                                  ["named-uuid", "weak_row1"],
>  > >>>>                                  ["named-uuid", "weak_row2"]]
>  > >>>>                               ]}}]']],
>  > >>>> -  [['condition simple []' \
>  > >>>> +  [['condition simple [true]' \
>  > >>>>        'condition simple [["s","==","row1_s"]]' \
>  > >>>>        '["idltest",
>  > >>>>          {"op": "update",
>  > >>>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
>  > >>>> index 36fb9cc08..ace87b13d 100644
>  > >>>> --- a/tests/test-ovsdb.c
>  > >>>> +++ b/tests/test-ovsdb.c
>  > >>>> @@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct 
> idltest_singleton *sng, int step)
>  > >>>>    static void
>  > >>>>    print_idl(struct ovsdb_idl *idl, int step)
>  > >>>>    {
>  > >>>> +    const struct idltest_simple6 *s6;
>  > >>>>        const struct idltest_simple *s;
>  > >>>>        const struct idltest_link1 *l1;
>  > >>>>        const struct idltest_link2 *l2;
>  > >>>> @@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
>  > >>>>            print_idl_row_link2(l2, step);
>  > >>>>            n++;
>  > >>>>        }
>  > >>>> +    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
>  > >>>> +        print_idl_row_simple6(s6, step);
>  > >>>> +        n++;
>  > >>>> +    }
>  > >>>>        IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
>  > >>>>            print_idl_row_singleton(sng, step);
>  > >>>>            n++;
>  > >>>> @@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
>  > >>>>                  /* Print update. */
>  > >>>>                if (track) {
>  > >>>> -                print_idl_track(idl, step++);
>  > >>>> +                print_idl_track(idl, step);
>  > >>>> +                print_and_log("%03d: print_idl[1]", step);
>  > >>>> +                print_idl(idl, step);
>  > >>>>                    ovsdb_idl_track_clear(idl);
>  > >>>> +                print_and_log("%03d: print_idl[2]", step);
>  > >>>> +                print_idl(idl, step++);
>  > >>>>                } else {
>  > >>>>                    print_idl(idl, step++);
>  > >>>>                }
>  > >>>> ---
>  > >>>>
>  > >>>> By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', 
> you will have
>  > >>>> some output for the commands in the log.
>  > >>>>
>  > >>>>
>  > >>>> The first condition for a 'simple' table is 'true', so we will 
> receive all
>  > >>>> 3 rows from this table and also row from the 'simple6'.
>  > >>>> All 3 rows are in a weak references:
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s 
> <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s 
> <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s 
> <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>  > >>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: 
> name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 
> 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>  > >>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name 
> weak_ref
>  > >>>>
>  > >>>>
>  > >>>> All of them are accessible for a user before the track_clear():
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|001: print_idl[1]
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s 
> <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s 
> <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s 
> <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
>  > >>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row: 
> name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 
> 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>  > >>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name 
> weak_ref
>  > >>>>
>  > >>>> And after:
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|001: print_idl[2]
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s 
> <> uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s 
> <> uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
>  > >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s 
> <> uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
>  > >>>> test-ovsdb|test_ovsdb|001: table simple6: name=first_row 
> weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada 
> 1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
>  > >>>>
>  > >>>>
>  > >>>> Now condition is changed.  This makes 2 of 3 rows to become 
> orphan rows.
>  > >>>> Note that the update notification only removes orphan rows, but 
> doesn't
>  > >>>> modify the row in simple6, because rows wasn't actually removed, 
> just
>  > >>>> not needed anymore:
>  > >>>>
>  > >>>> test-ovsdb|jsonrpc|unix:socket: send request, 
> method="monitor_cond_change", 
> params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], 
> id=5
>  > >>>> test-ovsdb|jsonrpc|unix:socket: received notification, 
> method="update3", 
> params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
>  > >>>> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
>  > >>>>
>  > >>>> Track list is empty, but 2 orphan rows should be on a track list 
> as deleted:
>  > >>>
>  > >>> This actually uncovers a new potential issue: the 2 orphan rows 
> are not on the tracked list even without my change.  That's because 
> ovsdb_idl_row_destroy() doesn't get called if the row becomes orphan so 
> it won't be added to 'row->table->track_list'.
>  > >>
>  > >> Hmm.  Thanks for sponning that.
>  > >
>  > > *spotting
>  > >
>  > >>
>  > >> On current master these 2 rows appears on a tracked list only after
>  > >> occasional unrelated modification of a row in 'simple6' and that is
>  > >> not good:
>  > >>
>  > >> test-ovsdb|test_ovsdb|002: change conditions
>  > >> test-ovsdb|jsonrpc|unix:socket: send request, 
> method="monitor_cond_change", 
> params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}], 
> id=5
>  > >> test-ovsdb|jsonrpc|unix:socket: received notification, 
> method="update3", 
> params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78":{"delete":null},"5a1d0257-fc27-4046-9e18-25539f122052":{"delete":null}}}]
>  > >> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
>  > >> test-ovsdb|test_ovsdb|003: empty
>  > >> test-ovsdb|test_ovsdb|003: print_idl[1]
>  > >> test-ovsdb|test_ovsdb|003: table simple: s=row1_s 
> uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
>  > >> test-ovsdb|test_ovsdb|003: table simple6: name=first_row 
> weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
>  > >> test-ovsdb|test_ovsdb|003: print_idl[2]
>  > >> test-ovsdb|test_ovsdb|003: table simple: s=row1_s 
> uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
>  > >> test-ovsdb|test_ovsdb|003: table simple6: name=first_row 
> weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
>  > >>
>  > >> test-ovsdb|jsonrpc|unix:socket: send request, method="transact", 
> params=["idltest",{"where":[],"row":{"name":"new_name"},"op":"update","table":"simple6"}], 
> id=6
>  > >> test-ovsdb|jsonrpc|unix:socket: received reply, 
> result=[{"count":1}], id=6
>  > >> test-ovsdb|test_ovsdb|004: {"error":null,"result":[{"count":1}]}
>  > >> test-ovsdb|jsonrpc|unix:socket: received notification, 
> method="update3", 
> params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"618cef34-e635-4841-9dc7-a14eb2b9627e":{"modify":{"name":"new_name"}}}}]
>  > >> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s 
> uuid=5a1d0257-fc27-4046-9e18-25539f122052
>  > >> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s 
> uuid=0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78
>  > >> test-ovsdb|test_ovsdb|005: table simple6: name=new_name 
> weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
>  > >> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>  > >>
>  > >>
>  > >> It's probably because ovsdb_idl_row_reparse_backrefs() calls
>  > >> ovsdb_idl_row_clear_arcs() with destroy_dsts=false.  So, re-parsing
>  > >> invoked for two deleted rows doesn't actually destroy them.
>  > >>
>  > >>>
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|003: empty
>  > >>>>
>  > >>>> But all 3 rows are still accessible for a user from the row in 
> 'simple6' even
>  > >>>> though table 'simple' has only 1 row accessible:
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|003: print_idl[1]
>  > >>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> 
> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>  > >>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row 
> weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 
> b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>  > >>>>
>  > >>>> And they are still here after the track_clear():
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|003: print_idl[2]
>  > >>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <> 
> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>  > >>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row 
> weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8 
> b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
>  > >>>>
>  > >>>>
>  > >>>> When we have unrelated update on a row in 'simple6' it finally gets
>  > >>>> re-parsed and we have tracked orphan rows as deleted and they are
>  > >>>> no longer accessible for a user from a row in 'simple6':
>  > >>>>
>  > >>>> test-ovsdb|jsonrpc|unix:socket: received notification, 
> method="update3", 
> params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]
>  > >>>>
>  > >>>> Tracked:
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s 
> <> uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
>  > >>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s 
> <> uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
>  > >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name 
> weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>  > >>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>  > >>>>
>  > >>>> Before track_clear():
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|005: print_idl[1]
>  > >>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> 
> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>  > >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name 
> weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>  > >>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
>  > >>>>
>  > >>>> After:
>  > >>>>
>  > >>>> test-ovsdb|test_ovsdb|005: print_idl[2]
>  > >>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <> 
> uuid=b8315694-7855-43e7-a963-ca9a8666682a
>  > >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name 
> weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
>  > >>>>
>  > >>>
>  > >>> To cover all cases above I'm thinking of the following approach:
>  > >>>
>  > >>> 1. Delay ovsdb_idl_row_reparse_backrefs() for deleted rows until 
> ovsdb_idl_parse_update() has finished.
>  > >>>
>  > >>> 2. After ovsdb_idl_parse_update(), walk all deleted rows and 
> reparse backrefs but only for src rows that have not been deleted.
> 
> Sounds good. This step seems natural because in step 1) 
> ovsdb_idl_delete_row() will clear forward arcs for deleted rows, so in 
> this step 2) it won't reparse for deleted src rows.
> I *hope* it covers all cases. This part of code becomes really tricky. I 
> think maybe the tracking code needs a refactor, but I am also worried 
> about creating even more problems. (I haven't checked yet how ddlog 
> change tracking is done).
> 

Thanks, Han, Ilya and Numan for reviews!

I just sent a v3:
https://patchwork.ozlabs.org/project/openvswitch/list/?series=233663

Numan, I didn't add your ack because I had to make quite a few changes 
to address the bug Ilya pointed out above.  I'd appreciate it very much 
if you could have another look at v3.

Thanks,
Dumitru