From f00da959eaa70a7e77059655c0050137cee78568 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 6 Mar 2023 18:14:31 +0100 Subject: [PATCH] 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 --- doc/source/admin/fast-track.rst | 13 ++++ doc/source/admin/interfaces/deploy.rst | 40 ++++++++++ doc/source/install/refarch/common.rst | 5 +- ironic/drivers/modules/deploy_utils.py | 35 ++++++--- .../unit/drivers/modules/test_deploy_utils.py | 73 +++++++++++++++++-- .../no-recalculate-653e524fd6160e72.yaml | 5 ++ 6 files changed, 150 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/no-recalculate-653e524fd6160e72.yaml diff --git a/doc/source/admin/fast-track.rst b/doc/source/admin/fast-track.rst index 20ca6199f0..e429428181 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 ` 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 7db5a24ff3..79d004ad09 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 \ + --instance-info image_source=http://server/myimage.img \ + --instance-info image_os_hash_algo=sha512 \ + --instance-info image_os_hash_value= \ + --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 800632fd5d..ce0dedfb12 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 13f91e9cd1..fd83f9f087 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 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' + 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 - 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 1177e97438..7220697cbb 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 0000000000..3d2e6dad4f --- /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.