On 10/12/21 15:50, Terry Wilson wrote:
> On Mon, Sep 20, 2021 at 1:18 PM Ilya Maximets <i.maxim...@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.
Sorry for delay, I just returned from my PTO.
>
>>>
>>> On Mon, Sep 20, 2021 at 1:05 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>>>
>>>> On 9/20/21 19:58, Terry Wilson wrote:
>>>>> On Mon, Sep 20, 2021 at 12:29 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>>>>>
>>>>>> On 9/20/21 18:15, Terry Wilson wrote:
>>>>>>> On Thu, Sep 2, 2021 at 10:53 AM Dumitru Ceara <dce...@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 <twil...@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 <dce...@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
>>>>>>> d...@openvswitch.org
>>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
