Merge "Add tests that constrain db query count"

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 <http://docs.sqlalchemy.org/en/latest/orm/loading_relationships.html#using-loader-strategies-lazy-loading-eager-loading>`_. For instance a joined relationship can
   be very efficient over others (some examples include `router gateways <https://review.openstack.org/#/c/88665/>`_
   or `network availability zones <https://review.openstack.org/#/c/257086/>`_).
+* 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 <https://review.openstack.org/#/c/88665/>`_
+  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 <https://review.openstack.org/#/c/168214/>`_ 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 <https://review.openstack.org/#/c/257086/>`_ which changed the
+  availability zone code from the incorrect style to a database friendly one.
 
 System development
 ~~~~~~~~~~~~~~~~~~

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))

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."""
 

neutron/tests/unit/plugins/ml2/test_plugin.py

@@ -453,6 +453,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):