On Wed, Mar 3, 2021 at 1:47 PM Brian Haley <[email protected]> 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 <[email protected]> > > --- > > 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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
