[ovs-dev] [PATCH v2 2/2] ovsdb-idl: Preserve references for deleted rows.
Dumitru Ceara
dceara at redhat.com
Tue Mar 9 17:03:03 UTC 2021
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.
>>>
>>> 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