openstack
/
ironic-inspector
Code Issues Proposed changes

Refactoring: split discoverd.py into 2 modules 

Wrote some missing tests as well.

Change-Id: I2d4d4619cb5825422cb2fc684f7cd028f99b991d
This commit is contained in:
Dmitry Tantsur 2014-12-08 18:27:42 +01:00
parent bc9a5d9e5d
commit b8e1d97e81
5 changed files with 189 additions and 136 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

104
ironic_discoverd/discover.py Normal file
View File

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

2
ironic_discoverd/firewall.py
View File

@ -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

9
ironic_discoverd/main.py
View File

@ -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:

144
ironic_discoverd/discoverd.py → ironic_discoverd/process.py
View File

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

66
ironic_discoverd/test.py
View File

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