Support file:/// images for the direct deploy

Implemented via the same mechanism as for image_download_source=http.
Forcing raw format (and thus streaming) is supported as well.

Since we always re-calculate checksums for such images, the requirement
on providing them via the API is lifted.

Change-Id: Ife385c3b363c28559c90e5a54e9c6f6807d207ff
Story: #2008075
Task: #40764

doc/source/install/standalone.rst

@@ -88,22 +88,21 @@ Preparing images
 If you don't use Image service, it's possible to provide images to Bare Metal
 service via a URL.
 
-.. note::
-   At the moment, only two types of URLs are acceptable instead of Image
-   service UUIDs: HTTP(S) URLs (for example, "http://my.server.net/images/img")
-   and file URLs (file:///images/img).
+At the moment, only two types of URLs are acceptable instead of Image
+service UUIDs: HTTP(S) URLs (for example, "http://my.server.net/images/img")
+and file URLs (file:///images/img).
 
 There are however some limitations for different hardware interfaces:
 
-* If you're using :ref:`direct-deploy`, you have to provide the Bare Metal
-  service with the MD5 checksum of your instance image. To compute it, you can
-  use the following command::
+* If you're using :ref:`direct-deploy` with HTTP(s) URLs, you have to provide
+  the Bare Metal service with the MD5 checksum of your instance image.
+  To compute it, you can use the following command::
 
    md5sum image.qcow2
    ed82def8730f394fb85aef8a208635f6  image.qcow2
 
-* :ref:`direct-deploy` requires the instance image be accessible through a
-  HTTP(s) URL.
+* :ref:`direct-deploy` started supporting ``file://`` images in the Victoria
+  release cycle, before that only HTTP(s) had been supported.
 
 .. note::
    The Bare Metal service tracks content changes for non-Glance images by

ironic/common/images.py

@@ -414,7 +414,7 @@ def fetch(context, image_href, path, force_raw=False):
         image_to_raw(image_href, path, "%s.part" % path)
 
 
-def force_raw_get_source_format(image_href, path):
+def get_source_format(image_href, path):
     data = disk_utils.qemu_img_info(path)
 
     fmt = data.file_format
@@ -435,7 +435,7 @@ def force_raw_get_source_format(image_href, path):
 
 def force_raw_will_convert(image_href, path_tmp):
     with fileutils.remove_path_on_error(path_tmp):
-        fmt = force_raw_get_source_format(image_href, path_tmp)
+        fmt = get_source_format(image_href, path_tmp)
     if fmt != "raw":
         return True
     return False
@@ -443,7 +443,7 @@ def force_raw_will_convert(image_href, path_tmp):
 
 def image_to_raw(image_href, path, path_tmp):
     with fileutils.remove_path_on_error(path_tmp):
-        fmt = force_raw_get_source_format(image_href, path_tmp)
+        fmt = get_source_format(image_href, path_tmp)
 
         if fmt != "raw":
             staged = "%s.converted" % path

ironic/drivers/modules/agent.py

@@ -168,8 +168,12 @@ def validate_http_provisioning_configuration(node):
     :raises: MissingParameterValue if required option(s) is not set.
     """
     image_source = node.instance_info.get('image_source')
-    if (not service_utils.is_glance_image(image_source)
-            or CONF.agent.image_download_source != 'http'):
+    # NOTE(dtantsur): local HTTP configuration is required in two cases:
+    # 1. Glance images with image_download_source == http
+    # 2. File images (since we need to serve them to IPA)
+    if (not image_source.startswith('file://')
+            and (not service_utils.is_glance_image(image_source)
+                 or CONF.agent.image_download_source == 'swift')):
         return
 
     params = {
@@ -379,8 +383,7 @@ class AgentDeployMixin(agent_base.AgentDeployMixin):
             manager_utils.node_set_boot_device(task, 'disk', persistent=True)
 
         # Remove symbolic link when deploy is done.
-        if CONF.agent.image_download_source == 'http':
-            deploy_utils.remove_http_instance_symlink(task.node.uuid)
+        deploy_utils.remove_http_instance_symlink(task.node.uuid)
 
 
 class AgentDeploy(AgentDeployMixin, agent_base.AgentBaseMixin,
@@ -442,7 +445,10 @@ class AgentDeploy(AgentDeployMixin, agent_base.AgentBaseMixin,
 
         deploy_utils.check_for_missing_params(params, error_msg)
 
-        if not service_utils.is_glance_image(image_source):
+        # NOTE(dtantsur): glance images contain a checksum; for file images we
+        # will recalculate the checksum anyway.
+        if (not service_utils.is_glance_image(image_source)
+                and not image_source.startswith('file://')):
 
             def _raise_missing_checksum_exception(node):
                 raise exception.MissingParameterValue(_(
@@ -629,8 +635,7 @@ class AgentDeploy(AgentDeployMixin, agent_base.AgentBaseMixin,
         :param task: a TaskManager instance.
         """
         super(AgentDeploy, self).clean_up(task)
-        if CONF.agent.image_download_source == 'http':
-            deploy_utils.destroy_http_instance_images(task.node)
+        deploy_utils.destroy_http_instance_images(task.node)
 
 
 class AgentRAID(base.RAIDInterface):

ironic/drivers/modules/deploy_utils.py

@@ -30,6 +30,7 @@ from ironic.common import faults
 from ironic.common.glance_service import service_utils
 from ironic.common.i18n import _
 from ironic.common import image_service
+from ironic.common import images
 from ironic.common import keystone
 from ironic.common import states
 from ironic.common import utils
@@ -966,6 +967,85 @@ def destroy_http_instance_images(node):
     destroy_images(node.uuid)
 
 
+def _validate_image_url(node, url, secret=False):
+    """Validates image URL through the HEAD request.
+
+    :param url: URL to be validated
+    :param secret: if URL is secret (e.g. swift temp url),
+        it will not be shown in logs.
+    """
+    try:
+        image_service.HttpImageService().validate_href(url, secret)
+    except exception.ImageRefValidationFailed as e:
+        with excutils.save_and_reraise_exception():
+            LOG.error("The specified URL is not a valid HTTP(S) URL or is "
+                      "not reachable for node %(node)s. Error: %(msg)s",
+                      {'node': node.uuid, 'msg': e})
+
+
+def _cache_and_convert_image(task, instance_info, image_info=None):
+    """Cache an image locally and covert it to RAW if needed."""
+    # Ironic cache and serve images from httpboot server
+    force_raw = direct_deploy_should_convert_raw_image(task.node)
+    _, image_path = cache_instance_image(task.context, task.node,
+                                         force_raw=force_raw)
+    if force_raw or image_info is None:
+        if force_raw:
+            instance_info['image_disk_format'] = 'raw'
+        else:
+            LOG.debug('Detecting image format for the locally cached image '
+                      '%(image)s for node %(node)s',
+                      {'image': image_path, 'node': task.node.uuid})
+            instance_info['image_disk_format'] = \
+                images.get_source_format(instance_info['image_source'],
+                                         image_path)
+
+        # Standard behavior is for image_checksum to be MD5,
+        # so if the hash algorithm is None, then we will use
+        # sha256.
+        if image_info is None:
+            os_hash_algo = instance_info.get('image_os_hash_algo')
+        else:
+            os_hash_algo = image_info.get('os_hash_algo')
+
+        if not os_hash_algo or os_hash_algo == 'md5':
+            LOG.debug("Checksum algorithm for image %(image)s for node "
+                      "%(node)s is set to '%(algo)s', changing to 'sha256'",
+                      {'algo': os_hash_algo, 'node': task.node.uuid,
+                       'image': image_path})
+            os_hash_algo = 'sha256'
+
+        LOG.debug('Recalculating checksum for image %(image)s for node '
+                  '%(node)s due to image conversion',
+                  {'image': image_path, 'node': task.node.uuid})
+        instance_info['image_checksum'] = None
+        hash_value = compute_image_checksum(image_path, os_hash_algo)
+        instance_info['image_os_hash_algo'] = os_hash_algo
+        instance_info['image_os_hash_value'] = hash_value
+    else:
+        instance_info['image_checksum'] = image_info['checksum']
+        instance_info['image_disk_format'] = image_info['disk_format']
+        instance_info['image_os_hash_algo'] = image_info[
+            'os_hash_algo']
+        instance_info['image_os_hash_value'] = image_info[
+            'os_hash_value']
+
+    # Create symlink and update image url
+    symlink_dir = _get_http_image_symlink_dir_path()
+    fileutils.ensure_tree(symlink_dir)
+    symlink_path = _get_http_image_symlink_file_path(task.node.uuid)
+    utils.create_link_without_raise(image_path, symlink_path)
+
+    base_url = CONF.deploy.http_url
+    if base_url.endswith('/'):
+        base_url = base_url[:-1]
+    http_image_url = '/'.join(
+        [base_url, CONF.deploy.http_image_subdir,
+         task.node.uuid])
+    _validate_image_url(task.node, http_image_url, secret=False)
+    instance_info['image_url'] = http_image_url
+
+
 @METRICS.timer('build_instance_info_for_deploy')
 def build_instance_info_for_deploy(task):
     """Build instance_info necessary for deploying to a node.
@@ -976,23 +1056,6 @@ def build_instance_info_for_deploy(task):
     :raises: exception.ImageRefValidationFailed if image_source is not
         Glance href and is not HTTP(S) URL.
     """
-    def validate_image_url(url, secret=False):
-        """Validates image URL through the HEAD request.
-
-        :param url: URL to be validated
-        :param secret: if URL is secret (e.g. swift temp url),
-            it will not be shown in logs.
-        """
-        try:
-            image_service.HttpImageService().validate_href(url, secret)
-        except exception.ImageRefValidationFailed as e:
-            with excutils.save_and_reraise_exception():
-                LOG.error("Agent deploy supports only HTTP(S) URLs as "
-                          "instance_info['image_source'] or swift "
-                          "temporary URL. Either the specified URL is not "
-                          "a valid HTTP(S) URL or is not reachable "
-                          "for node %(node)s. Error: %(msg)s",
-                          {'node': node.uuid, 'msg': e})
     node = task.node
     instance_info = node.instance_info
     iwdi = node.driver_internal_info.get('is_whole_disk_image')
@@ -1005,56 +1068,14 @@ def build_instance_info_for_deploy(task):
                   {'info': image_info, 'node': node.uuid})
         if CONF.agent.image_download_source == 'swift':
             swift_temp_url = glance.swift_temp_url(image_info)
-            validate_image_url(swift_temp_url, secret=True)
+            _validate_image_url(node, swift_temp_url, secret=True)
             instance_info['image_url'] = swift_temp_url
             instance_info['image_checksum'] = image_info['checksum']
             instance_info['image_disk_format'] = image_info['disk_format']
             instance_info['image_os_hash_algo'] = image_info['os_hash_algo']
             instance_info['image_os_hash_value'] = image_info['os_hash_value']
         else:
-            # Ironic cache and serve images from httpboot server
-            force_raw = direct_deploy_should_convert_raw_image(node)
-            _, image_path = cache_instance_image(task.context, node,
-                                                 force_raw=force_raw)
-            if force_raw:
-                instance_info['image_disk_format'] = 'raw'
-                # Standard behavior is for image_checksum to be MD5,
-                # so if the hash algorithm is None, then we will use
-                # sha256.
-                os_hash_algo = image_info.get('os_hash_algo')
-                if os_hash_algo == 'md5':
-                    LOG.debug('Checksum calculation for image %(image)s is '
-                              'set to \'%(algo)s\', changing to \'sha256\'',
-                              {'algo': os_hash_algo,
-                               'image': image_path})
-                    os_hash_algo = 'sha256'
-                LOG.debug('Recalculating checksum for image %(image)s due to '
-                          'image conversion.', {'image': image_path})
-                instance_info['image_checksum'] = None
-                hash_value = compute_image_checksum(image_path, os_hash_algo)
-                instance_info['image_os_hash_algo'] = os_hash_algo
-                instance_info['image_os_hash_value'] = hash_value
-            else:
-                instance_info['image_checksum'] = image_info['checksum']
-                instance_info['image_disk_format'] = image_info['disk_format']
-                instance_info['image_os_hash_algo'] = image_info[
-                    'os_hash_algo']
-                instance_info['image_os_hash_value'] = image_info[
-                    'os_hash_value']
-
-            # Create symlink and update image url
-            symlink_dir = _get_http_image_symlink_dir_path()
-            fileutils.ensure_tree(symlink_dir)
-            symlink_path = _get_http_image_symlink_file_path(node.uuid)
-            utils.create_link_without_raise(image_path, symlink_path)
-            base_url = CONF.deploy.http_url
-            if base_url.endswith('/'):
-                base_url = base_url[:-1]
-            http_image_url = '/'.join(
-                [base_url, CONF.deploy.http_image_subdir,
-                 node.uuid])
-            validate_image_url(http_image_url, secret=True)
-            instance_info['image_url'] = http_image_url
+            _cache_and_convert_image(task, instance_info, image_info)
 
         instance_info['image_container_format'] = (
             image_info['container_format'])
@@ -1064,8 +1085,10 @@ def build_instance_info_for_deploy(task):
         if not iwdi:
             instance_info['kernel'] = image_info['properties']['kernel_id']
             instance_info['ramdisk'] = image_info['properties']['ramdisk_id']
+    elif image_source.startswith('file://'):
+        _cache_and_convert_image(task, instance_info)
     else:
-        validate_image_url(image_source)
+        _validate_image_url(node, image_source)
         instance_info['image_url'] = image_source
 
     if not iwdi:

ironic/tests/unit/drivers/modules/test_agent.py

@@ -166,7 +166,10 @@ class TestAgentMethods(db_base.DbTestCase):
             show_mock.assert_called_once_with(self.context, 'fake-image')
 
     @mock.patch.object(deploy_utils, 'check_for_missing_params', autospec=True)
-    def test_validate_http_provisioning_not_glance(self, utils_mock):
+    def test_validate_http_provisioning_http_image(self, utils_mock):
+        i_info = self.node.instance_info
+        i_info['image_source'] = 'http://image-ref'
+        self.node.instance_info = i_info
         agent.validate_http_provisioning_configuration(self.node)
         utils_mock.assert_not_called()
 
@@ -189,6 +192,16 @@ class TestAgentMethods(db_base.DbTestCase):
                                agent.validate_http_provisioning_configuration,
                                self.node)
 
