From edf532db91bb7ac40e9a8382cf9fbedeeac814e4 Mon Sep 17 00:00:00 2001 From: Lucas Alvares Gomes Date: Wed, 28 Jan 2015 16:20:06 +0000 Subject: [PATCH] Add logic to store the config drive passed by Nova This patch is extending the /nodes//provision API endpoint to accept an optional "configdrive" parameter as part of the request BODY. If present, Ironic will get the config drive and store it either directly on the Node's instance_info field or it will upload it to Swift first, generates a temp URL and then store it on Node's instance_info to be processed later after the deployment started. Two new config options were added to the conductor: * configdrive_use_swift: Whether to upload the config drive to Swift ot not. Defaults to False * configdrive_swift_container: The name of the container in Swift to store the config drive. Defaults to ironic_configdrive_container Implements: blueprint expose-configdrive Change-Id: Icc39af604af6439e85f14d1beb2c19b10e983635 --- etc/ironic/ironic.conf.sample | 16 +++++ ironic/api/controllers/v1/node.py | 26 +++++-- ironic/conductor/manager.py | 85 ++++++++++++++++++++--- ironic/conductor/rpcapi.py | 11 +-- ironic/tests/api/v1/test_nodes.py | 23 ++++++- ironic/tests/conductor/test_manager.py | 94 +++++++++++++++++++++++++- ironic/tests/conductor/test_rpcapi.py | 5 +- 7 files changed, 233 insertions(+), 27 deletions(-) diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index 6f2302524e..975833a366 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -581,6 +581,13 @@ # the check entirely. (integer value) #sync_local_state_interval=180 +# Whether upload the config drive to Swift. (boolean value) +#configdrive_use_swift=false + +# The Swift config drive container to store data. (string +# value) +#configdrive_swift_container=ironic_configdrive_container + [console] @@ -817,6 +824,15 @@ # (string value) #swift_container=glance +# This should match a config by the same name in the Glance +# configuration file. When set to 0, a single-tenant store +# will only use one container to store all images. When set to +# an integer value between 1 and 32, a single-tenant store +# will use multiple containers to store images, and this value +# will determine how many containers are created. (integer +# value) +#swift_store_multiple_containers_seed=0 + # # Options defined in ironic.common.image_service diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 0299f94425..0e4359708a 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -304,8 +304,9 @@ class NodeStatesController(rest.RestController): url_args = '/'.join([node_uuid, 'states']) pecan.response.location = link.build_url('nodes', url_args) - @wsme_pecan.wsexpose(None, types.uuid, wtypes.text, status_code=202) - def provision(self, node_uuid, target): + @wsme_pecan.wsexpose(None, types.uuid, wtypes.text, wtypes.text, + status_code=202) + def provision(self, node_uuid, target, configdrive=None): """Asynchronous trigger the provisioning of the node. This will set the target provision state of the node, and a @@ -317,6 +318,9 @@ class NodeStatesController(rest.RestController): :param node_uuid: UUID of a node. :param target: The desired provision state of the node. + :param configdrive: Optional. A gzipped and base64 encoded + configdrive. Only valid when setting provision state + to "active". :raises: ClientSideError (HTTP 409) if the node is already being provisioned. :raises: ClientSideError (HTTP 400) if the node is already in @@ -346,14 +350,22 @@ class NodeStatesController(rest.RestController): % rpc_node.uuid) raise wsme.exc.ClientSideError(msg, status_code=409) # Conflict + if configdrive and target != ir_states.ACTIVE: + msg = (_('Adding a config drive is only supported when setting ' + 'provision state to %s') % ir_states.ACTIVE) + raise wsme.exc.ClientSideError(msg, status_code=400) + # Note that there is a race condition. The node state(s) could change # by the time the RPC call is made and the TaskManager manager gets a # lock. - - if target in (ir_states.ACTIVE, ir_states.REBUILD): - rebuild = (target == ir_states.REBUILD) - pecan.request.rpcapi.do_node_deploy( - pecan.request.context, node_uuid, rebuild, topic) + if target == ir_states.ACTIVE: + pecan.request.rpcapi.do_node_deploy(pecan.request.context, + node_uuid, False, + configdrive, topic) + elif target == ir_states.REBUILD: + pecan.request.rpcapi.do_node_deploy(pecan.request.context, + node_uuid, True, + None, topic) elif target == ir_states.DELETED: pecan.request.rpcapi.do_node_tear_down( pecan.request.context, node_uuid, topic) diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 9fabff4b20..6ca5bad7d2 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -43,6 +43,7 @@ a change, etc. import collections import datetime +import tempfile import threading import eventlet @@ -65,6 +66,7 @@ from ironic.common.i18n import _LW from ironic.common import keystone from ironic.common import rpc from ironic.common import states +from ironic.common import swift from ironic.common import utils as ironic_utils from ironic.conductor import task_manager from ironic.conductor import utils @@ -152,6 +154,12 @@ conductor_opts = [ 'conductor will check for nodes that it should ' '"take over". Set it to a negative value to disable ' 'the check entirely.'), + cfg.BoolOpt('configdrive_use_swift', + default=False, + help='Whether upload the config drive to Swift.'), + cfg.StrOpt('configdrive_swift_container', + default='ironic_configdrive_container', + help='The Swift config drive container to store data.'), ] CONF = cfg.CONF @@ -162,7 +170,7 @@ class ConductorManager(periodic_task.PeriodicTasks): """Ironic Conductor manager main class.""" # NOTE(rloo): This must be in sync with rpcapi.ConductorAPI's. - RPC_API_VERSION = '1.21' + RPC_API_VERSION = '1.22' target = messaging.Target(version=RPC_API_VERSION) @@ -593,7 +601,8 @@ class ConductorManager(periodic_task.PeriodicTasks): exception.InstanceDeployFailure, exception.InvalidParameterValue, exception.MissingParameterValue) - def do_node_deploy(self, context, node_id, rebuild=False): + def do_node_deploy(self, context, node_id, rebuild=False, + configdrive=None): """RPC method to initiate deployment to a node. Initiate the deployment of a node. Validations are done @@ -606,6 +615,7 @@ class ConductorManager(periodic_task.PeriodicTasks): recreate the instance on the same node, overwriting all disk. The ephemeral partition, if it exists, can optionally be preserved. + :param configdrive: Optional. A gzipped and base64 encoded configdrive. :raises: InstanceDeployFailure :raises: NodeInMaintenance if the node is in maintenance mode. :raises: NoFreeConductorWorker when there is no free worker to start @@ -649,7 +659,8 @@ class ConductorManager(periodic_task.PeriodicTasks): task.process_event(event, callback=self._spawn_worker, call_args=(do_node_deploy, task, - self.conductor.id), + self.conductor.id, + configdrive), err_handler=provisioning_error_handler) except exception.InvalidState: raise exception.InstanceDeployFailure(_( @@ -1345,16 +1356,70 @@ def provisioning_error_handler(e, node, provision_state, 'tgt_prov_state': target_provision_state}) -def do_node_deploy(task, conductor_id): +def _get_configdrive_obj_name(node): + """Generate the object name for the config drive.""" + return 'configdrive-%s' % node.uuid + + +def _store_configdrive(node, configdrive): + """Handle the storage of the config drive. + + Whether update the Node's instance_info with the config driver + directly or upload it to Swift first and update the Node with an + temp URL pointing to the Swift object. + + :param node: an Ironic node object. + :param configdrive: A gzipped and base64 encoded configdrive. + :raises: SwiftOperationError if an error occur when uploading the + config drive to Swift. + + """ + if CONF.conductor.configdrive_use_swift: + # NOTE(lucasagomes): No reason to use a different timeout than + # the one used for deploying the node + timeout = CONF.conductor.deploy_callback_timeout + container = CONF.conductor.configdrive_swift_container + object_name = _get_configdrive_obj_name(node) + + object_headers = {'X-Delete-After': timeout} + + with tempfile.NamedTemporaryFile() as fileobj: + fileobj.write(configdrive) + fileobj.flush() + + swift_api = swift.SwiftAPI() + swift_api.create_object(container, object_name, fileobj.name, + object_headers=object_headers) + configdrive = swift_api.get_temp_url(container, object_name, + timeout) + + i_info = node.instance_info + i_info['configdrive'] = configdrive + node.instance_info = i_info + + +def do_node_deploy(task, conductor_id, configdrive=None): """Prepare the environment and deploy a node.""" node = task.node + + def handle_failure(e, task, logmsg, errmsg): + # NOTE(deva): there is no need to clear conductor_affinity + task.process_event('fail') + args = {'node': task.node.uuid, 'err': e} + LOG.warning(logmsg, args) + node.last_error = errmsg % e + try: - def handle_failure(e, task, logmsg, errmsg): - # NOTE(deva): there is no need to clear conductor_affinity - task.process_event('fail') - args = {'node': task.node.uuid, 'err': e} - LOG.warning(logmsg, args) - node.last_error = errmsg % e + try: + if configdrive: + _store_configdrive(node, configdrive) + except exception.SwiftOperationError as e: + with excutils.save_and_reraise_exception(): + handle_failure(e, task, + _LW('Error while uploading the configdrive for ' + '%(node)s to Swift'), + _('Failed to upload the configdrive to Swift. ' + 'Error %s')) try: task.driver.deploy.prepare(task) diff --git a/ironic/conductor/rpcapi.py b/ironic/conductor/rpcapi.py index 68f7c469f6..487335cf03 100644 --- a/ironic/conductor/rpcapi.py +++ b/ironic/conductor/rpcapi.py @@ -64,11 +64,12 @@ class ConductorAPI(object): | driver_vendor_passthru | 1.21 - Added get_node_vendor_passthru_methods and | get_driver_vendor_passthru_methods + | 1.22 - Added configdrive parameter to do_node_deploy. """ # NOTE(rloo): This must be in sync with manager.ConductorManager's. - RPC_API_VERSION = '1.21' + RPC_API_VERSION = '1.22' def __init__(self, topic=None): super(ConductorAPI, self).__init__() @@ -260,12 +261,14 @@ class ConductorAPI(object): return cctxt.call(context, 'get_driver_vendor_passthru_methods', driver_name=driver_name) - def do_node_deploy(self, context, node_id, rebuild, topic=None): + def do_node_deploy(self, context, node_id, rebuild, configdrive, + topic=None): """Signal to conductor service to perform a deployment. :param context: request context. :param node_id: node id or uuid. :param rebuild: True if this is a rebuild request. + :param configdrive: Optional. A gzipped and base64 encoded configdrive. :param topic: RPC topic. Defaults to self.topic. :raises: InstanceDeployFailure :raises: InvalidParameterValue if validation fails @@ -277,9 +280,9 @@ class ConductorAPI(object): undeployed state before this method is called. """ - cctxt = self.client.prepare(topic=topic or self.topic, version='1.15') + cctxt = self.client.prepare(topic=topic or self.topic, version='1.22') return cctxt.call(context, 'do_node_deploy', node_id=node_id, - rebuild=rebuild) + rebuild=rebuild, configdrive=configdrive) def do_node_tear_down(self, context, node_id, topic=None): """Signal to conductor service to tear down a deployment. diff --git a/ironic/tests/api/v1/test_nodes.py b/ironic/tests/api/v1/test_nodes.py index e2848a0224..2bcdd1e8f5 100644 --- a/ironic/tests/api/v1/test_nodes.py +++ b/ironic/tests/api/v1/test_nodes.py @@ -1119,13 +1119,32 @@ class TestPut(api_base.FunctionalTest): self.assertEqual(202, ret.status_code) self.assertEqual('', ret.body) self.mock_dnd.assert_called_once_with( - mock.ANY, self.node.uuid, False, 'test-topic') + mock.ANY, self.node.uuid, False, None, 'test-topic') # Check location header self.assertIsNotNone(ret.location) expected_location = '/v1/nodes/%s/states' % self.node.uuid self.assertEqual(urlparse.urlparse(ret.location).path, expected_location) + def test_provision_with_deploy_configdrive(self): + ret = self.put_json('/nodes/%s/states/provision' % self.node.uuid, + {'target': states.ACTIVE, 'configdrive': 'foo'}) + self.assertEqual(202, ret.status_code) + self.assertEqual('', ret.body) + self.mock_dnd.assert_called_once_with( + mock.ANY, self.node.uuid, False, 'foo', 'test-topic') + # Check location header + self.assertIsNotNone(ret.location) + expected_location = '/v1/nodes/%s/states' % self.node.uuid + self.assertEqual(urlparse.urlparse(ret.location).path, + expected_location) + + def test_provision_with_configdrive_not_active(self): + ret = self.put_json('/nodes/%s/states/provision' % self.node.uuid, + {'target': states.DELETED, 'configdrive': 'foo'}, + expect_errors=True) + self.assertEqual(400, ret.status_code) + def test_provision_with_tear_down(self): ret = self.put_json('/nodes/%s/states/provision' % self.node.uuid, {'target': states.DELETED}) @@ -1181,7 +1200,7 @@ class TestPut(api_base.FunctionalTest): self.assertEqual(202, ret.status_code) self.assertEqual('', ret.body) self.mock_dnd.assert_called_once_with( - mock.ANY, node.uuid, False, 'test-topic') + mock.ANY, node.uuid, False, None, 'test-topic') # Check location header self.assertIsNotNone(ret.location) expected_location = '/v1/nodes/%s/states' % node.uuid diff --git a/ironic/tests/conductor/test_manager.py b/ironic/tests/conductor/test_manager.py index bdb3204734..cc8ad0a545 100644 --- a/ironic/tests/conductor/test_manager.py +++ b/ironic/tests/conductor/test_manager.py @@ -31,6 +31,7 @@ from ironic.common import driver_factory from ironic.common import exception from ironic.common import keystone from ironic.common import states +from ironic.common import swift from ironic.common import utils as ironic_utils from ironic.conductor import manager from ironic.conductor import task_manager @@ -969,8 +970,9 @@ class DoNodeDeployTearDownTestCase(_ServiceSetUpMixin, self.assertIsNotNone(node.last_error) mock_deploy.assert_called_once_with(mock.ANY) + @mock.patch.object(manager, '_store_configdrive') @mock.patch('ironic.drivers.modules.fake.FakeDeploy.deploy') - def test__do_node_deploy_ok(self, mock_deploy): + def test__do_node_deploy_ok(self, mock_deploy, mock_store): self._start_service() # test when driver.deploy.deploy returns DEPLOYDONE mock_deploy.return_value = states.DEPLOYDONE @@ -985,6 +987,53 @@ class DoNodeDeployTearDownTestCase(_ServiceSetUpMixin, self.assertEqual(states.NOSTATE, node.target_provision_state) self.assertIsNone(node.last_error) mock_deploy.assert_called_once_with(mock.ANY) + # assert _store_configdrive wasn't invoked + self.assertFalse(mock_store.called) + + @mock.patch.object(manager, '_store_configdrive') + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.deploy') + def test__do_node_deploy_ok_configdrive(self, mock_deploy, mock_store): + self._start_service() + # test when driver.deploy.deploy returns DEPLOYDONE + mock_deploy.return_value = states.DEPLOYDONE + node = obj_utils.create_test_node(self.context, driver='fake', + provision_state=states.DEPLOYING, + target_provision_state=states.ACTIVE) + task = task_manager.TaskManager(self.context, node.uuid) + configdrive = 'foo' + + manager.do_node_deploy(task, self.service.conductor.id, + configdrive=configdrive) + node.refresh() + self.assertEqual(states.ACTIVE, node.provision_state) + self.assertEqual(states.NOSTATE, node.target_provision_state) + self.assertIsNone(node.last_error) + mock_deploy.assert_called_once_with(mock.ANY) + mock_store.assert_called_once_with(task.node, configdrive) + + @mock.patch.object(swift, 'SwiftAPI') + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.deploy') + def test__do_node_deploy_configdrive_swift_error(self, mock_deploy, + mock_swift): + CONF.set_override('configdrive_use_swift', True, group='conductor') + self._start_service() + # test when driver.deploy.deploy returns DEPLOYDONE + mock_deploy.return_value = states.DEPLOYDONE + node = obj_utils.create_test_node(self.context, driver='fake', + provision_state=states.DEPLOYING, + target_provision_state=states.ACTIVE) + task = task_manager.TaskManager(self.context, node.uuid) + + mock_swift.side_effect = exception.SwiftOperationError('error') + self.assertRaises(exception.SwiftOperationError, + manager.do_node_deploy, task, + self.service.conductor.id, + configdrive='fake config drive') + node.refresh() + self.assertEqual(states.DEPLOYFAIL, node.provision_state) + self.assertEqual(states.ACTIVE, node.target_provision_state) + self.assertIsNotNone(node.last_error) + self.assertFalse(mock_deploy.called) @mock.patch('ironic.drivers.modules.fake.FakeDeploy.deploy') def test__do_node_deploy_ok_2(self, mock_deploy): @@ -1022,7 +1071,8 @@ class DoNodeDeployTearDownTestCase(_ServiceSetUpMixin, self.assertIsNone(node.last_error) # Verify reservation has been cleared. self.assertIsNone(node.reservation) - mock_spawn.assert_called_once_with(mock.ANY, mock.ANY, mock.ANY) + mock_spawn.assert_called_once_with(mock.ANY, mock.ANY, + mock.ANY, None) @mock.patch('ironic.drivers.modules.fake.FakeDeploy.deploy') def test_do_node_deploy_rebuild_active_state(self, mock_deploy): @@ -2839,3 +2889,43 @@ class ManagerSyncLocalStateTestCase(_CommonMixIn, tests_db_base.DbTestCase): self.task.spawn_after.assert_called_once_with( self.service._spawn_worker, self.service._do_takeover, self.task) + + +@mock.patch.object(swift, 'SwiftAPI') +class StoreConfigDriveTestCase(tests_base.TestCase): + + def setUp(self): + super(StoreConfigDriveTestCase, self).setUp() + self.node = obj_utils.get_test_node(self.context, driver='fake', + instance_info=None) + + def test_store_configdrive(self, mock_swift): + manager._store_configdrive(self.node, 'foo') + expected_instance_info = {'configdrive': 'foo'} + self.assertEqual(expected_instance_info, self.node.instance_info) + self.assertFalse(mock_swift.called) + + def test_store_configdrive_swift(self, mock_swift): + container_name = 'foo_container' + timeout = 123 + expected_obj_name = 'configdrive-%s' % self.node.uuid + expected_obj_header = {'X-Delete-After': timeout} + expected_instance_info = {'configdrive': 'http://1.2.3.4'} + + # set configs and mocks + CONF.set_override('configdrive_use_swift', True, group='conductor') + CONF.set_override('configdrive_swift_container', container_name, + group='conductor') + CONF.set_override('deploy_callback_timeout', timeout, + group='conductor') + mock_swift.return_value.get_temp_url.return_value = 'http://1.2.3.4' + + manager._store_configdrive(self.node, 'foo') + + mock_swift.assert_called_once_with() + mock_swift.return_value.create_object.assert_called_once_with( + container_name, expected_obj_name, mock.ANY, + object_headers=expected_obj_header) + mock_swift.return_value.get_temp_url(container_name, + expected_obj_name, timeout) + self.assertEqual(expected_instance_info, self.node.instance_info) diff --git a/ironic/tests/conductor/test_rpcapi.py b/ironic/tests/conductor/test_rpcapi.py index c177ec70a7..d0fabe441b 100644 --- a/ironic/tests/conductor/test_rpcapi.py +++ b/ironic/tests/conductor/test_rpcapi.py @@ -202,9 +202,10 @@ class RPCAPITestCase(base.DbTestCase): def test_do_node_deploy(self): self._test_rpcapi('do_node_deploy', 'call', - version='1.15', + version='1.22', node_id=self.fake_node['uuid'], - rebuild=False) + rebuild=False, + configdrive=None) def test_do_node_tear_down(self): self._test_rpcapi('do_node_tear_down',