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

[Han Zhou]

On Thu, Mar 18, 2021 at 6:27 PM Han Zhou <hzhou at ovn.org> wrote:
>
>
>
> On Thu, Mar 18, 2021 at 11:07 AM Ilya Maximets <i.maximets at 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 <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?
> >
>
> 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?
>

I took a second look at the patch, and it seems when tracking is not
enabled it would result in the rows in "orphan_rows" leaked, because in
ovsdb_idl_process_orphans() the rows are moved to "track_list" ONLY IF
tracking is enabled for the table. If I am correct, would the above
suggested refactoring help avoiding such kinds of problems?

Thanks,
Han

> >
> > 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.