From 840ce1666830633bae74db74b677a9ae9a013d9f Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 2 Sep 2020 15:10:33 +0200 Subject: [PATCH] Allow setting image_download_source per node Allows certain flexibility when it comes to low RAM vs high RAM nodes, and large vs small images. Also deploy_interface is settable per node, so this feature makes it easier to migrate from the iscsi deploy. Story: #2008075 Task: #40766 Change-Id: Idf3bbc6d24042ce1d9a895095b5cb0979dd3183d --- doc/source/admin/interfaces/deploy.rst | 9 +++ doc/source/install/standalone.rst | 7 ++ ironic/drivers/modules/agent.py | 16 ++++- ironic/drivers/modules/deploy_utils.py | 12 +++- .../tests/unit/drivers/modules/test_agent.py | 27 +++++++ .../unit/drivers/modules/test_deploy_utils.py | 72 +++++++++++++++++++ ...mage_download_source-842282c70b226e93.yaml | 6 ++ 7 files changed, 145 insertions(+), 4 deletions(-) create mode 100644 releasenotes/notes/image_download_source-842282c70b226e93.yaml diff --git a/doc/source/admin/interfaces/deploy.rst b/doc/source/admin/interfaces/deploy.rst index 74e50dd46a..d8c0e1b09a 100644 --- a/doc/source/admin/interfaces/deploy.rst +++ b/doc/source/admin/interfaces/deploy.rst @@ -71,6 +71,15 @@ This configuration affects *glance* and ``file://`` images. If you want [agent] image_download_source = local +.. note:: + This option can also be set per node in ``driver_info``:: + + openstack baremetal node set --driver-info image_download_source=local + + or per instance in ``instance_info``:: + + openstack baremetal node set --instance-info image_download_source=local + You need to set up a workable HTTP server at each conductor node which with ``direct`` deploy interface enabled, and check http related options in the ironic configuration file to match the HTTP server configurations. diff --git a/doc/source/install/standalone.rst b/doc/source/install/standalone.rst index 60c865355d..8a693aedbd 100644 --- a/doc/source/install/standalone.rst +++ b/doc/source/install/standalone.rst @@ -228,6 +228,13 @@ Populating instance_info --instance-info image_source=$IMG \ --instance-info image_checksum=$MD5HASH +#. When using low RAM nodes with ``http://`` images that are not in the RAW + format, you may want them cached locally, converted to raw and served from + the conductor's HTTP server:: + + openstack baremetal node set $NODE_UUID \ + --instance-info image_download_source=local + #. :ref:`Boot mode ` can be specified per instance:: openstack baremetal node set $NODE_UUID \ diff --git a/ironic/drivers/modules/agent.py b/ironic/drivers/modules/agent.py index 558052466f..8c60c0ad83 100644 --- a/ironic/drivers/modules/agent.py +++ b/ironic/drivers/modules/agent.py @@ -57,6 +57,12 @@ OPTIONAL_PROPERTIES = { 'a dot to prefix the domain name. This value will be ' 'ignored if ``image_http_proxy`` and ' '``image_https_proxy`` are not specified. Optional.'), + 'image_download_source': _('Specifies whether direct deploy interface ' + 'should try to use the image source directly ' + 'or if ironic should cache the image on the ' + 'conductor and serve it from ironic\'s own ' + 'HTTP server. Accepted values are "swift", ' + '"http" and "local". Optional.'), } _RAID_APPLY_CONFIGURATION_ARGSINFO = { @@ -168,13 +174,19 @@ def validate_http_provisioning_configuration(node): :raises: MissingParameterValue if required option(s) is not set. """ image_source = node.instance_info.get('image_source') + image_download_source = deploy_utils.get_image_download_source(node) + if image_download_source not in ('swift', 'http', 'local'): + raise exception.InvalidParameterValue( + _('Invalid value for image_download_source: "%s". Valid values ' + 'are swift, http or local.') % image_download_source) + # 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 CONF.agent.image_download_source != 'local' + and image_download_source != 'local' and (not service_utils.is_glance_image(image_source) - or CONF.agent.image_download_source == 'swift')): + or image_download_source == 'swift')): return params = { diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 662460a1f7..f75c97cf5a 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -1046,6 +1046,13 @@ def _cache_and_convert_image(task, instance_info, image_info=None): instance_info['image_url'] = http_image_url +def get_image_download_source(node): + """Get the effective value of image_download_source for the node.""" + return (node.instance_info.get('image_download_source') + or node.driver_info.get('image_download_source') + or CONF.agent.image_download_source) + + @METRICS.timer('build_instance_info_for_deploy') def build_instance_info_for_deploy(task): """Build instance_info necessary for deploying to a node. @@ -1060,13 +1067,14 @@ 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'] + image_download_source = get_image_download_source(node) if service_utils.is_glance_image(image_source): glance = image_service.GlanceImageService(context=task.context) image_info = glance.show(image_source) LOG.debug('Got image info: %(info)s for node %(node)s.', {'info': image_info, 'node': node.uuid}) - if CONF.agent.image_download_source == 'swift': + if image_download_source == 'swift': swift_temp_url = glance.swift_temp_url(image_info) _validate_image_url(node, swift_temp_url, secret=True) instance_info['image_url'] = swift_temp_url @@ -1086,7 +1094,7 @@ def build_instance_info_for_deploy(task): instance_info['kernel'] = image_info['properties']['kernel_id'] instance_info['ramdisk'] = image_info['properties']['ramdisk_id'] elif (image_source.startswith('file://') - or CONF.agent.image_download_source == 'local'): + or image_download_source == 'local'): _cache_and_convert_image(task, instance_info) else: _validate_image_url(node, image_source) diff --git a/ironic/tests/unit/drivers/modules/test_agent.py b/ironic/tests/unit/drivers/modules/test_agent.py index 51c6dccca5..f817779c28 100644 --- a/ironic/tests/unit/drivers/modules/test_agent.py +++ b/ironic/tests/unit/drivers/modules/test_agent.py @@ -213,6 +213,33 @@ class TestAgentMethods(db_base.DbTestCase): agent.validate_http_provisioning_configuration, self.node) + def test_validate_http_provisioning_missing_args_local_via_node(self): + CONF.set_override('http_url', None, group='deploy') + i_info = self.node.instance_info + i_info['image_source'] = 'http://image-ref' + i_info['image_download_source'] = 'local' + self.node.instance_info = i_info + self.assertRaisesRegex(exception.MissingParameterValue, + 'failed to validate http provisoning', + agent.validate_http_provisioning_configuration, + self.node) + + def test_validate_http_provisioning_invalid_image_download_source(self): + CONF.set_override('http_url', None, group='deploy') + self.node.instance_info['image_source'] = 'http://image-ref' + self.node.instance_info['image_download_source'] = 'fridge' + self.assertRaisesRegex(exception.InvalidParameterValue, 'fridge', + agent.validate_http_provisioning_configuration, + self.node) + + def test_validate_http_provisioning_invalid_image_download_source2(self): + CONF.set_override('http_url', None, group='deploy') + self.node.instance_info['image_source'] = 'http://image-ref' + self.node.driver_info['image_download_source'] = 'fridge' + self.assertRaisesRegex(exception.InvalidParameterValue, 'fridge', + agent.validate_http_provisioning_configuration, + self.node) + class TestAgentDeploy(db_base.DbTestCase): def setUp(self): diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 549727e6cf..85ecd46e0b 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -2106,6 +2106,78 @@ 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_via_iinfo(self, + validate_href_mock): + cfg.CONF.set_override('image_download_source', 'http', 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_checksum'] = 'aa' + i_info['image_download_source'] = 'local' + 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) + + @mock.patch.object(image_service.HttpImageService, 'validate_href', + autospec=True) + def test_build_instance_info_local_image_via_dinfo(self, + validate_href_mock): + cfg.CONF.set_override('image_download_source', 'http', group='agent') + i_info = self.node.instance_info + d_info = self.node.driver_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_checksum'] = 'aa' + d_info['image_download_source'] = 'local' + driver_internal_info['is_whole_disk_image'] = True + self.node.instance_info = i_info + self.node.driver_info = d_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): diff --git a/releasenotes/notes/image_download_source-842282c70b226e93.yaml b/releasenotes/notes/image_download_source-842282c70b226e93.yaml new file mode 100644 index 0000000000..c762add387 --- /dev/null +++ b/releasenotes/notes/image_download_source-842282c70b226e93.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + The ``image_download_source`` configuration option can now also be set + per node in the ``instance_info`` or ``driver_info`` (the former having + the highest priority).