From 904db03f3e9c6e9bd727f7448217a164daba2c77 Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Date: Fri, 1 Sep 2023 11:43:24 +0000 Subject: [PATCH] Revert "[OVN][Trunk] Add port binding info on subport when parent is bound" This reverts commit 955e621167987626806128d259a29d4e62acf102. Reason for revert: the port binding handling done in this patch is incorrect and leads to issues during the cold migration process with trunk ports in ML2/OVN. Change-Id: Ifc2d37e2042fad43dd838821953defd99a5f8665 Closes-Bug: #2033887 (cherry picked from commit 1b034f8d62091724a6fb639f34c2f8936329b501) --- .../trunk/drivers/ovn/trunk_driver.py | 26 ++--------- .../trunk/drivers/ovn/test_trunk_driver.py | 44 ++++++------------- .../trunk/drivers/ovn/test_trunk_driver.py | 2 - 3 files changed, 17 insertions(+), 55 deletions(-) diff --git a/neutron/services/trunk/drivers/ovn/trunk_driver.py b/neutron/services/trunk/drivers/ovn/trunk_driver.py index e404ee0b1f7..fcc260ff4e0 100644 --- a/neutron/services/trunk/drivers/ovn/trunk_driver.py +++ b/neutron/services/trunk/drivers/ovn/trunk_driver.py @@ -51,13 +51,11 @@ class OVNTrunkHandler(object): context = n_context.get_admin_context() db_parent_port = port_obj.Port.get_object(context, id=parent_port) parent_port_status = db_parent_port.status - parent_port_bindings = db_parent_port.bindings[0] for subport in subports: with db_api.CONTEXT_WRITER.using(context), ( txn(check_error=True)) as ovn_txn: port = self._set_binding_profile(context, subport, parent_port, - parent_port_status, - parent_port_bindings, ovn_txn) + parent_port_status, ovn_txn) db_rev.bump_revision(context, port, ovn_const.TYPE_PORTS) def _unset_sub_ports(self, subports): @@ -71,8 +69,7 @@ class OVNTrunkHandler(object): @db_base_plugin_common.convert_result_to_dict def _set_binding_profile(self, context, subport, parent_port, - parent_port_status, - parent_port_bindings, ovn_txn): + parent_port_status, ovn_txn): LOG.debug("Setting parent %s for subport %s", parent_port, subport.port_id) db_port = port_obj.Port.get_object(context, id=subport.port_id) @@ -84,9 +81,6 @@ class OVNTrunkHandler(object): check_rev_cmd = self.plugin_driver.nb_ovn.check_revision_number( db_port.id, db_port, ovn_const.TYPE_PORTS) ovn_txn.add(check_rev_cmd) - parent_binding_host = '' - if parent_port_bindings.host: - parent_binding_host = parent_port_bindings.host try: # NOTE(flaviof): We expect binding's host to be set. Otherwise, # sub-port will not transition from DOWN to ACTIVE. @@ -102,7 +96,6 @@ class OVNTrunkHandler(object): port_obj.PortBinding.update_object( context, {'profile': binding.profile, - 'host': parent_binding_host, 'vif_type': portbindings.VIF_TYPE_OVS}, port_id=subport.port_id, host=binding.host) @@ -168,14 +161,6 @@ class OVNTrunkHandler(object): def _is_port_bound(port): return n_utils.is_port_bound(port, log_message=False) - def trunk_updated(self, trunk): - # Check if parent port is handled by OVN. - if not self.plugin_driver.nb_ovn.lookup('Logical_Switch_Port', - trunk.port_id, default=None): - return - if trunk.sub_ports: - self._set_sub_ports(trunk.port_id, trunk.sub_ports) - def trunk_created(self, trunk): # Check if parent port is handled by OVN. if not self.plugin_driver.nb_ovn.lookup('Logical_Switch_Port', @@ -210,8 +195,6 @@ class OVNTrunkHandler(object): def trunk_event(self, resource, event, trunk_plugin, payload): if event == events.AFTER_CREATE: self.trunk_created(payload.states[0]) - elif event == events.AFTER_UPDATE: - self.trunk_updated(payload.states[0]) elif event == events.AFTER_DELETE: self.trunk_deleted(payload.states[0]) elif event == events.PRECOMMIT_CREATE: @@ -248,9 +231,8 @@ class OVNTrunkDriver(trunk_base.DriverBase): super(OVNTrunkDriver, self).register( resource, event, trigger, payload=payload) self._handler = OVNTrunkHandler(self.plugin_driver) - for _event in (events.AFTER_CREATE, events.AFTER_UPDATE, - events.AFTER_DELETE, events.PRECOMMIT_CREATE, - events.PRECOMMIT_DELETE): + for _event in (events.AFTER_CREATE, events.AFTER_DELETE, + events.PRECOMMIT_CREATE, events.PRECOMMIT_DELETE): registry.subscribe(self._handler.trunk_event, resources.TRUNK, _event) diff --git a/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py b/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py index 099c7d63bdb..fa281232c42 100644 --- a/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py +++ b/neutron/tests/functional/services/trunk/drivers/ovn/test_trunk_driver.py @@ -17,7 +17,7 @@ import contextlib from neutron_lib.api.definitions import portbindings from neutron_lib.callbacks import exceptions as n_exc from neutron_lib import constants as n_consts -from neutron_lib.db import api as db_api +from neutron_lib.objects import registry as obj_reg from neutron_lib.plugins import utils from neutron_lib.services.trunk import constants as trunk_consts from oslo_utils import uuidutils @@ -30,8 +30,8 @@ from neutron.tests.functional import base class TestOVNTrunkDriver(base.TestOVNFunctionalBase): - def setUp(self, **kwargs): - super().setUp(**kwargs) + def setUp(self): + super(TestOVNTrunkDriver, self).setUp() self.trunk_plugin = trunk_plugin.TrunkPlugin() self.trunk_plugin.add_segmentation_type( trunk_consts.SEGMENTATION_TYPE_VLAN, @@ -42,8 +42,7 @@ class TestOVNTrunkDriver(base.TestOVNFunctionalBase): sub_ports = sub_ports or [] with self.network() as network: with self.subnet(network=network) as subnet: - with self.port(subnet=subnet, - device_owner='compute:nova') as parent_port: + with self.port(subnet=subnet) as parent_port: tenant_id = uuidutils.generate_uuid() trunk = {'trunk': { 'port_id': parent_port['port']['id'], @@ -68,14 +67,17 @@ class TestOVNTrunkDriver(base.TestOVNFunctionalBase): if row.parent_name and row.tag: device_owner = row.external_ids[ ovn_const.OVN_DEVICE_OWNER_EXT_ID_KEY] + revision_number = row.external_ids[ + ovn_const.OVN_REV_NUM_EXT_ID_KEY] ovn_trunk_info.append({'port_id': row.name, 'parent_port_id': row.parent_name, 'tag': row.tag, 'device_owner': device_owner, + 'revision_number': revision_number, }) return ovn_trunk_info - def _verify_trunk_info(self, trunk, has_items, host=''): + def _verify_trunk_info(self, trunk, has_items): ovn_subports_info = self._get_ovn_trunk_info() neutron_subports_info = [] for subport in trunk.get('sub_ports', []): @@ -84,12 +86,12 @@ class TestOVNTrunkDriver(base.TestOVNFunctionalBase): 'parent_port_id': [trunk['port_id']], 'tag': [subport['segmentation_id']], 'device_owner': trunk_consts.TRUNK_SUBPORT_OWNER, + 'revision_number': '2', }) - # Check the subport binding. - pb = port_obj.PortBinding.get_object( - self.context, port_id=subport['port_id'], host=host) - self.assertEqual(n_consts.PORT_STATUS_ACTIVE, pb.status) - self.assertEqual(host, pb.host) + # Check that the subport has the binding is active. + binding = obj_reg.load_class('PortBinding').get_object( + self.context, port_id=subport['port_id'], host='') + self.assertEqual(n_consts.PORT_STATUS_ACTIVE, binding['status']) self.assertCountEqual(ovn_subports_info, neutron_subports_info) self.assertEqual(has_items, len(neutron_subports_info) != 0) @@ -97,14 +99,6 @@ class TestOVNTrunkDriver(base.TestOVNFunctionalBase): if trunk.get('status'): self.assertEqual(trunk_consts.TRUNK_ACTIVE_STATUS, trunk['status']) - def _bind_port(self, port_id, host): - with db_api.CONTEXT_WRITER.using(self.context): - pb = port_obj.PortBinding.get_object(self.context, - port_id=port_id, host='') - pb.delete() - port_obj.PortBinding(self.context, port_id=port_id, host=host, - vif_type=portbindings.VIF_TYPE_OVS).create() - def test_trunk_create(self): with self.trunk() as trunk: self._verify_trunk_info(trunk, has_items=False) @@ -141,22 +135,10 @@ class TestOVNTrunkDriver(base.TestOVNFunctionalBase): new_trunk = self.trunk_plugin.get_trunk(self.context, trunk['id']) self._verify_trunk_info(new_trunk, has_items=True) - # Bind parent port. That will trigger the binding of the - # trunk subports too, using the same host ID. - self._bind_port(trunk['port_id'], 'host1') - self.mech_driver.set_port_status_up(trunk['port_id']) - self._verify_trunk_info(new_trunk, has_items=True, - host='host1') def test_subport_delete(self): with self.subport() as subport: with self.trunk([subport]) as trunk: - # Bind parent port. - self._bind_port(trunk['port_id'], 'host1') - self.mech_driver.set_port_status_up(trunk['port_id']) - self._verify_trunk_info(trunk, has_items=True, - host='host1') - self.trunk_plugin.remove_subports(self.context, trunk['id'], {'sub_ports': [subport]}) new_trunk = self.trunk_plugin.get_trunk(self.context, diff --git a/neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py b/neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py index 5891c2ab2c4..894520bf860 100644 --- a/neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py +++ b/neutron/tests/unit/services/trunk/drivers/ovn/test_trunk_driver.py @@ -122,7 +122,6 @@ class TestTrunkHandler(base.BaseTestCase): mock.call(mock.ANY, {'profile': {'parent_name': trunk.port_id, 'tag': s_port.segmentation_id}, - 'host': mock.ANY, 'vif_type': portbindings.VIF_TYPE_OVS}, host=mock.ANY, port_id=s_port.port_id) @@ -153,7 +152,6 @@ class TestTrunkHandler(base.BaseTestCase): self.mock_update_pb.assert_called_once_with( mock.ANY, {'profile': {'parent_name': self.sub_port_1.trunk_id, 'tag': self.sub_port_1.segmentation_id}, - 'host': 'foo.com', 'vif_type': portbindings.VIF_TYPE_OVS}, host='foo.com', port_id=self.sub_port_1.port_id) self.mock_port_update.assert_not_called()