[ovs-dev] [PATCH] ovsdb-idl: Fix *_is_new() IDL functions

Dumitru Ceara dceara at redhat.com
Mon Oct 19 18:59:55 UTC 2020


On 10/19/20 8:42 PM, Ilya Maximets wrote:
> On 10/19/20 8:22 PM, Han Zhou wrote:
>>
>>
>> On Mon, Oct 12, 2020 at 7:32 AM Mark Gray <mark.d.gray at redhat.com <mailto:mark.d.gray at redhat.com>> wrote:
>>>
>>> On 10/10/2020 16:42, Mark Gray wrote:
>>>> On 08/10/2020 19:57, Dumitru Ceara wrote:
>>>>> On 10/2/20 1:13 AM, Han Zhou wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, Oct 1, 2020 at 2:30 AM Mark Gray <mark.d.gray at redhat.com <mailto:mark.d.gray at redhat.com>
>>>>>> <mailto:mark.d.gray at redhat.com <mailto:mark.d.gray at redhat.com>>> wrote:
>>>>>>>
>>>>>>> On 30/09/2020 21:04, Han Zhou wrote:
>>>>>>>> On Wed, Sep 30, 2020 at 2:21 AM Dumitru Ceara <dceara at redhat.com <mailto:dceara at redhat.com>
>>>>>> <mailto:dceara at redhat.com <mailto: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 <mailto:mark.d.gray at redhat.com>
>>>>>> <mailto:mark.d.gray at redhat.com <mailto:mark.d.gray at redhat.com>>>
>>>>>>>>>> Suggested-by: Dumitru Ceara <dceara at redhat.com <mailto:dceara at redhat.com>
>>>>>> <mailto:dceara at redhat.com <mailto: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.
>>>>>>
>>>>>> Hmm, this is not a problem because if a row inserted in a previous run
>>>>>> is not modified/deleted in the current run, it won't be added to
>>>>>> tracking so no one would call _is_new() for the row. However, there will
>>>>>> be a problem when a row is deleted (see below).
>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>> Indeed, we should use OVSDB_IDL_CHANGE_INSERT to check _is_new() because
>>>>>> if row is deleted without any modification after insertion, the
>>>>>> OVSDB_IDL_CHANGE_MODIFY seqno would still be zero when it is being
>>>>>> deleted. In that case calling the _is_new() function would return true
>>>>>> for a deleted row, which is wrong. So, your change LGTM.
>>>>>>
>>>>>>> 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?
>>>>>>>
>>>>>> Yes, I think so. More accurately, it will get called once on the
>>>>>> inserted row but will not trigger any recursive calls.
>>>>>>
>>>>>>>>>> ---
>>>>>>>>>>  lib/ovsdb-idl.c     | 39 +++++++++++++++++++++++++++------------
>>>>>>>>>>  ovsdb/ovsdb-idlc.in <http://ovsdb-idlc.in> <http://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 <http://ovsdb-idlc.in> <http://ovsdb-idlc.in>
>>>>>> b/ovsdb/ovsdb-idlc.in <http://ovsdb-idlc.in> <http://ovsdb-idlc.in>
>>>>>>>>>> index 698fe25f3..89c3eb682 100755
>>>>>>>>>> --- a/ovsdb/ovsdb-idlc.in <http://ovsdb-idlc.in> <http://ovsdb-idlc.in>
>>>>>>>>>> +++ b/ovsdb/ovsdb-idlc.in <http://ovsdb-idlc.in> <http://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.
>>>>>>>
>>>>>> I am confused here. Which UT fails and what's the suggested fix? If the
>>>>>> row is added locally, I don't expect it to be tracked at all in the
>>>>>> current iteration (while it will be in the iteration after the
>>>>>> transaction commit). Did I miss anything?
>>>>>>
>>>>>> Thanks,
>>>>>> Han
>>>>>>
>>>>>
>>>>> Hi Han,
>>>>>
>>>>> One way to see the issue is:
>>>>>
>>>>> make sandbox
>>>>> ovn-sbctl destroy chassis .
>>>>>
>>>>> At this point ovn-controller continuously tries to transact insert
>>>>> Chassis_Private even though Chassis_Private was only updated:
>>>>>
>>>>> 2020-10-08T18:42:27.784Z|00024|jsonrpc|DBG|unix:sb1.ovsdb: send request,
>>>>> method="transact",
>>>>>
>> params=["OVN_Southbound",{"uuid-name":"row3e499288_5f8c_4859_b82a_adcc474eb114","row":{"name":"chassis-1","chassis":["named-uuid","row6534b1df_bcbb_4b91_8eb1_5bc823f1673e"]},"op":"insert","table":"Chassis_Private"},{"uuid-name":"row9d86c159_0be5_4d13_a8d6_a3414a855511","row":{"ip":"127.0.0.1","options":["map",[["csum","true"]]],"chassis_name":"chassis-1","type":"geneve"},"op":"insert","table":"Encap"},{"uuid-name":"row6534b1df_bcbb_4b91_8eb1_5bc823f1673e","row":{"name":"chassis-1","hostname":"sandbox","other_config":["map",[["datapath-type",""],["iface-types","dummy,dummy-internal,dummy-pmd,erspan,geneve,gre,gtpu,internal,ip6erspan,ip6gre,lisp,patch,stt,system,tap,vxlan"],["is-interconn","false"],["ovn-bridge-mappings",""],["ovn-chassis-mac-mappings",""],["ovn-cms-options",""],["ovn-enable-lflow-cache","true"],["ovn-monitor-all","false"]]],"encaps":["named-uuid","row9d86c159_0be5_4d13_a8d6_a3414a855511"],"external_ids":["map",[["datapath-type",""],["iface-types","dummy,dummy-internal,dummy-pmd,erspan,geneve,gre,gtpu,internal,ip6erspan,ip6gre,lisp,patch,stt,system,tap,vxlan"],["is-interconn","false"],["ovn-bridge-mappings",""],["ovn-chassis-mac-mappings",""],["ovn-cms-options",""],["ovn-enable-lflow-cache","true"],["ovn-monitor-all","false"]]]},"op":"insert","table":"Chassis"},{"comment":"ovn-controller:
>>>>> registering chassis 'chassis-1'","op":"comment"}], id=14
>>>>>
>>>>> While, if I set OVSDB_IDL_CHANGE_INSERT in ovsdb_idl_txn_insert() and
>>>>> redo the same steps as above, ovn-controller doesn't try to insert
>>>>> Chassis_Private but instead it updates the record.
>>>>>
>>>>> The only use of _is_new() for potentially locally created records is
>>>>> here afaik:
>>>>> https://github.com/ovn-org/ovn/blob/9d2e8d32fb9865513b70408a665184a67564390d/controller/ovn-controller.c#L184
>>>>>
>>
>> Hi Dumitru and Mark,
>>
>> Thanks for this information and sorry for the delayed response. As discussed in the last OVN meeting, I think the original design of _is_new and _is_deleted were only for evaluating against tracked changes, which always come from server updates.
>>
>> If we make the change so that _is_new() can be used to evaluate against local (uncommitted) changes, it may work but can be confusing. If a row R is inserted by current transaction locally, _is_new(R) returns true, but when the transaction is committed the local copy will be discarded, and if the commit is successful the IDL will finally receive a server notification and the _is_new(R) evaluation will return true again. I think this could cause problems for the code that relies on _is_new(R). Moreover, if we update the OVSDB_IDL_CHANGE_INSERT seq for locally inserted rows, we may need to do the same for locally modified/deleted rows, to be consistent, which needs more careful considerations for the implications of the change.
>>
>> Because of the above concerns, I'd rather suggest to stick to the original design (maybe document it more clearly) that the functions used only for tracked changes.
> 
> One even bigger concern from my side is what process should not take any actions
> until ovsdb-server accepts the change.  Otherwise it might be very tricky to
> handle rejected transactions.  Even if it might be not a likely event in OVN case.
> 

Actually, the single use of _is_new() on potentially locally inserted records
in ovn-controller is exactly to avoid taking actions on records that have just
been created locally without ovsdb-server accepting the transaction [0]:

https://github.com/ovn-org/ovn/blob/master/controller/ovn-controller.c#L184

We avoid using chassis->header_.uuid as a clause in the monitor condition if
chassis is not yet committed to the DB.

>>
>> For the only place the _is_new() currently used for evaluating non-tracked change [0], it was introduced in [1], which we have discussed earlier that it is better to avoid the complex logic for handling system-id change, but let the operator to follow the steps of completely deleting and re-adding a chassis [2]. So I guess this kind of usage would not be necessary at least for existing use cases.

As mentioned above, even if we refactor [1] by implementing [2] I'm not sure if
we get rid of the need of checking sbrec_chassis_is_new(chassis) in
update_sb_monitors().  I have to look again closely at that part.

Regards,
Dumitru

>>
>> P.S. Ilya found an old patch that I submitted a year ago [3] but never merged for fixing the _is_new() problem (that I've forgotten myself), which is similar to v1 but didn't use the "INSERT" seqno. I think the current patch (v1) from Mark is better, so @Ilya Maximets <mailto:i.maximets at ovn.org> please ignore my old patch.
> 
> OK.  I'll mark it as superseded.
> 
>>
>> [0] https://github.com/ovn-org/ovn/blob/9d2e8d32fb9865513b70408a665184a67564390d/controller/ovn-controller.c#L184
>> [1] commit dce1af31b5 ("chassis: Fix chassis_private record updates when the system-id changes.")
>> [2] https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374801.html
>> [3] https://patchwork.ozlabs.org/project/openvswitch/patch/1564100775-80657-1-git-send-email-hzhou8@ebay.com/
>>
>> Thanks,
>> Han
>>
>>>>> I didn't dig further though.
>>>>
>>>> Sorry for the delay. The following test fails:
>>>>
>>>>  90: ovn -- ensure one gw controller restart in HA doesn't bounce the
>>>> master FAILED (ovs-macros.at:220 <http://ovs-macros.at:220>)
>>>>
>>>> <snip>
>>>> ovn.at:12213 <http://ovn.at:12213>: waiting until test 1 = `ovn-sbctl --bare --columns=_uuid
>>>> find Chassis name=gw2 | wc -l`...
>>>> ovn.at:12213 <http://ovn.at:12213>: wait failed after 30 seconds
>>>> ./ovs-macros.at:220 <http://ovs-macros.at:220>: hard failure
>>>> 90. ovn.at:12139 <http://ovn.at:12139>: 90. ovn -- ensure one gw controller restart in HA
>>>> doesn't bounce the master (ovn.at:12139 <http://ovn.at:12139>): FAILED (ovs-macros.at:220 <http://ovs-macros.at:220>)
>>>> <snip>
>>>>
>>>> This is similar failure as the repoducer highlighted by Dumitru above.
>>>> It is resolved by:
>>>>
>>>> +++ b/lib/ovsdb-idl.c
>>>> @@ -4858,6 +4858,11 @@ ovsdb_idl_txn_insert(struct ovsdb_idl_txn *txn,
>>>>      hmap_insert(&row->table->rows, &row->hmap_node, uuid_hash(&row->uuid));
>>>>      hmap_insert(&txn->txn_rows, &row->txn_node, uuid_hash(&row->uuid));
>>>>      ovsdb_idl_add_to_indexes(row);
>>>> +
>>>> +    row->change_seqno[OVSDB_IDL_CHANGE_INSERT]
>>>> +        = row->table->change_seqno[OVSDB_IDL_CHANGE_INSERT]
>>>> +        = row->table->db->change_seqno + 1;
>>>> +
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>> Dumitru
>>>>>
>>>>
>>>
>>> v2 published at
>>> https://mail.openvswitch.org/pipermail/ovs-dev/2020-October/375962.html
> 



More information about the dev mailing list