Move port creation to validate_interfaces hook

Currently port logic is placed inconsistently: port creation is in core processing,
while port deletion is in validate_interfaces before_update. This changes moves
port creation there as well. This should only affect deployments that tamper with
validation_interfaces hook, as previously port creation was run just before running
before_update hooks.

This allows deployments to replace port creation logic by replacing the
validate_interfaces hook.

Change-Id: Idd8f748fdf31fc694bd7b554837e509024716c18
Partial-Bug: #1667472

ironic_inspector/plugins/standard.py

@@ -252,7 +252,10 @@ class ValidateInterfacesHook(base.ProcessingHook):
         introspection_data['macs'] = valid_macs
 
     def before_update(self, introspection_data, node_info, **kwargs):
-        """Drop ports that are not present in the data."""
+        """Create new ports and drop ports that are not present in the data."""
+        interfaces = introspection_data.get('interfaces')
+        node_info.create_ports(list(interfaces.values()))
+
         if CONF.processing.keep_ports == 'present':
             expected_macs = {
                 iface['mac']

ironic_inspector/process.py

@@ -268,8 +268,6 @@ def _run_post_hooks(node_info, introspection_data):
 def _process_node(node_info, node, introspection_data):
     # NOTE(dtantsur): repeat the check in case something changed
     ir_utils.check_provision_state(node)
-    interfaces = introspection_data.get('interfaces')
-    node_info.create_ports(list(interfaces.values()))
     _run_post_hooks(node_info, introspection_data)
     _store_data(node_info, introspection_data)
 
@@ -435,8 +433,6 @@ def _reapply_with_data(node_info, introspection_data):
                             'introspection on stored data:\n%s') %
                           '\n'.join(failures), node_info=node_info)
 
-    interfaces = introspection_data.get('interfaces')
-    node_info.create_ports(list(interfaces.values()))
     _run_post_hooks(node_info, introspection_data)
     _store_data(node_info, introspection_data)
     node_info.invalidate_cache()

ironic_inspector/test/unit/test_plugins_standard.py

@@ -96,18 +96,7 @@ class TestSchedulerHook(test_base.NodeTest):
         self.assertCalledWithPatch(patch, mock_patch)
 
 
-class TestValidateInterfacesHook(test_base.NodeTest):
-    def setUp(self):
-        super(TestValidateInterfacesHook, self).setUp()
-        self.hook = std_plugins.ValidateInterfacesHook()
-        self.existing_ports = [mock.Mock(spec=['address', 'uuid'],
-                                         address=a)
-                               for a in (self.macs[1],
-                                         '44:44:44:44:44:44')]
-        self.node_info = node_cache.NodeInfo(uuid=self.uuid, started_at=0,
-                                             node=self.node,
-                                             ports=self.existing_ports)
-
+class TestValidateInterfacesHookLoad(test_base.NodeTest):
     def test_hook_loadable_by_name(self):
         CONF.set_override('processing_hooks', 'validate_interfaces',
                           'processing')
@@ -122,6 +111,12 @@ class TestValidateInterfacesHook(test_base.NodeTest):
         CONF.set_override('keep_ports', 'foobar', 'processing')
         self.assertRaises(SystemExit, std_plugins.ValidateInterfacesHook)
 
+
+class TestValidateInterfacesHookBeforeProcessing(test_base.NodeTest):
+    def setUp(self):
+        super(TestValidateInterfacesHookBeforeProcessing, self).setUp()
+        self.hook = std_plugins.ValidateInterfacesHook()
+
     def test_no_interfaces(self):
         self.assertRaisesRegex(utils.Error,
                                'Hardware inventory is empty or missing',
@@ -212,27 +207,61 @@ class TestValidateInterfacesHook(test_base.NodeTest):
         self.assertRaisesRegex(utils.Error, 'No suitable interfaces found',
                                self.hook.before_processing, self.data)
 
-    @mock.patch.object(node_cache.NodeInfo, 'delete_port', autospec=True)
-    def test_keep_all(self, mock_delete_port):
+
+@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):
+    def setUp(self):
+        super(TestValidateInterfacesHookBeforeUpdate, self).setUp()
+        self.hook = std_plugins.ValidateInterfacesHook()
+        self.interfaces_to_create = sorted(self.valid_interfaces.values(),
+                                           key=lambda i: i['mac'])
+        self.existing_ports = [mock.Mock(spec=['address', 'uuid'],
+                                         address=a)
+                               for a in (self.macs[1],
+                                         '44:44:44:44:44:44')]
+        self.node_info = node_cache.NodeInfo(uuid=self.uuid, started_at=0,
+                                             node=self.node,
+                                             ports=self.existing_ports)
+
+    def test_keep_all(self, mock_create_ports, mock_delete_port):
         self.hook.before_update(self.data, self.node_info)
+
+        # NOTE(dtantsur): dictionary ordering is not defined
+        mock_create_ports.assert_called_once_with(self.node_info, mock.ANY)
+        self.assertEqual(self.interfaces_to_create,
+                         sorted(mock_create_ports.call_args[0][1],
+                                key=lambda i: i['mac']))
+
         self.assertFalse(mock_delete_port.called)
 
-    @mock.patch.object(node_cache.NodeInfo, 'delete_port')
-    def test_keep_present(self, mock_delete_port):
+    def test_keep_present(self, mock_create_ports, mock_delete_port):
         CONF.set_override('keep_ports', 'present', 'processing')
         self.data['all_interfaces'] = self.all_interfaces
         self.hook.before_update(self.data, self.node_info)
 
-        mock_delete_port.assert_called_once_with(self.existing_ports[1])
+        mock_create_ports.assert_called_once_with(self.node_info, mock.ANY)
+        self.assertEqual(self.interfaces_to_create,
+                         sorted(mock_create_ports.call_args[0][1],
+                                key=lambda i: i['mac']))
 
-    @mock.patch.object(node_cache.NodeInfo, 'delete_port')
-    def test_keep_added(self, mock_delete_port):
+        mock_delete_port.assert_called_once_with(self.node_info,
+                                                 self.existing_ports[1])
+
+    def test_keep_added(self, mock_create_ports, mock_delete_port):
         CONF.set_override('keep_ports', 'added', 'processing')
         self.data['macs'] = [self.pxe_mac]
         self.hook.before_update(self.data, self.node_info)
 
-        mock_delete_port.assert_any_call(self.existing_ports[0])
-        mock_delete_port.assert_any_call(self.existing_ports[1])
+        mock_create_ports.assert_called_once_with(self.node_info, mock.ANY)
+        self.assertEqual(self.interfaces_to_create,
+                         sorted(mock_create_ports.call_args[0][1],
+                                key=lambda i: i['mac']))
+
+        mock_delete_port.assert_any_call(self.node_info,
+                                         self.existing_ports[0])
+        mock_delete_port.assert_any_call(self.node_info,
+                                         self.existing_ports[1])
 
 
 class TestRootDiskSelection(test_base.NodeTest):

releasenotes/notes/port-creation-plugin-c0405ec646b1051d.yaml

@@ -0,0 +1,7 @@
+---
+upgrade:
+  - |
+    Ports creating logic was moved from core processing code to the
+    ``validate_interfaces`` processing hook. This may affect deployments
+    that disable this hook or replace it with something else. Also make
+    sure to place this hook before any hooks expecting ports to be created.