Set pxe_enabled on new and existing ports on introspection

Set it to True for the PXE-booting port, to False for all the others.
Create an extended functional tests covering various operations with ports.

Change-Id: I435a5c04884b6c4da70cb7260b305fbde23eebc0
Closes-Bug: #1667472

doc/source/usage.rst

@@ -153,8 +153,11 @@ unless you understand what you're doing:
       node driver ``root_device`` hints to prevent unexpected HW failures
       passing silently.
 
-``validate_interfaces``
-    validates network interfaces information.
+``validate_interfaces`` validates network interfaces information. Creates new
+    ports, optionally deletes ports that were not present in the introspection
+    data. Also sets the ``pxe_enabled`` flag for the PXE-booting port and
+    unsets it for all the other ports to avoid **nova** picking a random port
+    to boot the node.
 
 The following plugins are enabled by default, but can be disabled if not
 needed:

ironic_inspector/common/ironic.py

@@ -29,8 +29,8 @@ LOG = utils.getProcessingLogger(__name__)
 VALID_STATES = {'enroll', 'manageable', 'inspecting', 'inspect failed'}
 SET_CREDENTIALS_VALID_STATES = {'enroll'}
 
-# 1.11 is API version, which support 'enroll' state
-DEFAULT_IRONIC_API_VERSION = '1.11'
+# 1.19 is API version, which supports port.pxe_enabled
+DEFAULT_IRONIC_API_VERSION = '1.19'
 
 IRONIC_GROUP = 'ironic'
 

ironic_inspector/node_cache.py

@@ -349,14 +349,17 @@ class NodeInfo(object):
         for port in ports:
             mac = port
             extra = {}
+            pxe_enabled = True
             if isinstance(port, dict):
                 mac = port['mac']
                 client_id = port.get('client_id')
                 if client_id:
                     extra = {'client-id': client_id}
+                pxe_enabled = port.get('pxe', True)
 
             if mac not in self.ports():
-                self._create_port(mac, ironic=ironic, extra=extra)
+                self._create_port(mac, ironic=ironic, extra=extra,
+                                  pxe_enabled=pxe_enabled)
             else:
                 existing_macs.append(mac)
 
