diff --git a/doc/source/admin/anaconda-deploy-interface.rst b/doc/source/admin/anaconda-deploy-interface.rst index 30848a1451..e2dad8febc 100644 --- a/doc/source/admin/anaconda-deploy-interface.rst +++ b/doc/source/admin/anaconda-deploy-interface.rst @@ -153,6 +153,93 @@ ironic node: openstack baremetal node set \ --instance_info ks_template=glance://uuid +.. warning:: + In the Ironic Project terminology, the word ``template`` often refers to + a file which is supplied to the deployment, which Ironic supplies + parameters to render a specific output. One critical example of this in + the Ironic workflow, specifically with this driver, is that the generated + ``agent token`` is conveyed to the booting ramdisk, facilitating it to call + back to Ironic and indicate the state. This token is randomly generated + for every deploy, and is required. Specifically this is leveraged in the + template's ``pre``, ``onerror``, and ``post`` steps. + For more infomation on Agent Token, please see :doc:`/admin/agent-token`. + +Standalone deployments +---------------------- + +While this deployment interface driver was developed around the use of other +OpenStack services, it is not explicitly required. For example HTTP(S) URLs +can be supplied by the API user to explictly set the expected baremetal node +``instance_info`` fields + +.. code-block:: shell + + baremetal node set \ + --instance_info image_source= \ + --instance_info kernel= \ + --instance_info ramdisk= \ + --instance_info stage2= + +When doing so, you may wish to also utilize a customized kickstart template, +which can also be a URL. Please reference the ironic community provided +template *ks.cfg.template* and use it as a basis of your own kickstart +as it accounts for the particular stages and appropriate callbacks to +Ironic. + +.. warning:: + The default template expects a ``instance_info\liveimg_url`` setting to + be provided by the user, which serves as a base operating system image. + In the context of the anaconda driver, it should be thought of almost + like "stage3". If you're using a custom template, it may not be required, + but proceed with caution. + See `pykickstart documentation `_ + for more information on liveimg file format, structure, and use. + +.. code-block:: shell + + baremetal node set \ + --instance_info ks_template= + +If you do choose to use a liveimg with a customized template, or if you wish +to use the stock template with a liveimg, you will need to provide parameter. + +.. code-block:: shell + + baremetal node set \ + --instance_info liveimg_url= + +.. warning:: + This is required if you do *not* utilize a customised template. As in use + Ironic's stock template. + +The pattern of deployment in this case is identical to a deployment case +where Ironic is integrated with OpenStack, however in this case Ironic +collects the files, and stages them appropriately. + +At this point, you should be able to request the baremetal node to deploy. + +Deployment Process +------------------ + +At a high level, the mechanics of the anaconda driver works in the following +flow, where we also note the stages and purpose of each part for informational +purposes. + +#. Network Boot Program (Such as iPXE) downloads the kernel, and initial + ramdisk. +#. Kernel launches, uncompresses initial ramdisk, and executes init inside + of the ramdisk. +#. The initial ramdisk boot scripts, such as Dracut, recognize the kernel + command line parameters Ironic supplied with the boot configuration, + and downloads the second stage artifacts, in this case called the + ``stage2`` image. This image contains Anaconda and base dependencies. +#. Anaconda downloads and parses the kickstart configuration which was + also supplied on the kernel command line, and executes the commands + as defined in the kickstart template. +#. The kickstart template, if specified in its contents, downloads a + ``liveimg`` which is used as the base operating system image to + start with. + Limitations ----------- diff --git a/ironic/common/exception.py b/ironic/common/exception.py index ca01f6794b..ddbce6f47f 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -835,3 +835,19 @@ class IncorrectConfiguration(IronicException): class NodeVerifyFailure(IronicException): _msg_fmt = _("Failed to verify node %(node)s: %(reason)s") + + +class ImageRefIsARedirect(IronicException): + _msg_fmt = _("Received a URL redirect when attempting to evaluate " + "image reference %(image_ref)s pointing to " + "%(redirect_url)s. This may, or may not be recoverable.") + redirect_url = None + + def __init__(self, image_ref=None, redirect_url=None, msg=None): + self.redirect_url = redirect_url + # Kwargs are expected by ironic_lib's IronicException to convert + # the message. + super(ImageRefIsARedirect, self).__init__( + message=msg, + image_ref=image_ref, + redirect_url=redirect_url) diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index e878f8e63d..5280ee6bc3 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -86,6 +86,9 @@ class HttpImageService(BaseImageService): shown in exception message. :raises: exception.ImageRefValidationFailed if HEAD request failed or returned response code not equal to 200. + :raises: exception.ImageRefIsARedirect if the supplied URL is a + redirect to a different URL. The caller may be able to handle + this. :returns: Response to HEAD request. """ output_url = 'secreturl' if secret else image_href @@ -97,8 +100,46 @@ class HttpImageService(BaseImageService): verify = CONF.webserver_verify_ca try: + # NOTE(TheJulia): Head requests do not work on things that are not + # files, but they can be responded with redirects or a 200 OK.... + # We don't want to permit endless redirects either, thus not + # request an override to the requests default to try and resolve + # redirects as otherwise we might end up with something like + # HTTPForbidden or a list of files. Both should be okay to at + # least know things are okay in a limited fashion. response = requests.head(image_href, verify=verify, timeout=CONF.webserver_connection_timeout) + + if response.status_code == http_client.MOVED_PERMANENTLY: + # NOTE(TheJulia): In the event we receive a redirect, we need + # to notify the caller. Before this we would just fail, + # but a url which is missing a trailing slash results in a + # redirect to a target path, and the caller *may* actually + # care about that. + redirect = requests.Session().get_redirect_target(response) + + # Extra guard because this is pointless if there is no + # location in the field. Requests also properly formats + # our string for us, or gives us None. + if redirect: + raise exception.ImageRefIsARedirect( + image_ref=image_href, + redirect_url=redirect) + + if (response.status_code == http_client.FORBIDDEN + and str(image_href).endswith('/')): + LOG.warning('Attempted to validate a URL %s, however we ' + 'received an HTTP Forbidden response and the ' + 'url ends with trailing slash (/), suggesting ' + 'non-image deploy may be in progress with ' + 'a webserver which is not permitting an index ' + 'to be generated. We will treat this as valid, ' + 'but return the response.', image_href) + return response + + # NOTE(TheJulia): Any file list reply will proceed past here just + # fine as they are conveyed as an HTTP 200 OK response with a + # server rendered HTML document payload. if response.status_code != http_client.OK: raise exception.ImageRefValidationFailed( image_href=output_url, diff --git a/ironic/common/images.py b/ironic/common/images.py index bffff88075..2b52b789b2 100644 --- a/ironic/common/images.py +++ b/ironic/common/images.py @@ -580,7 +580,8 @@ def is_whole_disk_image(ctx, instance_info): :param instance_info: a node's instance info dict :returns: True for whole disk images and False for partition images - and None on no image_source or Error. + and None on no image_source, the source being a path, or upon an + Error. """ image_source = instance_info.get('image_source') if not image_source: @@ -606,6 +607,11 @@ def is_whole_disk_image(ctx, instance_info): and not iproperties.get('ramdisk_id')) else: # Non glance image ref + if is_source_a_path(ctx, instance_info.get('image_source')): + # Nothing is returned if not valid or there was an error. + # A third possibility is it is not a disk image, which would + # still be None. + return if (not instance_info.get('kernel') and not instance_info.get('ramdisk')): is_whole_disk_image = True @@ -613,6 +619,73 @@ def is_whole_disk_image(ctx, instance_info): return is_whole_disk_image +def is_source_a_path(ctx, image_source): + """Determine if the image source is a path. + + This method determines if a supplied URL is a path. + + :param ctx: an admin/process context. + :param image_source: The supplied image source, expected to be a + URL, which can be used to attempt to determine + if the source is a path. + :returns: True if the image_source appears to be a path as opposed + to an image to be downloaded. If the image source is not + a path, False is returned. If any error is detected, + None is returned. + """ + if not image_source: + return + image_service = service.get_image_service(image_source, + context=ctx) + try: + res = image_service.validate_href(image_source) + if 'headers' in dir(res): + # response/result is from the HTTP check path. + headers = res.headers + else: + # We have no headers. + headers = {} + except exception.ImageRefIsARedirect as e: + # Our exception handling formats this for us in this + # case. \o/ + LOG.debug(str(e)) + # Servers redirect to a proper folder ending in a slash if + # not supplied originally. + if e.redirect_url and e.redirect_url.endswith('/'): + return True + except Exception: + # NOTE(TheJulia): I don't really like this pattern, *but* + # the wholedisk image support is similar. + return + # When issuing a head request, folders have no length + # A list can be generated by the server.. This is a solid + # hint. + if 'Content-Length' in headers: + LOG.debug('Evaluated %(url)s to determine if it is a URL to a path ' + 'or a file. A Content-Length header was returned ' + 'suggesting file.', + {'url': image_source}) + # NOTE(TheJulia): Files on a webserver have a length which is returned + # when headres are queried. + return False + if ('Content-Type' in headers + and str(headers['Content-Type']).startswith('text') + and 'Content-Length' not in headers): + LOG.debug('Evaluated %(url)s to determine if it is a URL to a path ' + 'or a file. A Content-Type header was returned with a text ' + 'content, which suggests a file list was returned.', + {'url': image_source}) + return True + # NOTE(TheJulia): Files should have been caught almost exclusively + # before with the Content-Length check. + if image_source.endswith('/'): + # If all else fails, looks like a URL, and the server didn't give + # us any hints. + return True + # We were unable to determine if this was a folder or a file. + return False + + def _extract_iso(extract_iso, extract_dir): # NOTE(rpittau): we could probably just extract the files we need # if we find them. Also we probably need to detect the correct iso diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 1b15f7856b..e1914221f0 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -1096,6 +1096,7 @@ class ConductorManager(base_manager.BaseConductorManager): node.del_driver_internal_info('instance') node.del_driver_internal_info('root_uuid_or_disk_id') node.del_driver_internal_info('is_whole_disk_image') + node.del_driver_internal_info('is_source_a_path') node.del_driver_internal_info('deploy_boot_mode') if node.driver_internal_info.get('automatic_lessee'): # Remove lessee, as it was automatically added diff --git a/ironic/conductor/utils.py b/ironic/conductor/utils.py index 115c4bee19..c107f076f3 100644 --- a/ironic/conductor/utils.py +++ b/ironic/conductor/utils.py @@ -1658,7 +1658,21 @@ def update_image_type(context, node): """ iwdi = images.is_whole_disk_image(context, node.instance_info) if iwdi is None: - return False + isap = images.is_source_a_path( + context, + node.instance_info.get('image_source') + ) + if isap is None: + return False + node.set_driver_internal_info('is_source_a_path', isap) + # TBD(TheJulia): should we need to set image_type back? + # rloo doesn't believe we should. I'm kind of on board with that + # idea since it is also user-settable, but laregely is just geared + # to take what is in glance. Line below should we wish to uncomment. + # node.set_instance_info('image_type', images.IMAGE_TYPE_DIRECTORY) + # An alternative is to explictly allow it to be configured by the + # caller/requester. + return True node.set_driver_internal_info('is_whole_disk_image', iwdi) # We need to gradually phase out is_whole_disk_image in favour of diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index d86cc27e43..e70fcd17b6 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -561,8 +561,10 @@ def validate_image_properties(task, deploy_info): :raises: MissingParameterValue if the image doesn't contain the mentioned properties. """ + node = task.node image_href = deploy_info.get('image_source') boot_iso = deploy_info.get('boot_iso') + isap = task.node.driver_internal_info.get('is_source_a_path') if image_href and boot_iso: raise exception.InvalidParameterValue(_( "An 'image_source' and 'boot_iso' parameter may not be " @@ -573,15 +575,22 @@ def validate_image_properties(task, deploy_info): boot_option = get_boot_option(task.node) if (boot_iso - or task.node.driver_internal_info.get('is_whole_disk_image') - or boot_option == 'local'): + or node.driver_internal_info.get('is_whole_disk_image') + or boot_option == 'local' + or isap): # No image properties are required in this case, but validate that the # image at least looks reasonable. try: + # This doesn't actually test *getting* the defined url or file + # but actually validates we can parse the data *to* connect. image_service.get_image_service(image_href, context=task.context) except exception.ImageRefValidationFailed as e: raise exception.InvalidParameterValue(err=e) - return + if not isap: + # If the source is not a path, but otherwise matches, we need to + # exit validation here. Deployments, such as ramdisk or anaconda + # need further parameter validation and this supplies it. + return if service_utils.is_glance_image(image_href): properties = ['kernel_id', 'ramdisk_id'] @@ -589,6 +598,7 @@ def validate_image_properties(task, deploy_info): properties.append('stage2_id') image_props = get_image_properties(task.context, image_href) else: + # We are likely netbooting in this case... properties = ['kernel', 'ramdisk'] image_props = {} @@ -829,7 +839,7 @@ def get_image_instance_info(node): _ERR_MSG_INVALID_DEPLOY = _("Invalid parameter %(param)s: %(reason)s") -def parse_instance_info(node): +def parse_instance_info(node, image_deploy=True): """Gets the instance specific Node deployment info. This method validates whether the 'instance_info' property of the @@ -837,6 +847,9 @@ def parse_instance_info(node): deploy images to the node. :param node: a single Node. + :param image_deploy: If the deployment interface is aware this + is an image based deployment, default + True. :returns: A dict with the instance_info values. :raises: MissingParameterValue, if any of the required parameters are missing. @@ -856,7 +869,12 @@ def parse_instance_info(node): i_info['image_source'])): i_info['kernel'] = info.get('kernel') i_info['ramdisk'] = info.get('ramdisk') - i_info['root_gb'] = info.get('root_gb') + if image_deploy: + # root_gb is expected with partition images. + i_info['root_gb'] = info.get('root_gb') + + # NOTE(TheJulia): Kernel/ramdisk may be optional and originated + # with pure workload network booting. error_msg = _("Some parameters were missing in node's instance_info") check_for_missing_params(i_info, error_msg) @@ -879,7 +897,8 @@ def parse_instance_info(node): err_msg_invalid = _("Cannot deploy whole disk image with " "swap or ephemeral size set") raise exception.InvalidParameterValue(err_msg_invalid) - else: + elif image_deploy: + # NOTE(TheJulia): This only applies to partition image deploys. _validate_layout_properties(node, info, i_info) i_info['configdrive'] = info.get('configdrive') @@ -1066,6 +1085,10 @@ def _validate_image_url(node, url, secret=False): it will not be shown in logs. """ try: + # NOTE(TheJulia): This method only validates that an exception + # is NOT raised. In other words, that the endpoint does not + # return a 200. If we're fed a folder list, this will still + # work, which is a good and bad thing at the same time. :/ image_service.HttpImageService().validate_href(url, secret) except exception.ImageRefValidationFailed as e: with excutils.save_and_reraise_exception(): @@ -1178,6 +1201,10 @@ def build_instance_info_for_deploy(task): instance_info = node.instance_info iwdi = node.driver_internal_info.get('is_whole_disk_image') image_source = instance_info['image_source'] + isap = node.driver_internal_info.get('is_source_a_path') + # If our url ends with a /, i.e. we have been supplied with a path, + # we can only deploy this in limited cases for drivers and tools + # which are aware of such. i.e. anaconda. image_download_source = get_image_download_source(node) boot_option = get_boot_option(task.node) @@ -1207,17 +1234,35 @@ def build_instance_info_for_deploy(task): instance_info['ramdisk'] = image_info['properties']['ramdisk_id'] elif (image_source.startswith('file://') or image_download_source == 'local'): + # NOTE(TheJulia): Intentionally only supporting file:/// as image + # based deploy source since we don't want to, nor should we be in + # in the business of copying large numbers of files as it is a + # huge performance impact. _cache_and_convert_image(task, instance_info) else: - _validate_image_url(node, image_source) - instance_info['image_url'] = image_source + try: + _validate_image_url(node, image_source) + # image_url is internal, and used by IPA and some boot templates. + # in most cases, it needs to come from image_source explicitly. + instance_info['image_url'] = image_source + except exception.ImageRefIsARedirect as e: + if e.redirect_url: + instance_info['image_url'] = e.redirect_url + else: + raise - if not iwdi: - instance_info['image_type'] = images.IMAGE_TYPE_PARTITION - i_info = parse_instance_info(node) - instance_info.update(i_info) + if not isap: + if not iwdi: + instance_info['image_type'] = images.IMAGE_TYPE_PARTITION + i_info = parse_instance_info(node) + instance_info.update(i_info) + else: + instance_info['image_type'] = images.IMAGE_TYPE_WHOLE_DISK else: - instance_info['image_type'] = images.IMAGE_TYPE_WHOLE_DISK + # Call central parsing so we retain things like config drives. + i_info = parse_instance_info(node, image_deploy=False) + instance_info.update(i_info) + return instance_info diff --git a/ironic/drivers/modules/ks.cfg.template b/ironic/drivers/modules/ks.cfg.template index 941d3c37d1..825ea38c8c 100644 --- a/ironic/drivers/modules/ks.cfg.template +++ b/ironic/drivers/modules/ks.cfg.template @@ -16,6 +16,9 @@ clearpart --all --initlabel autopart # Downloading and installing OS image using liveimg section is mandatory +# in a *default* ironic configuration. Users (infrastructure operators) +# may choose to customize this pattern, or use release specific kickstart +# configurations which may already point to a mirror. liveimg --url {{ ks_options.liveimg_url }} # Following %pre and %onerror sections are mandatory diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py index 30b6f4f37f..197518e1ad 100644 --- a/ironic/tests/unit/common/test_image_service.py +++ b/ironic/tests/unit/common/test_image_service.py @@ -194,6 +194,34 @@ class HttpImageServiceTestCase(base.TestCase): head_mock.assert_called_once_with(self.href, verify=False, timeout=60) + @mock.patch.object(requests, 'head', autospec=True) + def test_validate_href_path_forbidden(self, head_mock): + cfg.CONF.set_override('webserver_verify_ca', 'True') + + response = head_mock.return_value + response.status_code = http_client.FORBIDDEN + url = self.href + '/' + resp = self.service.validate_href(url) + head_mock.assert_called_once_with(url, verify=True, + timeout=60) + self.assertEqual(http_client.FORBIDDEN, resp.status_code) + + @mock.patch.object(requests, 'head', autospec=True) + def test_validate_href_path_redirected(self, head_mock): + cfg.CONF.set_override('webserver_verify_ca', 'True') + + response = head_mock.return_value + response.status_code = http_client.MOVED_PERMANENTLY + url = self.href + '/' + new_url = 'http://new-url' + response.headers = {'location': new_url} + exc = self.assertRaises(exception.ImageRefIsARedirect, + self.service.validate_href, + url) + self.assertEqual(new_url, exc.redirect_url) + head_mock.assert_called_once_with(url, verify=True, + timeout=60) + @mock.patch.object(requests, 'head', autospec=True) def _test_show(self, head_mock, mtime, mtime_date): head_mock.return_value.status_code = http_client.OK diff --git a/ironic/tests/unit/common/test_images.py b/ironic/tests/unit/common/test_images.py index 98cbedf275..dd782c687a 100644 --- a/ironic/tests/unit/common/test_images.py +++ b/ironic/tests/unit/common/test_images.py @@ -274,26 +274,86 @@ class IronicImagesTestCase(base.TestCase): def test_is_whole_disk_image_partition_non_glance(self, mock_igi, mock_gip): mock_igi.return_value = False - instance_info = {'image_source': 'partition_image', - 'kernel': 'kernel', - 'ramdisk': 'ramdisk'} + instance_info = { + 'image_source': 'fcf5a777-d9d2-4b86-b3da-bb0b61d5a291', + 'kernel': 'kernel', + 'ramdisk': 'ramdisk'} is_whole_disk_image = images.is_whole_disk_image('context', instance_info) self.assertFalse(is_whole_disk_image) self.assertFalse(mock_gip.called) mock_igi.assert_called_once_with(instance_info['image_source']) + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) @mock.patch.object(images, 'get_image_properties', autospec=True) @mock.patch.object(glance_utils, 'is_glance_image', autospec=True) def test_is_whole_disk_image_whole_disk_non_glance(self, mock_igi, - mock_gip): + mock_gip, + mock_validate): mock_igi.return_value = False - instance_info = {'image_source': 'whole_disk_image'} + instance_info = { + 'image_source': 'http://whole-disk-image'} is_whole_disk_image = images.is_whole_disk_image('context', instance_info) self.assertTrue(is_whole_disk_image) self.assertFalse(mock_gip.called) mock_igi.assert_called_once_with(instance_info['image_source']) + mock_validate.assert_called_once_with(mock.ANY, + 'http://whole-disk-image') + + def test_is_source_a_path_returns_none(self): + self.assertIsNone(images.is_source_a_path('context', {})) + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_simple(self, validate_mock): + mock_response = mock.Mock() + mock_response.headers = {} + validate_mock.return_value = mock_response + self.assertTrue(images.is_source_a_path('context', 'http://foo/bar/')) + validate_mock.assert_called_once_with(mock.ANY, 'http://foo/bar/') + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_content_length(self, validate_mock): + mock_response = mock.Mock() + mock_response.headers = {'Content-Length': 1} + validate_mock.return_value = mock_response + self.assertFalse(images.is_source_a_path('context', 'http://foo/bar/')) + validate_mock.assert_called_once_with(mock.ANY, 'http://foo/bar/') + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_content_type(self, validate_mock): + mock_response = mock.Mock() + mock_response.headers = {'Content-Type': 'text/html'} + validate_mock.return_value = mock_response + self.assertTrue(images.is_source_a_path('context', 'http://foo/bar')) + validate_mock.assert_called_once_with(mock.ANY, 'http://foo/bar') + + @mock.patch.object(images, 'LOG', autospec=True) + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_redirect(self, validate_mock, log_mock): + url = 'http://foo/bar' + redirect_url = url + '/' + validate_mock.side_effect = exception.ImageRefIsARedirect( + url, redirect_url) + self.assertTrue(images.is_source_a_path('context', url)) + log_mock.debug.assert_called_once_with('Received a URL redirect when ' + 'attempting to evaluate image ' + 'reference http://foo/bar ' + 'pointing to http://foo/bar/. ' + 'This may, or may not be ' + 'recoverable.') + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_is_source_a_path_other_error(self, validate_mock): + url = 'http://foo/bar' + validate_mock.side_effect = OSError + self.assertIsNone(images.is_source_a_path('context', url)) class FsImageTestCase(base.TestCase): diff --git a/ironic/tests/unit/conductor/test_deployments.py b/ironic/tests/unit/conductor/test_deployments.py index ed722c1076..4834f2da4a 100644 --- a/ironic/tests/unit/conductor/test_deployments.py +++ b/ironic/tests/unit/conductor/test_deployments.py @@ -385,6 +385,104 @@ class DoNodeDeployTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def test_start_deploy_records_lessee(self): self._test_start_deploy(automatic_lessee=True) + @mock.patch.object(images, 'is_source_a_path', autospec=True) + @mock.patch.object(task_manager.TaskManager, 'process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.validate', + autospec=True) + @mock.patch.object(conductor_steps, + 'validate_user_deploy_steps_and_templates', + autospec=True) + @mock.patch.object(conductor_utils, 'validate_instance_info_traits', + autospec=True) + @mock.patch.object(images, 'is_whole_disk_image', autospec=True) + def test_start_deploy_source_path( + self, mock_iwdi, mock_validate_traits, + mock_validate_deploy_user_steps_and_templates, + mock_deploy_validate, mock_power_validate, + mock_process_event, mock_is_source_a_path): + self._start_service() + mock_iwdi.return_value = None + mock_is_source_a_path.return_value = True + deploy_steps = [{"interface": "bios", "step": "factory_reset", + "priority": 95}] + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.AVAILABLE, + target_provision_state=states.ACTIVE) + task = task_manager.TaskManager(self.context, node.uuid) + + deployments.start_deploy(task, self.service, configdrive=None, + event='deploy', deploy_steps=deploy_steps) + node.refresh() + mock_iwdi.assert_called_once_with(task.context, + task.node.instance_info) + mock_is_source_a_path.assert_called_once_with( + task.context, + task.node.instance_info.get('image_source') + ) + self.assertNotIn('is_whole_disk_iamge', node.driver_internal_info) + self.assertTrue(node.driver_internal_info['is_source_a_path']) + mock_power_validate.assert_called_once_with(task.driver.power, task) + mock_deploy_validate.assert_called_once_with(task.driver.deploy, task) + mock_validate_traits.assert_called_once_with(task.node) + mock_validate_deploy_user_steps_and_templates.assert_called_once_with( + task, deploy_steps, skip_missing=True) + mock_process_event.assert_called_with( + mock.ANY, 'deploy', call_args=( + deployments.do_node_deploy, task, 1, None, deploy_steps), + callback=mock.ANY, err_handler=mock.ANY) + + @mock.patch.object(images, 'is_source_a_path', autospec=True) + @mock.patch.object(task_manager.TaskManager, 'process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.validate', + autospec=True) + @mock.patch.object(conductor_steps, + 'validate_user_deploy_steps_and_templates', + autospec=True) + @mock.patch.object(conductor_utils, 'validate_instance_info_traits', + autospec=True) + @mock.patch.object(images, 'is_whole_disk_image', autospec=True) + def test_start_deploy_source_path_none( + self, mock_iwdi, mock_validate_traits, + mock_validate_deploy_user_steps_and_templates, + mock_deploy_validate, mock_power_validate, + mock_process_event, mock_is_source_a_path): + self._start_service() + mock_iwdi.return_value = None + mock_is_source_a_path.return_value = None + deploy_steps = [{"interface": "bios", "step": "factory_reset", + "priority": 95}] + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.AVAILABLE, + target_provision_state=states.ACTIVE) + task = task_manager.TaskManager(self.context, node.uuid) + + deployments.start_deploy(task, self.service, configdrive=None, + event='deploy', deploy_steps=deploy_steps) + node.refresh() + mock_iwdi.assert_called_once_with(task.context, + task.node.instance_info) + mock_is_source_a_path.assert_called_once_with( + task.context, + task.node.instance_info.get('image_source') + ) + self.assertNotIn('is_whole_disk_iamge', node.driver_internal_info) + self.assertNotIn('is_source_a_path', node.driver_internal_info) + mock_power_validate.assert_called_once_with(task.driver.power, task) + mock_deploy_validate.assert_called_once_with(task.driver.deploy, task) + mock_validate_traits.assert_called_once_with(task.node) + mock_validate_deploy_user_steps_and_templates.assert_called_once_with( + task, deploy_steps, skip_missing=True) + mock_process_event.assert_called_with( + mock.ANY, 'deploy', call_args=( + deployments.do_node_deploy, task, 1, None, deploy_steps), + callback=mock.ANY, err_handler=mock.ANY) + @mgr_utils.mock_record_keepalive class DoNextDeployStepTestCase(mgr_utils.ServiceSetUpMixin, diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 2c8ddcbd49..ddd9cbae47 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2410,7 +2410,8 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): provision_state=states.DELETING, target_provision_state=states.AVAILABLE, instance_info={'foo': 'bar'}, - driver_internal_info={'is_whole_disk_image': False}) + driver_internal_info={'is_whole_disk_image': False, + 'is_source_a_path': None}) task = task_manager.TaskManager(self.context, node.uuid) self._start_service() @@ -2463,7 +2464,8 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def _test__do_node_tear_down_ok(self, mock_tear_down, mock_clean, mock_unbind, mock_console, enabled_console=False, - with_allocation=False): + with_allocation=False, + source_a_path=False): # test when driver.deploy.tear_down succeeds node = obj_utils.create_test_node( self.context, driver='fake-hardware', @@ -2488,6 +2490,10 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): alloc.node_id = node.id alloc.save() node.refresh() + if source_a_path: + d_ii = node.driver_internal_info + d_ii['is_source_a_path'] = True + node.driver_internal_info = d_ii task = task_manager.TaskManager(self.context, node.uuid) self._start_service() @@ -2507,6 +2513,7 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): self.assertNotIn('root_uuid_or_disk_id', node.driver_internal_info) self.assertNotIn('is_whole_disk_image', node.driver_internal_info) self.assertNotIn('automatic_lessee', node.driver_internal_info) + self.assertNotIn('is_source_a_path', node.driver_internal_info) mock_tear_down.assert_called_once_with(task.driver.deploy, task) mock_clean.assert_called_once_with(task) self.assertEqual({}, port.internal_info) @@ -2529,6 +2536,9 @@ class DoNodeTearDownTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def test__do_node_tear_down_with_allocation(self): self._test__do_node_tear_down_ok(with_allocation=True) + def test__do_node_tear_down_with_source_path(self): + self._test__do_node_tear_down_ok(source_a_path=True) + @mock.patch('ironic.drivers.modules.fake.FakeRescue.clean_up', autospec=True) @mock.patch('ironic.conductor.cleaning.do_node_clean', autospec=True) @@ -7333,7 +7343,7 @@ class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): provision_state=states.ADOPTING, instance_info={ 'capabilities': {'boot_option': 'netboot'}, - 'image_source': 'image', + 'image_source': 'http://127.0.0.1/image', }) task = task_manager.TaskManager(self.context, node.uuid) diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index e85ab631d6..230eec8f16 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -1487,6 +1487,24 @@ class ValidateImagePropertiesTestCase(db_base.DbTestCase): utils.validate_image_properties(self.task, inst_info) image_service_show_mock.assert_not_called() + @mock.patch.object(utils, 'get_boot_option', autospec=True, + return_value='kickstart') + @mock.patch.object(image_service.HttpImageService, 'show', autospec=True) + def test_validate_image_properties_anaconda_deploy_image_source( + self, image_service_show_mock, boot_options_mock): + d_ii = self.node.driver_internal_info + d_ii.pop('is_whole_disk_image') + d_ii['is_source_a_path'] = True + instance_info = { + 'kernel': 'file://kernel', + 'ramdisk': 'file://initrd', + 'image_source': 'http://foo/bar/' + } + self.node.instance_info = instance_info + inst_info = utils.get_image_instance_info(self.node) + utils.validate_image_properties(self.task, inst_info) + image_service_show_mock.assert_not_called() + class ValidateParametersTestCase(db_base.DbTestCase): @@ -1869,6 +1887,25 @@ class InstanceInfoTestCase(db_base.DbTestCase): self.assertNotIn('ephemeral_mb', instance_info) self.assertNotIn('swap_mb', instance_info) + def test_parse_instance_info_non_image_deploy(self): + driver_internal_info = dict(DRV_INTERNAL_INFO_DICT) + driver_internal_info['is_whole_disk_image'] = None + instance_info = {'image_source': 'http://cat/meow/', + 'kernel': 'corgi', + 'ramdisk': 'mushroom'} + node = obj_utils.create_test_node( + self.context, instance_info=instance_info, + driver_internal_info=driver_internal_info, + deploy_interface='anaconda' + ) + instance_info = utils.parse_instance_info(node, image_deploy=False) + self.assertIsNotNone(instance_info['image_source']) + self.assertNotIn('root_mb', instance_info) + self.assertNotIn('ephemeral_mb', instance_info) + self.assertNotIn('swap_mb', instance_info) + self.assertIn('kernel', instance_info) + self.assertIn('ramdisk', instance_info) + class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): def setUp(self): @@ -2133,6 +2170,59 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): self.assertRaises(exception.ImageRefValidationFailed, utils.build_instance_info_for_deploy, task) + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_build_instance_info_for_deploy_source_is_a_path( + self, validate_href_mock): + i_info = self.node.instance_info + driver_internal_info = self.node.driver_internal_info + i_info['image_source'] = 'http://image-url/folder/' + driver_internal_info.pop('is_whole_disk_image', None) + driver_internal_info['is_source_a_path'] = True + self.node.instance_info = i_info + self.node.driver_internal_info = driver_internal_info + self.node.save() + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + + info = utils.build_instance_info_for_deploy(task) + + self.assertEqual(self.node.instance_info['image_source'], + info['image_url']) + self.assertNotIn('root_gb', info) + validate_href_mock.assert_called_once_with( + mock.ANY, 'http://image-url/folder/', False) + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_build_instance_info_for_deploy_source_redirect( + self, validate_href_mock): + i_info = self.node.instance_info + driver_internal_info = self.node.driver_internal_info + url = 'http://image-url/folder' + r_url = url + '/' + i_info['image_source'] = 'http://image-url/folder' + driver_internal_info.pop('is_whole_disk_image', None) + driver_internal_info['is_source_a_path'] = True + self.node.instance_info = i_info + self.node.driver_internal_info = driver_internal_info + self.node.save() + validate_href_mock.side_effect = exception.ImageRefIsARedirect( + image_ref=url, + redirect_url=r_url) + + with task_manager.acquire( + self.context, self.node.uuid, shared=False) as task: + + info = utils.build_instance_info_for_deploy(task) + + self.assertNotEqual(self.node.instance_info['image_source'], + info['image_url']) + self.assertEqual(r_url, info['image_url']) + validate_href_mock.assert_called_once_with( + mock.ANY, 'http://image-url/folder', False) + class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase): def setUp(self): diff --git a/releasenotes/notes/anaconda-based-deploy-option-sanity-b98fa138747c16d2.yaml b/releasenotes/notes/anaconda-based-deploy-option-sanity-b98fa138747c16d2.yaml new file mode 100644 index 0000000000..455d553eec --- /dev/null +++ b/releasenotes/notes/anaconda-based-deploy-option-sanity-b98fa138747c16d2.yaml @@ -0,0 +1,21 @@ +--- +fixes: + - | + Fixes an issue where ``root_gb`` became a required field when using the + ``anaconda`` deployment interface, with a recent bug fix as the code path + largely expected all deployment operations to utilize images, which is + not the case. The case handling for non-image based deployments is now + explicitly in internal parameter validation code. + - | + Fixes handling of ``image_source`` parameters where internal validations + would not gracefully handle received redirects and treat it as a failure. + We now no longer explicitly fail when a redirect is received. + - | + Fixes an issue where an ``image_source`` could not be set to a mirror URL + to facilitate deployments using a mirror with the ``anaconda`` deployment + interface. Ironic still presently has an explicit requirement on a + ``stage2`` parameter to be explicitly defined. +other: + - | + Adds documentation of standalone deployment use case with the ``anaconda`` + deployment interface.