From 876da6f7c809cb17ae7bf507e165fd7765af5b52 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Tue, 11 Aug 2020 13:12:47 -0500 Subject: [PATCH] Add lock when calling idl.run() python-ovs transparently handles DB reconnections during calls to Idl.run(). Since ovsdbapp uses a separate thread for the Connection (so it can handle responding to OVSDB keep-alive echo requests) and for processing RowEvent notifcations, it is possible that a thread could be trying to access OVSDB data at the same time that the Connection thread is updating it. Especially during a reconnect where the in-memory copy of the DB will be completely re-written, this causes a problem. Theoretically, an application that uses ovsdbapp shouldn't access anything in the backend code directly, but since there is now only one supported Backend, that restriction has slipped a bit. If all accesses were in Command objects, mostly there wouldn't be a problem as they would be handled in the Connection thread. But with both ReadOnlyCommands and use of lookup() and anything in idlutils, the main thread can access the in-memory db directly and potentially cause problems. The most common problematic method to be called outside of Command objects is lookup() and ReadOnlyCommands. This patch adds a lock around calls to idl.run() and lookup() and execute() for the ReadOnlyCommand case. row_by_value() is another target, but it is left untouched because it doesn't have access to the API instance and it is *mostly* called from inside Command objects. User code that doesn't will be easy to find because user code basically shouldn't use idlutils. Conflicts: ovsdbapp/tests/unit/backend/test_ovs_idl.py Change-Id: I98c37771883103e1fb0468de9cf85364071993fa Closes-Bug: #1888878 (cherry picked from commit 3cf8a427c0a4dfc248e0778c3b1ee65671d1d07b) --- ovsdbapp/backend/ovs_idl/__init__.py | 3 ++- ovsdbapp/backend/ovs_idl/command.py | 5 +++-- ovsdbapp/backend/ovs_idl/connection.py | 16 ++++++++-------- ovsdbapp/tests/unit/backend/test_ovs_idl.py | 4 ++-- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/ovsdbapp/backend/ovs_idl/__init__.py b/ovsdbapp/backend/ovs_idl/__init__.py index 276f1ede..9eea90ea 100644 --- a/ovsdbapp/backend/ovs_idl/__init__.py +++ b/ovsdbapp/backend/ovs_idl/__init__.py @@ -104,7 +104,8 @@ class Backend(object): def lookup(self, table, record, default=_NO_DEFAULT): try: - return self._lookup(table, record) + with self.ovsdb_connection.lock: + return self._lookup(table, record) except idlutils.RowNotFound: if default is not _NO_DEFAULT: return default diff --git a/ovsdbapp/backend/ovs_idl/command.py b/ovsdbapp/backend/ovs_idl/command.py index 0b886010..acce8ad1 100644 --- a/ovsdbapp/backend/ovs_idl/command.py +++ b/ovsdbapp/backend/ovs_idl/command.py @@ -35,8 +35,9 @@ class BaseCommand(api.Command): def execute(self, check_error=False, log_errors=True, **kwargs): try: if self.READ_ONLY: - self.run_idl(None) - return self.result + with self.api.ovsdb_connection.lock: + self.run_idl(None) + return self.result with self.api.transaction(check_error, log_errors, **kwargs) as t: t.add(self) return self.result diff --git a/ovsdbapp/backend/ovs_idl/connection.py b/ovsdbapp/backend/ovs_idl/connection.py index 347f95c9..b06fefdd 100644 --- a/ovsdbapp/backend/ovs_idl/connection.py +++ b/ovsdbapp/backend/ovs_idl/connection.py @@ -65,7 +65,7 @@ class Connection(object): """ self.timeout = timeout self.txns = TransactionQueue(1) - self.lock = threading.Lock() + self.lock = threading.RLock() self.idl = idl self.thread = None self._is_running = None @@ -99,19 +99,18 @@ class Connection(object): try: self.idl.wait(self.poller) self.poller.fd_wait(self.txns.alert_fileno, poller.POLLIN) - # TODO(jlibosva): Remove next line once losing connection to - # ovsdb is solved. - self.poller.timer_wait(self.timeout * 1000) self.poller.block() - self.idl.run() + with self.lock: + self.idl.run() except Exception as e: # This shouldn't happen, but is possible if there is a bug # in python-ovs errors += 1 LOG.exception(e) if errors <= 3: - self.idl.force_reconnect() - idlutils.wait_for_change(self.idl, self.timeout) + with self.lock: + self.idl.force_reconnect() + idlutils.wait_for_change(self.idl, self.timeout) continue self._is_running = False break @@ -119,7 +118,8 @@ class Connection(object): txn = self.txns.get_nowait() if txn is not None: try: - txn.results.put(txn.do_commit()) + with self.lock: + txn.results.put(txn.do_commit()) except Exception as ex: er = idlutils.ExceptionResult(ex=ex, tb=traceback.format_exc()) diff --git a/ovsdbapp/tests/unit/backend/test_ovs_idl.py b/ovsdbapp/tests/unit/backend/test_ovs_idl.py index 50793fc2..2a930655 100644 --- a/ovsdbapp/tests/unit/backend/test_ovs_idl.py +++ b/ovsdbapp/tests/unit/backend/test_ovs_idl.py @@ -33,13 +33,13 @@ class FakeBackend(ovs_idl.Backend): lookup_table = {'Faketable': idlutils.RowLookup('Faketable', 'name', None)} def start_connection(self, connection): - pass + self.ovsdb_connection = connection class TestBackendOvsIdl(base.TestCase): def setUp(self): super(TestBackendOvsIdl, self).setUp() - self.backend = FakeBackend(mock.Mock()) + self.backend = FakeBackend(mock.MagicMock()) def test_lookup_found(self): row = self.backend.lookup('Faketable', 'Fake1')