commit 218cf7cc0b7da4a64ff30cdeaf8bf1cbb8ec66a2 Author: Terry Wilson Date: Wed Sep 23 18:46:53 2020 +0000 Avoid race condition with RowEvent handling Row objects are inherently tied to the transaction processing of the Idl. This means that if you have a reference to a Row in one thread, and another thread starts a transaction that modifies that row, the Row can change w/o you knowing it. This is especially noticeable when using the RowEventHandler. It is possible for a Row that is passed to notify() by the Idl class to change between being matched and the RowEvent.run() method being called. This patch returns an immutable representation of the row by using the same class that custom indexes use for searching. This should be safe to pass to other threads. Conflicts: ovsdbapp/backend/ovs_idl/idlutils.py Change-Id: Iff6e9fdfe032e1c007e35fc7b018114e66acc895 Closes-Bug: #1896816 (cherry picked from commit dc09d6696fe175e53c0c1e4a4263d1a3e1513c9c) diff --git a/ovsdbapp/backend/ovs_idl/event.py b/ovsdbapp/backend/ovs_idl/event.py index 4fbe960..33491d1 100644 --- a/ovsdbapp/backend/ovs_idl/event.py +++ b/ovsdbapp/backend/ovs_idl/event.py @@ -47,3 +47,9 @@ class RowEvent(ovsdb_event.RowEvent): # pylint: disable=abstract-method class WaitEvent(RowEvent, ovsdb_event.WaitEvent): pass + + +class RowEventHandler(ovsdb_event.RowEventHandler): + def notify(self, event, row, updates=None): + row = idlutils.frozen_row(row) + super(RowEventHandler, self).notify(event, row, updates) diff --git a/ovsdbapp/backend/ovs_idl/idlutils.py b/ovsdbapp/backend/ovs_idl/idlutils.py index 025f567..9a8b6cf 100644 --- a/ovsdbapp/backend/ovs_idl/idlutils.py +++ b/ovsdbapp/backend/ovs_idl/idlutils.py @@ -19,6 +19,25 @@ import sys import time import uuid +# custom_index.py added in ovs 2.10. We just need the IndexEntryClass function +# which is copied from OVS below since the current lower constraint is < 2.10 +try: + from ovs.db.custom_index import IndexEntryClass +except ImportError: + from ovs.db import data + + def IndexEntryClass(table): + def defaults_uuid_to_row(atom, base): + return atom.value + + columns = ['uuid'] + list(table.columns.keys()) + cls = collections.namedtuple(table.name, columns) + cls._table = table + cls.__new__.__defaults__ = (None,) + tuple( + data.Datum.default(c.type).to_python(defaults_uuid_to_row) + for c in table.columns.values()) + return cls + from ovs.db import idl from ovs import jsonrpc from ovs import poller @@ -321,3 +340,23 @@ def row2str(row): return "%s(%s)" % (row._table.name, ", ".join( "%s=%s" % (col, idl._row_to_uuid(getattr(row, col))) for col in row._table.columns if hasattr(row, col))) + + +def frozen_row(row): + """Return a namedtuple representation of a idl.Row object + + Row objects are inherently tied to the transaction processing of the Idl. + This means that if you have a reference to a Row in one thread, and + another thread starts a transaction that modifies that row, the Row can + change w/o you knowing it. This is especially noticeable when using the + RowEventHandler. It is possible for a Row that is passed to notify() by + the Idl class to change between being matched and the RowEvent.run() + method being called. This returns an immutable representation of the row + by using the same class that custom indexes use for searching. This + should be safe to pass to other threads. + """ + IndexEntry = IndexEntryClass(row._table) + return IndexEntry( + uuid=row.uuid, + **{col: getattr(row, col) + for col in row._table.columns if hasattr(row, col)}) diff --git a/ovsdbapp/event.py b/ovsdbapp/event.py index 285fc89..047a755 100644 --- a/ovsdbapp/event.py +++ b/ovsdbapp/event.py @@ -150,6 +150,17 @@ class RowEventHandler(object): LOG.exception('Unexpected exception in notify_loop') def notify(self, event, row, updates=None): + """Method for calling backend to call for each DB update + + :param event: Backend representation of event type, e.g. + create, update, delete + :param row: Backend representation of a Row object. If it is not + immutable, it should be converted or guaranteed not to + be changed in other threads. + :param updates: Backend representation of updates to a Row. e.g. + a Row object with just changed attributes, a + dictionary of changes, etc. + """ matching = self.matching_events( event, row, updates) for match in matching: diff --git a/ovsdbapp/tests/functional/schema/ovn_southbound/test_impl_idl.py b/ovsdbapp/tests/functional/schema/ovn_southbound/test_impl_idl.py index 9d794fd..b585ad9 100644 --- a/ovsdbapp/tests/functional/schema/ovn_southbound/test_impl_idl.py +++ b/ovsdbapp/tests/functional/schema/ovn_southbound/test_impl_idl.py @@ -10,8 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +from ovsdbapp.backend.ovs_idl import event as ovsdb_event from ovsdbapp.backend.ovs_idl import idlutils -from ovsdbapp import event as ovsdb_event from ovsdbapp.schema.ovn_northbound import impl_idl as nbidl from ovsdbapp.schema.ovn_southbound import impl_idl from ovsdbapp.tests.functional import base