The problem is that has_ever_connected() doesn't imply that we have
downloaded the db into memory since the _Server stuff was added.
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
