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
