From 417dd1727ba38966f53e83c69583650f2cf190fc Mon Sep 17 00:00:00 2001 From: Salvatore Orlando Date: Wed, 21 Aug 2013 05:14:54 -0700 Subject: [PATCH] Avoid performing extra query for fetching mac learning binding Bug 1214879 Add a relationship performing eager load in the Port model, thus preventing the 'extend' function from performing an extra query. This patch also replaces assertTrue with assertEqual in unit tests as it needs to evaluate whether the value of the mac_learning_enabled attribute is equal to the boolean literal True, whereas assertTrue verifies that the expression passed to it evaluates to True. This means that any value but False will be enough for the test to pass, which is not correct. Change-Id: I03345ef3a125fdfa1ec7f6c3363d76efc12ced82 --- neutron/plugins/nicira/NeutronPlugin.py | 10 ++----- neutron/plugins/nicira/dbexts/maclearning.py | 29 ++++++++++++------- neutron/tests/unit/nicira/test_maclearning.py | 26 +++++++++++++---- 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/neutron/plugins/nicira/NeutronPlugin.py b/neutron/plugins/nicira/NeutronPlugin.py index a08ccbcb3..131f856c0 100644 --- a/neutron/plugins/nicira/NeutronPlugin.py +++ b/neutron/plugins/nicira/NeutronPlugin.py @@ -1228,8 +1228,6 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2, with context.session.begin(subtransactions=True): neutron_lports = super(NvpPluginV2, self).get_ports( context, filters) - for neutron_lport in neutron_lports: - self._extend_port_mac_learning_state(context, neutron_lport) if (filters.get('network_id') and len(filters.get('network_id')) and self._network_is_external(context, filters['network_id'][0])): # Do not perform check on NVP platform @@ -1422,6 +1420,9 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2, with context.session.begin(subtransactions=True): ret_port = super(NvpPluginV2, self).update_port( context, id, port) + # Save current mac learning state to check whether it's + # being updated or not + old_mac_learning_state = ret_port.get(mac_ext.MAC_LEARNING) # copy values over - except fixed_ips as # they've alreaby been processed port['port'].pop('fixed_ips', None) @@ -1459,15 +1460,11 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2, context, ret_port) # Populate the mac learning attribute new_mac_learning_state = port['port'].get(mac_ext.MAC_LEARNING) - old_mac_learning_state = self._get_mac_learning_state(context, id) if (new_mac_learning_state is not None and old_mac_learning_state != new_mac_learning_state): self._update_mac_learning_state(context, id, new_mac_learning_state) ret_port[mac_ext.MAC_LEARNING] = new_mac_learning_state - elif (new_mac_learning_state is None and - old_mac_learning_state is not None): - ret_port[mac_ext.MAC_LEARNING] = old_mac_learning_state self._delete_port_queue_mapping(context, ret_port['id']) self._process_port_queue_mapping(context, ret_port) LOG.warn(_("Update port request: %s"), port) @@ -1561,7 +1558,6 @@ class NvpPluginV2(db_base_plugin_v2.NeutronDbPluginV2, neutron_db_port = super(NvpPluginV2, self).get_port(context, id, fields) self._extend_port_qos_queue(context, neutron_db_port) - self._extend_port_mac_learning_state(context, neutron_db_port) if self._network_is_external(context, neutron_db_port['network_id']): diff --git a/neutron/plugins/nicira/dbexts/maclearning.py b/neutron/plugins/nicira/dbexts/maclearning.py index 909fc56dd..cf89485c0 100644 --- a/neutron/plugins/nicira/dbexts/maclearning.py +++ b/neutron/plugins/nicira/dbexts/maclearning.py @@ -16,9 +16,13 @@ # import sqlalchemy as sa +from sqlalchemy import orm from sqlalchemy.orm import exc +from neutron.api.v2 import attributes +from neutron.db import db_base_plugin_v2 from neutron.db import model_base +from neutron.db import models_v2 from neutron.openstack.common import log as logging from neutron.plugins.nicira.extensions import maclearning as mac @@ -32,6 +36,13 @@ class MacLearningState(model_base.BASEV2): primary_key=True) mac_learning_enabled = sa.Column(sa.Boolean(), nullable=False) + # Add a relationship to the Port model using the backref attribute. + # This will instruct SQLAlchemy to eagerly load this association. + port = orm.relationship( + models_v2.Port, + backref=orm.backref("mac_learning_state", lazy='joined', + uselist=False, cascade='delete')) + class MacLearningDbMixin(object): """Mixin class for mac learning.""" @@ -41,18 +52,14 @@ class MacLearningDbMixin(object): mac.MAC_LEARNING: port[mac.MAC_LEARNING]} return self._fields(res, fields) - def _get_mac_learning_state(self, context, port_id): - try: - query = self._model_query(context, MacLearningState) - state = query.filter(MacLearningState.port_id == port_id).one() - except exc.NoResultFound: - return None - return state[mac.MAC_LEARNING] + def _extend_port_mac_learning_state(self, port_res, port_db): + state = port_db.mac_learning_state + if state and state.mac_learning_enabled: + port_res[mac.MAC_LEARNING] = state.mac_learning_enabled - def _extend_port_mac_learning_state(self, context, port): - state = self._get_mac_learning_state(context, port['id']) - if state: - port[mac.MAC_LEARNING] = state + # Register dict extend functions for ports + db_base_plugin_v2.NeutronDbPluginV2.register_dict_extend_funcs( + attributes.PORTS, [_extend_port_mac_learning_state]) def _update_mac_learning_state(self, context, port_id, enabled): try: diff --git a/neutron/tests/unit/nicira/test_maclearning.py b/neutron/tests/unit/nicira/test_maclearning.py index 6b169b67f..9534ccae0 100644 --- a/neutron/tests/unit/nicira/test_maclearning.py +++ b/neutron/tests/unit/nicira/test_maclearning.py @@ -91,15 +91,18 @@ class MacLearningDBTestCase(test_db_plugin.NeutronDbPluginV2TestCase): def test_create_with_mac_learning(self): with self.port(arg_list=('mac_learning_enabled',), mac_learning_enabled=True) as port: + # Validate create operation response + self.assertEqual(True, port['port']['mac_learning_enabled']) + # Verify that db operation successfully set mac learning state req = self.new_show_request('ports', port['port']['id'], self.fmt) sport = self.deserialize(self.fmt, req.get_response(self.api)) - self.assertTrue(sport['port']['mac_learning_enabled']) + self.assertEqual(True, sport['port']['mac_learning_enabled']) - def test_create_port_without_mac_learning(self): + def test_create_and_show_port_without_mac_learning(self): with self.port() as port: req = self.new_show_request('ports', port['port']['id'], self.fmt) sport = self.deserialize(self.fmt, req.get_response(self.api)) - self.assertNotIn('mac_learning', sport['port']) + self.assertNotIn('mac_learning_enabled', sport['port']) def test_update_port_with_mac_learning(self): with self.port(arg_list=('mac_learning_enabled',), @@ -107,7 +110,7 @@ class MacLearningDBTestCase(test_db_plugin.NeutronDbPluginV2TestCase): data = {'port': {'mac_learning_enabled': True}} req = self.new_update_request('ports', data, port['port']['id']) res = self.deserialize(self.fmt, req.get_response(self.api)) - self.assertTrue(res['port']['mac_learning_enabled']) + self.assertEqual(True, res['port']['mac_learning_enabled']) def test_update_preexisting_port_with_mac_learning(self): with self.port() as port: @@ -116,8 +119,13 @@ class MacLearningDBTestCase(test_db_plugin.NeutronDbPluginV2TestCase): self.assertNotIn('mac_learning_enabled', sport['port']) data = {'port': {'mac_learning_enabled': True}} req = self.new_update_request('ports', data, port['port']['id']) + # Validate update operation response res = self.deserialize(self.fmt, req.get_response(self.api)) - self.assertTrue(res['port']['mac_learning_enabled']) + self.assertEqual(True, res['port']['mac_learning_enabled']) + # Verify that db operation successfully updated mac learning state + req = self.new_show_request('ports', port['port']['id'], self.fmt) + sport = self.deserialize(self.fmt, req.get_response(self.api)) + self.assertEqual(True, sport['port']['mac_learning_enabled']) def test_list_ports(self): # for this test we need to enable overlapping ips @@ -129,4 +137,10 @@ class MacLearningDBTestCase(test_db_plugin.NeutronDbPluginV2TestCase): self.port(arg_list=('mac_learning_enabled',), mac_learning_enabled=True)): for port in self._list('ports')['ports']: - self.assertTrue(port['mac_learning_enabled']) + self.assertEqual(True, port['mac_learning_enabled']) + + def test_show_port(self): + with self.port(arg_list=('mac_learning_enabled',), + mac_learning_enabled=True) as p: + port_res = self._show('ports', p['port']['id'])['port'] + self.assertEqual(True, port_res['mac_learning_enabled']) -- 2.45.2