+    def test_validate_http_provisioning_missing_args_file(self):
+        CONF.set_override('http_url', None, group='deploy')
+        i_info = self.node.instance_info
+        i_info['image_source'] = 'file://image-ref'
+        self.node.instance_info = i_info
+        self.assertRaisesRegex(exception.MissingParameterValue,
+                               'failed to validate http provisoning',
+                               agent.validate_http_provisioning_configuration,
+                               self.node)
+
 
 class TestAgentDeploy(db_base.DbTestCase):
     def setUp(self):
@@ -211,6 +224,7 @@ class TestAgentDeploy(db_base.DbTestCase):
         self.ports = [
             object_utils.create_test_port(self.context, node_id=self.node.id)]
         dhcp_factory.DHCPFactory._dhcp_provider = None
+        CONF.set_override('http_url', 'http://example.com', group='deploy')
 
     def test_get_properties(self):
         expected = agent.COMMON_PROPERTIES
@@ -353,6 +367,24 @@ class TestAgentDeploy(db_base.DbTestCase):
             show_mock.assert_called_once_with(self.context,
                                               'http://image-ref')
 
+    @mock.patch.object(image_service.FileImageService, 'validate_href',
+                       autospec=True)
+    @mock.patch.object(pxe.PXEBoot, 'validate', autospec=True)
+    def test_validate_file_image_no_checksum(
+            self, pxe_boot_validate_mock, validate_mock):
+        i_info = self.node.instance_info
+        i_info['image_source'] = 'file://image-ref'
+        del i_info['image_checksum']
+        self.node.instance_info = i_info
+        self.node.save()
+
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+            self.driver.validate(task)
+            pxe_boot_validate_mock.assert_called_once_with(
+                task.driver.boot, task)
+            validate_mock.assert_called_once_with(mock.ANY, 'file://image-ref')
+
     @mock.patch.object(agent, 'validate_http_provisioning_configuration',
                        autospec=True)
     @mock.patch.object(images, 'image_show', autospec=True)
