diff --git a/doc/source/install/standalone.rst b/doc/source/install/standalone.rst index 6faaa16be5..680ae72766 100644 --- a/doc/source/install/standalone.rst +++ b/doc/source/install/standalone.rst @@ -117,10 +117,12 @@ Steps to start a deployment are pretty similar to those when using Compute: while :ref:`iscsi-deploy` also accepts links to local files (prefixed with ``file://``). - * ``root_gb`` - size of the root partition, mandatory. + * ``root_gb`` - size of the root partition, required for partition images. - .. TODO(dtantsur): root_gb should not be mandatory for whole disk images, - but it seems to be. + .. note:: + Older versions of the Bare Metal service used to require a positive + integer for ``root_gb`` even for whole-disk images. You may want to set + it for compatibility. * ``image_checksum`` - MD5 checksum of the image specified by ``image_source``, only required for :ref:`direct-deploy`. diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 30c8ae6528..9ff7fe3ce5 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -978,6 +978,10 @@ def get_image_instance_info(node): return info +_ERR_MSG_INVALID_DEPLOY = _("Cannot validate parameter for driver deploy. " + "Invalid parameter %(param)s. Reason: %(reason)s") + + def parse_instance_info(node): """Gets the instance specific Node deployment info. @@ -1003,57 +1007,64 @@ def parse_instance_info(node): i_info['image_source'])): i_info['kernel'] = info.get('kernel') i_info['ramdisk'] = info.get('ramdisk') - i_info['root_gb'] = info.get('root_gb') + i_info['root_gb'] = info.get('root_gb') error_msg = _("Cannot validate driver deploy. Some parameters were missing" " in node's instance_info") check_for_missing_params(i_info, error_msg) - # NOTE(vdrok): We're casting disk layout parameters to int only after - # ensuring that it is possible - i_info['swap_mb'] = info.get('swap_mb', 0) - i_info['ephemeral_gb'] = info.get('ephemeral_gb', 0) - err_msg_invalid = _("Cannot validate parameter for driver deploy. " - "Invalid parameter %(param)s. Reason: %(reason)s") - for param in DISK_LAYOUT_PARAMS: - try: - int(i_info[param]) - except ValueError: - reason = _("%s is not an integer value.") % i_info[param] - raise exception.InvalidParameterValue(err_msg_invalid % - {'param': param, - 'reason': reason}) - - i_info['root_mb'] = 1024 * int(i_info['root_gb']) - i_info['swap_mb'] = int(i_info['swap_mb']) - i_info['ephemeral_mb'] = 1024 * int(i_info['ephemeral_gb']) - - if iwdi: - if i_info['swap_mb'] > 0 or i_info['ephemeral_mb'] > 0: - err_msg_invalid = _("Cannot deploy whole disk image with " - "swap or ephemeral size set") - raise exception.InvalidParameterValue(err_msg_invalid) - i_info['ephemeral_format'] = info.get('ephemeral_format') - i_info['configdrive'] = info.get('configdrive') - - if i_info['ephemeral_gb'] and not i_info['ephemeral_format']: - i_info['ephemeral_format'] = CONF.pxe.default_ephemeral_format - + # This is used in many places, so keep it even for whole-disk images. + # There is also a potential use case of creating an ephemeral partition via + # cloud-init and telling ironic to avoid metadata wipe via setting + # preserve_ephemeral (not saying it will work, but it seems possible). preserve_ephemeral = info.get('preserve_ephemeral', False) try: i_info['preserve_ephemeral'] = ( strutils.bool_from_string(preserve_ephemeral, strict=True)) except ValueError as e: raise exception.InvalidParameterValue( - err_msg_invalid % {'param': 'preserve_ephemeral', 'reason': e}) + _ERR_MSG_INVALID_DEPLOY % {'param': 'preserve_ephemeral', + 'reason': e}) + + if iwdi: + if i_info.get('swap_mb') or i_info.get('ephemeral_mb'): + err_msg_invalid = _("Cannot deploy whole disk image with " + "swap or ephemeral size set") + raise exception.InvalidParameterValue(err_msg_invalid) + else: + _validate_layout_properties(node, info, i_info) + + i_info['configdrive'] = info.get('configdrive') + + return i_info + + +def _validate_layout_properties(node, info, i_info): + i_info['swap_mb'] = info.get('swap_mb', 0) + i_info['ephemeral_gb'] = info.get('ephemeral_gb', 0) + # NOTE(vdrok): We're casting disk layout parameters to int only after + # ensuring that it is possible + for param in DISK_LAYOUT_PARAMS: + try: + int(i_info[param]) + except ValueError: + reason = _("%s is not an integer value.") % i_info[param] + raise exception.InvalidParameterValue(_ERR_MSG_INVALID_DEPLOY % + {'param': param, + 'reason': reason}) + + i_info['root_mb'] = 1024 * int(i_info['root_gb']) + i_info['swap_mb'] = int(i_info['swap_mb']) + i_info['ephemeral_mb'] = 1024 * int(i_info['ephemeral_gb']) + i_info['ephemeral_format'] = info.get('ephemeral_format') + if i_info['ephemeral_gb'] and not i_info['ephemeral_format']: + i_info['ephemeral_format'] = CONF.pxe.default_ephemeral_format # NOTE(Zhenguo): If rebuilding with preserve_ephemeral option, check # that the disk layout is unchanged. if i_info['preserve_ephemeral']: _check_disk_layout_unchanged(node, i_info) - return i_info - def _check_disk_layout_unchanged(node, i_info): """Check whether disk layout is unchanged. diff --git a/ironic/tests/unit/drivers/modules/ansible/test_deploy.py b/ironic/tests/unit/drivers/modules/ansible/test_deploy.py index 1391bcc5ec..c9f0e455f4 100644 --- a/ironic/tests/unit/drivers/modules/ansible/test_deploy.py +++ b/ironic/tests/unit/drivers/modules/ansible/test_deploy.py @@ -806,7 +806,10 @@ class TestAnsibleDeploy(AnsibleDeployTestCaseBase): prepare_vars_mock.return_value = _vars driver_internal_info = self.node.driver_internal_info driver_internal_info['is_whole_disk_image'] = True + instance_info = self.node.instance_info + del instance_info['root_mb'] self.node.driver_internal_info = driver_internal_info + self.node.instance_info = instance_info self.node.extra = {'ham': 'spam'} self.node.save() diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 779605de6a..179230290b 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -2268,17 +2268,26 @@ class InstanceInfoTestCase(db_base.DbTestCase): ) instance_info = utils.parse_instance_info(node) self.assertIsNotNone(instance_info['image_source']) - self.assertIsNotNone(instance_info['root_mb']) - self.assertEqual(0, instance_info['swap_mb']) - self.assertEqual(0, instance_info['ephemeral_mb']) + self.assertNotIn('root_mb', instance_info) + self.assertNotIn('ephemeral_mb', instance_info) + self.assertNotIn('swap_mb', instance_info) self.assertIsNone(instance_info['configdrive']) def test_parse_instance_info_whole_disk_image_missing_root(self): + driver_internal_info = dict(DRV_INTERNAL_INFO_DICT) + driver_internal_info['is_whole_disk_image'] = True info = dict(INST_INFO_DICT) del info['root_gb'] - node = obj_utils.create_test_node(self.context, instance_info=info) - self.assertRaises(exception.InvalidParameterValue, - utils.parse_instance_info, node) + node = obj_utils.create_test_node( + self.context, instance_info=info, + driver_internal_info=driver_internal_info + ) + + instance_info = utils.parse_instance_info(node) + self.assertIsNotNone(instance_info['image_source']) + self.assertNotIn('root_mb', instance_info) + self.assertNotIn('ephemeral_mb', instance_info) + self.assertNotIn('swap_mb', instance_info) class TestBuildInstanceInfoForDeploy(db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py b/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py index d9d5826c2b..01cf03040f 100644 --- a/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py +++ b/ironic/tests/unit/drivers/modules/test_iscsi_deploy.py @@ -128,6 +128,18 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase): iscsi_deploy.check_image_size(task) self.assertFalse(get_image_mb_mock.called) + @mock.patch.object(disk_utils, 'get_image_mb', autospec=True) + def test_check_image_size_whole_disk_image_no_root(self, + get_image_mb_mock): + get_image_mb_mock.return_value = 1025 + with task_manager.acquire(self.context, self.node.uuid, + shared=False) as task: + del task.node.instance_info['root_gb'] + task.node.driver_internal_info['is_whole_disk_image'] = True + # No error for whole disk images + iscsi_deploy.check_image_size(task) + self.assertFalse(get_image_mb_mock.called) + @mock.patch.object(disk_utils, 'get_image_mb', autospec=True) def test_check_image_size_fails(self, get_image_mb_mock): get_image_mb_mock.return_value = 1025 @@ -452,6 +464,18 @@ class IscsiDeployMethodsTestCase(db_base.DbTestCase): self.assertEqual('target-iqn', ret_val['iqn']) self.assertEqual('My configdrive', ret_val['configdrive']) + def test_get_deploy_info_whole_disk_image_no_root(self): + instance_info = self.node.instance_info + instance_info['configdrive'] = 'My configdrive' + del instance_info['root_gb'] + self.node.instance_info = instance_info + self.node.driver_internal_info['is_whole_disk_image'] = True + kwargs = {'address': '1.1.1.1', 'iqn': 'target-iqn'} + ret_val = iscsi_deploy.get_deploy_info(self.node, **kwargs) + self.assertEqual('1.1.1.1', ret_val['address']) + self.assertEqual('target-iqn', ret_val['iqn']) + self.assertEqual('My configdrive', ret_val['configdrive']) + @mock.patch.object(iscsi_deploy, 'continue_deploy', autospec=True) def test_do_agent_iscsi_deploy_okay(self, continue_deploy_mock): agent_client_mock = mock.MagicMock(spec_set=agent_client.AgentClient) diff --git a/releasenotes/notes/whole-disk-root-gb-9132e5a354e6cb9d.yaml b/releasenotes/notes/whole-disk-root-gb-9132e5a354e6cb9d.yaml new file mode 100644 index 0000000000..fbfc3fb41f --- /dev/null +++ b/releasenotes/notes/whole-disk-root-gb-9132e5a354e6cb9d.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + The ``instance_info[root_gb]`` property is no longer required for + whole-disk images. It has always been ignored for them, but the validation + code still expected it to be present.