On Thu, Mar 18, 2021 at 11:07 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>
> 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?
>
I agree with Ilya that the "orphan_rows" is confusing. However,
"deleted_rows" may also be confusing, because the current code also
maintains deleted rows in the track_list of each table (no matter if
tracking is enabled or not). If tracking is enabled, the rows are destroyed
(for real) when ovsdb_idl_track_clear() is called; if not, it is destroyed
in ovsdb_idl_row_destroy_postprocess(). The part of logic was really
confusing. This patch changes that behavior further by introducing this
orphan_rows list as a temporary place (for some situations) before moving
to the track_list. Since this patch already did some refactoring (which is
great), I'd suggest doing a little more to avoid the above confusion: for
deleted rows, shall we just put the untracked rows in a
"deleted_untracked_rows" list (i.e. rename the "orphan_rows" of this
patch), and let the "track_list" only hold the tracked ones, and update the
ovsdb_idl_row_destroy_postprocess() to destroy deleted_untracked_rows
without worrying the track_list. track_list should exist only if tracking
is enabled, and should be taken care by ovsdb_idl_track_clear().
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
