From 03eb9e022e1a9bc15d6e90b6e163d27102da412f Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 17 Feb 2015 13:09:13 +0100 Subject: [PATCH] Add module for in-band inspection using ironic-discoverd Also adds FakeDiscoverdDriver for the sake of testing. Change-Id: I49463e7eb897c73b42e1d2816b4f2001b885c7f6 Implements: blueprint inband-properties-discovery --- driver-requirements.txt | 1 + etc/ironic/ironic.conf.sample | 24 ++- ironic/api/controllers/v1/node.py | 5 +- ironic/common/states.py | 5 + ironic/drivers/fake.py | 13 ++ ironic/drivers/modules/discoverd.py | 193 ++++++++++++++++++ ironic/tests/drivers/test_discoverd.py | 187 +++++++++++++++++ .../tests/drivers/third_party_driver_mocks.py | 11 + setup.cfg | 1 + 9 files changed, 436 insertions(+), 4 deletions(-) create mode 100644 ironic/drivers/modules/discoverd.py create mode 100644 ironic/tests/drivers/test_discoverd.py diff --git a/driver-requirements.txt b/driver-requirements.txt index df025b47e2..a840e306d2 100644 --- a/driver-requirements.txt +++ b/driver-requirements.txt @@ -4,6 +4,7 @@ # python projects they should package as optional dependencies for Ironic. # These are available on pypi +ironic-discoverd>=1.0.0 proliantutils>=2.0.1 pyghmi pysnmp diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 7d5bfbf932..87a2335af8 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -15,7 +15,7 @@ #rpc_conn_pool_size=30 # Qpid broker hostname. (string value) -#qpid_hostname=ironic +#qpid_hostname=localhost # Qpid broker port. (integer value) #qpid_port=5672 @@ -76,7 +76,7 @@ # The RabbitMQ broker address where a single node is used. # (string value) -#rabbit_host=ironic +#rabbit_host=localhost # The RabbitMQ broker port where a single node is used. # (integer value) @@ -781,6 +781,26 @@ #dhcp_provider=neutron +[discoverd] + +# +# Options defined in ironic.drivers.modules.discoverd +# + +# whether to enable inspection using ironic-discoverd (boolean +# value) +#enabled=false + +# ironic-discoverd HTTP endpoint. If this is not set, the +# ironic-discoverd client default (http://127.0.0.1:5050) will +# be used. (string value) +#service_url= + +# period (in seconds) to check status of nodes on inspection +# (integer value) +#status_check_period=60 + + [disk_partitioner] # diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index acb5b4f8a2..7c577fea01 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -1012,10 +1012,11 @@ class NodesController(rest.RestController): rpc_node = _get_rpc_node(node_ident) - # Check if node is transitioning state, although nodes in DEPLOYFAIL + # Check if node is transitioning state, although nodes in some states # can be updated. if ((rpc_node.target_power_state or rpc_node.target_provision_state) - and rpc_node.provision_state != ir_states.DEPLOYFAIL): + and rpc_node.provision_state not in + ir_states.UPDATE_ALLOWED_STATES): msg = _("Node %s can not be updated while a state transition " "is in progress.") raise wsme.exc.ClientSideError(msg % node_ident, status_code=409) diff --git a/ironic/common/states.py b/ironic/common/states.py index 544fc78339..9baf695cf5 100644 --- a/ironic/common/states.py +++ b/ironic/common/states.py @@ -149,6 +149,11 @@ inspected node shall transition to MANAGEABLE status. INSPECTFAIL = 'inspect failed' """ Node inspection failed. """ + +UPDATE_ALLOWED_STATES = (DEPLOYFAIL, INSPECTING, INSPECTFAIL) +"""Transitional states in which we allow updating a node.""" + + ############## # Power states ############## diff --git a/ironic/drivers/fake.py b/ironic/drivers/fake.py index 49a7790c0e..31228d2914 100644 --- a/ironic/drivers/fake.py +++ b/ironic/drivers/fake.py @@ -23,6 +23,7 @@ from ironic.common import exception from ironic.common.i18n import _ from ironic.drivers import base from ironic.drivers.modules import agent +from ironic.drivers.modules import discoverd from ironic.drivers.modules.drac import management as drac_mgmt from ironic.drivers.modules.drac import power as drac_power from ironic.drivers.modules import fake @@ -201,3 +202,15 @@ class FakeVirtualBoxDriver(base.BaseDriver): self.power = virtualbox.VirtualBoxPower() self.deploy = fake.FakeDeploy() self.management = virtualbox.VirtualBoxManagement() + + +class FakeIPMIToolDiscoverdDriver(base.BaseDriver): + """Fake Discoverd driver.""" + + def __init__(self): + self.power = ipmitool.IPMIPower() + self.console = ipmitool.IPMIShellinaboxConsole() + self.deploy = fake.FakeDeploy() + self.vendor = ipmitool.VendorPassthru() + self.management = ipmitool.IPMIManagement() + self.inspect = discoverd.DiscoverdInspect() diff --git a/ironic/drivers/modules/discoverd.py b/ironic/drivers/modules/discoverd.py new file mode 100644 index 0000000000..4bc7b8fc36 --- /dev/null +++ b/ironic/drivers/modules/discoverd.py @@ -0,0 +1,193 @@ +# 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. + +""" +Modules required to work with ironic_discoverd: + https://pypi.python.org/pypi/ironic-discoverd +""" + +import eventlet +from oslo_config import cfg +from oslo_utils import importutils + +from ironic.common import exception +from ironic.common.i18n import _ +from ironic.common.i18n import _LE +from ironic.common.i18n import _LI +from ironic.common import keystone +from ironic.common import states +from ironic.conductor import task_manager +from ironic.drivers import base +from ironic.openstack.common import log as logging + + +LOG = logging.getLogger(__name__) + + +discoverd_opts = [ + cfg.BoolOpt('enabled', default=False, + help='whether to enable inspection using ironic-discoverd'), + cfg.StrOpt('service_url', + help='ironic-discoverd HTTP endpoint. If this is not set, the ' + 'ironic-discoverd client default (http://127.0.0.1:5050) ' + 'will be used.'), + cfg.IntOpt('status_check_period', default=60, + help='period (in seconds) to check status of nodes ' + 'on inspection') +] + +CONF = cfg.CONF +CONF.register_opts(discoverd_opts, group='discoverd') + + +ironic_discoverd = importutils.try_import('ironic_discoverd') +if ironic_discoverd: + from ironic_discoverd import client + + +class DiscoverdInspect(base.InspectInterface): + """In-band inspection via ironic-discoverd project.""" + + def __init__(self): + if not CONF.discoverd.enabled: + LOG.info('ironic-discoverd support is disabled') + return + + if not ironic_discoverd: + raise exception.DriverLoadError( + _('ironic-discoverd Python module not found')) + + # NOTE(dtantsur): __version_info__ attribute appeared in 1.0.0 + version = getattr(ironic_discoverd, '__version_info__', (0, 2)) + if version < (1, 0): + raise exception.DriverLoadError( + _('ironic-discoverd version is too old: required >= 1.0.0 ' + 'got %s') % '.'.join(str(x) for x in version)) + + def get_properties(self): + """Return the properties of the interface. + + :returns: dictionary of : entries. + """ + return {} # no properties + + def validate(self, task): + """Validate the driver-specific management information. + + If invalid, raises an exception; otherwise returns None. + + :param task: a task from TaskManager. + :raises: UnsupportedDriverExtension if discoverd support is disabled + """ + if not CONF.discoverd.enabled: + raise exception.UnsupportedDriverExtension( + _('ironic-discoverd support is disabled in ' + 'configuration, set [discoverd]enabled to true ' + 'to enable')) + + def inspect_hardware(self, task): + """Inspect hardware to obtain the hardware properties. + + This particular implementation only starts inspection using + ironic-discoverd. Results will be checked in a periodic task. + + :param task: a task from TaskManager. + :returns: states.INSPECTING + """ + LOG.debug('Starting inspection for node %(uuid)s using ' + 'ironic-discoverd client %(version)s', + {'uuid': task.node.uuid, 'version': + ironic_discoverd.__version__}) + + # NOTE(dtantsur): we're spawning a short-living green thread so that + # we can release a lock as soon as possible and allow ironic-discoverd + # to operate on a node. + eventlet.spawn_n(_start_inspection, task.node.uuid, task.context) + return states.INSPECTING + + @base.driver_periodic_task(spacing=CONF.discoverd.status_check_period, + enabled=CONF.discoverd.enabled) + def _periodic_check_result(self, manager, context): + """Periodic task checking results of inspection.""" + filters = {'provision_state': states.INSPECTING} + node_iter = manager.iter_nodes(filters=filters) + + for node_uuid, driver in node_iter: + try: + # TODO(dtantsur): we need an exclusive lock only once + # inspection is finished. + with task_manager.acquire(context, node_uuid) as task: + _check_status(task) + except (exception.NodeLocked, exception.NodeNotFound): + continue + + +def _call_discoverd(func, uuid, context): + """Wrapper around calls to discoverd.""" + # NOTE(dtantsur): due to bug #1428652 None is not accepted for base_url. + kwargs = {} + if CONF.discoverd.service_url: + kwargs['base_url'] = CONF.discoverd.service_url + return func(uuid, auth_token=context.auth_token, **kwargs) + + +def _start_inspection(node_uuid, context): + """Call to discoverd to start inspection.""" + try: + _call_discoverd(client.introspect, node_uuid, context) + except Exception as exc: + LOG.exception(_LE('Exception during contacting ironic-discoverd ' + 'for inspection of node %(node)s: %(err)s'), + {'node': node_uuid, 'err': exc}) + # NOTE(dtantsur): if acquire fails our last option is to rely on + # timeout + with task_manager.acquire(context, node_uuid) as task: + task.node.last_error = _('Failed to start inspection: %s') % exc + task.process_event('fail') + else: + LOG.info(_LI('Node %s was sent to inspection to ironic-discoverd'), + node_uuid) + + +def _check_status(task): + """Check inspection status for node given by a task.""" + node = task.node + if node.provision_state != states.INSPECTING: + return + if not isinstance(task.driver.inspect, DiscoverdInspect): + return + + LOG.debug('Calling to discoverd to check status of node %s', + task.node.uuid) + + # NOTE(dtantsur): periodic tasks do not have proper tokens in context + task.context.auth_token = keystone.get_admin_auth_token() + try: + status = _call_discoverd(client.get_status, node.uuid, task.context) + except Exception: + # NOTE(dtantsur): get_status should not normally raise + # let's assume it's a transient failure and retry later + LOG.exception(_LE('Unexpected exception while getting ' + 'inspection status for node %s, will retry later'), + node.uuid) + return + + if status.get('error'): + LOG.error(_LE('Inspection failed for node %(uuid)s ' + 'with error: %(err)s'), + {'uuid': node.uuid, 'err': status['error']}) + node.last_error = _('ironic-discoverd: %s') % status['error'] + task.process_event('fail') + elif status.get('finished'): + LOG.info(_LI('Inspection finished successfully for node %s'), + node.uuid) + task.process_event('done') diff --git a/ironic/tests/drivers/test_discoverd.py b/ironic/tests/drivers/test_discoverd.py new file mode 100644 index 0000000000..85ee8f38d3 --- /dev/null +++ b/ironic/tests/drivers/test_discoverd.py @@ -0,0 +1,187 @@ +# 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. + +import eventlet +import ironic_discoverd +from ironic_discoverd import client +import mock + +from ironic.common import driver_factory +from ironic.common import exception +from ironic.common import keystone +from ironic.common import states +from ironic.conductor import task_manager +from ironic.drivers.modules import discoverd +from ironic.tests.conductor import utils as mgr_utils +from ironic.tests.db import base as db_base +from ironic.tests.objects import utils as obj_utils + + +class BaseTestCase(db_base.DbTestCase): + def setUp(self): + super(BaseTestCase, self).setUp() + mgr_utils.mock_the_extension_manager("fake_discoverd") + self.driver = driver_factory.get_driver("fake_discoverd") + self.node = obj_utils.get_test_node(self.context) + self.task = mock.Mock(spec=task_manager.TaskManager) + self.task.context = mock.Mock() + self.task.shared = False + self.task.node = self.node + self.task.driver = self.driver + self.config(enabled=True, group='discoverd') + + +class ValidateTestCase(BaseTestCase): + def test_validate_ok(self): + self.driver.inspect.validate(self.task) + + def test_validate_disabled(self): + self.config(enabled=False, group='discoverd') + self.assertRaises(exception.UnsupportedDriverExtension, + self.driver.inspect.validate, self.task) + + @mock.patch.object(ironic_discoverd, '__version_info__', (1, 0, 0)) + def test_init_ok(self): + discoverd.DiscoverdInspect() + self.config(enabled=False, group='discoverd') + discoverd.DiscoverdInspect() + + @mock.patch.object(ironic_discoverd, '__version_info__', (0, 2, 2)) + def test_old_version(self): + self.assertRaises(exception.DriverLoadError, + discoverd.DiscoverdInspect) + self.config(enabled=False, group='discoverd') + discoverd.DiscoverdInspect() + + @mock.patch.object(discoverd, 'ironic_discoverd', None) + def test_not_imported(self): + self.assertRaises(exception.DriverLoadError, + discoverd.DiscoverdInspect) + self.config(enabled=False, group='discoverd') + discoverd.DiscoverdInspect() + + +@mock.patch.object(eventlet, 'spawn_n', lambda f, *a, **kw: f(*a, **kw)) +@mock.patch.object(client, 'introspect') +class InspectHardwareTestCase(BaseTestCase): + def test_ok(self, mock_introspect): + self.assertEqual(states.INSPECTING, + self.driver.inspect.inspect_hardware(self.task)) + mock_introspect.assert_called_once_with( + self.node.uuid, + auth_token=self.task.context.auth_token) + + def test_url(self, mock_introspect): + self.config(service_url='meow', group='discoverd') + self.assertEqual(states.INSPECTING, + self.driver.inspect.inspect_hardware(self.task)) + mock_introspect.assert_called_once_with( + self.node.uuid, + auth_token=self.task.context.auth_token, + base_url='meow') + + @mock.patch.object(task_manager, 'acquire') + def test_error(self, mock_acquire, mock_introspect): + mock_introspect.side_effect = RuntimeError('boom') + self.driver.inspect.inspect_hardware(self.task) + mock_introspect.assert_called_once_with( + self.node.uuid, + auth_token=self.task.context.auth_token) + task = mock_acquire.return_value.__enter__.return_value + self.assertIn('boom', task.node.last_error) + task.process_event.assert_called_once_with('fail') + + +@mock.patch.object(keystone, 'get_admin_auth_token', lambda: 'the token') +@mock.patch.object(client, 'get_status') +class CheckStatusTestCase(BaseTestCase): + def setUp(self): + super(CheckStatusTestCase, self).setUp() + self.node.provision_state = states.INSPECTING + + def test_not_inspecting(self, mock_get): + self.node.provision_state = states.MANAGEABLE + discoverd._check_status(self.task) + self.assertFalse(mock_get.called) + + def test_not_discoverd(self, mock_get): + self.task.driver.inspect = object() + discoverd._check_status(self.task) + self.assertFalse(mock_get.called) + + def test_not_finished(self, mock_get): + mock_get.return_value = {} + discoverd._check_status(self.task) + mock_get.assert_called_once_with(self.node.uuid, + auth_token='the token') + self.assertFalse(self.task.process_event.called) + + def test_exception_ignored(self, mock_get): + mock_get.side_effect = RuntimeError('boom') + discoverd._check_status(self.task) + mock_get.assert_called_once_with(self.node.uuid, + auth_token='the token') + self.assertFalse(self.task.process_event.called) + + def test_status_ok(self, mock_get): + mock_get.return_value = {'finished': True} + discoverd._check_status(self.task) + mock_get.assert_called_once_with(self.node.uuid, + auth_token='the token') + self.task.process_event.assert_called_once_with('done') + + def test_status_error(self, mock_get): + mock_get.return_value = {'error': 'boom'} + discoverd._check_status(self.task) + mock_get.assert_called_once_with(self.node.uuid, + auth_token='the token') + self.task.process_event.assert_called_once_with('fail') + self.assertIn('boom', self.node.last_error) + + def test_service_url(self, mock_get): + self.config(service_url='meow', group='discoverd') + mock_get.return_value = {'finished': True} + discoverd._check_status(self.task) + mock_get.assert_called_once_with(self.node.uuid, + auth_token='the token', + base_url='meow') + self.task.process_event.assert_called_once_with('done') + + +@mock.patch.object(eventlet.greenthread, 'spawn_n', + lambda f, *a, **kw: f(*a, **kw)) +@mock.patch.object(ironic_discoverd, '__version_info__', (1, 0, 0)) +@mock.patch.object(task_manager, 'acquire', autospec=True) +@mock.patch.object(discoverd, '_check_status', autospec=True) +class PeriodicTaskTestCase(BaseTestCase): + def test_ok(self, mock_check, mock_acquire): + mgr = mock.Mock(spec=['iter_nodes']) + mgr.iter_nodes.return_value = [('1', 'd1'), ('2', 'd2')] + tasks = [mock.sentinel.task1, mock.sentinel.task2] + mock_acquire.side_effect = ( + mock.MagicMock(__enter__=mock.Mock(return_value=task)) + for task in tasks + ) + discoverd.DiscoverdInspect()._periodic_check_result( + mgr, mock.sentinel.context) + mock_check.assert_any_call(tasks[0]) + mock_check.assert_any_call(tasks[1]) + self.assertEqual(2, mock_acquire.call_count) + + def test_node_locked(self, mock_check, mock_acquire): + mgr = mock.Mock(spec=['iter_nodes']) + mgr.iter_nodes.return_value = [('1', 'd1'), ('2', 'd2')] + mock_acquire.side_effect = exception.NodeLocked("boom") + discoverd.DiscoverdInspect()._periodic_check_result( + mgr, mock.sentinel.context) + self.assertFalse(mock_check.called) + self.assertEqual(2, mock_acquire.call_count) diff --git a/ironic/tests/drivers/third_party_driver_mocks.py b/ironic/tests/drivers/third_party_driver_mocks.py index 36dbf9b50b..87ced95dac 100644 --- a/ironic/tests/drivers/third_party_driver_mocks.py +++ b/ironic/tests/drivers/third_party_driver_mocks.py @@ -176,3 +176,14 @@ if not pyremotevbox: sys.modules['pyremotevbox'] = pyremotevbox if 'ironic.drivers.modules.virtualbox' in sys.modules: reload(sys.modules['ironic.drivers.modules.virtualbox']) + + +ironic_discoverd = importutils.try_import('ironic_discoverd') +if not ironic_discoverd: + ironic_discoverd = mock.MagicMock() + ironic_discoverd.__version_info__ = (1, 0, 0) + ironic_discoverd.__version__ = "1.0.0" + sys.modules['ironic_discoverd'] = ironic_discoverd + sys.modules['ironic_discoverd.client'] = ironic_discoverd.client + if 'ironic.drivers.modules.discoverd' in sys.modules: + reload(sys.modules['ironic.drivers.modules.discoverd']) diff --git a/setup.cfg b/setup.cfg index 6f9a28e3c1..fd886a0d5d 100644 --- a/setup.cfg +++ b/setup.cfg @@ -41,6 +41,7 @@ ironic.drivers = agent_vbox = ironic.drivers.agent:AgentAndVirtualBoxDriver fake = ironic.drivers.fake:FakeDriver fake_agent = ironic.drivers.fake:FakeAgentDriver + fake_discoverd = ironic.drivers.fake:FakeIPMIToolDiscoverdDriver fake_ipmitool = ironic.drivers.fake:FakeIPMIToolDriver fake_ipminative = ironic.drivers.fake:FakeIPMINativeDriver fake_ssh = ironic.drivers.fake:FakeSSHDriver