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

Han Zhou hzhou at ovn.org
Wed Mar 10 08:33:36 UTC 2021


On Tue, Mar 9, 2021 at 9:03 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 3/9/21 5:56 PM, Ilya Maximets wrote:
> > On 3/9/21 5:54 PM, Ilya Maximets wrote:
> >> On 3/9/21 5:26 PM, Dumitru Ceara wrote:
> >>> On 3/9/21 4:07 PM, Ilya Maximets wrote:
> >>>> On 3/3/21 3:40 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 until the row
has
> >>>>> been removed from the table's hmap and until the IDL client has
> >>>>> processed all tracked changes (ovsdb_idl_track_clear() was called).
> >>>>>
> >>>>> 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>
> >>>>> ---
> >>>>> v2:
> >>>>>     - Added ovsdb-idl.at test for strong references.
> >>>>> ---
> >>>>>    lib/ovsdb-idl.c    |    6 ++-
> >>>>>    tests/ovsdb-idl.at |  118
++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>>>    tests/test-ovsdb.c |   45 ++++++++++++++++++++
> >>>>>    3 files changed, 167 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> >>>>> index 9e1e787..ecd4924 100644
> >>>>> --- a/lib/ovsdb-idl.c
> >>>>> +++ b/lib/ovsdb-idl.c
> >>>>> @@ -163,6 +163,7 @@ static void ovsdb_idl_row_unparse(struct
ovsdb_idl_row *);
> >>>>>    static void ovsdb_idl_row_clear_old(struct ovsdb_idl_row *);
> >>>>>    static void ovsdb_idl_row_clear_new(struct ovsdb_idl_row *);
> >>>>>    static void ovsdb_idl_row_clear_arcs(struct ovsdb_idl_row *,
bool destroy_dsts);
> >>>>> +static void ovsdb_idl_row_reparse_backrefs(struct ovsdb_idl_row
*row);
> >>>>>      static void ovsdb_idl_txn_abort_all(struct ovsdb_idl *);
> >>>>>    static bool ovsdb_idl_txn_extract_mutations(struct ovsdb_idl_row
*,
> >>>>> @@ -367,6 +368,8 @@ ovsdb_idl_clear(struct ovsdb_idl *db)
> >>>>>                    ovsdb_idl_row_unparse(row);
> >>>>>                }
> >>>>>                LIST_FOR_EACH_SAFE (arc, next_arc, src_node,
&row->src_arcs) {
> >>>>> +                ovs_list_remove(&arc->src_node);
> >>>>> +                ovs_list_remove(&arc->dst_node);
> >>>>>                    free(arc);
> >>>>>                }
> >>>>>                /* No need to do anything with dst_arcs: some node
has those arcs
> >>>>> @@ -1234,6 +1237,7 @@ ovsdb_idl_track_clear__(struct ovsdb_idl
*idl, bool flush_all)
> >>>>>                    ovs_list_remove(&row->track_node);
> >>>>>                    ovs_list_init(&row->track_node);
> >>>>>                    if (ovsdb_idl_row_is_orphan(row)) {
> >>>>> +                    ovsdb_idl_row_reparse_backrefs(row);
> >>>>>                        ovsdb_idl_row_unparse(row);
> >>>>>                        if (row->tracked_old_datum) {
> >>>>>                            const struct ovsdb_idl_table_class
*class =
> >>>>> @@ -2156,8 +2160,6 @@ ovsdb_idl_delete_row(struct ovsdb_idl_row
*row)
> >>>>>        ovsdb_idl_row_clear_old(row);
> >>>>>        if (ovs_list_is_empty(&row->dst_arcs)) {
> >>>>>            ovsdb_idl_row_destroy(row);
> >>>>> -    } else {
> >>>>> -        ovsdb_idl_row_reparse_backrefs(row);
> >>>>>        }
> >>>>
> >>>> By removing the ovsdb_idl_row_reparse_backrefs() you're making an
assumption
> >>>> that we always have a modification event for all rows that are
referencing
> >>>> this one.  But that is not always the case.
> >>>>
> >>>> There is scenario where this re-parsing is needed.  In short, if
you're
> >>>> reducing the scope of conditional monitoring and some rows becomes
> >>>> orphan rows.  In this case ovsdb-server will not generate 'modify'
event
> >>>> for the row that references our newly orphan row, so it will not be
> >>>> re-parsed and these orphan rows will be accessible the row that
references
> >>>> them.
> >>>
> >>> You're right, thanks for spotting this and for the test case.
> >>>
> >>>>
> >>>> Here is a quick'n'dirty testcase, how to reproduce:
> >>>> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> >>>> index 702502280..c6bb3f348 100644
> >>>> --- a/tests/ovsdb-idl.at
> >>>> +++ b/tests/ovsdb-idl.at
> >>>> @@ -1174,7 +1174,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl,
initially populated],
> >>>>    dnl This test creates database with weak references and checks
that orphan
> >>>>    dnl rows created for weak references are not available for
iteration via
> >>>>    dnl list of tracked changes.
> >>>> -OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated,
orphan weak references],
> >>>> +OVSDB_CHECK_IDL_TRACK([track, simple idl, initially populated,
orphan weak references quickndirty],
> >>>>      [['["idltest",
> >>>>          {"op": "insert",
> >>>>           "table": "simple",
> >>>> @@ -1196,7 +1196,7 @@ OVSDB_CHECK_IDL_TRACK([track, simple idl,
initially populated, orphan weak refer
> >>>>                                  ["named-uuid", "weak_row1"],
> >>>>                                  ["named-uuid", "weak_row2"]]
> >>>>                               ]}}]']],
> >>>> -  [['condition simple []' \
> >>>> +  [['condition simple [true]' \
> >>>>        'condition simple [["s","==","row1_s"]]' \
> >>>>        '["idltest",
> >>>>          {"op": "update",
> >>>> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> >>>> index 36fb9cc08..ace87b13d 100644
> >>>> --- a/tests/test-ovsdb.c
> >>>> +++ b/tests/test-ovsdb.c
> >>>> @@ -2157,6 +2157,7 @@ print_idl_row_singleton(const struct
idltest_singleton *sng, int step)
> >>>>    static void
> >>>>    print_idl(struct ovsdb_idl *idl, int step)
> >>>>    {
> >>>> +    const struct idltest_simple6 *s6;
> >>>>        const struct idltest_simple *s;
> >>>>        const struct idltest_link1 *l1;
> >>>>        const struct idltest_link2 *l2;
> >>>> @@ -2175,6 +2176,10 @@ print_idl(struct ovsdb_idl *idl, int step)
> >>>>            print_idl_row_link2(l2, step);
> >>>>            n++;
> >>>>        }
> >>>> +    IDLTEST_SIMPLE6_FOR_EACH (s6, idl) {
> >>>> +        print_idl_row_simple6(s6, step);
> >>>> +        n++;
> >>>> +    }
> >>>>        IDLTEST_SINGLETON_FOR_EACH (sng, idl) {
> >>>>            print_idl_row_singleton(sng, step);
> >>>>            n++;
> >>>> @@ -2638,8 +2643,12 @@ do_idl(struct ovs_cmdl_context *ctx)
> >>>>                  /* Print update. */
> >>>>                if (track) {
> >>>> -                print_idl_track(idl, step++);
> >>>> +                print_idl_track(idl, step);
> >>>> +                print_and_log("%03d: print_idl[1]", step);
> >>>> +                print_idl(idl, step);
> >>>>                    ovsdb_idl_track_clear(idl);
> >>>> +                print_and_log("%03d: print_idl[2]", step);
> >>>> +                print_idl(idl, step++);
> >>>>                } else {
> >>>>                    print_idl(idl, step++);
> >>>>                }
> >>>> ---
> >>>>
> >>>> By running make -j8 check TESTSUITEFLAGS='-k quickndirty -v', you
will have
> >>>> some output for the commands in the log.
> >>>>
> >>>>
> >>>> The first condition for a 'simple' table is 'true', so we will
receive all
> >>>> 3 rows from this table and also row from the 'simple6'.
> >>>> All 3 rows are in a weak references:
> >>>>
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <>
uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <>
uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <>
uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row:
name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada
1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> >>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name
weak_ref
> >>>>
> >>>>
> >>>> All of them are accessible for a user before the track_clear():
> >>>>
> >>>> test-ovsdb|test_ovsdb|001: print_idl[1]
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <>
uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <>
uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <>
uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> >>>> test-ovsdb|test_ovsdb|001: table simple: updated columns: s
> >>>> test-ovsdb|test_ovsdb|001: table simple6: inserted row:
name=first_row weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada
1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> >>>> test-ovsdb|test_ovsdb|001: table simple6: updated columns: name
weak_ref
> >>>>
> >>>> And after:
> >>>>
> >>>> test-ovsdb|test_ovsdb|001: print_idl[2]
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row0_s <>
uuid=2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row2_s <>
uuid=1fd88ded-06f0-4fee-a21b-4e552d8a2f50
> >>>> test-ovsdb|test_ovsdb|001: table simple: inserted row: s=row1_s <>
uuid=124ddccb-7eb8-4def-b768-887cb8d84ada
> >>>> test-ovsdb|test_ovsdb|001: table simple6: name=first_row
weak_ref=[124ddccb-7eb8-4def-b768-887cb8d84ada
1fd88ded-06f0-4fee-a21b-4e552d8a2f50 2309fce8-a6dd-46e5-a603-fe1ea1ab6bb0]
> >>>>
> >>>>
> >>>> Now condition is changed.  This makes 2 of 3 rows to become orphan
rows.
> >>>> Note that the update notification only removes orphan rows, but
doesn't
> >>>> modify the row in simple6, because rows wasn't actually removed, just
> >>>> not needed anymore:
> >>>>
> >>>> test-ovsdb|jsonrpc|unix:socket: send request,
method="monitor_cond_change",
params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}],
id=5
> >>>> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"3aa79e4a-d752-4d53-8e16-b62887d73ad8":{"delete":null},"d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a":{"delete":null}}}]
> >>>> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
> >>>>
> >>>> Track list is empty, but 2 orphan rows should be on a track list as
deleted:
> >>>
> >>> This actually uncovers a new potential issue: the 2 orphan rows are
not on the tracked list even without my change.  That's because
ovsdb_idl_row_destroy() doesn't get called if the row becomes orphan so it
won't be added to 'row->table->track_list'.
> >>
> >> Hmm.  Thanks for sponning that.
> >
> > *spotting
> >
> >>
> >> On current master these 2 rows appears on a tracked list only after
> >> occasional unrelated modification of a row in 'simple6' and that is
> >> not good:
> >>
> >> test-ovsdb|test_ovsdb|002: change conditions
> >> test-ovsdb|jsonrpc|unix:socket: send request,
method="monitor_cond_change",
params=[["monid","idltest"],["monid","idltest"],{"simple":[{"where":[["s","==","row1_s"]]}]}],
id=5
> >> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple":{"0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78":{"delete":null},"5a1d0257-fc27-4046-9e18-25539f122052":{"delete":null}}}]
> >> test-ovsdb|jsonrpc|unix:socket: received reply, result={}, id=5
> >> test-ovsdb|test_ovsdb|003: empty
> >> test-ovsdb|test_ovsdb|003: print_idl[1]
> >> test-ovsdb|test_ovsdb|003: table simple: s=row1_s
uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
> >> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> >> test-ovsdb|test_ovsdb|003: print_idl[2]
> >> test-ovsdb|test_ovsdb|003: table simple: s=row1_s
uuid=77db422f-042f-4e1f-9a40-6487e5cdd3ae
> >> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> >>
> >> test-ovsdb|jsonrpc|unix:socket: send request, method="transact",
params=["idltest",{"where":[],"row":{"name":"new_name"},"op":"update","table":"simple6"}],
id=6
> >> test-ovsdb|jsonrpc|unix:socket: received reply, result=[{"count":1}],
id=6
> >> test-ovsdb|test_ovsdb|004: {"error":null,"result":[{"count":1}]}
> >> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"618cef34-e635-4841-9dc7-a14eb2b9627e":{"modify":{"name":"new_name"}}}}]
> >> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s
uuid=5a1d0257-fc27-4046-9e18-25539f122052
> >> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s
uuid=0f94ec57-2fc2-4a70-a0cf-b96b11fe0d78
> >> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[77db422f-042f-4e1f-9a40-6487e5cdd3ae]
> >> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> >>
> >>
> >> It's probably because ovsdb_idl_row_reparse_backrefs() calls
> >> ovsdb_idl_row_clear_arcs() with destroy_dsts=false.  So, re-parsing
> >> invoked for two deleted rows doesn't actually destroy them.
> >>
> >>>
> >>>>
> >>>> test-ovsdb|test_ovsdb|003: empty
> >>>>
> >>>> But all 3 rows are still accessible for a user from the row in
'simple6' even
> >>>> though table 'simple' has only 1 row accessible:
> >>>>
> >>>> test-ovsdb|test_ovsdb|003: print_idl[1]
> >>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8
b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
> >>>>
> >>>> And they are still here after the track_clear():
> >>>>
> >>>> test-ovsdb|test_ovsdb|003: print_idl[2]
> >>>> test-ovsdb|test_ovsdb|003: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|003: table simple6: name=first_row
weak_ref=[3aa79e4a-d752-4d53-8e16-b62887d73ad8
b8315694-7855-43e7-a963-ca9a8666682a d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a]
> >>>>
> >>>>
> >>>> When we have unrelated update on a row in 'simple6' it finally gets
> >>>> re-parsed and we have tracked orphan rows as deleted and they are
> >>>> no longer accessible for a user from a row in 'simple6':
> >>>>
> >>>> test-ovsdb|jsonrpc|unix:socket: received notification,
method="update3",
params=[["monid","idltest"],"00000000-0000-0000-0000-000000000000",{"simple6":{"6fcb8599-4161-4314-901c-2ca4a18686c3":{"modify":{"name":"new_name"}}}}]
> >>>>
> >>>> Tracked:
> >>>>
> >>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row0_s <>
uuid=d1b023f3-3b4d-4bb5-b4d9-cc9791afde7a
> >>>> test-ovsdb|test_ovsdb|005: table simple: deleted row: s=row2_s <>
uuid=3aa79e4a-d752-4d53-8e16-b62887d73ad8
> >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> >>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> >>>>
> >>>> Before track_clear():
> >>>>
> >>>> test-ovsdb|test_ovsdb|005: print_idl[1]
> >>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> >>>> test-ovsdb|test_ovsdb|005: table simple6: updated columns: name
> >>>>
> >>>> After:
> >>>>
> >>>> test-ovsdb|test_ovsdb|005: print_idl[2]
> >>>> test-ovsdb|test_ovsdb|005: table simple: s=row1_s <>
uuid=b8315694-7855-43e7-a963-ca9a8666682a
> >>>> test-ovsdb|test_ovsdb|005: table simple6: name=new_name
weak_ref=[b8315694-7855-43e7-a963-ca9a8666682a]
> >>>>
> >>>
> >>> To cover all cases above I'm thinking of the following approach:
> >>>
> >>> 1. Delay ovsdb_idl_row_reparse_backrefs() for deleted rows until
ovsdb_idl_parse_update() has finished.
> >>>
> >>> 2. After ovsdb_idl_parse_update(), walk all deleted rows and reparse
backrefs but only for src rows that have not been deleted.

Sounds good. This step seems natural because in step 1)
ovsdb_idl_delete_row() will clear forward arcs for deleted rows, so in this
step 2) it won't reparse for deleted src rows.
I *hope* it covers all cases. This part of code becomes really tricky. I
think maybe the tracking code needs a refactor, but I am also worried about
creating even more problems. (I haven't checked yet how ddlog change
tracking is done).

> >>>
> >>> 3. IDL user (e.g., ovn-controller) processes tracked changes.  At
this point:
> >>> - non-deleted src rows with a weak reference to orphaned dst rows
have ref == NULL.
> >>> - deleted src rows with a weak/strong reference to orphaned dst rows
have ref != NULL but that's fine because they'll both be cleaned up at the
next step below.
> >>>
> >>> 4. ovsdb_idl_track_clear()
> >>>
> >>> What do you think?
> >>
> >> Sounds OK.  I'm not sure how the implementation should look like,
though.
> >> Will this also fix the issue with not reacking deleted orphaned rows
right away?
>
> I think so.
>
> If not, I'll add it as a third patch in the series.
>
> Thanks,
> Dumitru
>


More information about the dev mailing list