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 <dce...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