@@ -1045,11 +1077,14 @@ class TestAgentDeploy(db_base.DbTestCase):
             self.assertFalse(build_options_mock.called)
             self.assertFalse(pxe_prepare_ramdisk_mock.called)
 
+    @mock.patch.object(deploy_utils, 'destroy_http_instance_images',
+                       autospec=True)
     @mock.patch('ironic.common.dhcp_factory.DHCPFactory', autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'clean_up_instance', autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'clean_up_ramdisk', autospec=True)
     def test_clean_up(self, pxe_clean_up_ramdisk_mock,
-                      pxe_clean_up_instance_mock, dhcp_factor_mock):
+                      pxe_clean_up_instance_mock, dhcp_factor_mock,
+                      destroy_images_mock):
         with task_manager.acquire(
                 self.context, self.node['uuid'], shared=False) as task:
             self.driver.clean_up(task)
@@ -1058,13 +1093,17 @@ class TestAgentDeploy(db_base.DbTestCase):
             pxe_clean_up_instance_mock.assert_called_once_with(
                 task.driver.boot, task)
             dhcp_factor_mock.assert_called_once_with()
+            destroy_images_mock.assert_called_once_with(task.node)
 
+    @mock.patch.object(deploy_utils, 'destroy_http_instance_images',
+                       autospec=True)
     @mock.patch('ironic.common.dhcp_factory.DHCPFactory', autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'clean_up_instance', autospec=True)
     @mock.patch.object(pxe.PXEBoot, 'clean_up_ramdisk', autospec=True)
     def test_clean_up_manage_agent_boot_false(self, pxe_clean_up_ramdisk_mock,
                                               pxe_clean_up_instance_mock,
-                                              dhcp_factor_mock):
+                                              dhcp_factor_mock,
+                                              destroy_images_mock):
         with task_manager.acquire(
                 self.context, self.node['uuid'], shared=False) as task:
             self.config(group='agent', manage_agent_boot=False)
