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