From b8e1d97e81d45092e4d61a63fbd811c850cd6bbd Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 8 Dec 2014 18:27:42 +0100 Subject: [PATCH] Refactoring: split discoverd.py into 2 modules Wrote some missing tests as well. Change-Id: I2d4d4619cb5825422cb2fc684f7cd028f99b991d --- ironic_discoverd/discover.py | 104 +++++++++++++ ironic_discoverd/firewall.py | 2 +- ironic_discoverd/main.py | 9 +- ironic_discoverd/{discoverd.py => process.py} | 144 +++++------------- ironic_discoverd/test.py | 66 +++++--- 5 files changed, 189 insertions(+), 136 deletions(-) create mode 100644 ironic_discoverd/discover.py rename ironic_discoverd/{discoverd.py => process.py} (52%) diff --git a/ironic_discoverd/discover.py b/ironic_discoverd/discover.py new file mode 100644 index 000000000..f8acafe98 --- /dev/null +++ b/ironic_discoverd/discover.py @@ -0,0 +1,104 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Handling discovery request.""" + +import logging +import time + +import eventlet +from ironicclient import exceptions + +from ironic_discoverd import firewall +from ironic_discoverd import node_cache +from ironic_discoverd import utils + + +LOG = logging.getLogger("ironic_discoverd.discover") + + +def discover(uuids): + """Initiate discovery for given node uuids.""" + if not uuids: + raise utils.DiscoveryFailed("No nodes to discover") + + ironic = utils.get_client() + LOG.debug('Validating nodes %s', uuids) + nodes = [] + for uuid in uuids: + try: + node = ironic.node.get(uuid) + except exceptions.NotFound: + LOG.error('Node %s cannot be found', uuid) + raise utils.DiscoveryFailed("Cannot find node %s" % uuid, code=404) + except exceptions.HttpError as exc: + LOG.exception('Cannot get node %s', uuid) + raise utils.DiscoveryFailed("Cannot get node %s: %s" % (uuid, exc)) + + _validate(ironic, node) + nodes.append(node) + + LOG.info('Proceeding with discovery on nodes %s', [n.uuid for n in nodes]) + for node in nodes: + eventlet.greenthread.spawn_n(_background_start_discover, ironic, node) + + +def _validate(ironic, node): + if node.instance_uuid: + LOG.error('Refusing to discover node %s with assigned instance_uuid', + node.uuid) + raise utils.DiscoveryFailed( + 'Refusing to discover node %s with assigned instance uuid' % + node.uuid) + + power_state = node.power_state + if (not node.maintenance and power_state is not None + and power_state.lower() != 'power off'): + LOG.error('Refusing to discover node %s with power_state "%s" ' + 'and maintenance mode off', + node.uuid, power_state) + raise utils.DiscoveryFailed( + 'Refusing to discover node %s with power state "%s" and ' + 'maintenance mode off' % + (node.uuid, power_state)) + + validation = ironic.node.validate(node.uuid) + if not validation.power['result']: + LOG.error('Failed validation of power interface for node %s, ' + 'reason: %s', node.uuid, validation.power['reason']) + raise utils.DiscoveryFailed('Failed validation of power interface for ' + 'node %s' % node.uuid) + + +def _background_start_discover(ironic, node): + patch = [{'op': 'add', 'path': '/extra/on_discovery', 'value': 'true'}, + {'op': 'add', 'path': '/extra/discovery_timestamp', + 'value': str(time.time())}] + ironic.node.update(node.uuid, patch) + + # TODO(dtantsur): pagination + macs = [p.address for p in ironic.node.list_ports(node.uuid, limit=0)] + node_cache.add_node(node.uuid, + bmc_address=node.driver_info.get('ipmi_address'), + mac=macs) + + if macs: + LOG.info('Whitelisting MAC\'s %s for node %s on the firewall', + macs, node.uuid) + firewall.update_filters(ironic) + + try: + ironic.node.set_power_state(node.uuid, 'reboot') + except Exception as exc: + LOG.error('Failed to power on node %s, check it\'s power ' + 'management configuration:\n%s', node.uuid, exc) diff --git a/ironic_discoverd/firewall.py b/ironic_discoverd/firewall.py index 686246b46..4b965433c 100644 --- a/ironic_discoverd/firewall.py +++ b/ironic_discoverd/firewall.py @@ -21,7 +21,7 @@ from ironic_discoverd import node_cache from ironic_discoverd import utils -LOG = logging.getLogger("discoverd") +LOG = logging.getLogger("ironic_discoverd.firewall") NEW_CHAIN = 'discovery_temp' CHAIN = 'discovery' INTERFACE = None diff --git a/ironic_discoverd/main.py b/ironic_discoverd/main.py index 8d8b51eac..4164e81d4 100644 --- a/ironic_discoverd/main.py +++ b/ironic_discoverd/main.py @@ -23,14 +23,15 @@ from flask import Flask, request # noqa from keystoneclient import exceptions from ironic_discoverd import conf -from ironic_discoverd import discoverd +from ironic_discoverd import discover from ironic_discoverd import firewall from ironic_discoverd import node_cache +from ironic_discoverd import process from ironic_discoverd import utils app = Flask(__name__) -LOG = discoverd.LOG +LOG = logging.getLogger('ironic_discoverd.main') @app.route('/v1/continue', methods=['POST']) @@ -38,7 +39,7 @@ def post_continue(): data = request.get_json(force=True) LOG.debug("Got JSON %s, going into processing thread", data) try: - res = discoverd.process(data) + res = process.process(data) except utils.DiscoveryFailed as exc: return str(exc), exc.http_code else: @@ -61,7 +62,7 @@ def post_discover(): data = request.get_json(force=True) LOG.debug("Got JSON %s", data) try: - discoverd.discover(data) + discover.discover(data) except utils.DiscoveryFailed as exc: return str(exc), exc.http_code else: diff --git a/ironic_discoverd/discoverd.py b/ironic_discoverd/process.py similarity index 52% rename from ironic_discoverd/discoverd.py rename to ironic_discoverd/process.py index 764e61097..abf479b8e 100644 --- a/ironic_discoverd/discoverd.py +++ b/ironic_discoverd/process.py @@ -11,6 +11,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +"""Handling discovery data from the ramdisk.""" + import logging import time @@ -24,19 +26,24 @@ from ironic_discoverd.plugins import base as plugins_base from ironic_discoverd import utils -LOG = logging.getLogger("discoverd") +LOG = logging.getLogger("ironic_discoverd.process") + +_POWER_OFF_CHECK_PERIOD = 5 def process(node_info): - """Process data from discovery ramdisk.""" + """Process data from discovery ramdisk. + + This function heavily relies on the hooks to do the actual data processing. + """ hooks = plugins_base.processing_hooks_manager() for hook_ext in hooks: hook_ext.obj.pre_discover(node_info) - bmc_address = node_info.get('ipmi_address') + cached_node = node_cache.pop_node( + bmc_address=node_info.get('ipmi_address'), + mac=node_info.get('macs')) - cached_node = node_cache.pop_node(bmc_address=bmc_address, - mac=node_info.get('macs')) ironic = utils.get_client() try: node = ironic.node.get(cached_node.uuid) @@ -53,9 +60,28 @@ def process(node_info): return {'node': updated.to_dict()} -def _process_node(ironic, node, node_info, cached_node): +def _run_post_hooks(node, ports, node_info): hooks = plugins_base.processing_hooks_manager() + port_instances = list(ports.values()) + node_patches = [] + port_patches = {} + for hook_ext in hooks: + hook_patch = hook_ext.obj.post_discover(node, port_instances, + node_info) + if not hook_patch: + continue + + node_patches.extend(hook_patch[0]) + port_patches.update(hook_patch[1]) + + node_patches = [p for p in node_patches if p] + port_patches = {mac: patch for (mac, patch) in port_patches.items() + if mac in ports and patch} + return node_patches, port_patches + + +def _process_node(ironic, node, node_info, cached_node): ports = {} for mac in (node_info.get('macs') or ()): try: @@ -67,22 +93,8 @@ def _process_node(ironic, node, node_info, cached_node): 'database - skipping', {'mac': mac, 'node': node.uuid}) - node_patches = [] - port_patches = {} - for hook_ext in hooks: - hook_patch = hook_ext.obj.post_discover(node, list(ports.values()), - node_info) - if not hook_patch: - continue - - node_patches.extend(hook_patch[0]) - port_patches.update(hook_patch[1]) - node_patches = [p for p in node_patches if p] - port_patches = {mac: patch for (mac, patch) in port_patches.items() - if mac in ports and patch} - + node_patches, port_patches = _run_post_hooks(node, ports, node_info) node = ironic.node.update(node.uuid, node_patches) - for mac, patches in port_patches.items(): ironic.port.update(ports[mac].uuid, patches) @@ -104,9 +116,6 @@ def _process_node(ironic, node, node_info, cached_node): return node -_POWER_OFF_CHECK_PERIOD = 5 - - def _wait_for_power_off(ironic, cached_node): deadline = cached_node.started_at + conf.getint('discoverd', 'timeout') # NOTE(dtantsur): even VM's don't power off instantly, sleep first @@ -114,91 +123,14 @@ def _wait_for_power_off(ironic, cached_node): eventlet.greenthread.sleep(_POWER_OFF_CHECK_PERIOD) node = ironic.node.get(cached_node.uuid) if (node.power_state or 'power off').lower() == 'power off': - patch = [{'op': 'add', 'path': '/extra/newly_discovered', - 'value': 'true'}, - {'op': 'remove', 'path': '/extra/on_discovery'}] - ironic.node.update(cached_node.uuid, patch) + _finish_discovery(ironic, node) return LOG.error('Timeout waiting for power off state of node %s after discovery', cached_node.uuid) -def discover(uuids): - """Initiate discovery for given node uuids.""" - if not uuids: - raise utils.DiscoveryFailed("No nodes to discover") - - ironic = utils.get_client() - LOG.debug('Validating nodes %s', uuids) - nodes = [] - for uuid in uuids: - try: - node = ironic.node.get(uuid) - except exceptions.NotFound: - LOG.error('Node %s cannot be found', uuid) - raise utils.DiscoveryFailed("Cannot find node %s" % uuid, code=404) - except exceptions.HttpError as exc: - LOG.exception('Cannot get node %s', uuid) - raise utils.DiscoveryFailed("Cannot get node %s: %s" % (uuid, exc)) - - _validate(ironic, node) - nodes.append(node) - - LOG.info('Proceeding with discovery on nodes %s', [n.uuid for n in nodes]) - eventlet.greenthread.spawn_n(_background_discover, ironic, nodes) - - -def _validate(ironic, node): - if node.instance_uuid: - LOG.error('Refusing to discover node %s with assigned instance_uuid', - node.uuid) - raise utils.DiscoveryFailed( - 'Refusing to discover node %s with assigned instance uuid' % - node.uuid) - - power_state = node.power_state - if (not node.maintenance and power_state is not None - and power_state.lower() != 'power off'): - LOG.error('Refusing to discover node %s with power_state "%s" ' - 'and maintenance mode off', - node.uuid, power_state) - raise utils.DiscoveryFailed( - 'Refusing to discover node %s with power state "%s" and ' - 'maintenance mode off' % - (node.uuid, power_state)) - - validation = ironic.node.validate(node.uuid) - if not validation.power['result']: - LOG.error('Failed validation of power interface for node %s, ' - 'reason: %s', node.uuid, validation.power['reason']) - raise utils.DiscoveryFailed('Failed validation of power interface for ' - 'node %s' % node.uuid) - - -def _background_discover(ironic, nodes): - patch = [{'op': 'add', 'path': '/extra/on_discovery', 'value': 'true'}, - {'op': 'add', 'path': '/extra/discovery_timestamp', - 'value': str(time.time())}] - for node in nodes: - ironic.node.update(node.uuid, patch) - - all_macs = set() - for node in nodes: - # TODO(dtantsur): pagination - macs = [p.address for p in ironic.node.list_ports(node.uuid, limit=0)] - all_macs.update(macs) - node_cache.add_node(node.uuid, - bmc_address=node.driver_info.get('ipmi_address'), - mac=macs) - - if all_macs: - LOG.info('Whitelisting MAC\'s %s in the firewall', all_macs) - firewall.update_filters(ironic) - - for node in nodes: - try: - ironic.node.set_power_state(node.uuid, 'reboot') - except Exception as exc: - LOG.error('Failed to power on node %s, check it\'s power ' - 'management configuration:\n%s', node.uuid, exc) +def _finish_discovery(ironic, node): + patch = [{'op': 'add', 'path': '/extra/newly_discovered', 'value': 'true'}, + {'op': 'remove', 'path': '/extra/on_discovery'}] + ironic.node.update(node.uuid, patch) diff --git a/ironic_discoverd/test.py b/ironic_discoverd/test.py index d206fc97d..3b3270ab6 100644 --- a/ironic_discoverd/test.py +++ b/ironic_discoverd/test.py @@ -24,12 +24,13 @@ from mock import patch, Mock, ANY # noqa from ironic_discoverd import client from ironic_discoverd import conf -from ironic_discoverd import discoverd +from ironic_discoverd import discover from ironic_discoverd import firewall from ironic_discoverd import main from ironic_discoverd import node_cache from ironic_discoverd.plugins import base as plugins_base from ironic_discoverd.plugins import example as example_plugin +from ironic_discoverd import process from ironic_discoverd import utils @@ -116,7 +117,7 @@ class TestProcess(BaseTest): self.node.to_dict.return_value = self.data cli.node.update.return_value = self.node - res = discoverd.process(self.data) + res = process.process(self.data) self.assertEqual({'node': self.data}, res) pop_mock.assert_called_once_with(**self.attributes) @@ -158,10 +159,19 @@ class TestProcess(BaseTest): started_at=1000) cli.node.get.return_value = self.node - discoverd.process(self.data) + process.process(self.data) cli.node.update.assert_called_once_with(self.node.uuid, self.patch1) + def test_power_failure(self, client_mock, pop_mock, filters_mock, pre_mock, + post_mock): + conf.CONF.set('discoverd', 'power_off_after_discovery', 'true') + client_mock.return_value.node.set_power_state.side_effect = ( + exceptions.Conflict()) + self.assertRaisesRegexp(utils.DiscoveryFailed, 'Failed to power off', + self._do_test, client_mock, pop_mock, + filters_mock, pre_mock, post_mock) + def test_no_ipmi(self, client_mock, pop_mock, filters_mock, pre_mock, post_mock): del self.data['ipmi_address'] @@ -198,7 +208,7 @@ class TestProcess(BaseTest): self.assertRaisesRegexp(utils.DiscoveryFailed, 'boom', - discoverd.process, self.data) + process.process, self.data) self.assertFalse(cli.node.update.called) self.assertFalse(cli.port.create.called) @@ -213,7 +223,7 @@ class TestProcess(BaseTest): self.assertRaisesRegexp(utils.DiscoveryFailed, 'not found in Ironic', - discoverd.process, self.data) + process.process, self.data) cli.node.get.assert_called_once_with(self.node.uuid) self.assertFalse(cli.node.update.called) @@ -225,14 +235,14 @@ class TestProcess(BaseTest): self.data['error'] = 'BOOM' self.assertRaisesRegexp(utils.DiscoveryFailed, 'BOOM', - discoverd.process, self.data) + process.process, self.data) def test_missing(self, client_mock, pop_mock, filters_mock, pre_mock, post_mock): del self.data['cpus'] self.assertRaisesRegexp(utils.DiscoveryFailed, 'missing', - discoverd.process, self.data) + process.process, self.data) @patch.object(eventlet.greenthread, 'spawn_n', @@ -276,11 +286,15 @@ class TestDiscover(BaseTest): ports = [ [Mock(address='1-1'), Mock(address='1-2')], [Mock(address='2-1'), Mock(address='2-2')], - [Mock(address='3-1'), Mock(address='3-2')], + [], ] cli.node.list_ports.side_effect = ports + # Failure to powering on does not cause total failure + cli.node.set_power_state.side_effect = [None, + exceptions.Conflict(), + None] - discoverd.discover(['uuid1', 'uuid2', 'uuid3']) + discover.discover(['uuid1', 'uuid2', 'uuid3']) self.assertEqual(3, cli.node.get.call_count) self.assertEqual(3, cli.node.list_ports.call_count) @@ -296,8 +310,9 @@ class TestDiscover(BaseTest): mac=['2-1', '2-2']) add_mock.assert_any_call(self.node3.uuid, bmc_address='1.2.3.5', - mac=['3-1', '3-2']) - filters_mock.assert_called_once_with(cli) + mac=[]) + filters_mock.assert_called_with(cli) + self.assertEqual(2, filters_mock.call_count) # 1 node w/o ports self.assertEqual(3, cli.node.set_power_state.call_count) cli.node.set_power_state.assert_called_with(ANY, 'reboot') patch = [{'op': 'add', 'path': '/extra/on_discovery', 'value': 'true'}, @@ -307,8 +322,9 @@ class TestDiscover(BaseTest): cli.node.update.assert_any_call('uuid2', patch) cli.node.update.assert_any_call('uuid3', patch) self.assertEqual(3, cli.node.update.call_count) - spawn_mock.assert_called_once_with(discoverd._background_discover, - cli, ANY) + spawn_mock.assert_called_with(discover._background_start_discover, + cli, ANY) + self.assertEqual(3, spawn_mock.call_count) def test_failed_to_get_node(self, client_mock, add_mock, filters_mock, spawn_mock): @@ -319,7 +335,7 @@ class TestDiscover(BaseTest): ] self.assertRaisesRegexp(utils.DiscoveryFailed, 'Cannot find node uuid2', - discoverd.discover, ['uuid1', 'uuid2']) + discover.discover, ['uuid1', 'uuid2']) cli.node.get.side_effect = [ exceptions.Conflict(), @@ -327,7 +343,7 @@ class TestDiscover(BaseTest): ] self.assertRaisesRegexp(utils.DiscoveryFailed, 'Cannot get node uuid1', - discoverd.discover, ['uuid1', 'uuid2']) + discover.discover, ['uuid1', 'uuid2']) self.assertEqual(3, cli.node.get.call_count) self.assertEqual(0, cli.node.list_ports.call_count) @@ -350,7 +366,7 @@ class TestDiscover(BaseTest): self.assertRaisesRegexp( utils.DiscoveryFailed, 'Failed validation of power interface for node uuid2', - discoverd.discover, ['uuid1', 'uuid2']) + discover.discover, ['uuid1', 'uuid2']) self.assertEqual(2, cli.node.get.call_count) self.assertEqual(2, cli.node.validate.call_count) @@ -363,7 +379,7 @@ class TestDiscover(BaseTest): def test_no_uuids(self, client_mock, add_mock, filters_mock, spawn_mock): self.assertRaisesRegexp(utils.DiscoveryFailed, 'No nodes to discover', - discoverd.discover, []) + discover.discover, []) self.assertFalse(client_mock.called) self.assertFalse(add_mock.called) @@ -378,7 +394,7 @@ class TestDiscover(BaseTest): self.assertRaisesRegexp( utils.DiscoveryFailed, 'node uuid2 with assigned instance uuid', - discoverd.discover, ['uuid1', 'uuid2']) + discover.discover, ['uuid1', 'uuid2']) self.assertEqual(2, cli.node.get.call_count) self.assertEqual(0, cli.node.list_ports.call_count) @@ -399,7 +415,7 @@ class TestDiscover(BaseTest): self.assertRaisesRegexp( utils.DiscoveryFailed, 'node uuid2 with power state "power on"', - discoverd.discover, ['uuid1', 'uuid2']) + discover.discover, ['uuid1', 'uuid2']) self.assertEqual(2, cli.node.get.call_count) self.assertEqual(0, cli.node.list_ports.call_count) @@ -415,14 +431,14 @@ class TestApi(BaseTest): main.app.config['TESTING'] = True self.app = main.app.test_client() - @patch.object(discoverd, 'discover', autospec=True) + @patch.object(discover, 'discover', autospec=True) def test_discover_no_authentication(self, discover_mock): conf.CONF.set('discoverd', 'authenticate', 'false') res = self.app.post('/v1/discover', data='["uuid1"]') self.assertEqual(202, res.status_code) discover_mock.assert_called_once_with(["uuid1"]) - @patch.object(discoverd, 'discover', autospec=True) + @patch.object(discover, 'discover', autospec=True) def test_discover_failed(self, discover_mock): conf.CONF.set('discoverd', 'authenticate', 'false') discover_mock.side_effect = utils.DiscoveryFailed("boom") @@ -431,7 +447,7 @@ class TestApi(BaseTest): self.assertEqual(b"boom", res.data) discover_mock.assert_called_once_with(["uuid1"]) - @patch.object(discoverd, 'discover', autospec=True) + @patch.object(discover, 'discover', autospec=True) def test_discover_missing_authentication(self, discover_mock): conf.CONF.set('discoverd', 'authenticate', 'true') res = self.app.post('/v1/discover', data='["uuid1"]') @@ -439,7 +455,7 @@ class TestApi(BaseTest): self.assertFalse(discover_mock.called) @patch.object(utils, 'get_keystone', autospec=True) - @patch.object(discoverd, 'discover', autospec=True) + @patch.object(discover, 'discover', autospec=True) def test_discover_failed_authentication(self, discover_mock, keystone_mock): conf.CONF.set('discoverd', 'authenticate', 'true') @@ -450,7 +466,7 @@ class TestApi(BaseTest): self.assertFalse(discover_mock.called) keystone_mock.assert_called_once_with(token='token') - @patch.object(discoverd, 'process', autospec=True) + @patch.object(process, 'process', autospec=True) def test_continue(self, process_mock): process_mock.return_value = [42] res = self.app.post('/v1/continue', data='"JSON"') @@ -458,7 +474,7 @@ class TestApi(BaseTest): process_mock.assert_called_once_with("JSON") self.assertEqual(b'[42]', res.data) - @patch.object(discoverd, 'process', autospec=True) + @patch.object(process, 'process', autospec=True) def test_continue_failed(self, process_mock): process_mock.side_effect = utils.DiscoveryFailed("boom") res = self.app.post('/v1/continue', data='"JSON"')