@@ -1073,6 +1112,7 @@ class TestAgentDeploy(db_base.DbTestCase):
             pxe_clean_up_instance_mock.assert_called_once_with(
                 task.driver.boot, task)
             dhcp_factor_mock.assert_called_once_with()
+            destroy_images_mock.assert_called_once_with(task.node)
 
     @mock.patch.object(agent_base, 'get_steps', autospec=True)
     def test_get_clean_steps(self, mock_get_steps):

ironic/tests/unit/drivers/modules/test_deploy_utils.py

@@ -1921,12 +1921,12 @@ class TestBuildInstanceInfoForDeploy(db_base.DbTestCase):
 
     @mock.patch.object(image_service.HttpImageService, 'validate_href',
                        autospec=True)
-    def test_build_instance_info_for_deploy_nonsupported_image(
+    def test_build_instance_info_for_deploy_image_not_found(
             self, validate_href_mock):
         validate_href_mock.side_effect = exception.ImageRefValidationFailed(
-            image_href='file://img.qcow2', reason='fail')
+            image_href='http://img.qcow2', reason='fail')
         i_info = self.node.instance_info
-        i_info['image_source'] = 'file://img.qcow2'
+        i_info['image_source'] = 'http://img.qcow2'
         i_info['image_checksum'] = 'aa'
         self.node.instance_info = i_info
         self.node.save()
