[ovs-dev] [PATCH v2] python: Send notifications after the transaction ends

Terry Wilson twilson at redhat.com
Wed Mar 3 20:18:35 UTC 2021


On Wed, Mar 3, 2021 at 1:47 PM Brian Haley <haleyb.dev at gmail.com> wrote:
>
> Hi Terry,
>
> Thanks for the patch, comments below.
>
> On 3/3/21 9:24 AM, Terry Wilson wrote:
> > The Python IDL notification mechanism was sending a notification
> > for each processed update in a transaction as it was processed.
> > This causes isseues with multi-row change that contain references
> > to each other.
> >
> > For example, if a Logical_Router_Port is created along with a
> > Gateway_Chassis, and the LRP.gateway_chassis set to that GC, then
> > when the notify() passes the CREATE event for the LRP, the GC will
> > not yet have been processed, so __getattr__ when _uuid_to_row fails
> > to find the GC, will return the default value for LRP.gateway_chassis
> > which is [].
> >
> > This patch has the process_update methods return the notifications
> > that would be produced when a row changes, so they can be queued
> > and sent after all rows have been processed.
> >
> > Fixes: d7d417fcddf9 ("Allow subclasses of Idl to define a notification hook")
> > Signed-off-by: Terry Wilson <twilson at redhat.com>
> > ---
> >   python/ovs/db/idl.py | 41 ++++++++++++++++++++++++-----------------
> >   tests/ovsdb-idl.at   | 22 ++++++++++++++++++++++
> >   tests/test-ovsdb.py  |  7 +++++--
> >   3 files changed, 51 insertions(+), 19 deletions(-)
> >
> > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> > index 5850ac7ab..4b441bd85 100644
> > --- a/python/ovs/db/idl.py
> > +++ b/python/ovs/db/idl.py
> <snip>
> >       def __process_update2(self, table, uuid, row_update):
> > +        """Returns Notice if a column changed, False otherwise."""
> >           row = table.rows.get(uuid)
> > -        changed = False
> >           if "delete" in row_update:
> >               if row:
> >                   del table.rows[uuid]
> > -                self.notify(ROW_DELETE, row)
> > -                changed = True
> > +                return Notice(ROW_DELETE, row)
> >               else:
> >                   # XXX rate-limit
> >                   vlog.warn("cannot delete missing row %s from table"
> > @@ -680,30 +691,27 @@ class Idl(object):
> >               self.__add_default(table, row_update)
> >               changed = self.__row_update(table, row, row_update)
> >               table.rows[uuid] = row
> > -            if changed:
> > -                self.notify(ROW_CREATE, row)
> > +            return Notice(ROW_CREATE, row) if changed else False
>
> I think this should still be under the 'if changed' since it's based on
> the return value of the self.__row_update() call.  See below.
>
> >           elif "modify" in row_update:
> >               if not row:
> >                   raise error.Error('Modify non-existing row')
> >
> >               old_row = self.__apply_diff(table, row, row_update['modify'])
> > -            self.notify(ROW_UPDATE, row, Row(self, table, uuid, old_row))
> > -            changed = True
> > +            return Notice(ROW_UPDATE, row, Row(self, table, uuid, old_row))
> >           else:
> >               raise error.Error('<row-update> unknown operation',
> >                                 row_update)
> > -        return changed
> > +        return False
> >
> >       def __process_update(self, table, uuid, old, new):
> > -        """Returns True if a column changed, False otherwise."""
> > +        """Returns Notice if a column changed, False otherwise."""
> >           row = table.rows.get(uuid)
> >           changed = False
> >           if not new:
> >               # Delete row.
> >               if row:
> >                   del table.rows[uuid]
> > -                changed = True
> > -                self.notify(ROW_DELETE, row)
> > +                return Notice(ROW_DELETE, row)
> >               else:
> >                   # XXX rate-limit
> >                   vlog.warn("cannot delete missing row %s from table %s"
> > @@ -722,8 +730,7 @@ class Idl(object):
> >               changed |= self.__row_update(table, row, new)
> >               if op == ROW_CREATE:
> >                   table.rows[uuid] = row
> > -            if changed:
> > -                self.notify(ROW_CREATE, row)
> > +            return Notice(ROW_CREATE, row) if changed else False
> >           else:
> >               op = ROW_UPDATE
> >               if not row:
> > @@ -737,8 +744,8 @@ class Idl(object):
> >               if op == ROW_CREATE:
> >                   table.rows[uuid] = row
> >               if changed:
> > -                self.notify(op, row, Row.from_json(self, table, uuid, old))
> > -        return changed
> > +                return Notice(op, row, Row.from_json(self, table, uuid, old))
>
> This is a similar call as above, but the return stayed under the 'if
> changed' block.
>
> -Brian

If I'm reading your comment right, the one above it is also "under an
if changed block", it's just in-line with return Notice(ROW_CREATE,
row) if changed else False so that it returns one way or another there
instead of falling through. I changed all of the fall-throughs to the
final "return changed" because changed was boolean and for the True
case, we're now returning a Notice and the '|=' operations don't work
with the different types and there was no common code outside the
if/else trees except that return.



More information about the dev mailing list