From db0cdd9a4ffd64d99a6b012b5d0bf2c5bd4f96f4 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Thu, 7 May 2020 21:34:38 +0000 Subject: [PATCH] Add indexed column support to ovsdbapp Add support for python-ovs multi-column indexes. This is currently disabled by default, though in a simple test of creating 10k Logical_Switches the time went from "I give up after 10 mins" to 4 seconds. The only reason I didn't enable it by default was in case users want to tailor which columns they index. There are slight API changes w/ Backend.ovsdb_connection properties added and start_connection becoming an instance method instead of a classmethod. But since ovsdb_connection was the item being stored on the class and it still is via an overridable property, users shouldn't be broken by the change. Depends-On: https://review.opendev.org/#/c/727232/ Change-Id: I9c7994d50066adf02b0066cb8efb1beb61d2e1f0 --- ovsdbapp/backend/ovs_idl/__init__.py | 113 ++++++++++++++---- ovsdbapp/backend/ovs_idl/idlutils.py | 36 +++++- ovsdbapp/schema/ovn_southbound/impl_idl.py | 3 - ovsdbapp/tests/functional/backend/__init__.py | 0 .../functional/backend/ovs_idl/__init__.py | 0 .../backend/ovs_idl/test_indexing.py | 77 ++++++++++++ .../unit/backend/ovs_idl/test_idlutils.py | 12 ++ ovsdbapp/tests/unit/backend/test_ovs_idl.py | 8 +- .../unit/schema/open_vswitch/test_impl_idl.py | 4 +- 9 files changed, 221 insertions(+), 32 deletions(-) create mode 100644 ovsdbapp/tests/functional/backend/__init__.py create mode 100644 ovsdbapp/tests/functional/backend/ovs_idl/__init__.py create mode 100644 ovsdbapp/tests/functional/backend/ovs_idl/test_indexing.py diff --git a/ovsdbapp/backend/ovs_idl/__init__.py b/ovsdbapp/backend/ovs_idl/__init__.py index fed53a0b..ef7fbab6 100644 --- a/ovsdbapp/backend/ovs_idl/__init__.py +++ b/ovsdbapp/backend/ovs_idl/__init__.py @@ -24,33 +24,97 @@ _NO_DEFAULT = object() class Backend(object): lookup_table = {} - ovsdb_connection = None + _ovsdb_connection = None - def __init__(self, connection, start=True, **kwargs): + def __init__(self, connection, start=True, auto_index=True, **kwargs): super(Backend, self).__init__(**kwargs) + self.ovsdb_connection = connection + if auto_index: + self.autocreate_indices() if start: self.start_connection(connection) - @classmethod - def start_connection(cls, connection): + @property + def ovsdb_connection(self): + return self.__class__._ovsdb_connection + + @ovsdb_connection.setter + def ovsdb_connection(self, connection): + if self.__class__._ovsdb_connection is None: + self.__class__._ovsdb_connection = connection + + def create_index(self, table, *columns): + """Create a multi-column index on a table + + :param table: The table on which to create an index + :type table: string + :param columns: The columns in the index + :type columns: string + """ + + index_name = idlutils.index_name(*columns) + idx = self.tables[table].rows.index_create(index_name) + idx.add_columns(*columns) + + def autocreate_indices(self): + """Create simple one-column indexes + + This creates indexes for all lookup_table entries and for all defined + indexes columns in the OVSDB schema, as long as they are simple + one-column indexes (e.g. 'name') fields. + """ + + tables = set(self.idl.tables.keys()) + # lookup table indices + for table, (lt, col, uuid_col) in self.lookup_table.items(): + if table != lt or not col or uuid_col or table not in tables: + # Just handle simple cases where we are looking up a single + # column on a single table + continue + index_name = idlutils.index_name(col) + try: + idx = self.idl.tables[table].rows.index_create(index_name) + except ValueError: + # index already exists + pass + else: + idx.add_column(col) + LOG.debug("Created index %s", index_name) + tables.remove(table) + + # Simple ovsdb-schema indices + for table in self.idl.tables.values(): + if table.name not in tables: + continue + col = idlutils.get_index_column(table) + if not col: + continue + index_name = idlutils.index_name(col) + try: + idx = table.rows.index_create(index_name) + except ValueError: + pass # index already exists + else: + idx.add_column(col) + LOG.debug("Created index %s", index_name) + tables.remove(table.name) + + def start_connection(self, connection): try: - if cls.ovsdb_connection is None: - cls.ovsdb_connection = connection - cls.ovsdb_connection.start() + self.ovsdb_connection.start() except Exception as e: connection_exception = exceptions.OvsdbConnectionUnavailable( - db_schema=cls.schema, error=e) + db_schema=self.schema, error=e) LOG.exception(connection_exception) raise connection_exception - @classmethod - def restart_connection(cls): - cls.ovsdb_connection.stop() - cls.ovsdb_connection.start() + def restart_connection(self): + self.ovsdb_connection.stop() + self.ovsdb_connection.start() @property def idl(self): - return self.__class__.ovsdb_connection.idl + return self.ovsdb_connection.idl @property def tables(self): @@ -60,8 +124,8 @@ class Backend(object): def create_transaction(self, check_error=False, log_errors=True, **kwargs): return transaction.Transaction( - self, self.__class__.ovsdb_connection, - self.__class__.ovsdb_connection.timeout, + self, self.ovsdb_connection, + self.ovsdb_connection.timeout, check_error, log_errors) def db_create(self, table, **col_values): @@ -119,15 +183,18 @@ class Backend(object): return record.result t = self.tables[table] - try: - if isinstance(record, uuid.UUID): - return t.rows[record] + if isinstance(record, uuid.UUID): try: - uuid_ = uuid.UUID(record) - return t.rows[uuid_] - except ValueError: - # Not a UUID string, continue lookup by other means - pass + return t.rows[record] + except KeyError: + raise idlutils.RowNotFound(table=table, col='uuid', + match=record) + try: + uuid_ = uuid.UUID(record) + return t.rows[uuid_] + except ValueError: + # Not a UUID string, continue lookup by other means + pass except KeyError: # If record isn't found by UUID , go ahead and look up by the table pass diff --git a/ovsdbapp/backend/ovs_idl/idlutils.py b/ovsdbapp/backend/ovs_idl/idlutils.py index ab924b10..4a2640e4 100644 --- a/ovsdbapp/backend/ovs_idl/idlutils.py +++ b/ovsdbapp/backend/ovs_idl/idlutils.py @@ -53,12 +53,42 @@ class RowNotFound(exceptions.OvsdbAppException): message = "Cannot find %(table)s with %(col)s=%(match)s" +def index_name(*columns): + assert columns + return "_".join(sorted(columns)) + + +def index_lookup(table, **matches): + """Find a value in Table by index + + :param table: The table to search in + :type table: ovs.db.schema.TableSchema + :param matches: The column/value pairs of the index to search + :type matches: The types of the columns being matched + :returns: A Row object + """ + idx = table.rows.indexes[index_name(*matches.keys())] + search = table.rows.IndexEntry(**matches) + return next(idx.irange(search, search)) + + +def table_lookup(table, column, match): + return next(r for r in table.rows.values() if getattr(r, column) == match) + + def row_by_value(idl_, table, column, match, default=_NO_DEFAULT): """Lookup an IDL row in a table by column/value""" tab = idl_.tables[table] - for r in tab.rows.values(): - if getattr(r, column) == match: - return r + try: + return index_lookup(tab, **{column: match}) + except KeyError: # no index column + try: + return table_lookup(tab, column, match) + except StopIteration: + pass + except StopIteration: # match not found via index + pass + if default is not _NO_DEFAULT: return default raise RowNotFound(table=table, col=column, match=match) diff --git a/ovsdbapp/schema/ovn_southbound/impl_idl.py b/ovsdbapp/schema/ovn_southbound/impl_idl.py index 916d9ac6..fdf2f8f1 100644 --- a/ovsdbapp/schema/ovn_southbound/impl_idl.py +++ b/ovsdbapp/schema/ovn_southbound/impl_idl.py @@ -22,9 +22,6 @@ class OvnSbApiIdlImpl(ovs_idl.Backend, api.API): 'Chassis': idlutils.RowLookup('Chassis', 'name', None), } - def __init__(self, connection): - super(OvnSbApiIdlImpl, self).__init__(connection) - def chassis_add(self, chassis, encap_types, encap_ip, may_exist=False, **columns): return cmd.ChassisAddCommand(self, chassis, encap_types, encap_ip, diff --git a/ovsdbapp/tests/functional/backend/__init__.py b/ovsdbapp/tests/functional/backend/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ovsdbapp/tests/functional/backend/ovs_idl/__init__.py b/ovsdbapp/tests/functional/backend/ovs_idl/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ovsdbapp/tests/functional/backend/ovs_idl/test_indexing.py b/ovsdbapp/tests/functional/backend/ovs_idl/test_indexing.py new file mode 100644 index 00000000..9d327445 --- /dev/null +++ b/ovsdbapp/tests/functional/backend/ovs_idl/test_indexing.py @@ -0,0 +1,77 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from unittest import mock + +from ovsdbapp.backend.ovs_idl import idlutils +from ovsdbapp.schema.ovn_northbound import impl_idl +from ovsdbapp.tests.functional import base +from ovsdbapp.tests import utils + + +class TestOvnNbIndex(base.FunctionalTestCase): + schemas = ['OVN_Northbound'] + + def setUp(self): + super(TestOvnNbIndex, self).setUp() + self.api = impl_idl.OvnNbApiIdlImpl(self.connection) + + def test_find(self): + # This test will easily time out if indexing isn't used + length = 2000 + basename = utils.get_rand_device_name('testswitch') + with self.api.transaction(check_error=True) as txn: + for i in range(length): + txn.add(self.api.ls_add("%s%d" % (basename, i))) + match = "%s%d" % (basename, length / 2) + sw = self.api.lookup('Logical_Switch', match) + self.assertEqual(sw.name, match) + + def test_default_indices(self): + self.assertTrue(self.api.lookup_table) + for key, (table, col, _) in self.api.lookup_table.items(): + idl_table = self.api.tables[table] + self.assertIn(col, idl_table.rows.indexes) + + +class TestOvnNbWithoutIndex(base.FunctionalTestCase): + schemas = ['OVN_Northbound'] + + # Due to ovsdbapp by default creating singleton connections, it's possible + # that a test is run where we already have a connection/idl set up that + # already has indexing set up on it. + class NewNbApiIdlImpl(impl_idl.OvnNbApiIdlImpl): + @property + def ovsdb_connection(self): + return self._ovsdb_connection + + @ovsdb_connection.setter + def ovsdb_connection(self, connection): + self._ovsdb_connection = connection + + def setUp(self): + super(TestOvnNbWithoutIndex, self).setUp() + self.api = self.NewNbApiIdlImpl(self.connection, start=False, + auto_index=False) + + @mock.patch.object(idlutils, 'table_lookup') + def test_create_index(self, table_lookup): + self.assertFalse(self.api.tables['Logical_Switch'].rows.indexes) + self.api.create_index("Logical_Switch", "name") + self.api.start_connection(self.connection) + name = utils.get_rand_device_name("testswitch") + self.api.ls_add(name).execute(check_error=True) + sw = self.api.lookup('Logical_Switch', name) + self.assertEqual(sw.name, name) + self.assertRaises(idlutils.RowNotFound, self.api.lookup, + 'Logical_Switch', 'nothere') + table_lookup.assert_not_called() diff --git a/ovsdbapp/tests/unit/backend/ovs_idl/test_idlutils.py b/ovsdbapp/tests/unit/backend/ovs_idl/test_idlutils.py index df176721..7dffc555 100644 --- a/ovsdbapp/tests/unit/backend/ovs_idl/test_idlutils.py +++ b/ovsdbapp/tests/unit/backend/ovs_idl/test_idlutils.py @@ -169,3 +169,15 @@ class TestIdlUtils(base.TestCase): mock.sentinel.table_name, FAKE_RECORD_GUID) self.assertEqual(mock.sentinel.row_value, res) + + def test_index_name(self): + expected = { + ('one',): 'one', + ('abc', 'def'): 'abc_def', + ('def', 'abc'): 'abc_def', + ('one', 'two', 'three'): 'one_three_two', + } + for args, result in expected.items(): + self.assertEqual(result, idlutils.index_name(*args)) + + self.assertRaises(AssertionError, idlutils.index_name) diff --git a/ovsdbapp/tests/unit/backend/test_ovs_idl.py b/ovsdbapp/tests/unit/backend/test_ovs_idl.py index 3c36ef74..2d32fe10 100644 --- a/ovsdbapp/tests/unit/backend/test_ovs_idl.py +++ b/ovsdbapp/tests/unit/backend/test_ovs_idl.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import collections from unittest import mock from ovsdbapp.backend import ovs_idl @@ -23,7 +24,9 @@ class FakeRow(object): class FakeTable(object): - rows = {'fake-id-1': FakeRow(uuid='fake-id-1', name='Fake1')} + rows = collections.UserDict({'fake-id-1': + FakeRow(uuid='fake-id-1', name='Fake1')}) + rows.indexes = {} indexes = [] @@ -35,6 +38,9 @@ class FakeBackend(ovs_idl.Backend): def start_connection(self, connection): pass + def autocreate_indices(self): + pass + class TestBackendOvsIdl(base.TestCase): def setUp(self): diff --git a/ovsdbapp/tests/unit/schema/open_vswitch/test_impl_idl.py b/ovsdbapp/tests/unit/schema/open_vswitch/test_impl_idl.py index c199df7c..63867d48 100644 --- a/ovsdbapp/tests/unit/schema/open_vswitch/test_impl_idl.py +++ b/ovsdbapp/tests/unit/schema/open_vswitch/test_impl_idl.py @@ -48,5 +48,5 @@ class TestOvsdbIdl(base.TestCase): def test_init_session(self): conn = mock.MagicMock() - backend = impl_idl.OvsdbIdl(conn, start=False) - self.assertIsNone(backend.ovsdb_connection) + impl_idl.OvsdbIdl(conn, start=False) + conn.start_connection.assert_not_called()