[ovs-dev] [PATCH v3] [python] Avoid sending transactions when the DB is not synced up

Tue Oct 12 15:02:32 UTC 2021

On 10/12/21 15:56, Ilya Maximets wrote:
> On 10/12/21 15:50, Terry Wilson wrote:
>> On Mon, Sep 20, 2021 at 1:18 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>>>
>>> On 9/20/21 20:10, Terry Wilson wrote:
>>>> The problem is that has_ever_connected() doesn't imply that we have
>>>> downloaded the db into memory since the _Server stuff was added.
>>>
>>> Hmm.  That does sound like a bug to me.  has_ever_connected() should
>>> reflect only changes in the actual database, not the '_Server' one.
>>>
>>> This patch itself seems OK.  But the root cause of neutron issues
>>> seems to be that has_ever_connected() is broken.
>>
>> I can investigate has_ever_connected() as a separate patch, but since
>> this patch just mirrors the behavior already in the C IDL code, can we
>> go ahead and merge it?
> 
> Yes, sure.  I'm looking at it right now.  Will push as soon as CI will pass.
> Will also backport down to 2.13.

Hmm.  It doesn't work on 2.14 and 2.13 since there were some code changes.
I'll backport down to 2.15.  If you think this needs to be backported
further, please, submit a new patch for older branches.

Best regards, Ilya Maximets.

> 
> Sorry for delay, I just returned from my PTO.
> 
>>
>>>>
>>>> On Mon, Sep 20, 2021 at 1:05 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>>>>>
>>>>> On 9/20/21 19:58, Terry Wilson wrote:
>>>>>> On Mon, Sep 20, 2021 at 12:29 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>>>>>>>
>>>>>>> On 9/20/21 18:15, Terry Wilson wrote:
>>>>>>>> On Thu, Sep 2, 2021 at 10:53 AM Dumitru Ceara <dceara at redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> On 9/2/21 5:34 PM, Terry Wilson wrote:
>>>>>>>>>> This ports the C IDL change f50714b to the Python IDL:
>>>>>>>>>>
>>>>>>>>>> Until now the code here would happily try to send transactions to the
>>>>>>>>>> database server even if the database connection was not in the correct
>>>>>>>>>> state.  In some cases this could lead to strange behavior, such as sending
>>>>>>>>>> a database transaction for a database that the IDL had just learned did not
>>>>>>>>>> exist on the server.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Terry Wilson <twilson at redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>  python/ovs/db/idl.py | 5 +++++
>>>>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
>>>>>>>>>> index ecae5e143..87ee06cde 100644
>>>>>>>>>> --- a/python/ovs/db/idl.py
>>>>>>>>>> +++ b/python/ovs/db/idl.py
>>>>>>>>>> @@ -1505,6 +1505,11 @@ class Transaction(object):
>>>>>>>>>>          if self != self.idl.txn:
>>>>>>>>>>              return self._status
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Sorry, I should've probably mentioned this in the previous review, but I
>>>>>>>>> missed it.
>>>>>>>>>
>>>>>>>>> Nit: the comment from f50714bf9a72 ("ovsdb-idl: Avoid sending
>>>>>>>>> transactions when the DB is not synced up.") would be nice to have here too:
>>>>>>>>>
>>>>>>>>> # If we're still connecting or re-connecting, don't bother sending a
>>>>>>>>> # transaction.
>>>>>>>>>
>>>>>>>>> I guess this can be fixed up at apply time so:
>>>>>>>>>
>>>>>>>>> Acked-by: Dumitru Ceara <dceara at redhat.com>
>>>>>>>>
>>>>>>>> Just checking to see if we can get this merged? I'm happy to re-spin
>>>>>>>> with the comment, but discussion here and IRC made it sound like it
>>>>>>>> wasn't necessary.
>>>>>>>
>>>>>>> Hi, Terry.  I wanted to apply this patch last week, but I wanted to ask
>>>>>>> if we need to treat that as a bug fix and backport to stable branches
>>>>>>> or is it just a thing that is nice to have?  It's hard to tell what kind
>>>>>>> of issues this missing check is cousing.
>>>>>>>
>>>>>>> Best regards, Ilya Maximets.
>>>>>>
>>>>>> It's a bugfix that needs backports. What can happen is that if neutron
>>>>>> starts up and makes a transaction quickly after connecting, it can
>>>>>> create objects before it has gotten the monitor requests set up.
>>>>>
>>>>> Shouldn't it check for idl.has_ever_connected() before sending transactions?
>>>>> has_ever_connected() should be true only after successful monitor request.
>>>>>
>>>>>> If
>>>>>> you successfully insert an object, then call get_insert_uuid() on it,
>>>>>> you get the UUID of the transaction but cannot actually access the row
>>>>>> in Idl.tables because you aren't getting monitor updates yet.
>>>>>>
>>>>>> I'm a little worried about a more general case with get_insert_uuid()
>>>>>> in that it returns the UUID from the insert txn response, which if it
>>>>>> arrives before the monitor update notification, would still throw a
>>>>>> KeyError if you tried to access idl.tables[...][uuid]. The way the IDL
>>>>>> works (at least the python version), is that after the first call to
>>>>>> commit() where we generate/send the transaction message, we clear all
>>>>>> of the column data from the Row and don't get it back until we get the
>>>>>> monitor update. So it puts us in the weird position of being able to
>>>>>> call get_insert_uuid() immediately after a successful commit(), but
>>>>>> not be sure that we can actually access the Row object by that UUID or
>>>>>> have the column info that we wrote. That is, unless we are guaranteed
>>>>>> to always get monitor updates before transaction insert
>>>>>> responses--which it seems that we at least *mostly* do, but I'm just
>>>>>> not sure if it is guaranteed or a happy accident.
>>>>>>
>>>>>> In any case, this was a case that was easy to solve and was already
>>>>>> solved in the C IDL long ago and needs backports. Thanks!
>>>>>>
>>>>>> Terry
>>>>>>
>>>>>>>>
>>>>>>>>>> +        if self.idl.state != Idl.IDL_S_MONITORING:
>>>>>>>>>> +            self._status = Transaction.TRY_AGAIN
>>>>>>>>>> +            self.__disassemble()
>>>>>>>>>> +            return self._status
>>>>>>>>>> +
>>>>>>>>>>          # If we need a lock but don't have it, give up quickly.
>>>>>>>>>>          if self.idl.lock_name and not self.idl.has_lock:
>>>>>>>>>>              self._status = Transaction.NOT_LOCKED
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> dev mailing list
>>>>>>>> dev at openvswitch.org
>>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 