On 3/3/21 3:18 PM, Terry Wilson wrote:
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 FalseI 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.
If I had read to the end of the line I would have noticed you didn't change the behavior, I think my eyes were drawn to the difference of this change and the one below, which falls-through to the 'return False'. So my comment has morphed into I think we should keep the flow the same in both, however you decide to do it, if possible.
-Brian
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. -BrianIf 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
