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.
>
> 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
