From 4885ef4885de072b0321858a2ae334b708da4bea Mon Sep 17 00:00:00 2001 From: Kevin Benton Date: Mon, 14 Dec 2015 00:44:16 -0800 Subject: [PATCH] Add tests that constrain db query count This patch adds unit tests to ML2 and L3 that ensure that the number of DB calls during list operations for ports, networks, subnets, routers, and floating IPs remains constant regardless of the number of ports. These will prevent changes from slipping in that result in a separate DB query for each object in a list operation (for changes to the extensions used by ML2 and the DVR plugin). Related-Bug: #1525295 Related-Bug: #1513782 Related-Bug: #1525423 Related-Bug: #1525740 Related-Bug: #1526644 Change-Id: I1958fc7c318bbf73238a3ad5be133fa7800c8290 --- doc/source/devref/effective_neutron.rst | 21 +++++++++++ .../tests/unit/db/test_db_base_plugin_v2.py | 33 +++++++++++++++++ neutron/tests/unit/extensions/test_l3.py | 37 +++++++++++++++++++ neutron/tests/unit/plugins/ml2/test_plugin.py | 35 ++++++++++++++++++ 4 files changed, 126 insertions(+) diff --git a/doc/source/devref/effective_neutron.rst b/doc/source/devref/effective_neutron.rst index 13c9bd8300b..fe8d625620d 100644 --- a/doc/source/devref/effective_neutron.rst +++ b/doc/source/devref/effective_neutron.rst @@ -108,6 +108,27 @@ Document common pitfalls as well as good practices done during database developm you model the relationships' loading `strategy `_. For instance a joined relationship can be very efficient over others (some examples include `router gateways `_ or `network availability zones `_). +* If you add a relationship to a Neutron object that will be referenced in the + majority of cases where the object is retrieved, be sure to use the + lazy='joined' parameter to the relationship so the related objects are loaded + as part of the same query. Otherwise, the default method is 'select', which + emits a new DB query to retrieve each related object adversely impacting + performance. For example, see `this patch `_ + which resulted in a significant improvement since router retrieval functions + always include the gateway interface. +* Conversely, do not use lazy='joined' if the relationship is only used in + corner cases because the JOIN statement comes at a cost that may be + significant if the relationship contains many objects. For example, see + `this patch `_ which reduced a + subnet retrieval by ~90% by avoiding a join to the IP allocation table. +* When writing extensions to existing objects (e.g. Networks), ensure that + they are written in a way that the data on the object can be calculated + without additional DB lookup. If that's not possible, ensure the DB lookup + is performed once in bulk during a list operation. Otherwise a list call + for a 1000 objects will change from a constant small number of DB queries + to 1000 DB queries. For example, see + `this patch `_ which changed the + availability zone code from the incorrect style to a database friendly one. System development ~~~~~~~~~~~~~~~~~~ diff --git a/neutron/tests/unit/db/test_db_base_plugin_v2.py b/neutron/tests/unit/db/test_db_base_plugin_v2.py index 587a5a2f843..f302e2a2865 100644 --- a/neutron/tests/unit/db/test_db_base_plugin_v2.py +++ b/neutron/tests/unit/db/test_db_base_plugin_v2.py @@ -22,6 +22,7 @@ import netaddr from oslo_config import cfg from oslo_utils import importutils import six +from sqlalchemy import event from sqlalchemy import orm import testtools from testtools import matchers @@ -40,6 +41,7 @@ from neutron.common import ipv6_utils from neutron.common import test_lib from neutron.common import utils from neutron import context +from neutron.db import api as db_api from neutron.db import db_base_plugin_common from neutron.db import ipam_non_pluggable_backend as non_ipam from neutron.db import l3_db @@ -5867,3 +5869,34 @@ class TestNetworks(testlib_api.SqlTestCase): def test_update_shared_net_used_by_floating_ip(self): self._test_update_shared_net_used( constants.DEVICE_OWNER_FLOATINGIP) + + +class DbOperationBoundMixin(object): + """Mixin to support tests that assert constraints on DB operations.""" + + def setUp(self, *args, **kwargs): + super(DbOperationBoundMixin, self).setUp(*args, **kwargs) + self._db_execute_count = 0 + + def _event_incrementer(*args, **kwargs): + self._db_execute_count += 1 + + engine = db_api.get_engine() + event.listen(engine, 'after_execute', _event_incrementer) + self.addCleanup(event.remove, engine, 'after_execute', + _event_incrementer) + + def _list_and_count_queries(self, resource): + self._db_execute_count = 0 + self.assertNotEqual([], self._list(resource)) + query_count = self._db_execute_count + # sanity check to make sure queries are being observed + self.assertNotEqual(0, query_count) + return query_count + + def _assert_object_list_queries_constant(self, obj_creator, plural): + obj_creator() + before_count = self._list_and_count_queries(plural) + # one more thing shouldn't change the db query count + obj_creator() + self.assertEqual(before_count, self._list_and_count_queries(plural)) diff --git a/neutron/tests/unit/extensions/test_l3.py b/neutron/tests/unit/extensions/test_l3.py index ac0e0ef4eb9..6edcd1f1acb 100644 --- a/neutron/tests/unit/extensions/test_l3.py +++ b/neutron/tests/unit/extensions/test_l3.py @@ -51,10 +51,12 @@ from neutron.plugins.common import constants as service_constants from neutron.tests import base from neutron.tests.common import helpers from neutron.tests import fake_notifier +from neutron.tests.unit.api import test_extensions from neutron.tests.unit.api.v2 import test_base from neutron.tests.unit.db import test_db_base_plugin_v2 from neutron.tests.unit.extensions import base as test_extensions_base from neutron.tests.unit.extensions import test_agent +from neutron.tests.unit.plugins.ml2 import base as ml2_base LOG = logging.getLogger(__name__) @@ -2914,6 +2916,41 @@ class L3AgentDbSepTestCase(L3BaseForSepTests, L3AgentDbTestCaseBase): self.plugin = TestL3NatServicePlugin() +class TestL3DbOperationBounds(test_db_base_plugin_v2.DbOperationBoundMixin, + L3NatTestCaseMixin, + ml2_base.ML2TestFramework): + def setUp(self): + super(TestL3DbOperationBounds, self).setUp() + ext_mgr = L3TestExtensionManager() + self.ext_api = test_extensions.setup_extensions_middleware(ext_mgr) + + def test_router_list_queries_constant(self): + with self.subnet() as s: + self._set_net_external(s['subnet']['network_id']) + + def router_maker(): + ext_info = {'network_id': s['subnet']['network_id']} + self._create_router(self.fmt, _uuid(), + arg_list=('external_gateway_info',), + external_gateway_info=ext_info) + + self._assert_object_list_queries_constant(router_maker, 'routers') + + def test_floatingip_list_queries_constant(self): + with self.floatingip_with_assoc() as flip: + internal_port = self._show('ports', flip['floatingip']['port_id']) + internal_net_id = internal_port['port']['network_id'] + + def float_maker(): + port = self._make_port(self.fmt, internal_net_id) + self._make_floatingip( + self.fmt, flip['floatingip']['floating_network_id'], + port_id=port['port']['id']) + + self._assert_object_list_queries_constant(float_maker, + 'floatingips') + + class L3NatDBTestCaseMixin(object): """L3_NAT_dbonly_mixin specific test cases.""" diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index c0011fb30ad..bace91040d6 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -436,6 +436,41 @@ class TestMl2SubnetsV2(test_plugin.TestSubnetsV2, self.assertEqual(204, res.status_int) +class TestMl2DbOperationBounds(test_plugin.DbOperationBoundMixin, + Ml2PluginV2TestCase): + """Test cases to assert constant query count for list operations. + + These test cases assert that an increase in the number of objects + does not result in an increase of the number of db operations. All + database lookups during a list operation should be performed in bulk + so the number of queries required for 2 objects instead of 1 should + stay the same. + """ + + def make_network(self): + return self._make_network(self.fmt, 'name', True) + + def make_subnet(self): + net = self.make_network() + setattr(self, '_subnet_count', getattr(self, '_subnet_count', 0) + 1) + cidr = '1.%s.0.0/24' % self._subnet_count + return self._make_subnet(self.fmt, net, None, cidr) + + def make_port(self): + net = self.make_network() + return self._make_port(self.fmt, net['network']['id']) + + def test_network_list_queries_constant(self): + self._assert_object_list_queries_constant(self.make_network, + 'networks') + + def test_subnet_list_queries_constant(self): + self._assert_object_list_queries_constant(self.make_subnet, 'subnets') + + def test_port_list_queries_constant(self): + self._assert_object_list_queries_constant(self.make_port, 'ports') + + class TestMl2PortsV2(test_plugin.TestPortsV2, Ml2PluginV2TestCase): def test_update_port_status_build(self):