From 56570b1f41e84f40ea18f534217fc8b093ce6e9a Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Fri, 30 Dec 2016 15:53:41 +0200 Subject: [PATCH] Validate port info before assume we may use it Ironic 'neutron' network driver requires local_link_connection to be present on ironic port to be able to work with it (plug to specific network). This patch ignores ironic ports without local_link_connection when attaching VIF to node with 'neutron' network driver. Also make sure that we do not pick such port for provisioning and cleaning. Closes-Bug: #1653249 Change-Id: Icb8298b9be0d8ba62192580263feafa0e1708129 --- ironic/common/neutron.py | 31 ++++++-- ironic/drivers/modules/network/common.py | 3 + ironic/drivers/modules/network/flat.py | 2 +- ironic/tests/unit/common/test_neutron.py | 57 +++++++++++++-- .../drivers/modules/network/test_common.py | 72 ++++++++++++++++--- .../unit/drivers/modules/network/test_flat.py | 2 +- ...info-before-using-it-e26135982d37c698.yaml | 5 ++ 7 files changed, 151 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/validate-port-info-before-using-it-e26135982d37c698.yaml diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index 8efecdcb5b..e5decbff5b 100644 --- a/ironic/common/neutron.py +++ b/ironic/common/neutron.py @@ -162,8 +162,7 @@ def _verify_security_groups(security_groups, client): raise exception.NetworkError(msg) -def add_ports_to_network(task, network_uuid, is_flat=False, - security_groups=None): +def add_ports_to_network(task, network_uuid, security_groups=None): """Create neutron ports to boot the ramdisk. Create neutron ports for each pxe_enabled port on task.node to boot @@ -172,7 +171,6 @@ def add_ports_to_network(task, network_uuid, is_flat=False, :param task: a TaskManager instance. :param network_uuid: UUID of a neutron network where ports will be created. - :param is_flat: Indicates whether it is a flat network or not. :param security_groups: List of Security Groups UUIDs to be used for network. :raises: NetworkError @@ -199,7 +197,7 @@ def add_ports_to_network(task, network_uuid, is_flat=False, if security_groups: body['port']['security_groups'] = security_groups - if not is_flat: + if node.network_interface != 'flat': # NOTE(vdrok): It seems that change # I437290affd8eb87177d0626bf7935a165859cbdd to neutron broke the # possibility to always bind port. Set binding:host_id only in @@ -216,6 +214,10 @@ def add_ports_to_network(task, network_uuid, is_flat=False, portmap = get_node_portmap(task) pxe_enabled_ports = [p for p in task.ports if p.pxe_enabled] for ironic_port in pxe_enabled_ports: + # Skip ports that are missing required information for deploy. + if not validate_port_info(node, ironic_port): + failures.append(ironic_port.uuid) + continue body['port']['mac_address'] = ironic_port.address binding_profile = {'local_link_information': [portmap[ironic_port.uuid]]} @@ -394,6 +396,27 @@ def validate_network(uuid_or_name, net_type=_('network')): return networks[0] +def validate_port_info(node, port): + """Check that port contains enough information for deploy. + + Neutron network interface requires that local_link_information field is + filled before we can use this port. + + :param node: Ironic node object. + :param port: Ironic port object. + :returns: True if port info is valid, False otherwise. + """ + if (node.network_interface == 'neutron' and + not port.local_link_connection): + LOG.warning(_LW("The local_link_connection is required for" + "'neutron' network interface and is not present " + "in the nodes %(node)s port %(port)s"), + {'node': node.uuid, 'port': port.uuid}) + return False + + return True + + class NeutronNetworkInterfaceMixin(object): _cleaning_network_uuid = None diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index 800e8a7994..32f5b7b8f7 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -75,6 +75,9 @@ def _get_free_portgroups_and_ports(task, vif_id): non_usable_portgroups = set() for p in task.ports: + # Validate that port has needed information + if not neutron.validate_port_info(task.node, p): + continue if _vif_attached(p, vif_id): # Consider such portgroup unusable. The fact that we can have None # added in this set is not a problem diff --git a/ironic/drivers/modules/network/flat.py b/ironic/drivers/modules/network/flat.py index 62e8be57e7..1f72076dc7 100644 --- a/ironic/drivers/modules/network/flat.py +++ b/ironic/drivers/modules/network/flat.py @@ -123,7 +123,7 @@ class FlatNetwork(common.VIFPortIDMixin, neutron.NeutronNetworkInterfaceMixin, neutron.rollback_ports(task, self.get_cleaning_network_uuid()) LOG.info(_LI('Adding cleaning network to node %s'), task.node.uuid) vifs = neutron.add_ports_to_network( - task, self.get_cleaning_network_uuid(), is_flat=True) + task, self.get_cleaning_network_uuid()) for port in task.ports: if port.uuid in vifs: internal_info = port.internal_info diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py index f6175f2e44..1aaddc83fd 100644 --- a/ironic/tests/unit/common/test_neutron.py +++ b/ironic/tests/unit/common/test_neutron.py @@ -139,6 +139,8 @@ class TestNeutronNetworkActions(db_base.DbTestCase): def _test_add_ports_to_vlan_network(self, is_client_id, security_groups=None): # Ports will be created only if pxe_enabled is True + self.node.network_interface = 'neutron' + self.node.save() object_utils.create_test_port( self.context, node_id=self.node.id, uuid=uuidutils.generate_uuid(), @@ -272,6 +274,8 @@ class TestNeutronNetworkActions(db_base.DbTestCase): self._test_add_ports_to_vlan_network(is_client_id=True) def _test_add_ports_to_flat_network(self, is_client_id): + self.node.network_interface = 'flat' + self.node.save() port = self.ports[0] if is_client_id: extra = port.extra @@ -299,20 +303,26 @@ class TestNeutronNetworkActions(db_base.DbTestCase): 'port': self.neutron_port} expected = {port.uuid: self.neutron_port['id']} with task_manager.acquire(self.context, self.node.uuid) as task: - ports = neutron.add_ports_to_network(task, self.network_uuid, - is_flat=True) + ports = neutron.add_ports_to_network(task, self.network_uuid) self.assertEqual(expected, ports) self.client_mock.create_port.assert_called_once_with( expected_body) - def test_add_ports_to_flat_network(self): + @mock.patch.object(neutron, 'validate_port_info', autospec=True, + return_value=True) + def test_add_ports_to_flat_network(self, vpi_mock): self._test_add_ports_to_flat_network(is_client_id=False) + self.assertTrue(vpi_mock.called) - def test_add_ports_with_client_id_to_flat_network(self): + @mock.patch.object(neutron, 'validate_port_info', autospec=True, + return_value=True) + def test_add_ports_with_client_id_to_flat_network(self, vpi_mock): self._test_add_ports_to_flat_network(is_client_id=True) - def test_add_ports_to_vlan_network_instance_uuid(self): + @mock.patch.object(neutron, 'validate_port_info', autospec=True) + def test_add_ports_to_vlan_network_instance_uuid(self, vpi_mock): self.node.instance_uuid = uuidutils.generate_uuid() + self.node.network_interface = 'neutron' self.node.save() port = self.ports[0] expected_body = { @@ -329,6 +339,7 @@ class TestNeutronNetworkActions(db_base.DbTestCase): } } } + vpi_mock.return_value = True # Ensure we can create ports self.client_mock.create_port.return_value = {'port': self.neutron_port} expected = {port.uuid: self.neutron_port['id']} @@ -336,6 +347,7 @@ class TestNeutronNetworkActions(db_base.DbTestCase): ports = neutron.add_ports_to_network(task, self.network_uuid) self.assertEqual(expected, ports) self.client_mock.create_port.assert_called_once_with(expected_body) + self.assertTrue(vpi_mock.called) @mock.patch.object(neutron, 'rollback_ports') def test_add_network_all_ports_fail(self, rollback_mock): @@ -448,6 +460,41 @@ class TestNeutronNetworkActions(db_base.DbTestCase): neutron.rollback_ports(task, self.network_uuid) self.assertTrue(log_mock.exception.called) + @mock.patch.object(neutron, 'LOG') + def test_validate_port_info_neutron_interface(self, log_mock): + self.node.network_interface = 'neutron' + self.node.save() + port = object_utils.create_test_port( + self.context, node_id=self.node.id, uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:33') + res = neutron.validate_port_info(self.node, port) + self.assertTrue(res) + self.assertFalse(log_mock.warning.called) + + @mock.patch.object(neutron, 'LOG') + def test_validate_port_info_neutron_interface_missed_info(self, log_mock): + self.node.network_interface = 'neutron' + self.node.save() + llc = {} + port = object_utils.create_test_port( + self.context, node_id=self.node.id, uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:33', local_link_connection=llc) + res = neutron.validate_port_info(self.node, port) + self.assertFalse(res) + self.assertTrue(log_mock.warning.called) + + @mock.patch.object(neutron, 'LOG') + def test_validate_port_info_flat_interface(self, log_mock): + self.node.network_interface = 'flat' + self.node.save() + llc = {} + port = object_utils.create_test_port( + self.context, node_id=self.node.id, uuid=uuidutils.generate_uuid(), + address='52:54:00:cf:2d:33', local_link_connection=llc) + res = neutron.validate_port_info(self.node, port) + self.assertTrue(res) + self.assertFalse(log_mock.warning.called) + @mock.patch.object(neutron, 'get_client', autospec=True) class TestValidateNetwork(base.TestCase): diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index 5064c00af8..07c958dbf5 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -77,6 +77,8 @@ class TestCommonFunctions(db_base.DbTestCase): return pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports def test__get_free_portgroups_and_ports(self): + self.node.network_interface = 'flat' + self.node.save() pg1, pg1_ports, pg2, pg2_ports, pg3, pg3_ports = self._objects_setup() with task_manager.acquire(self.context, self.node.id) as task: free_portgroups, free_ports = ( @@ -86,12 +88,42 @@ class TestCommonFunctions(db_base.DbTestCase): [p.uuid for p in free_ports]) self.assertItemsEqual([pg1.uuid], [p.uuid for p in free_portgroups]) - def test_get_free_port_like_object_ports(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True) + def test__get_free_portgroups_and_ports_neutron_missed(self, vpi_mock): + vpi_mock.return_value = False + with task_manager.acquire(self.context, self.node.id) as task: + free_portgroups, free_ports = ( + common._get_free_portgroups_and_ports(task, self.vif_id)) + self.assertItemsEqual([], free_ports) + + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True) + def test__get_free_portgroups_and_ports_neutron(self, vpi_mock): + vpi_mock.return_value = True + with task_manager.acquire(self.context, self.node.id) as task: + free_portgroups, free_ports = ( + common._get_free_portgroups_and_ports(task, self.vif_id)) + self.assertItemsEqual( + [self.port.uuid], [p.uuid for p in free_ports]) + + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True) + def test__get_free_portgroups_and_ports_flat(self, vpi_mock): + self.node.network_interface = 'flat' + self.node.save() + vpi_mock.return_value = True + with task_manager.acquire(self.context, self.node.id) as task: + free_portgroups, free_ports = ( + common._get_free_portgroups_and_ports(task, self.vif_id)) + + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_ports(self, vpi_mock): with task_manager.acquire(self.context, self.node.id) as task: res = common.get_free_port_like_object(task, self.vif_id) self.assertEqual(self.port.uuid, res.uuid) - def test_get_free_port_like_object_ports_pxe_enabled_first(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_ports_pxe_enabled_first(self, vpi_mock): self.port.pxe_enabled = False self.port.save() other_port = obj_utils.create_test_port( @@ -101,7 +133,9 @@ class TestCommonFunctions(db_base.DbTestCase): res = common.get_free_port_like_object(task, self.vif_id) self.assertEqual(other_port.uuid, res.uuid) - def test_get_free_port_like_object_portgroup_first(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_portgroup_first(self, vpi_mock): pg = obj_utils.create_test_portgroup( self.context, node_id=self.node.id) obj_utils.create_test_port( @@ -111,13 +145,18 @@ class TestCommonFunctions(db_base.DbTestCase): res = common.get_free_port_like_object(task, self.vif_id) self.assertEqual(pg.uuid, res.uuid) - def test_get_free_port_like_object_ignores_empty_portgroup(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_ignores_empty_portgroup(self, vpi_mock): obj_utils.create_test_portgroup(self.context, node_id=self.node.id) with task_manager.acquire(self.context, self.node.id) as task: res = common.get_free_port_like_object(task, self.vif_id) self.assertEqual(self.port.uuid, res.uuid) - def test_get_free_port_like_object_ignores_standalone_portgroup(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_ignores_standalone_portgroup( + self, vpi_mock): self.port.destroy() pg = obj_utils.create_test_portgroup( self.context, node_id=self.node.id) @@ -132,7 +171,10 @@ class TestCommonFunctions(db_base.DbTestCase): res = common.get_free_port_like_object(task, self.vif_id) self.assertEqual(free_port.uuid, res.uuid) - def test_get_free_port_like_object_vif_attached_to_portgroup(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_vif_attached_to_portgroup( + self, vpi_mock): pg = obj_utils.create_test_portgroup( self.context, node_id=self.node.id, internal_info={common.TENANT_VIF_KEY: self.vif_id}) @@ -145,7 +187,10 @@ class TestCommonFunctions(db_base.DbTestCase): r"already attached to Ironic Portgroup", common.get_free_port_like_object, task, self.vif_id) - def test_get_free_port_like_object_vif_attached_to_portgroup_extra(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_vif_attached_to_portgroup_extra( + self, vpi_mock): pg = obj_utils.create_test_portgroup( self.context, node_id=self.node.id, extra={'vif_port_id': self.vif_id}) @@ -158,7 +203,9 @@ class TestCommonFunctions(db_base.DbTestCase): r"already attached to Ironic Portgroup", common.get_free_port_like_object, task, self.vif_id) - def test_get_free_port_like_object_vif_attached_to_port(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_vif_attached_to_port(self, vpi_mock): self.port.internal_info = {common.TENANT_VIF_KEY: self.vif_id} self.port.save() with task_manager.acquire(self.context, self.node.id) as task: @@ -167,7 +214,10 @@ class TestCommonFunctions(db_base.DbTestCase): r"already attached to Ironic Port\b", common.get_free_port_like_object, task, self.vif_id) - def test_get_free_port_like_object_vif_attached_to_port_extra(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_vif_attached_to_port_extra( + self, vpi_mock): self.port.extra = {'vif_port_id': self.vif_id} self.port.save() with task_manager.acquire(self.context, self.node.id) as task: @@ -176,7 +226,9 @@ class TestCommonFunctions(db_base.DbTestCase): r"already attached to Ironic Port\b", common.get_free_port_like_object, task, self.vif_id) - def test_get_free_port_like_object_nothing_free(self): + @mock.patch.object(neutron_common, 'validate_port_info', autospec=True, + return_value=True) + def test_get_free_port_like_object_nothing_free(self, vpi_mock): self.port.extra = {'vif_port_id': 'another-vif'} self.port.save() with task_manager.acquire(self.context, self.node.id) as task: diff --git a/ironic/tests/unit/drivers/modules/network/test_flat.py b/ironic/tests/unit/drivers/modules/network/test_flat.py index 0ea06694bf..d6380fff1c 100644 --- a/ironic/tests/unit/drivers/modules/network/test_flat.py +++ b/ironic/tests/unit/drivers/modules/network/test_flat.py @@ -92,7 +92,7 @@ class TestFlatInterface(db_base.DbTestCase): rollback_mock.assert_called_once_with( task, CONF.neutron.cleaning_network) add_mock.assert_called_once_with( - task, CONF.neutron.cleaning_network, is_flat=True) + task, CONF.neutron.cleaning_network) validate_mock.assert_called_once_with( CONF.neutron.cleaning_network, 'cleaning network') diff --git a/releasenotes/notes/validate-port-info-before-using-it-e26135982d37c698.yaml b/releasenotes/notes/validate-port-info-before-using-it-e26135982d37c698.yaml new file mode 100644 index 0000000000..a248165441 --- /dev/null +++ b/releasenotes/notes/validate-port-info-before-using-it-e26135982d37c698.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - Fixes an issue when attaching VIF to a port with missed + ``local_link_connection`` field was allowed when node network + interface was ``neutron``.