On 11/30/20 5:22 AM, Han Zhou wrote:
>
>
> On Tue, Nov 24, 2020 at 4:16 AM Dumitru Ceara <[email protected]
> <mailto:[email protected]>> wrote:
>>
>> It's possible that the IDL client processes multiple jsonrpc updates
>> in a single ovsdb_idl_run().
>>
>> Considering the following updates processed in a single IDL run:
>> 1. Delete row R1 from table A while R1 is also referenced by row R2 from
>> table B:
>> - because row R2 still refers to row R1, this will create an orphan
>> R1 but also sets row->tracked_old_datum to report to the IDL client
>> that the row has been deleted.
>> 2. Insert row R1 to table A.
>> - because orphan R1 already existed in the IDL, it will be reused.
>> - R1 still has row->tracked_old_datum set (and may also be on the
>> table->track_list).
>> 3. Delete row R2 from table B and row R1 from table A.
>> - row->tracked_old_datum is set again but the previous
>> tracked_old_datum was never freed.
>>
>> IDL clients don't currently use the deleted old_datum values but they
>
> The old_datum which is set to tracked_old_datum is in fact used when the
> client accesses the data of the tracked deleted row. At least it was
> used in ovn-controller.
>
You're right, I'll update the commit log.
>> might decide to do that in the future so when multiple delete
>> operations are received for a row, always track the first one as that
>> will match the contents of the row the IDL client knew about.
>>
>> Fixes: 72aeb243a52a ("ovsdb-idl: Tracking - preserve data for deleted
> rows.")
>> Signed-off-by: Dumitru Ceara <[email protected]
> <mailto:[email protected]>>
>> ---
>> lib/ovsdb-idl.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
>> index 6334061..bbc22e4 100644
>> --- a/lib/ovsdb-idl.c
>> +++ b/lib/ovsdb-idl.c
>> @@ -3219,7 +3219,7 @@ ovsdb_idl_row_clear_old(struct ovsdb_idl_row *row)
>> {
>> ovs_assert(row->old_datum == row->new_datum);
>> if (!ovsdb_idl_row_is_orphan(row)) {
>> - if (ovsdb_idl_track_is_set(row->table)) {
>> + if (ovsdb_idl_track_is_set(row->table) &&
> !row->tracked_old_datum) {
>> row->tracked_old_datum = row->old_datum;
>> } else {
>> const struct ovsdb_idl_table_class *class =
> row->table->class_;
>>
>
> Thanks for the patch. The change looks correct (at least not harmful),
> but I'm not sure if I understand the scenario completely. In step 2, how
> could someone insert a row that has already been deleted? Does the
> client specify the row UUID in this case? Would it be better to add a
> test case which could trigger the problem?
>
This is from the IDL's perspective. A "delete"/"insert" update message
received from ovsdb-server doesn't necessarily mean that row R1 was
actually deleted/inserted in the DB. It means the row has to be
deleted/inserted from the client's view of the DB.
One such case is when monitor conditions change.
As a matter of fact I managed to replicate it now without OVN so I'll be
adding a test case for it in the new revision.
> Thanks,
> Han
Thanks,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
