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. 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.
It's guaranteed that update always arrives before the transaction reply. If that is not happening, that should be a bug on the ovsdb-server side or monitor got cancelled or something like this. > > 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