From: Robert Kukura Date: Thu, 29 Jan 2015 22:13:00 +0000 (-0500) Subject: ML2: Use same port binding logic for DVR ports as non-DVR ports X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=f9cdb351c991adca4b0ed5fdcec1dc1d765adbda;p=openstack-build%2Fneutron-build.git ML2: Use same port binding logic for DVR ports as non-DVR ports DVR ports are now bound using the same function, Ml2Plugin._bind_port_if_needed(), that is used to bind non-DVR ports, so that concurrent binding attempts are properly handled and mechanism driver update_port_precommit() and update_port_postcommit() methods are called. The Ml2Plugin._commit_dvr_port_binding() function is eliminated, and the DvrPortContext class has been folded into the PortContext class, reducing duplicated logic. A followup patch will address the duplication of ML2 DB schema for DVR and further reduce the duplicated and special-case port binding logic supporting DVR. Closes-Bug: 1415526 Closes-Bug: 1416783 Partial-Bug: 1367391 Change-Id: Ic32241297c5f8c67dc77d0af836b1cc0a5df988a --- diff --git a/neutron/plugins/ml2/driver_context.py b/neutron/plugins/ml2/driver_context.py index 3c946d3b5..c0795e01a 100644 --- a/neutron/plugins/ml2/driver_context.py +++ b/neutron/plugins/ml2/driver_context.py @@ -103,6 +103,11 @@ class PortContext(MechanismDriverContext, api.PortContext): @property def status(self): + # REVISIT(rkukura): Eliminate special DVR case as part of + # resolving bug 1367391? + if self._port['device_owner'] == constants.DEVICE_OWNER_DVR_INTERFACE: + return self._binding.status + return self._port['status'] @property @@ -165,6 +170,11 @@ class PortContext(MechanismDriverContext, api.PortContext): @property def host(self): + # REVISIT(rkukura): Eliminate special DVR case as part of + # resolving bug 1367391? + if self._port['device_owner'] == constants.DEVICE_OWNER_DVR_INTERFACE: + return self._binding.host + return self._port.get(portbindings.HOST_ID) @property @@ -203,26 +213,3 @@ class PortContext(MechanismDriverContext, api.PortContext): def release_dynamic_segment(self, segment_id): return self._plugin.type_manager.release_dynamic_segment( self._plugin_context.session, segment_id) - - -class DvrPortContext(PortContext): - - def __init__(self, plugin, plugin_context, port, network, binding, - original_port=None): - super(DvrPortContext, self).__init__( - plugin, plugin_context, port, network, binding, - original_port=original_port) - - @property - def host(self): - if self._port['device_owner'] == constants.DEVICE_OWNER_DVR_INTERFACE: - return self._binding.host - - return super(DvrPortContext, self).host - - @property - def status(self): - if self._port['device_owner'] == constants.DEVICE_OWNER_DVR_INTERFACE: - return self._binding.status - - return super(DvrPortContext, self).status diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 8fe59dedb..7a78f2770 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -340,6 +340,29 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, oport = self._make_port_dict(port_db) port = self._make_port_dict(port_db) network = self.get_network(plugin_context, port['network_id']) + if port['device_owner'] == const.DEVICE_OWNER_DVR_INTERFACE: + # REVISIT(rkukura): The PortBinding instance from the + # ml2_port_bindings table, returned as cur_binding + # from db.get_locked_port_and_binding() above, is + # currently not used for DVR distributed ports, and is + # replaced here with the DVRPortBinding instance from + # the ml2_dvr_port_bindings table specific to the host + # on which the distributed port is being bound. It + # would be possible to optimize this code to avoid + # fetching the PortBinding instance in the DVR case, + # and even to avoid creating the unused entry in the + # ml2_port_bindings table. But the upcoming resolution + # for bug 1367391 will eliminate the + # ml2_dvr_port_bindings table, use the + # ml2_port_bindings table to store non-host-specific + # fields for both distributed and non-distributed + # ports, and introduce a new ml2_port_binding_hosts + # table for the fields that need to be host-specific + # in the distributed case. Since the PortBinding + # instance will then be needed, it does not make sense + # to optimize this code to avoid fetching it. + cur_binding = db.get_dvr_port_binding_by_host( + session, port_id, orig_binding.host) cur_context = driver_context.PortContext( self, plugin_context, port, network, cur_binding, original_port=oport) @@ -1045,52 +1068,11 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, binding = db.ensure_dvr_port_binding( session, id, host, router_id=device_id) network = self.get_network(context, orig_port['network_id']) - mech_context = driver_context.DvrPortContext(self, + mech_context = driver_context.PortContext(self, context, orig_port, network, binding, original_port=orig_port) self._process_dvr_port_binding(mech_context, context, attrs) - self.mechanism_manager.bind_port(mech_context) - # Now try to commit result of attempting to bind the port. - self._commit_dvr_port_binding(mech_context._plugin_context, - orig_port['id'], - host, - mech_context) - - def _commit_dvr_port_binding(self, plugin_context, - port_id, host, - mech_context): - session = plugin_context.session - new_binding = mech_context._binding - with contextlib.nested(lockutils.lock('db-access'), - session.begin(subtransactions=True)): - # Get the current port state and build a new PortContext - # reflecting this state as original state for subsequent - # mechanism driver update_port_*commit() calls. - cur_binding = db.get_dvr_port_binding_by_host(session, - port_id, - host) - if not cur_binding: - LOG.info(_LI("Binding info for port %s was not found, " - "it might have been deleted already."), - port_id) - return - # Commit our binding results only if port has not been - # successfully bound concurrently by another thread or - # process and no binding inputs have been changed. - commit = ((cur_binding.vif_type in - [portbindings.VIF_TYPE_UNBOUND, - portbindings.VIF_TYPE_BINDING_FAILED]) and - new_binding.host == cur_binding.host and - new_binding.vnic_type == cur_binding.vnic_type and - new_binding.profile == cur_binding.profile) - - if commit: - # Update the port's binding state with our binding - # results. - cur_binding.vif_type = new_binding.vif_type - cur_binding.vif_details = new_binding.vif_details - cur_binding.driver = new_binding.driver - cur_binding.segment = new_binding.segment + self._bind_port_if_needed(mech_context) def delete_port(self, context, id, l3_port_check=True): LOG.debug("Deleting port %s", id) @@ -1121,7 +1103,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, if device_owner == const.DEVICE_OWNER_DVR_INTERFACE: bindings = db.get_dvr_port_bindings(context.session, id) for bind in bindings: - mech_context = driver_context.DvrPortContext( + mech_context = driver_context.PortContext( self, context, port, network, bind) self.mechanism_manager.delete_port_precommit(mech_context) bound_mech_contexts.append(mech_context) @@ -1194,7 +1176,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, LOG.error(_LE("Binding info for DVR port %s not found"), port_id) return None - port_context = driver_context.DvrPortContext( + port_context = driver_context.PortContext( self, plugin_context, port, network, binding) else: # since eager loads are disabled in port_db query @@ -1265,7 +1247,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, original_port['network_id']) port.status = db.generate_dvr_port_status(session, port['id']) updated_port = self._make_port_dict(port) - mech_context = (driver_context.DvrPortContext( + mech_context = (driver_context.PortContext( self, context, updated_port, network, binding, original_port=original_port)) self.mechanism_manager.update_port_precommit(mech_context) diff --git a/neutron/tests/unit/ml2/test_driver_context.py b/neutron/tests/unit/ml2/test_driver_context.py index 9400f62e9..373ab304e 100644 --- a/neutron/tests/unit/ml2/test_driver_context.py +++ b/neutron/tests/unit/ml2/test_driver_context.py @@ -21,7 +21,12 @@ from neutron.plugins.ml2 import driver_context from neutron.tests import base -class TestDvrPortContext(base.BaseTestCase): +class TestPortContext(base.BaseTestCase): + + # REVISIT(rkukura): These was originally for DvrPortContext tests, + # but DvrPortContext functionality has been folded into the + # regular PortContext class. Tests for non-DVR-specific + # functionality are needed here as well. def test_host(self): plugin = mock.Mock() @@ -33,11 +38,11 @@ class TestDvrPortContext(base.BaseTestCase): binding.host = 'foohost' with mock.patch.object(driver_context.db, 'get_network_segments'): - ctx = driver_context.DvrPortContext(plugin, - plugin_context, - port, - network, - binding) + ctx = driver_context.PortContext(plugin, + plugin_context, + port, + network, + binding) self.assertEqual('foohost', ctx.host) def test_host_super(self): @@ -51,11 +56,11 @@ class TestDvrPortContext(base.BaseTestCase): binding.host = 'foohost' with mock.patch.object(driver_context.db, 'get_network_segments'): - ctx = driver_context.DvrPortContext(plugin, - plugin_context, - port, - network, - binding) + ctx = driver_context.PortContext(plugin, + plugin_context, + port, + network, + binding) self.assertEqual('host', ctx.host) def test_status(self): @@ -68,11 +73,11 @@ class TestDvrPortContext(base.BaseTestCase): binding.status = 'foostatus' with mock.patch.object(driver_context.db, 'get_network_segments'): - ctx = driver_context.DvrPortContext(plugin, - plugin_context, - port, - network, - binding) + ctx = driver_context.PortContext(plugin, + plugin_context, + port, + network, + binding) self.assertEqual('foostatus', ctx.status) def test_status_super(self): @@ -86,9 +91,9 @@ class TestDvrPortContext(base.BaseTestCase): binding.status = 'foostatus' with mock.patch.object(driver_context.db, 'get_network_segments'): - ctx = driver_context.DvrPortContext(plugin, - plugin_context, - port, - network, - binding) + ctx = driver_context.PortContext(plugin, + plugin_context, + port, + network, + binding) self.assertEqual('status', ctx.status) diff --git a/neutron/tests/unit/ml2/test_ml2_plugin.py b/neutron/tests/unit/ml2/test_ml2_plugin.py index 2b07e9eb7..867f2e566 100644 --- a/neutron/tests/unit/ml2/test_ml2_plugin.py +++ b/neutron/tests/unit/ml2/test_ml2_plugin.py @@ -481,7 +481,7 @@ class TestMl2PortBinding(Ml2PluginV2TestCase, with mock.patch.object(plugin, '_update_port_dict_binding'): with mock.patch.object(ml2_db, 'get_network_segments', return_value=[]): - mech_context = driver_context.DvrPortContext( + mech_context = driver_context.PortContext( self, context, 'port', mock_network, binding) plugin._process_dvr_port_binding(mech_context, context, attrs) self.assertEqual(new_router_id, diff --git a/neutron/tests/unit/ml2/test_port_binding.py b/neutron/tests/unit/ml2/test_port_binding.py index b2da2ab97..4028b7ed2 100644 --- a/neutron/tests/unit/ml2/test_port_binding.py +++ b/neutron/tests/unit/ml2/test_port_binding.py @@ -108,20 +108,6 @@ class PortBindingTestCase(test_plugin.NeutronDbPluginV2TestCase): self.assertIsNone( self.plugin.get_bound_port_context(ctx, port['port']['id'])) - def test_commit_dvr_port_binding(self): - ctx = context.get_admin_context() - - class MechContext(object): - pass - - mctx = MechContext() - mctx._binding = None - # making a shortcut: calling private method directly to - # avoid bothering with "concurrent" port binding deletion - res = self.plugin._commit_dvr_port_binding(ctx, 'anyUUID', - 'HostA', mctx) - self.assertIsNone(res) - def _test_update_port_binding(self, host, new_host=None): with mock.patch.object(self.plugin, '_notify_port_updated') as notify_mock: