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

[Brian Haley]

On 3/3/21 3:18 PM, Terry Wilson wrote:
> 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.

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