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

Flavio Fernandes flavio at flaviof.com
Thu Mar 4 00:04:44 UTC 2021


Acked-by: Flavio Fernandes <flavio at flaviof.com>


LGTM! Only one nit in the commit message.
Testing changes in Openstack Zuul: https://review.opendev.org/c/openstack/neutron/+/778595

> On Mar 3, 2021, at 4:49 PM, Terry Wilson <twilson at redhat.com> 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.

nit. typo: issues

> 
> 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")

Apr 25 2015..... wow is it me or is time flying by? :)

> Signed-off-by: Terry Wilson <twilson at redhat.com>
> ---
> python/ovs/db/idl.py | 39 ++++++++++++++++++++++++---------------
> tests/ovsdb-idl.at   | 22 ++++++++++++++++++++++
> tests/test-ovsdb.py  |  7 +++++--
> 3 files changed, 51 insertions(+), 17 deletions(-)
> 
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index 5850ac7ab..4226d1cb2 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -12,6 +12,7 @@
> # See the License for the specific language governing permissions and
> # limitations under the License.
> 
> +import collections
> import functools
> import uuid
> 
> @@ -39,6 +40,10 @@ OVSDB_UPDATE2 = 1
> CLUSTERED = "clustered"
> 
> 
> +Notice = collections.namedtuple('Notice', ('event', 'row', 'updates'))
> +Notice.__new__.__defaults__ = (None,)  # default updates=None
> +
> +
> class Idl(object):
>     """Open vSwitch Database Interface Definition Language (OVSDB IDL).
> 
> @@ -614,6 +619,7 @@ class Idl(object):
>             raise error.Error("<table-updates> is not an object",
>                               table_updates)
> 
> +        notices = []
>         for table_name, table_update in table_updates.items():
>             table = tables.get(table_name)
>             if not table:
> @@ -639,7 +645,9 @@ class Idl(object):
>                                       % (table_name, uuid_string))
> 
>                 if version == OVSDB_UPDATE2:
> -                    if self.__process_update2(table, uuid, row_update):
> +                    changes = self.__process_update2(table, uuid, row_update)
> +                    if changes:
> +                        notices.append(changes)
>                         self.change_seqno += 1
>                     continue
> 
> @@ -652,17 +660,20 @@ class Idl(object):
>                     raise error.Error('<row-update> missing "old" and '
>                                       '"new" members', row_update)
> 
> -                if self.__process_update(table, uuid, old, new):
> +                changes = self.__process_update(table, uuid, old, new)
> +                if changes:
> +                    notices.append(changes)
>                     self.change_seqno += 1
> +        for notice in notices:
> +            self.notify(*notice)
> 
>     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"
> @@ -681,29 +692,27 @@ class Idl(object):
>             changed = self.__row_update(table, row, row_update)
>             table.rows[uuid] = row
>             if changed:
> -                self.notify(ROW_CREATE, row)
> +                return Notice(ROW_CREATE, row)
>         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"
> @@ -723,7 +732,7 @@ class Idl(object):
>             if op == ROW_CREATE:
>                 table.rows[uuid] = row
>             if changed:
> -                self.notify(ROW_CREATE, row)
> +                return Notice(ROW_CREATE, row)
>         else:
>             op = ROW_UPDATE
>             if not row:
> @@ -737,8 +746,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))
> +        return False
> 
>     def __check_server_db(self):
>         """Returns True if this is a valid server database, False otherwise."""
> diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
> index 4b4791a7d..e00e67e94 100644
> --- a/tests/ovsdb-idl.at
> +++ b/tests/ovsdb-idl.at
> @@ -1486,6 +1486,28 @@ m4_define([OVSDB_CHECK_IDL_NOTIFY],
>    [OVSDB_CHECK_IDL_PY([$1], [], [$2], [$3], [notify $4], [$5])
>     OVSDB_CHECK_IDL_SSL_PY([$1], [], [$2], [$3], [notify $4], [$5])])
> 
> +OVSDB_CHECK_IDL_NOTIFY([simple link idl verify notify],
> +  [['track-notify' \
> +    '["idltest",
> +       {"op": "insert",
> +       "table": "link1",
> +       "row": {"i": 1, "k": ["named-uuid", "l1row"], "l2": ["set", [["named-uuid", "l2row"]]]},
> +       "uuid-name": "l1row"},
> +      {"op": "insert",
> +       "table": "link2",
> +       "uuid-name": "l2row",
> +       "row": {"i": 2, "l1": ["set", [["named-uuid", "l1row"]]]}}]']],
> +[[000: empty
> +000: event:create, row={uuid=<0>}, updates=None
> +000: event:create, row={uuid=<1>}, updates=None
> +001: {"error":null,"result":[{"uuid":["uuid","<2>"]},{"uuid":["uuid","<3>"]}]}
> +002: event:create, row={i=1 uuid=<2> l2=[<3>]}, updates=None
> +002: event:create, row={i=2 uuid=<3> l1=[<2>]}, updates=None
> +002: i=1 k=1 ka=[] l2=2 uuid=<2>
> +002: i=2 l1=1 uuid=<3>
> +003: done
> +]])
> +
> OVSDB_CHECK_IDL_NOTIFY([simple idl verify notify],
>   [['track-notify' \
>     '["idltest",
> diff --git a/tests/test-ovsdb.py b/tests/test-ovsdb.py
> index a19680274..0ea759195 100644
> --- a/tests/test-ovsdb.py
> +++ b/tests/test-ovsdb.py
> @@ -162,6 +162,8 @@ def get_simple_printable_row_string(row, columns):
>             if isinstance(value, dict):
>                 value = sorted((row_to_uuid(k), row_to_uuid(v))
>                                for k, v in value.items())
> +            if isinstance(value, (list, tuple)):
> +                value = sorted((row_to_uuid(v) for v in value))
>             s += "%s=%s " % (column, value)
>     s = s.strip()
>     s = re.sub('""|,|u?\'', "", s)
> @@ -172,9 +174,10 @@ def get_simple_printable_row_string(row, columns):
>     return s
> 
> 
> -def get_simple_table_printable_row(row):
> +def get_simple_table_printable_row(row, *additional_columns):
>     simple_columns = ["i", "r", "b", "s", "u", "ia",
>                       "ra", "ba", "sa", "ua", "uuid"]
> +    simple_columns.extend(additional_columns)
>     return get_simple_printable_row_string(row, simple_columns)
> 
> 
> @@ -637,7 +640,7 @@ def do_idl(schema_file, remote, *commands):
>     def mock_notify(event, row, updates=None):
>         output = "%03d: " % step
>         output += "event:" + str(event) + ", row={"
> -        output += get_simple_table_printable_row(row) + "}, updates="
> +        output += get_simple_table_printable_row(row, 'l2', 'l1') + "}, updates="
>         if updates is None:
>             output += "None"
>         else:
> --
> 2.17.1
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list