@@ -373,15 +376,15 @@ class NodeInfo(object):
         """
         if self._ports is None:
             ironic = ironic or self.ironic
-            self._ports = {p.address: p for p in
-                           ironic.node.list_ports(self.uuid, limit=0)}
+            port_list = ironic.node.list_ports(self.uuid, limit=0, detail=True)
+            self._ports = {p.address: p for p in port_list}
         return self._ports
 
-    def _create_port(self, mac, ironic=None, extra=None):
+    def _create_port(self, mac, ironic=None, **kwargs):
         ironic = ironic or self.ironic
         try:
             port = ironic.port.create(
-                node_uuid=self.uuid, address=mac, extra=extra)
+                node_uuid=self.uuid, address=mac, **kwargs)
         except exceptions.Conflict:
             LOG.warning('Port %s already exists, skipping',
                         mac, node_info=self)

ironic_inspector/plugins/local_link_connection.py

@@ -16,11 +16,9 @@
 import binascii
 
 from construct import core
-from ironicclient import exc as client_exc
 import netaddr
 from oslo_config import cfg
 
-from ironic_inspector.common import ironic
 from ironic_inspector.common import lldp_parsers
 from ironic_inspector.common import lldp_tlvs as tlv
 from ironic_inspector.plugins import base
@@ -30,8 +28,6 @@ LOG = utils.getProcessingLogger(__name__)
 
 CONF = cfg.CONF
 
-REQUIRED_IRONIC_VERSION = '1.19'
-
 PORT_ID_ITEM_NAME = "port_id"
 SWITCH_ID_ITEM_NAME = "switch_id"
 
@@ -150,18 +146,4 @@ class GenericLocalLinkConnectionHook(base.ProcessingHook):
                     if patch is not None:
                         patches.append(patch)
 
-            try:
-                # NOTE(sambetts) We need a newer version of Ironic API for this
-                # transaction, so create a new ironic client and explicitly
-                # pass it into the function.
-                cli = ironic.get_client(api_version=REQUIRED_IRONIC_VERSION)
-                node_info.patch_port(port, patches, ironic=cli)
-            except client_exc.NotAcceptable:
-                LOG.error("Unable to set Ironic port local link "
-                          "connection information because Ironic does not "
-                          "support the required version",
-                          node_info=node_info, data=introspection_data)
-                # NOTE(sambetts) May as well break out out of the loop here
-                # because Ironic version is not going to change for the other
-                # interfaces.
-                break
+            node_info.patch_port(port, patches)

ironic_inspector/plugins/standard.py

@@ -149,6 +149,8 @@ class ValidateInterfacesHook(base.ProcessingHook):
         result = {}
         inventory = utils.get_inventory(data)
 
+        pxe_mac = utils.get_pxe_mac(data)
+
         for iface in inventory['interfaces']:
             name = iface.get('name')
             mac = iface.get('mac_address')
@@ -178,7 +180,8 @@ class ValidateInterfacesHook(base.ProcessingHook):
                       'IP address "%(ip)s" and client_id "%(client_id)s"',
                       {'name': name, 'mac': mac, 'ip': ip,
                        'client_id': client_id}, data=data)
-            result[name] = {'ip': ip, 'mac': mac, 'client_id': client_id}
+            result[name] = {'ip': ip, 'mac': mac, 'client_id': client_id,
+                            'pxe': (mac == pxe_mac)}
 
         return result
 
@@ -199,16 +202,14 @@ class ValidateInterfacesHook(base.ProcessingHook):
         result = {}
 
         for name, iface in interfaces.items():
-            mac = iface.get('mac')
             ip = iface.get('ip')
-            client_id = iface.get('client_id')
+            pxe = iface.get('pxe', True)
 
             if name == 'lo' or (ip and netaddr.IPAddress(ip).is_loopback()):
                 LOG.debug('Skipping local interface %s', name, data=data)
                 continue
 
-            if (CONF.processing.add_ports == 'pxe' and pxe_mac
-                    and mac != pxe_mac):
+            if CONF.processing.add_ports == 'pxe' and pxe_mac and not pxe:
                 LOG.debug('Skipping interface %s as it was not PXE booting',
                           name, data=data)
                 continue
@@ -218,8 +219,7 @@ class ValidateInterfacesHook(base.ProcessingHook):
                           name, data=data)
                 continue
 
-            result[name] = {'ip': ip, 'mac': mac.lower(),
-                            'client_id': client_id}
+            result[name] = iface
 
         if not result:
             raise utils.Error(_('No suitable interfaces found in %s') %
@@ -263,19 +263,37 @@ class ValidateInterfacesHook(base.ProcessingHook):
             }
         elif CONF.processing.keep_ports == 'added':
             expected_macs = set(introspection_data['macs'])
-        else:
-            return
 
-        # list is required as we modify underlying dict
-        for port in list(node_info.ports().values()):
-            if port.address not in expected_macs:
-                LOG.info("Deleting port %(port)s as its MAC %(mac)s is "
-                         "not in expected MAC list %(expected)s",
-                         {'port': port.uuid,
-                          'mac': port.address,
-                          'expected': list(sorted(expected_macs))},
-                         node_info=node_info, data=introspection_data)
-                node_info.delete_port(port)
+        if CONF.processing.keep_ports != 'all':
+            # list is required as we modify underlying dict
+            for port in list(node_info.ports().values()):
+                if port.address not in expected_macs:
+                    LOG.info("Deleting port %(port)s as its MAC %(mac)s is "
+                             "not in expected MAC list %(expected)s",
+                             {'port': port.uuid,
+                              'mac': port.address,
+                              'expected': list(sorted(expected_macs))},
+                             node_info=node_info, data=introspection_data)
+                    node_info.delete_port(port)
+
+        if CONF.processing.overwrite_existing:
+            # Make sure pxe_enabled is up-to-date
+            ports = node_info.ports().copy()
+            for iface in introspection_data['interfaces'].values():
+                try:
+                    port = ports[iface['mac']]
+                except KeyError:
+                    continue
+
+                real_pxe = iface.get('pxe', True)
+                if port.pxe_enabled != real_pxe:
+                    LOG.info('Fixing pxe_enabled=%(val)s on port %(port)s '
+                             'to match introspected data',
+                             {'port': port.address, 'val': real_pxe},
+                             node_info=node_info, data=introspection_data)
+                    node_info.patch_port(port, [{'op': 'replace',
+                                                 'path': '/pxe_enabled',
+                                                 'value': real_pxe}])
 
 
 class RamdiskErrorHook(base.ProcessingHook):

ironic_inspector/test/base.py

@@ -101,9 +101,9 @@ class InventoryTest(BaseTest):
             'ff:00:00:00:00:00:02:00:00:02:c9:00:7c:fe:90:03:00:29:26:52')
         self.valid_interfaces = {
             self.pxe_iface_name: {'ip': self.ips[0], 'mac': self.macs[0],
-                                  'client_id': None},
+                                  'client_id': None, 'pxe': True},
             'ib0': {'ip': self.ips[2], 'mac': self.macs[2],
-                    'client_id': self.client_id}
+                    'client_id': self.client_id, 'pxe': False}
         }
         self.data = {
             'boot_interface': '01-' + self.pxe_mac.replace(':', '-'),
@@ -145,20 +145,18 @@ class InventoryTest(BaseTest):
         self.inventory = self.data['inventory']
         self.all_interfaces = {
             'eth1': {'mac': self.macs[0], 'ip': self.ips[0],
-                     'client_id': None},
-            'eth2': {'mac': self.inactive_mac, 'ip': None, 'client_id': None},
+                     'client_id': None, 'pxe': True},
+            'eth2': {'mac': self.inactive_mac, 'ip': None,
+                     'client_id': None, 'pxe': False},
             'eth3': {'mac': self.macs[1], 'ip': self.ips[1],
-                     'client_id': None},
+                     'client_id': None, 'pxe': False},
             'ib0': {'mac': self.macs[2], 'ip': self.ips[2],
-                    'client_id': self.client_id}
+                    'client_id': self.client_id, 'pxe': False}
         }
         self.active_interfaces = {
-            'eth1': {'mac': self.macs[0], 'ip': self.ips[0],
-                     'client_id': None},
-            'eth3': {'mac': self.macs[1], 'ip': self.ips[1],
-                     'client_id': None},
-            'ib0': {'mac': self.macs[2], 'ip': self.ips[2],
-                    'client_id': self.client_id}
+            name: data
+            for (name, data) in self.all_interfaces.items()
+            if data.get('ip')
         }
         self.pxe_interfaces = {
             self.pxe_iface_name: self.all_interfaces[self.pxe_iface_name]

ironic_inspector/test/functional.py

@@ -28,6 +28,7 @@ import mock
 from oslo_config import cfg
 from oslo_config import fixture as config_fixture
 from oslo_utils import timeutils
+from oslo_utils import uuidutils
 import pytz
 import requests
 import six
@@ -252,18 +253,30 @@ class Test(Base):
         self.cli.node.update.assert_called_once_with(self.uuid, mock.ANY)
         self.assertCalledWithPatch(self.patch, self.cli.node.update)
         self.cli.port.create.assert_called_once_with(
-            node_uuid=self.uuid, address='11:22:33:44:55:66', extra={})
+            node_uuid=self.uuid, address='11:22:33:44:55:66', extra={},
+            pxe_enabled=True)
 
         status = self.call_get_status(self.uuid)
         self.check_status(status, finished=True)
 
-    def test_bmc_with_client_id(self):
-        self.pxe_mac = self.macs[2]
-        self.data['boot_interface'] = ('20-' + self.pxe_mac.replace(':', '-'))
-        self.pxe_iface_name = 'ib0'
-        self.pxe_interfaces = {
-            self.pxe_iface_name: self.all_interfaces[self.pxe_iface_name]
-        }
+    def test_port_creation_update_and_deletion(self):
+        cfg.CONF.set_override('add_ports', 'active', 'processing')
+        cfg.CONF.set_override('keep_ports', 'added', 'processing')
+
+        uuid_to_delete = uuidutils.generate_uuid()
+        uuid_to_update = uuidutils.generate_uuid()
+        # Two ports already exist: one with incorrect pxe_enabled, the other
+        # should be deleted.
+        self.cli.node.list_ports.return_value = [
+            mock.Mock(address=self.macs[1], uuid=uuid_to_update,
+                      node_uuid=self.uuid, extra={}, pxe_enabled=True),
+            mock.Mock(address='foobar', uuid=uuid_to_delete,
+                      node_uuid=self.uuid, extra={}, pxe_enabled=True),
+        ]
+        # Two more ports are created, one with client_id. Make sure the
+        # returned object has the same properties as requested in create().
+        self.cli.port.create.side_effect = mock.Mock
+
         self.call_introspect(self.uuid)
         eventlet.greenthread.sleep(DEFAULT_SLEEP)
         self.cli.node.set_power_state.assert_called_once_with(self.uuid,
@@ -278,9 +291,17 @@ class Test(Base):
 
         self.cli.node.update.assert_called_once_with(self.uuid, mock.ANY)
         self.assertCalledWithPatch(self.patch, self.cli.node.update)
-        self.cli.port.create.assert_called_once_with(
-            node_uuid=self.uuid, address=self.macs[2],
-            extra={'client-id': self.client_id})
+        calls = [
+            mock.call(node_uuid=self.uuid, address=self.macs[0],
+                      extra={}, pxe_enabled=True),
+            mock.call(node_uuid=self.uuid, address=self.macs[2],
+                      extra={'client-id': self.client_id}, pxe_enabled=False),
+        ]
+        self.cli.port.create.assert_has_calls(calls, any_order=True)
+        self.cli.port.delete.assert_called_once_with(uuid_to_delete)
+        self.cli.port.update.assert_called_once_with(
+            uuid_to_update,
+            [{'op': 'replace', 'path': '/pxe_enabled', 'value': False}])
 
         status = self.call_get_status(self.uuid)
         self.check_status(status, finished=True)
@@ -310,7 +331,8 @@ class Test(Base):
         self.assertCalledWithPatch(self.patch + patch_credentials,
                                    self.cli.node.update)
         self.cli.port.create.assert_called_once_with(
-            node_uuid=self.uuid, address='11:22:33:44:55:66', extra={})
+            node_uuid=self.uuid, address='11:22:33:44:55:66', extra={},
+            pxe_enabled=True)
 
         status = self.call_get_status(self.uuid)
         self.check_status(status, finished=True)
@@ -513,7 +535,8 @@ class Test(Base):
 
         self.assertCalledWithPatch(self.patch_root_hints, self.cli.node.update)
         self.cli.port.create.assert_called_once_with(
-            node_uuid=self.uuid, address='11:22:33:44:55:66', extra={})
+            node_uuid=self.uuid, address='11:22:33:44:55:66', extra={},
+            pxe_enabled=True)
 
         status = self.call_get_status(self.uuid)
         self.check_status(status, finished=True)
@@ -747,7 +770,8 @@ class Test(Base):
         self.cli.node.update.assert_called_once_with(self.uuid, mock.ANY)
         self.assertCalledWithPatch(self.patch, self.cli.node.update)
         self.cli.port.create.assert_called_once_with(
-            node_uuid=self.uuid, extra={}, address='11:22:33:44:55:66')
+            node_uuid=self.uuid, extra={}, address='11:22:33:44:55:66',
+            pxe_enabled=True)
 
         status = self.call_get_status(self.uuid)
         self.check_status(status, finished=True)

ironic_inspector/test/unit/test_node_cache.py

@@ -579,7 +579,7 @@ class TestNodeCacheIronicObjects(unittest.TestCase):
 
         mock_ironic.assert_called_once_with()
         mock_ironic.return_value.node.list_ports.assert_called_once_with(
-            self.uuid, limit=0)
+            self.uuid, limit=0, detail=True)
 
     def test_ports_ironic_preset(self, mock_ironic):
         mock_ironic2 = mock.Mock()
@@ -591,7 +591,7 @@ class TestNodeCacheIronicObjects(unittest.TestCase):
 
         self.assertFalse(mock_ironic.called)
         mock_ironic2.node.list_ports.assert_called_once_with(
-            self.uuid, limit=0)
+            self.uuid, limit=0, detail=True)
 
 
 class TestUpdate(test_base.NodeTest):
@@ -725,8 +725,8 @@ class TestUpdate(test_base.NodeTest):
     def test_create_ports(self, mock_warn):
         ports = [
             'mac2',
-            {'mac': 'mac3', 'client_id': '42'},
-            {'mac': 'mac4'}
+            {'mac': 'mac3', 'client_id': '42', 'pxe': False},
+            {'mac': 'mac4', 'pxe': True}
         ]
 
         self.node_info.create_ports(ports)
@@ -734,10 +734,12 @@ class TestUpdate(test_base.NodeTest):
                          set(self.node_info.ports()))
 
         create_calls = [
-            mock.call(node_uuid=self.uuid, address='mac2', extra={}),
+            mock.call(node_uuid=self.uuid, address='mac2', extra={},
+                      pxe_enabled=True),
             mock.call(node_uuid=self.uuid, address='mac3',
-                      extra={'client-id': '42'}),
-            mock.call(node_uuid=self.uuid, address='mac4', extra={}),
+                      extra={'client-id': '42'}, pxe_enabled=False),
+            mock.call(node_uuid=self.uuid, address='mac4', extra={},
+                      pxe_enabled=True),
         ]
         self.assertEqual(create_calls, self.ironic.port.create.call_args_list)
         # No conflicts - cache was not cleared - no calls to port.list
@@ -752,15 +754,16 @@ class TestUpdate(test_base.NodeTest):
             'mac',
             {'mac': 'mac0'},
             'mac1',
-            {'mac': 'mac2', 'client_id': '42'},
+            {'mac': 'mac2', 'client_id': '42', 'pxe': False},
         ]
 
         self.node_info.create_ports(ports)
 
         create_calls = [
-            mock.call(node_uuid=self.uuid, address='mac', extra={}),
+            mock.call(node_uuid=self.uuid, address='mac', extra={},
+                      pxe_enabled=True),
             mock.call(node_uuid=self.uuid, address='mac2',
-                      extra={'client-id': '42'}),
+                      extra={'client-id': '42'}, pxe_enabled=False),
         ]
         self.assertEqual(create_calls, self.ironic.port.create.call_args_list)
         mock_warn.assert_called_once_with(mock.ANY, ['mac0', 'mac1'],

ironic_inspector/test/unit/test_plugins_standard.py

@@ -152,6 +152,8 @@ class TestValidateInterfacesHookBeforeProcessing(test_base.NodeTest):
     def test_only_pxe_no_boot_interface(self):
         del self.data['boot_interface']
         self.hook.before_processing(self.data)
+        self.active_interfaces[self.pxe_iface_name]['pxe'] = False
+        self.all_interfaces[self.pxe_iface_name]['pxe'] = False
 
         self.assertEqual(self.active_interfaces, self.data['interfaces'])
         self.assertEqual(sorted(i['mac'] for i in
@@ -210,9 +212,9 @@ class TestValidateInterfacesHookBeforeProcessing(test_base.NodeTest):
 
 @mock.patch.object(node_cache.NodeInfo, 'delete_port', autospec=True)
 @mock.patch.object(node_cache.NodeInfo, 'create_ports', autospec=True)
-class TestValidateInterfacesHookBeforeUpdate(test_base.NodeTest):
+class TestValidateInterfacesHookBeforeUpdateDeletion(test_base.NodeTest):
     def setUp(self):
-        super(TestValidateInterfacesHookBeforeUpdate, self).setUp()
+        super(TestValidateInterfacesHookBeforeUpdateDeletion, self).setUp()
         self.hook = std_plugins.ValidateInterfacesHook()
         self.interfaces_to_create = sorted(self.valid_interfaces.values(),
                                            key=lambda i: i['mac'])
@@ -264,6 +266,40 @@ class TestValidateInterfacesHookBeforeUpdate(test_base.NodeTest):
                                          self.existing_ports[1])
 
 
+@mock.patch.object(node_cache.NodeInfo, 'patch_port', autospec=True)
+@mock.patch.object(node_cache.NodeInfo, 'create_ports', autospec=True)
+class TestValidateInterfacesHookBeforeUpdatePXEEnabled(test_base.NodeTest):
+    def setUp(self):
+        super(TestValidateInterfacesHookBeforeUpdatePXEEnabled, self).setUp()
+        self.hook = std_plugins.ValidateInterfacesHook()
+        # Note(milan) assumes the ordering of self.macs from test_base.NodeTest
+        # where the first item '11:22:33:44:55:66' is the MAC of the
+        # self.pxe_iface_name 'eth1', the "real" PXE interface
+        sorted_interfaces = sorted(self.valid_interfaces.values(),
+                                   key=lambda i: i['mac'])
+        self.existing_ports = [
+            mock.Mock(spec=['address', 'uuid', 'pxe_enabled'],
+                      address=iface['mac'], pxe_enabled=True)
+            for iface in sorted_interfaces
+        ]
+        self.node_info = node_cache.NodeInfo(uuid=self.uuid, started_at=0,
+                                             node=self.node,
+                                             ports=self.existing_ports)
+
+    def test_fix_pxe_enabled(self, mock_create_ports, mock_patch_port):
+        self.hook.before_update(self.data, self.node_info)
+        # Note(milan) there are just 2 self.valid_interfaces, 'eth1' and 'ib0'
+        # eth1 is the PXE booting interface and eth1.mac < ib0.mac
+        mock_patch_port.assert_called_once_with(
+            self.node_info, self.existing_ports[1],
+            [{'op': 'replace', 'path': '/pxe_enabled', 'value': False}])
+
+    def test_no_overwrite(self, mock_create_ports, mock_patch_port):
+        CONF.set_override('overwrite_existing', False, 'processing')
+        self.hook.before_update(self.data, self.node_info)
+        self.assertFalse(mock_patch_port.called)
+
+
 class TestRootDiskSelection(test_base.NodeTest):
     def setUp(self):
         super(TestRootDiskSelection, self).setUp()

ironic_inspector/test/unit/test_process.py

@@ -348,7 +348,7 @@ class TestProcessNode(BaseTest):
         self.validate_attempts = 5
         self.data['macs'] = self.macs  # validate_interfaces hook
         self.valid_interfaces['eth3'] = {
-            'mac': self.macs[1], 'ip': self.ips[1], 'extra': {}
+            'mac': self.macs[1], 'ip': self.ips[1], 'extra': {}, 'pxe': False
         }
         self.data['interfaces'] = self.valid_interfaces
         self.ports = self.all_ports
@@ -403,10 +403,12 @@ class TestProcessNode(BaseTest):
 
         self.cli.port.create.assert_any_call(node_uuid=self.uuid,
                                              address=self.macs[0],
-                                             extra={})
+                                             extra={},
+                                             pxe_enabled=True)
         self.cli.port.create.assert_any_call(node_uuid=self.uuid,
                                              address=self.macs[1],
-                                             extra={})
+                                             extra={},
+                                             pxe_enabled=False)
         self.cli.node.set_power_state.assert_called_once_with(self.uuid, 'off')
         self.assertFalse(self.cli.node.validate.called)
 
@@ -421,10 +423,10 @@ class TestProcessNode(BaseTest):
 
         self.cli.port.create.assert_any_call(node_uuid=self.uuid,
                                              address=self.macs[0],
-                                             extra={})
+                                             extra={}, pxe_enabled=True)
         self.cli.port.create.assert_any_call(node_uuid=self.uuid,
                                              address=self.macs[1],
-                                             extra={})
+                                             extra={}, pxe_enabled=False)
 
     def test_set_ipmi_credentials(self):
         self.node_info.set_option('new_ipmi_credentials', self.new_creds)
@@ -667,7 +669,8 @@ class TestReapplyNode(BaseTest):
         self.cli.port.create.assert_called_once_with(
             node_uuid=self.uuid,
             address=swifted_data['macs'][0],
-            extra={}
+            extra={},
+            pxe_enabled=True
         )
 
     @prepare_mocks

releasenotes/notes/pxe-enabled-cbc3287ebe3fcd49.yaml

@@ -0,0 +1,9 @@
+---
+features:
+  - |
+    Update ``pxe_enabled`` field on ports. It is set to ``True`` for the
+    PXE-booting port and ``False`` for the remaining ports. Both newly
+    discovered and existing ports are affected.
+upgrade:
+  - |
+    Bare metal API version '1.19' is now required.