Do not recalculate checksum if disk_format is not changed

Even if a glance image is raw, we still recalculate the checksum after
"converting" it to raw. This process may take exceptionally long.

Change-Id: Id93d518b8d2b8064ff901f1a0452abd825e366c0

6 files changed, 151 insertions, 22 deletions

diff --git a/doc/source/admin/fast-track.rst b/doc/source/admin/fast-track.rst
index 20ca6199f..e42942818 100644
--- a/doc/source/admin/fast-track.rst
+++ b/doc/source/admin/fast-track.rst
@@ -15,6 +15,19 @@ provisioned within a short period of time.
    the ``noop`` networking. The case where inspection, cleaning and
    provisioning networks are different is not supported.
 
+.. note::
+   Fast track mode is very sensitive to long-running processes on the conductor
+   side that may prevent agent heartbeats from being registered.
+
+   For example, converting a large image to the raw format may take long enough
+   to reach the fast track timeout. In this case, you can either :ref:`use raw
+   images <stream_raw_images>` or move the conversion to the agent side with:
+
+   .. code-block:: ini
+
+    [DEFAULT]
+    force_raw_images = False
+
 Enabling
 ========
 
diff --git a/doc/source/admin/interfaces/deploy.rst b/doc/source/admin/interfaces/deploy.rst
index 7db5a24ff..79d004ad0 100644
--- a/doc/source/admin/interfaces/deploy.rst
+++ b/doc/source/admin/interfaces/deploy.rst
@@ -81,6 +81,46 @@ accessible from HTTP service. Please refer to configuration option
 ``FollowSymLinks`` if you are using Apache HTTP server, or
 ``disable_symlinks`` if Nginx HTTP server is in use.
 
+.. _stream_raw_images:
+
+Streaming raw images
+--------------------
+
+The Bare Metal service is capable of streaming raw images directly to the
+target disk of a node, without caching them in the node's RAM. When the source
+image is not already raw, the conductor will convert the image and calculate
+the new checksum.
+
+.. note::
+   If no algorithm is specified via the ``image_os_hash_algo`` field, or if
+   this field is set to ``md5``, SHA256 is used for the updated checksum.
+
+For HTTP or local file images that are already raw, you need to explicitly set
+the disk format to prevent the checksum from being unnecessarily re-calculated.
+For example:
+
+.. code-block:: shell
+
+    baremetal node set <node> \
+        --instance-info image_source=http://server/myimage.img \
+        --instance-info image_os_hash_algo=sha512 \
+        --instance-info image_os_hash_value=<SHA512 of the raw image> \
+        --instance-info image_disk_format=raw
+
+To disable this feature and cache images in the node's RAM, set
+
+.. code-block:: ini
+
+   [agent]
+   stream_raw_images = False
+
+To disable the conductor-side conversion completely, set
+
+.. code-block:: ini
+
+   [DEFAULT]
+   force_raw_images = False
+
 .. _ansible-deploy:
 
 Ansible deploy
diff --git a/doc/source/install/refarch/common.rst b/doc/source/install/refarch/common.rst
index 800632fd5..ce0dedfb1 100644
--- a/doc/source/install/refarch/common.rst
+++ b/doc/source/install/refarch/common.rst
@@ -277,9 +277,8 @@ the space requirements are different:
   In both cases a cached image is converted to raw if ``force_raw_images``
   is ``True`` (the default).
 
-  .. note::
-    ``image_download_source`` can also be provided in the node's
-    ``driver_info`` or ``instance_info``. See :ref:`image_download_source`.
+  See :ref:`image_download_source` and :ref:`stream_raw_images` for more
+  details.
 
 * When network boot is used, the instance image kernel and ramdisk are cached
   locally while the instance is active.
diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py
index 13f91e9cd..fd83f9f08 100644
--- a/ironic/drivers/modules/deploy_utils.py
+++ b/ironic/drivers/modules/deploy_utils.py
@@ -1093,6 +1093,11 @@ def _cache_and_convert_image(task, instance_info, image_info=None):
     _, image_path = cache_instance_image(task.context, task.node,
                                          force_raw=force_raw)
     if force_raw or image_info is None:
+        if image_info is None:
+            initial_format = instance_info.get('image_disk_format')
+        else:
+            initial_format = image_info.get('disk_format')
+
         if force_raw:
             instance_info['image_disk_format'] = 'raw'
         else:
@@ -1108,21 +1113,29 @@ def _cache_and_convert_image(task, instance_info, image_info=None):
         # sha256.
         if image_info is None:
             os_hash_algo = instance_info.get('image_os_hash_algo')
+            hash_value = instance_info.get('image_os_hash_value')
+            old_checksum = instance_info.get('image_checksum')
         else:
             os_hash_algo = image_info.get('os_hash_algo')
+            hash_value = image_info.get('os_hash_value')
+            old_checksum = image_info.get('checksum')
+
+        if initial_format != instance_info['image_disk_format']:
+            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)
+        else:
+            instance_info['image_checksum'] = old_checksum
 
