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

@@ -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,
+def add_ports_to_network(task, network_uuid, security_groups=None):
-                         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

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

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

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,
+            ports = neutron.add_ports_to_network(task, self.network_uuid)
-                                                 is_flat=True)
             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):

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:

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')

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``.