[ovs-dev] [PATCH] ovsdb-idl: Fix *_is_new() IDL functions
Mark Gray
mark.d.gray at redhat.com
Thu Oct 1 09:30:38 UTC 2020
On 30/09/2020 21:04, Han Zhou wrote:
> On Wed, Sep 30, 2020 at 2:21 AM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> On 9/29/20 8:34 PM, Mark Gray wrote:
>>> Currently all functions of the type *_is_new() always return
>>> 'false'. This patch resolves this issue by using the
>>> 'OVSDB_IDL_CHANGE_INSERT' 'change_seqno' instead of the
>>> 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno' to determine if a row
>>> is new and by resetting the 'OVSDB_IDL_CHANGE_INSERT'
>>> 'change_seqno' on clear.
>>>
>>> Further to this, the code is also updated to match this
>>> behaviour:
>>>
>>> When a row is inserted, the 'OVSDB_IDL_CHANGE_INSERT'
>>> 'change_seqno' is updated to match the new database
>>> change_seqno. The 'OVSDB_IDL_CHANGE_MODIFY' 'change_seqno'
>>> is not set for inserted rows (only for updated rows).
>>>
>>> At the end of a run, ovsdb_idl_db_track_clear() should be
>>> called to clear all tracking information, this includes
>>> resetting all row 'change_seqno' to zero. This will ensure
>>> that subsequent runs will not see a previously 'new' row.
>>>
>>> add_tracked_change_for_references() is updated to only
>>> track rows that reference the current row.
>>>
>>> Signed-off-by: Mark Gray <mark.d.gray at redhat.com>
>>> Suggested-by: Dumitru Ceara <dceara at redhat.com>
>>> Reported-at: https://bugzilla.redhat.com/1883562
>>
>> Hi Mark,
>>
>> Thanks for working on this, it definitely looks like a nasty bug to me!
>>
>> We were lucky to not hit it in ovn-controller before. That's just
>> because all the "update" operations were handled there as "delete" +
>> "insert" but that is not mandatory and might change.>>
>
> Agree. Thanks for this critical finding! It was my bad - commit ca545a787ac
> introduced this bug, which was fixing another bug related to is_new. I
> should have added test cases to avoid the miss. Apologize.
No worries, I will reference this commit in the commit message.
>
> For this fix, I don't understand why we need to check
> OVSDB_IDL_CHANGE_INSERT. I think the root cause is that in
> add_tracked_change_for_references() it updated the OVSDB_IDL_CHANGE_MODIFY
> seqno for the initial inserted *new* row, while it shouldn't. So should the
> fix only include the changes in add_tracked_change_for_references() and
> ovsdb_idl_row_change__()? Why do we need to change the implementation of
> _is_new() itself? Could you explain?
In the case of a newly inserted row, the change_seqno triplet will still
be {X, 0, 0}. It will remain as such until the row is modified, at which
point it will become {X, Y, 0}. This means that the _*is_new() function
will continue to return true until the row is modified which is not
correct behaviour. If we reset the change_seqno triplet on each run, it
will still not work. In the end, it is probably better to use the
OVSDB_IDL_CHANGE_INSERT element to track this.
On a seperate point, I don't expect add_tracked_change_for_references()
will ever get called on insert as nothing will be referenced at that
stage? Am I correct in my understanding?
>>> ---
>>> lib/ovsdb-idl.c | 39 +++++++++++++++++++++++++++------------
>>> ovsdb/ovsdb-idlc.in | 2 +-
>>> 2 files changed, 28 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
>>> index d8f221ca6..3cfd73871 100644
>>> --- a/lib/ovsdb-idl.c
>>> +++ b/lib/ovsdb-idl.c
>>> @@ -1959,6 +1959,11 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db)
>>> free(row->updated);
>>> row->updated = NULL;
>>> }
>>> +
>>> + row->change_seqno[OVSDB_IDL_CHANGE_INSERT] =
>>> + row->change_seqno[OVSDB_IDL_CHANGE_MODIFY] =
>>> + row->change_seqno[OVSDB_IDL_CHANGE_DELETE] = 0;
>>> +
>>> ovs_list_remove(&row->track_node);
>>> ovs_list_init(&row->track_node);
>>> if (ovsdb_idl_row_is_orphan(row) &&
> row->tracked_old_datum) {
>>> @@ -2684,24 +2689,27 @@ ovsdb_idl_process_update2(struct
> ovsdb_idl_table *table,
>>> return OVSDB_IDL_UPDATE_DB_CHANGED;
>>> }
>>>
>>> -/* Recursively add rows to tracked change lists for current row
>>> - * and the rows that reference this row. */
>>> +/* Recursively add rows to tracked change lists for all rows that
> reference
>>> + 'row'. */
>>> static void
>>> add_tracked_change_for_references(struct ovsdb_idl_row *row)
>>> {
>>> - if (ovs_list_is_empty(&row->track_node) &&
>>> - ovsdb_idl_track_is_set(row->table)) {
>>> - ovs_list_push_back(&row->table->track_list,
>>> - &row->track_node);
>>> - row->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
>>> - = row->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
>>> - = row->table->db->change_seqno + 1;
>>> -
>>> const struct ovsdb_idl_arc *arc;
>>> LIST_FOR_EACH (arc, dst_node, &row->dst_arcs) {
>>> - add_tracked_change_for_references(arc->src);
>>> + struct ovsdb_idl_row *ref = arc->src;
>>> +
>>> + if (ovs_list_is_empty(&ref->track_node) &&
>>> + ovsdb_idl_track_is_set(ref->table)) {
>>> + ovs_list_push_back(&ref->table->track_list,
>>> + &ref->track_node);
>>> +
>>> + ref->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
>>> + = ref->table->change_seqno[OVSDB_IDL_CHANGE_MODIFY]
>>> + = ref->table->db->change_seqno + 1;
>>> +
>>> + add_tracked_change_for_references(ref);
>>> + }
>>> }
>>> - }
>>> }
>>>
>>>
>>> @@ -2767,7 +2775,14 @@ ovsdb_idl_row_change__(struct ovsdb_idl_row
> *row, const struct json *row_json,
>>> row->change_seqno[change]
>>> = row->table->change_seqno[change]
>>> = row->table->db->change_seqno + 1;
>>> +
>>> if (table->modes[column_idx] & OVSDB_IDL_TRACK) {
>>> + if (ovs_list_is_empty(&row->track_node) &&
>>> + ovsdb_idl_track_is_set(row->table)) {
>>> + ovs_list_push_back(&row->table->track_list,
>>> + &row->track_node);
>>> + }
>>> +
>>> add_tracked_change_for_references(row);
>>> if (!row->updated) {
>>> row->updated =
> bitmap_allocate(class->n_columns);
>>> diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
>>> index 698fe25f3..89c3eb682 100755
>>> --- a/ovsdb/ovsdb-idlc.in
>>> +++ b/ovsdb/ovsdb-idlc.in
>>> @@ -282,7 +282,7 @@ const struct %(s)s
> *%(s)s_table_track_get_first(const struct %(s)s_table *);
>>> /* Returns true if 'row' was inserted since the last change tracking
> reset. */
>>> static inline bool %(s)s_is_new(const struct %(s)s *row)
>>> {
>>> - return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_MODIFY) == 0;
>>> + return %(s)s_row_get_seqno(row, OVSDB_IDL_CHANGE_INSERT) > 0;
>>
>> This causes some issues with records created by the IDL client (e.g.,
>> ovn-controller). Those are created with ovsdb_idl_txn_insert() and we
>> don't set the OVSDB_IDL_CHANGE_INSERT change_seqno in that case.
>>
>> Essentially *_is_new() will never return true for rows inserted locally.
>>
> Hi Dumitru, I have a different understanding here. I think this is not a
> problem. Whatever changes made locally by the IDL will be discarded when
> committing to server and then rely on the notifications from the server to
> finally update the IDL data, which updates the change_seqno accordingly.
Dumitru appears to be right. There is a failure on one ovn UT with this
change. It gets resolved by the fix that Dumitru suggests.
>
>> Regards,
>> Dumitru
>>
>>> }
>>>
>>> /* Returns true if 'row' was deleted since the last change tracking
> reset. */
>>>
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list