@@ -1958,9 +1958,11 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         self.checksum_mock.return_value = 'fake-checksum'
         self.cache_image_mock = self.useFixture(fixtures.MockPatchObject(
             utils, 'cache_instance_image', autospec=True)).mock
+        self.fake_path = '/var/lib/ironic/images/{}/disk'.format(
+            self.node.uuid)
         self.cache_image_mock.return_value = (
             '733d1c44-a2ea-414b-aca7-69decf20d810',
-            '/var/lib/ironic/images/{}/disk'.format(self.node.uuid))
+            self.fake_path)
         self.ensure_tree_mock = self.useFixture(fixtures.MockPatchObject(
             utils.fileutils, 'ensure_tree', autospec=True)).mock
         self.create_link_mock = self.useFixture(fixtures.MockPatchObject(
@@ -2003,7 +2005,7 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             self.create_link_mock.assert_called_once_with(image_path,
                                                           symlink_file)
             validate_mock.assert_called_once_with(mock.ANY, self.expected_url,
-                                                  secret=True)
+                                                  secret=False)
             return image_path, instance_info
 
     def test_build_instance_info_no_force_raw(self):
@@ -2039,6 +2041,38 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         calls = [mock.call(image_path, algorithm='sha256')]
         self.checksum_mock.assert_has_calls(calls)
 
+    @mock.patch.object(image_service.HttpImageService, 'validate_href',
+                       autospec=True)
+    def test_build_instance_info_file_image(self, validate_href_mock):
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        i_info['image_source'] = 'file://image-ref'
+        i_info['image_checksum'] = 'aa'
+        i_info['root_gb'] = 10
+        i_info['image_checksum'] = 'aa'
+        driver_internal_info['is_whole_disk_image'] = True
+        self.node.instance_info = i_info
+        self.node.driver_internal_info = driver_internal_info
+        self.node.save()
+
+        expected_url = (
+            'http://172.172.24.10:8080/agent_images/%s' % self.node.uuid)
+
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+
+            info = utils.build_instance_info_for_deploy(task)
+
+            self.assertEqual(expected_url, info['image_url'])
+            self.assertEqual('sha256', info['image_os_hash_algo'])
+            self.assertEqual('fake-checksum', info['image_os_hash_value'])
+            self.cache_image_mock.assert_called_once_with(
+                task.context, task.node, force_raw=True)
+            self.checksum_mock.assert_called_once_with(
+                self.fake_path, algorithm='sha256')
+            validate_href_mock.assert_called_once_with(
+                mock.ANY, expected_url, False)
+
 
 class TestStorageInterfaceUtils(db_base.DbTestCase):
     def setUp(self):

releasenotes/notes/direct-file-6f80728d76093530.yaml

@@ -0,0 +1,4 @@
+---
+features:
+  - |
+    ``file://`` images are now supported in the ``direct`` deploy interface.