| Message ID | 20210309143416.14334-1-twilson@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v6] python: Send notifications after the transaction ends | expand |
Acked-by: Brian Haley <haleyb.dev@gmail.com> -Brian On 3/9/21 9:34 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 issues with multi-row changes 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@redhat.com> > --- > python/ovs/db/idl.py | 39 ++++++++++++++++++++++++--------------- > tests/ovsdb-idl.at | 22 ++++++++++++++++++++++ > tests/test-ovsdb.py | 8 ++++++-- > 3 files changed, 52 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..9d3228f23 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,8 @@ 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: >
On 3/9/21 3:34 PM, 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 issues with multi-row changes 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@redhat.com> > --- I'm not too familiar with the python IDL code but the change looks good to me. I also verified and the new tests exercises the scenario described above. All other IDL tests also passed. Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks, Dumitru
On 3/9/21 3:34 PM, 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 issues with multi-row changes 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@redhat.com> > --- > python/ovs/db/idl.py | 39 ++++++++++++++++++++++++--------------- > tests/ovsdb-idl.at | 22 ++++++++++++++++++++++ > tests/test-ovsdb.py | 8 ++++++-- > 3 files changed, 52 insertions(+), 17 deletions(-) Thanks, Terry, Brian, Dumitru and Flavio! Applied to master and backported down to 2.13. Best regards, Ilya Maximets.
On Mon, Mar 15, 2021 at 11:50 AM Ilya Maximets <i.maximets@ovn.org> wrote: > Thanks, Terry, Brian, Dumitru and Flavio! > Applied to master and backported down to 2.13. > > Best regards, Ilya Maximets. Thanks! Can I get it backported down to 2.11 at least? I think the issue goes back to at least 2.10. There's probably very minor conflicts--I'm more than happy to do the backports myself if they'll be accepted. 2.11 is what the upstream OpenStack neutron CI uses for the stable/train branch, which Red Hat OSP 16.1/16.2 use. Terry
On 3/15/21 9:39 PM, Terry Wilson wrote: > On Mon, Mar 15, 2021 at 11:50 AM Ilya Maximets <i.maximets@ovn.org> wrote: > >> Thanks, Terry, Brian, Dumitru and Flavio! >> Applied to master and backported down to 2.13. >> >> Best regards, Ilya Maximets. > > Thanks! Can I get it backported down to 2.11 at least? I think the > issue goes back to at least 2.10. There's probably very minor > conflicts--I'm more than happy to do the backports myself if they'll > be accepted. 2.11 is what the upstream OpenStack neutron CI uses for > the stable/train branch, which Red Hat OSP 16.1/16.2 use. > > Terry > Conflict on 2.12 is very simple, so I can fix it and apply. No problem. For 2.11 is a little bit trickier, so if you will provide the backport, I can apply it. However, I should mention that branch 2.11 officially reached its EOL with the release of OVS 2.15. And while I can apply the patch to branch-2.11, Open vSwitch team will not provide any new official upstream releases from this branch. v2.11.7 was the last one. Best regards, Ilya Maximets.
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..9d3228f23 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,8 @@ 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:
The Python IDL notification mechanism was sending a notification for each processed update in a transaction as it was processed. This causes issues with multi-row changes 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@redhat.com> --- python/ovs/db/idl.py | 39 ++++++++++++++++++++++++--------------- tests/ovsdb-idl.at | 22 ++++++++++++++++++++++ tests/test-ovsdb.py | 8 ++++++-- 3 files changed, 52 insertions(+), 17 deletions(-)