From 40b55416a8ea603250cafa98418c513d5a5e51e4 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 8 Mar 2019 12:53:08 +0100 Subject: [PATCH] Stop requiring root_gb for whole-disk images This parameter makes no sense for them and is actually ignored after validation. This change makes it optional (still allowed for backward compatibility). Also cleanly separate validation code for partition and whole-disk images. Change-Id: I98da7d54cc6dd158d415e83c61659badc83fbeda Story: #2005165 Task: #29895 --- doc/source/install/standalone.rst | 8 +- ironic/drivers/modules/deploy_utils.py | 79 +++++++++++-------- .../drivers/modules/ansible/test_deploy.py | 3 + .../unit/drivers/modules/test_deploy_utils.py | 21 +++-- .../unit/drivers/modules/test_iscsi_deploy.py | 24 ++++++ .../whole-disk-root-gb-9132e5a354e6cb9d.yaml | 6 ++ 6 files changed, 98 insertions(+), 43 deletions(-) create mode 100644 releasenotes/notes/whole-disk-root-gb-9132e5a354e6cb9d.yaml 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 69237b312a..d294533030 100644 --- a/ironic/tests/unit/drivers/modules/ansible/test_deploy.py +++ b/ironic/tests/unit/drivers/modules/ansible/test_deploy.py @@ -777,7 +777,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.