-        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:
diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py
index 1177e9743..7220697cb 100644
--- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py
+++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py
@@ -1940,7 +1940,7 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         self.node.save()
 
         self.checksum_mock = self.useFixture(fixtures.MockPatchObject(
-            fileutils, 'compute_file_checksum')).mock
+            fileutils, 'compute_file_checksum', autospec=True)).mock
         self.checksum_mock.return_value = 'fake-checksum'
         self.cache_image_mock = self.useFixture(fixtures.MockPatchObject(
             utils, 'cache_instance_image', autospec=True)).mock
@@ -2012,9 +2012,25 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             image_info=self.image_info, expect_raw=True)
 
         self.assertIsNone(instance_info['image_checksum'])
+        self.assertEqual(instance_info['image_os_hash_algo'], 'sha512')
+        self.assertEqual(instance_info['image_os_hash_value'],
+                         'fake-checksum')
         self.assertEqual(instance_info['image_disk_format'], 'raw')
-        calls = [mock.call(image_path, algorithm='sha512')]
-        self.checksum_mock.assert_has_calls(calls)
+        self.checksum_mock.assert_called_once_with(image_path,
+                                                   algorithm='sha512')
+
+    def test_build_instance_info_already_raw(self):
+        cfg.CONF.set_override('force_raw_images', True)
+        self.image_info['disk_format'] = 'raw'
+        image_path, instance_info = self._test_build_instance_info(
+            image_info=self.image_info, expect_raw=True)
+
+        self.assertEqual(instance_info['image_checksum'], 'aa')
+        self.assertEqual(instance_info['image_os_hash_algo'], 'sha512')
+        self.assertEqual(instance_info['image_os_hash_value'],
+                         'fake-sha512')
+        self.assertEqual(instance_info['image_disk_format'], 'raw')
+        self.checksum_mock.assert_not_called()
 
     def test_build_instance_info_force_raw_drops_md5(self):
         cfg.CONF.set_override('force_raw_images', True)
@@ -2027,6 +2043,17 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         calls = [mock.call(image_path, algorithm='sha256')]
         self.checksum_mock.assert_has_calls(calls)
 
+    def test_build_instance_info_already_raw_keeps_md5(self):
+        cfg.CONF.set_override('force_raw_images', True)
+        self.image_info['os_hash_algo'] = 'md5'
+        self.image_info['disk_format'] = 'raw'
+        image_path, instance_info = self._test_build_instance_info(
+            image_info=self.image_info, expect_raw=True)
+
+        self.assertEqual(instance_info['image_checksum'], 'aa')
+        self.assertEqual(instance_info['image_disk_format'], 'raw')
+        self.checksum_mock.assert_not_called()
+
     @mock.patch.object(image_service.HttpImageService, 'validate_href',
                        autospec=True)
     def test_build_instance_info_file_image(self, validate_href_mock):
@@ -2035,7 +2062,6 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         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
@@ -2052,6 +2078,7 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             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.assertEqual('raw', info['image_disk_format'])
             self.cache_image_mock.assert_called_once_with(
                 task.context, task.node, force_raw=True)
             self.checksum_mock.assert_called_once_with(
@@ -2068,7 +2095,6 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         i_info['image_source'] = 'http://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
@@ -2102,7 +2128,6 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         i_info['image_source'] = 'http://image-ref'
         i_info['image_checksum'] = 'aa'
         i_info['root_gb'] = 10
-        i_info['image_checksum'] = 'aa'
         i_info['image_download_source'] = 'local'
         driver_internal_info['is_whole_disk_image'] = True
         self.node.instance_info = i_info
@@ -2138,7 +2163,6 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         i_info['image_source'] = 'http://image-ref'
         i_info['image_checksum'] = 'aa'
         i_info['root_gb'] = 10
-        i_info['image_checksum'] = 'aa'
         d_info['image_download_source'] = 'local'
         driver_internal_info['is_whole_disk_image'] = True
         self.node.instance_info = i_info
@@ -2164,6 +2188,41 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             validate_href_mock.assert_called_once_with(
                 mock.ANY, expected_url, False)
 
+    @mock.patch.object(image_service.HttpImageService, 'validate_href',
+                       autospec=True)
+    def test_build_instance_info_local_image_already_raw(self,
+                                                         validate_href_mock):
+        cfg.CONF.set_override('image_download_source', 'local', group='agent')
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        i_info['image_source'] = 'http://image-ref'
+        i_info['image_checksum'] = 'aa'
+        i_info['root_gb'] = 10
+        i_info['image_disk_format'] = 'raw'
+        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('aa', info['image_checksum'])
+            self.assertEqual('raw', info['image_disk_format'])
+            self.assertIsNone(info['image_os_hash_algo'])
+            self.assertIsNone(info['image_os_hash_value'])
+            self.cache_image_mock.assert_called_once_with(
+                task.context, task.node, force_raw=True)
+            self.checksum_mock.assert_not_called()
+            validate_href_mock.assert_called_once_with(
+                mock.ANY, expected_url, False)
+
 
 class TestStorageInterfaceUtils(db_base.DbTestCase):
     def setUp(self):
diff --git a/releasenotes/notes/no-recalculate-653e524fd6160e72.yaml b/releasenotes/notes/no-recalculate-653e524fd6160e72.yaml
new file mode 100644
index 000000000..3d2e6dad4
--- /dev/null
+++ b/releasenotes/notes/no-recalculate-653e524fd6160e72.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    No longer re-calculates checksums for images that are already raw.
+    Previously, it would cause significant delays in deploying raw images.