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

Dumitru Ceara dceara at redhat.com
Fri Mar 19 08:42:34 UTC 2021


On 3/19/21 2:41 AM, Han Zhou wrote:
> 
> 
> On Thu, Mar 18, 2021 at 6:27 PM Han Zhou <hzhou at ovn.org
> <mailto:hzhou at ovn.org>> wrote:
>>
>>
>>
>> On Thu, Mar 18, 2021 at 11:07 AM Ilya Maximets <i.maximets at ovn.org
> <mailto: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 <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 at redhat.com
> <mailto:dceara at redhat.com>>
>> > > ---
>> > >  lib/ovsdb-idl.c     |  135 ++++++++++++++++++++++++++--------
>> > >  tests/ovsdb-idl.at <http://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().

Thanks, Ilya and Han, for the review and suggestions.

Renaming 'orphan_rows' definitely makes sense, I'll do that.  However,
regarding using a different list for tables without change tracking I'm
not so sure if that's OK.  Even if the table doesn't have change
tracking its deleted rows should still first go to the "temporary"
orphan_rows list.

For example:

Table T1 (no change tracking):
- Row a = {A.uuid=<U1>}

Table T2 (change tracking enabled):
- Row b = {B.uuid=<U2>, B.ref_to_A=<U1>}

If in a transaction both rows 'a' and 'b' are deleted, and updates are
received in this order, if we wouldn't add 'a' to 'orphan_rows' then
we'd be reparsing backrefs for 'a' immediately leading to:

Row b (updated) = {B.uuid=<U2>, B.ref_to_A=None}

Then the update to delete 'b' is processed:

Row b (deleted) = {B.uuid=<U2>, B.ref_to_A=None}

The above is wrong and breaks consistency if ref_to_A is a strong reference.

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

Actually, there's no leak AFAICT.  ovsdb_idl_process_orphans() moves
rows to 'track_list' in two scenarios:
a. they're not referenced anymore (ovs_list_is_empty(&row->dst_arcs))
b. they're in a table with change tracking enabled.

If the table doesn't have change tracking enabled and the row is not
referenced anymore it will be added to 'track_list' and will be cleaned
up properly by ovsdb_idl_row_destroy_postprocess() (case "a" above).

If the table doesn't have change tracking enabled but the row (let's
call it 'R1') is still referenced (i.e., it became "orphan") then we
shouldn't delete it yet.  When, in the future, the rows referring to
this orphan row are deleted/updated (let's call them 'R2'),
ovsdb_idl_row_clear_arcs(R2, true) is called and the orphan row R1 is
now properly deleted.

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

Thanks, I'm not sure how we can improve the stability/reliability of
this code without major refactoring/rewrite.  Until then though, the
fact that we break consistency of the IDL client's view of the database
is something that needs to be addressed because it's taken for granted
by ovn-controller in quite a few cases.

>> >
>> > Best regards, Ilya Maximets.

Regards,
Dumitru



More information about the dev mailing list