Make anaconda non-image deploys sane

Ironic has a lot of logic built up around use of images for filesystems,
however several recent additions, such as the ``ramdisk`` and ``anaconda``
deployment interfaces have started to break this mold.

In working with some operators attempting to utilzie the anaconda
deployment interface outside the context of full OpenStack, we discovered
some issues which needed to be make simpler to help remove the need to
route around data validation checks for things that are not required.

Standalong users also have the ability to point to a URL with anaconda,
where as Operators using OpenStack can only do so with customized kickstart
files. While this is okay, the disparity in configuraiton checking
was also creating additional issues.

In this, we discovered we were not really graceful with redirects,
so we're now a little more graceful with them.

Story: 2009939
Story: 2009940
Task: 44834
Task: 44833
Change-Id: I8b0a50751014c6093faa26094d9f99e173dcdd38

doc/source/admin/anaconda-deploy-interface.rst

@@ -153,6 +153,93 @@ ironic node:
         openstack baremetal node set <node> \
             --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 <node> \
+           --instance_info image_source=<Mirror URL> \
+           --instance_info kernel=<Kernel URL> \
+           --instance_info ramdisk=<Initial Ramdisk URL> \
+           --instance_info stage2=<Installer Stage2 Ramdisk URL>
+
+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 <https://pykickstart.readthedocs.io/en/latest/kickstart-docs.html#liveimg>`_
+   for more information on liveimg file format, structure, and use.
+
+.. code-block:: shell
+
+        baremetal node set <node> \
+            --instance_info ks_template=<URL>
+
+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 <node> \
+            --instance_info liveimg_url=<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
 -----------
 

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)

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,

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

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

ironic/conductor/utils.py

@@ -1661,7 +1661,21 @@ def update_image_type(context, node):
     """
     iwdi = images.is_whole_disk_image(context, node.instance_info)
     if iwdi is None:
+        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

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,14 +575,21 @@ 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 node.driver_internal_info.get('is_whole_disk_image')
-            or boot_option == 'local'):
+            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)
+        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):
@@ -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,8 +869,13 @@ def parse_instance_info(node):
                     i_info['image_source'])):
             i_info['kernel'] = info.get('kernel')
             i_info['ramdisk'] = info.get('ramdisk')
+        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:
+        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 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:
+        # 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
 
 

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

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

ironic/tests/unit/common/test_images.py

@@ -274,7 +274,8 @@ 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',
+        instance_info = {
+            'image_source': 'fcf5a777-d9d2-4b86-b3da-bb0b61d5a291',
             'kernel': 'kernel',
             'ramdisk': 'ramdisk'}
         is_whole_disk_image = images.is_whole_disk_image('context',
@@ -283,17 +284,76 @@ class IronicImagesTestCase(base.TestCase):
         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):

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,

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)
 

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

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.