diff --git a/ironic_inspector/introspect.py b/ironic_inspector/introspect.py index 0d94cd5a1..9a3af2bf1 100644 --- a/ironic_inspector/introspect.py +++ b/ironic_inspector/introspect.py @@ -113,7 +113,7 @@ def introspect(uuid, new_ipmi_credentials=None): def _background_introspect(ironic, node_info): # TODO(dtantsur): pagination - macs = [p.address for p in node_info.ports(ironic)] + macs = list(node_info.ports(ironic)) if macs: node_info.add_attribute(node_cache.MACS_ATTRIBUTE, macs) LOG.info(_LI('Whitelisting MAC\'s %(macs)s for node %(node)s on the' diff --git a/ironic_inspector/node_cache.py b/ironic_inspector/node_cache.py index 069910f6a..c21d27286 100644 --- a/ironic_inspector/node_cache.py +++ b/ironic_inspector/node_cache.py @@ -21,9 +21,10 @@ import sqlite3 import sys import time +from ironicclient import exceptions from oslo_config import cfg -from ironic_inspector.common.i18n import _, _LC, _LE +from ironic_inspector.common.i18n import _, _LC, _LE, _LW from ironic_inspector import utils CONF = cfg.CONF @@ -61,6 +62,8 @@ class NodeInfo(object): self.error = error self.invalidate_cache() self._node = node + if ports is not None and not isinstance(ports, dict): + ports = {p.address: p for p in ports} self._ports = ports @property @@ -142,13 +145,44 @@ class NodeInfo(object): self._node = ironic.node.get(self.uuid) return self._node + def create_ports(self, macs, ironic=None): + """Create one or several ports for this node. + + A warning is issued if port already exists on a node. + """ + ironic = utils.get_client() if ironic is None else ironic + for mac in macs: + if mac not in self.ports(): + self._create_port(mac, ironic) + else: + LOG.warn(_LW('Port %(mac)s already exists for node %(uuid)s, ' + 'skipping'), {'mac': mac, 'uuid': self.uuid}) + def ports(self, ironic=None): - """Get Ironic port objects associated with the cached node record.""" + """Get Ironic port objects associated with the cached node record. + + This value is cached as well, use invalidate_cache() to clean. + + :return: dict MAC -> port object + """ if self._ports is None: ironic = utils.get_client() if ironic is None else ironic - self._ports = ironic.node.list_ports(self.uuid, limit=0) + self._ports = {p.address: p + for p in ironic.node.list_ports(self.uuid, limit=0)} return self._ports + def _create_port(self, mac, ironic): + try: + port = ironic.port.create(node_uuid=self.uuid, address=mac) + except exceptions.Conflict: + LOG.warn(_LW('Port %(mac)s already exists for node %(uuid)s, ' + 'skipping'), {'mac': mac, 'uuid': self.uuid}) + # NOTE(dtantsur): we didn't get port object back, so we have to + # reload ports on next access + self._ports = None + else: + self._ports[mac] = port + def init(): """Initialize the database.""" diff --git a/ironic_inspector/plugins/standard.py b/ironic_inspector/plugins/standard.py index c4013bcb4..573ff5ca4 100644 --- a/ironic_inspector/plugins/standard.py +++ b/ironic_inspector/plugins/standard.py @@ -143,7 +143,7 @@ class ValidateInterfacesHook(base.ProcessingHook): return ironic = utils.get_client() - for port in node_info.ports(): + for port in node_info.ports(ironic).values(): if port.address not in expected_macs: LOG.info(_LI("Deleting port %(port)s as its MAC %(mac)s is " "not in expected MAC list %(expected)s for node " diff --git a/ironic_inspector/process.py b/ironic_inspector/process.py index 0f934f55b..4ac2a254a 100644 --- a/ironic_inspector/process.py +++ b/ironic_inspector/process.py @@ -18,7 +18,7 @@ import logging import eventlet from ironicclient import exceptions -from ironic_inspector.common.i18n import _, _LE, _LI, _LW +from ironic_inspector.common.i18n import _, _LE, _LI from ironic_inspector import firewall from ironic_inspector import node_cache from ironic_inspector.plugins import base as plugins_base @@ -114,7 +114,7 @@ def _run_post_hooks(node_info, introspection_data): node_patches = [p for p in node_patches if p] port_patches = {mac: patch for (mac, patch) in port_patches.items() - if patch} + if patch and mac in node_info.ports()} return node_patches, port_patches @@ -122,25 +122,15 @@ def _process_node(ironic, node, introspection_data, node_info): # NOTE(dtantsur): repeat the check in case something changed utils.check_provision_state(node) - ports = {} - for mac in (introspection_data.get('macs') or ()): - try: - port = ironic.port.create(node_uuid=node.uuid, address=mac) - ports[mac] = port - except exceptions.Conflict: - LOG.warning(_LW('MAC %(mac)s appeared in introspection data for ' - 'node %(node)s, but already exists in ' - 'database - skipping') % - {'mac': mac, 'node': node.uuid}) + node_info.create_ports(introspection_data.get('macs') or (), ironic=ironic) - # NOTE(dtanstur): make sure plugins get the latest information - node_info.invalidate_cache() node_patches, port_patches = _run_post_hooks(node_info, introspection_data) node = utils.retry_on_conflict(ironic.node.update, node.uuid, node_patches) for mac, patches in port_patches.items(): - utils.retry_on_conflict(ironic.port.update, ports[mac].uuid, patches) + port = node_info.ports(ironic)[mac] + utils.retry_on_conflict(ironic.port.update, port.uuid, patches) LOG.debug('Node %s was updated with data from introspection process, ' 'patches %s, port patches %s', diff --git a/ironic_inspector/test/test_introspect.py b/ironic_inspector/test/test_introspect.py index f13d68bf6..d83b4bc1b 100644 --- a/ironic_inspector/test/test_introspect.py +++ b/ironic_inspector/test/test_introspect.py @@ -11,6 +11,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import collections + import eventlet from ironicclient import exceptions import mock @@ -37,8 +39,10 @@ class BaseTest(test_base.NodeTest): power_state='power on', provision_state='foobar') self.ports = [mock.Mock(address=m) for m in self.macs] + self.ports_dict = collections.OrderedDict((p.address, p) + for p in self.ports) self.node_info = mock.Mock(uuid=self.uuid, options={}) - self.node_info.ports.return_value = self.ports + self.node_info.ports.return_value = self.ports_dict def _prepare(self, client_mock): cli = client_mock.return_value @@ -154,7 +158,8 @@ class TestIntrospect(BaseTest): cli.node.validate.return_value = mock.Mock(power={'result': True}) add_mock.return_value = mock.Mock(uuid=self.node_compat.uuid, options={}) - add_mock.return_value.ports.return_value = self.ports + add_mock.return_value.ports.return_value = collections.OrderedDict( + (p.address, p) for p in self.ports) introspect.introspect(self.node_compat.uuid) @@ -175,7 +180,7 @@ class TestIntrospect(BaseTest): def test_no_macs(self, client_mock, add_mock, filters_mock): cli = self._prepare(client_mock) - self.ports[:] = [] + self.node_info.ports.return_value = [] add_mock.return_value = self.node_info introspect.introspect(self.node.uuid) diff --git a/ironic_inspector/test/test_node_cache.py b/ironic_inspector/test/test_node_cache.py index 34f9c2700..85f402ca7 100644 --- a/ironic_inspector/test/test_node_cache.py +++ b/ironic_inspector/test/test_node_cache.py @@ -325,6 +325,11 @@ class TestNodeInfoOptions(test_base.NodeTest): @mock.patch.object(utils, 'get_client') class TestNodeCacheIronicObjects(unittest.TestCase): + def setUp(self): + super(TestNodeCacheIronicObjects, self).setUp() + self.ports = {'mac1': mock.Mock(address='mac1', spec=['address']), + 'mac2': mock.Mock(address='mac2', spec=['address'])} + def test_node_provided(self, mock_ironic): node_info = node_cache.NodeInfo(uuid='uuid', started_at=0, node=mock.sentinel.node) @@ -355,17 +360,24 @@ class TestNodeCacheIronicObjects(unittest.TestCase): def test_ports_provided(self, mock_ironic): node_info = node_cache.NodeInfo(uuid='uuid', started_at=0, - ports=mock.sentinel.ports) - self.assertIs(mock.sentinel.ports, node_info.ports()) - self.assertIs(mock.sentinel.ports, node_info.ports(ironic='ironic')) + ports=self.ports) + self.assertIs(self.ports, node_info.ports()) + self.assertIs(self.ports, node_info.ports(ironic='ironic')) + self.assertFalse(mock_ironic.called) + + def test_ports_provided_list(self, mock_ironic): + node_info = node_cache.NodeInfo(uuid='uuid', started_at=0, + ports=list(self.ports.values())) + self.assertEqual(self.ports, node_info.ports()) + self.assertEqual(self.ports, node_info.ports(ironic='ironic')) self.assertFalse(mock_ironic.called) def test_ports_not_provided(self, mock_ironic): - mock_ironic.return_value.node.list_ports.return_value = ( - mock.sentinel.ports) + mock_ironic.return_value.node.list_ports.return_value = list( + self.ports.values()) node_info = node_cache.NodeInfo(uuid='uuid', started_at=0) - self.assertIs(mock.sentinel.ports, node_info.ports()) + self.assertEqual(self.ports, node_info.ports()) self.assertIs(node_info.ports(), node_info.ports()) mock_ironic.assert_called_once_with() @@ -374,10 +386,10 @@ class TestNodeCacheIronicObjects(unittest.TestCase): def test_ports_ironic_arg(self, mock_ironic): ironic2 = mock.Mock() - ironic2.node.list_ports.return_value = mock.sentinel.ports + ironic2.node.list_ports.return_value = list(self.ports.values()) node_info = node_cache.NodeInfo(uuid='uuid', started_at=0) - self.assertIs(mock.sentinel.ports, node_info.ports(ironic=ironic2)) + self.assertEqual(self.ports, node_info.ports(ironic=ironic2)) self.assertIs(node_info.ports(), node_info.ports(ironic=ironic2)) self.assertFalse(mock_ironic.called) diff --git a/ironic_inspector/test/test_process.py b/ironic_inspector/test/test_process.py index f0230e4cd..a91766805 100644 --- a/ironic_inspector/test/test_process.py +++ b/ironic_inspector/test/test_process.py @@ -305,7 +305,6 @@ class TestProcess(BaseTest): self.assertFalse(pop_mock.return_value.finished.called) -@mock.patch.object(node_cache.NodeInfo, 'invalidate_cache', lambda self: None) @mock.patch.object(utils, 'spawn_n', lambda f, *a: f(*a) and None) @mock.patch.object(eventlet.greenthread, 'sleep', lambda _: None) @@ -344,6 +343,7 @@ class TestProcessNode(BaseTest): [RuntimeError()] * self.validate_attempts + [None]) self.cli.port.create.side_effect = self.ports self.cli.node.update.return_value = self.node + self.cli.node.list_ports.return_value = [] @mock.patch.object(utils, 'get_client') def call(self, mock_cli): @@ -424,7 +424,8 @@ class TestProcessNode(BaseTest): self.assertEqual(2, self.cli.node.set_power_state.call_count) def test_port_failed(self, filters_mock, post_hook_mock): - self.ports[0] = exceptions.Conflict() + self.cli.port.create.side_effect = ( + [exceptions.Conflict()] + self.ports[1:]) self.call()