From 64674bf0ed966c3e8f241d555b772f03e7407951 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Mon, 17 Jun 2019 17:14:18 -0700 Subject: [PATCH] Block port deletions where vif is present We must not allow users to be able to delete ports when a VIF record is present. If they are able to, then the user risks orphaning ports in neutron which could result in address depletion prevent the port from being able to be used again. Co-Authored-By: Madhuri Kumari Story: 2006710 Task: 37074 Change-Id: I9f8eaf94a375984ea2f567f0a87c2ed1ed5cb3b7 --- ironic/conductor/manager.py | 20 +++++----- ironic/conductor/utils.py | 27 +++++++++++++ ironic/tests/unit/conductor/test_manager.py | 20 ++++++++-- ironic/tests/unit/conductor/test_utils.py | 39 +++++++++++++++++++ ...ts-with-vif-deletion-3edac3df5aa1becf.yaml | 7 ++++ 5 files changed, 99 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/prevent-ports-with-vif-deletion-3edac3df5aa1becf.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 8ba0015c79..e6ce9ff2bb 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -2043,23 +2043,21 @@ class ConductorManager(base_manager.BaseConductorManager): :raises: NodeLocked if node is locked by another conductor. :raises: NodeNotFound if the node associated with the port does not exist. + :raises: InvalidState if a vif is still attached to the port. """ LOG.debug('RPC destroy_port called for port %(port)s', {'port': port.uuid}) with task_manager.acquire(context, port.node_id, purpose='port deletion') as task: - node = task.node - vif = task.driver.network.get_current_vif(task, port) - if ((node.provision_state == states.ACTIVE or node.instance_uuid) - and not node.maintenance and vif): - msg = _("Cannot delete the port %(port)s as node " - "%(node)s is active or has " - "instance UUID assigned or port is bound " - "to vif %(vif)s") - raise exception.InvalidState(msg % {'node': node.uuid, - 'port': port.uuid, - 'vif': vif}) + vif, vif_use = utils.get_attached_vif(port) + if vif: + msg = _("Cannot delete the port %(port)s as it is bound " + "to VIF %(vif)s for %(use)s use.") + raise exception.InvalidState( + msg % {'port': port.uuid, + 'vif': vif, + 'use': vif_use}) port.destroy() LOG.info('Successfully deleted port %(port)s. ' 'The node associated with the port was %(node)s', diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index e537219c63..75b90486ac 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -1134,3 +1134,30 @@ def hash_password(password=''): :param value: Value to be hashed """ return crypt.crypt(password, make_salt()) + + +def get_attached_vif(port): + """Get any attached vif ID for the port + + :param port: The port object upon which to check for a vif + record. + :returns: Returns a tuple of the vif if found and the use of + the vif in the form of a string, 'tenant', 'cleaning' + 'provisioning', 'rescuing'. + :raises: InvalidState exception upon finding a port with a + transient state vif on the port. + """ + + tenant_vif = port.internal_info.get('tenant_vif_port_id') + if tenant_vif: + return (tenant_vif, 'tenant') + clean_vif = port.internal_info.get('cleaning_vif_port_id') + if clean_vif: + return (clean_vif, 'cleaning') + prov_vif = port.internal_info.get('provisioning_vif_port_id') + if prov_vif: + return (prov_vif, 'provisioning') + rescue_vif = port.internal_info.get('rescuing_vif_port_id') + if rescue_vif: + return (rescue_vif, 'rescuing') + return (None, None) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 7011b56e98..fd4b45fd44 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -6311,15 +6311,29 @@ class DestroyPortTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.context, port) self.assertEqual(exception.InvalidState, exc.exc_info[0]) - def test_destroy_port_node_active_and_maintenance(self): + def test_destroy_port_node_active_and_maintenance_vif_present(self): + instance_uuid = uuidutils.generate_uuid() + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + instance_uuid=instance_uuid, + provision_state='active', + maintenance=True) + port = obj_utils.create_test_port( + self.context, + node_id=node.id, + internal_info={'tenant_vif_port_id': 'fake-id'}) + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.destroy_port, + self.context, port) + self.assertEqual(exception.InvalidState, exc.exc_info[0]) + + def test_destroy_port_node_active_and_maintenance_no_vif(self): instance_uuid = uuidutils.generate_uuid() node = obj_utils.create_test_node(self.context, driver='fake-hardware', instance_uuid=instance_uuid, provision_state='active', maintenance=True) port = obj_utils.create_test_port(self.context, - node_id=node.id, - extra={'vif_port_id': 'fake-id'}) + node_id=node.id) self.service.destroy_port(self.context, port) self.assertRaises(exception.PortNotFound, self.dbapi.get_port_by_uuid, diff --git a/ironic/tests/unit/conductor/test_utils.py b/ironic/tests/unit/conductor/test_utils.py index 2ec5649c6e..8a5f7a33c0 100644 --- a/ironic/tests/unit/conductor/test_utils.py +++ b/ironic/tests/unit/conductor/test_utils.py @@ -2051,3 +2051,42 @@ class AgentTokenUtilsTestCase(tests_base.TestCase): conductor_utils.is_agent_token_supported('6.2.1')) self.assertFalse( conductor_utils.is_agent_token_supported('6.0.0')) + + +class GetAttachedVifTestCase(db_base.DbTestCase): + + def setUp(self): + super(GetAttachedVifTestCase, self).setUp() + self.node = obj_utils.create_test_node(self.context, + driver='fake-hardware') + self.port = obj_utils.get_test_port(self.context, + node_id=self.node.id) + + def test_get_attached_vif_none(self): + vif, use = conductor_utils.get_attached_vif(self.port) + self.assertIsNone(vif) + self.assertIsNone(use) + + def test_get_attached_vif_tenant(self): + self.port.internal_info = {'tenant_vif_port_id': '1'} + vif, use = conductor_utils.get_attached_vif(self.port) + self.assertEqual('1', vif) + self.assertEqual('tenant', use) + + def test_get_attached_vif_provisioning(self): + self.port.internal_info = {'provisioning_vif_port_id': '1'} + vif, use = conductor_utils.get_attached_vif(self.port) + self.assertEqual('1', vif) + self.assertEqual('provisioning', use) + + def test_get_attached_vif_cleaning(self): + self.port.internal_info = {'cleaning_vif_port_id': '1'} + vif, use = conductor_utils.get_attached_vif(self.port) + self.assertEqual('1', vif) + self.assertEqual('cleaning', use) + + def test_get_attached_vif_rescuing(self): + self.port.internal_info = {'rescuing_vif_port_id': '1'} + vif, use = conductor_utils.get_attached_vif(self.port) + self.assertEqual('1', vif) + self.assertEqual('rescuing', use) diff --git a/releasenotes/notes/prevent-ports-with-vif-deletion-3edac3df5aa1becf.yaml b/releasenotes/notes/prevent-ports-with-vif-deletion-3edac3df5aa1becf.yaml new file mode 100644 index 0000000000..3d7fed1e23 --- /dev/null +++ b/releasenotes/notes/prevent-ports-with-vif-deletion-3edac3df5aa1becf.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Fixes logic that is applied to port deletions to also consider the + presence of a VIF attachment record, which should be removed before + attempting to delete the node. Failure to do so can result in + erroneous records in the Networking Service.