openstack/ironic
Code Issues Proposed changes

Stop requiring root_gb for whole-disk images

Browse Source 
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
This commit is contained in:
Dmitry Tantsur 2019-03-08 12:53:08 +01:00
parent 77ee49f21c
commit 40b55416a8
6 changed files with 98 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
doc/source/install/standalone.rst
View File

@ -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`.

77
ironic/drivers/modules/deploy_utils.py
View File

@ -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.
@ -1009,51 +1013,58 @@ def parse_instance_info(node):
" 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.

3
ironic/tests/unit/drivers/modules/ansible/test_deploy.py
View File

@ -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()

21
ironic/tests/unit/drivers/modules/test_deploy_utils.py
View File

@ -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):

24
ironic/tests/unit/drivers/modules/test_iscsi_deploy.py
View File

@ -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)

6
releasenotes/notes/whole-disk-root-gb-9132e5a354e6cb9d.yaml Normal file
View File

@ -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.