[ovs-dev] [PATCH v3 2/3] ovsdb-idl: Preserve references for deleted rows.

[Ilya Maximets]

On 3/12/21 1:07 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 for deleted rows
> until all updates in the current run have been processed.
> 
> Without this change, in scenario 2 above, the tracked changes for table
> B would be:
> - deleted records: b = {B._uuid=<U2>, B.ref_a=[]}
> 
> In particular, for strong references, row 'a' can never be deleted in
> a transaction that happens strictly before row 'b' is deleted.  In some
> cases [0] both rows are deleted in the same transaction and having
> B.ref_a=[] would violate the integrity of the database from client
> perspective.  This would force the client to always validate that
> strong reference fields are non-NULL.  This is not really an option
> because the information in the original reference is required for
> incrementally processing the record deletion.
> 
> [0] with ovn-monitor-all=true, the following command triggers a crash
>     in ovn-controller because a strong reference field becomes NULL:
>     $ ovn-nbctl --wait=hv -- lr-add r -- lrp-add r rp 00:00:00:00:00:01 1.0.0.1/24
>     $ ovn-nbctl lr-del r
> 
> Reported-at: https://bugzilla.redhat.com/1932642
> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted rows.")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
>  lib/ovsdb-idl.c     |  135 ++++++++++++++++++++++++++--------
>  tests/ovsdb-idl.at  |  203 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/test-ovsdb.c  |   50 +++++++++++++
>  tests/test-ovsdb.py |   14 ++++
>  4 files changed, 370 insertions(+), 32 deletions(-)
> 
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 9e1e787..1542da9 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -92,6 +92,7 @@ struct ovsdb_idl {
>      struct ovsdb_idl_txn *txn;
>      struct hmap outstanding_txns;
>      bool verify_write_only;
> +    struct ovs_list orphan_rows; /* All deleted but still referenced rows. */

The name here is a bit confusing.  I mean it's not a list
of orphan rows, at least it's not a list of all the orphan
rows.  So, we should, probably, rename it to something like
'deleted_rows' instead and also rename all related functions.

What do you think?


For the rest of the code... It seems fine, but it's really hard
to review and I still have a feeling that something might go
wrong (not more wrong than in current code, though), but I can't
think of any example.  So, it's, probably, OK.

Best regards, Ilya Maximets.