Don't check flavor disk size when booting from volume
When creating a volume from an image, cinder copies the image metadata
into volume properties. When booting from the volume, we read this
metadata from the volume and use it as image metadata once again.
While fixing the check against min_ram,
change I861a78b5c7efa71e4bf7206d388b8d0d8048c78e introduced a
regression which prevents a user from booting a volume which is larger
than the flavor's disk. As we are not creating this disk, this check
does not make sense. Similarly, it checks the image metadata's
min_disk against the flavor disk size, which is not being used.
This change leaves the image metadata check unaltered when creating a
flavor disk. When booting from a volume, we check min_disk from image
metadata against the actual size of the volume. We don't check the
volume size at all. The check against min_ram is retained unaltered.
Closes-Bug: #1457517
Closes-Bug: #1459491
Closes-Bug: #1466305
Change-Id: I264493172da20b664df571e32876030246c2a87c
(cherry picked from commit 642c986f06)
This commit is contained in:
Matthew Booth
Liang Chen
parent
7ef454ab1c
commit
8794b938dc
|
|
@ -651,7 +651,8 @@ class API(base.Base):
|
|||
# reason, we rely on the DB to cast True to a String.
|
||||
return True if bool_val else ''
|
||||
|
||||
def _check_requested_image(self, context, image_id, image, instance_type):
|
||||
def _check_requested_image(self, context, image_id, image,
|
||||
instance_type, root_bdm):
|
||||
if not image:
|
||||
return
|
||||
|
||||
|
|
@ -668,15 +669,63 @@ class API(base.Base):
|
|||
if instance_type['memory_mb'] < int(image.get('min_ram') or 0):
|
||||
raise exception.FlavorMemoryTooSmall()
|
||||
|
||||
# NOTE(johannes): root_gb is allowed to be 0 for legacy reasons
|
||||
# since libvirt interpreted the value differently than other
|
||||
# drivers. A value of 0 means don't check size.
|
||||
root_gb = instance_type['root_gb']
|
||||
if root_gb:
|
||||
if int(image.get('size') or 0) > root_gb * (1024 ** 3):
|
||||
raise exception.FlavorDiskTooSmall()
|
||||
# Image min_disk is in gb, size is in bytes. For sanity, have them both
|
||||
# in bytes.
|
||||
image_min_disk = int(image.get('min_disk') or 0) * units.Gi
|
||||
image_size = int(image.get('size') or 0)
|
||||
|
||||
if int(image.get('min_disk') or 0) > root_gb:
|
||||
# Target disk is a volume. Don't check flavor disk size because it
|
||||
# doesn't make sense, and check min_disk against the volume size.
|
||||
if (root_bdm is not None and root_bdm.is_volume):
|
||||
# There are 2 possibilities here: either the target volume already
|
||||
# exists, or it doesn't, in which case the bdm will contain the
|
||||
# intended volume size.
|
||||
#
|
||||
# Cinder does its own check against min_disk, so if the target
|
||||
# volume already exists this has already been done and we don't
|
||||
# need to check it again here. In this case, volume_size may not be
|
||||
# set on the bdm.
|
||||
#
|
||||
# If we're going to create the volume, the bdm will contain
|
||||
# volume_size. Therefore we should check it if it exists. This will
|
||||
# still be checked again by cinder when the volume is created, but
|
||||
# that will not happen until the request reaches a host. By
|
||||
# checking it here, the user gets an immediate and useful failure
|
||||
# indication.
|
||||
#
|
||||
# The third possibility is that we have failed to consider
|
||||
# something, and there are actually more than 2 possibilities. In
|
||||
# this case cinder will still do the check at volume creation time.
|
||||
# The behaviour will still be correct, but the user will not get an
|
||||
# immediate failure from the api, and will instead have to
|
||||
# determine why the instance is in an error state with a task of
|
||||
# block_device_mapping.
|
||||
#
|
||||
# We could reasonably refactor this check into _validate_bdm at
|
||||
# some future date, as the various size logic is already split out
|
||||
# in there.
|
||||
dest_size = root_bdm.volume_size
|
||||
if dest_size is not None:
|
||||
dest_size *= units.Gi
|
||||
|
||||
if image_min_disk > dest_size:
|
||||
# TODO(mdbooth) Raise a more descriptive exception here.
|
||||
# This is the exception which calling code expects, but
|
||||
# it's potentially misleading to the user.
|
||||
raise exception.FlavorDiskTooSmall()
|
||||
|
||||
# Target disk is a local disk whose size is taken from the flavor
|
||||
else:
|
||||
dest_size = instance_type['root_gb'] * units.Gi
|
||||
|
||||
# NOTE(johannes): root_gb is allowed to be 0 for legacy reasons
|
||||
# since libvirt interpreted the value differently than other
|
||||
# drivers. A value of 0 means don't check size.
|
||||
if dest_size != 0:
|
||||
if image_size > dest_size:
|
||||
raise exception.FlavorDiskTooSmall()
|
||||
|
||||
if image_min_disk > dest_size:
|
||||
raise exception.FlavorDiskTooSmall()
|
||||
|
||||
def _get_image_defined_bdms(self, base_options, instance_type, image_meta,
|
||||
|
|
@ -767,10 +816,11 @@ class API(base.Base):
|
|||
|
||||
def _checks_for_create_and_rebuild(self, context, image_id, image,
|
||||
instance_type, metadata,
|
||||
files_to_inject):
|
||||
files_to_inject, root_bdm):
|
||||
self._check_metadata_properties_quota(context, metadata)
|
||||
self._check_injected_file_quota(context, files_to_inject)
|
||||
self._check_requested_image(context, image_id, image, instance_type)
|
||||
self._check_requested_image(context, image_id, image,
|
||||
instance_type, root_bdm)
|
||||
|
||||
def _validate_and_build_base_options(self, context, instance_type,
|
||||
boot_meta, image_href, image_id,
|
||||
|
|
@ -778,7 +828,7 @@ class API(base.Base):
|
|||
display_description, key_name,
|
||||
key_data, security_groups,
|
||||
availability_zone, forced_host,
|
||||
user_data, metadata, injected_files,
|
||||
user_data, metadata,
|
||||
access_ip_v4, access_ip_v6,
|
||||
requested_networks, config_drive,
|
||||
auto_disk_config, reservation_id,
|
||||
|
|
@ -810,9 +860,6 @@ class API(base.Base):
|
|||
except base64.binascii.Error:
|
||||
raise exception.InstanceUserDataMalformed()
|
||||
|
||||
self._checks_for_create_and_rebuild(context, image_id, boot_meta,
|
||||
instance_type, metadata, injected_files)
|
||||
|
||||
self._check_requested_secgroups(context, security_groups)
|
||||
|
||||
# Note: max_count is the number of instances requested by the user,
|
||||
|
|
@ -1095,7 +1142,7 @@ class API(base.Base):
|
|||
instance_type, boot_meta, image_href, image_id, kernel_id,
|
||||
ramdisk_id, display_name, display_description,
|
||||
key_name, key_data, security_groups, availability_zone,
|
||||
forced_host, user_data, metadata, injected_files, access_ip_v4,
|
||||
forced_host, user_data, metadata, access_ip_v4,
|
||||
access_ip_v6, requested_networks, config_drive,
|
||||
auto_disk_config, reservation_id, max_count)
|
||||
|
||||
|
|
@ -1115,6 +1162,12 @@ class API(base.Base):
|
|||
base_options, instance_type, boot_meta, min_count, max_count,
|
||||
block_device_mapping, legacy_bdm)
|
||||
|
||||
# We can't do this check earlier because we need bdms from all sources
|
||||
# to have been merged in order to get the root bdm.
|
||||
self._checks_for_create_and_rebuild(context, image_id, boot_meta,
|
||||
instance_type, metadata, injected_files,
|
||||
block_device_mapping.root_bdm())
|
||||
|
||||
instance_group = self._get_requested_instance_group(context,
|
||||
scheduler_hints, check_server_group_quota)
|
||||
|
||||
|
|
@ -2333,8 +2386,9 @@ class API(base.Base):
|
|||
self._check_auto_disk_config(image=image, **kwargs)
|
||||
|
||||
flavor = instance.get_flavor()
|
||||
root_bdm = self._get_root_bdm(context, instance)
|
||||
self._checks_for_create_and_rebuild(context, image_id, image,
|
||||
flavor, metadata, files_to_inject)
|
||||
flavor, metadata, files_to_inject, root_bdm)
|
||||
|
||||
kernel_id, ramdisk_id = self._handle_kernel_and_ramdisk(
|
||||
context, None, None, image)
|
||||
|
|
@ -3201,15 +3255,18 @@ class API(base.Base):
|
|||
uuids = [instance.uuid for instance in instances]
|
||||
return self.db.instance_fault_get_by_instance_uuids(context, uuids)
|
||||
|
||||
def is_volume_backed_instance(self, context, instance, bdms=None):
|
||||
if not instance.image_ref:
|
||||
return True
|
||||
|
||||
def _get_root_bdm(self, context, instance, bdms=None):
|
||||
if bdms is None:
|
||||
bdms = objects.BlockDeviceMappingList.get_by_instance_uuid(
|
||||
context, instance.uuid)
|
||||
|
||||
root_bdm = bdms.root_bdm()
|
||||
return bdms.root_bdm()
|
||||
|
||||
def is_volume_backed_instance(self, context, instance, bdms=None):
|
||||
if not instance.image_ref:
|
||||
return True
|
||||
|
||||
root_bdm = self._get_root_bdm(context, instance, bdms)
|
||||
if not root_bdm:
|
||||
return False
|
||||
return root_bdm.is_volume
|
||||
|
|
|
|||
|
|
@ -11409,6 +11409,8 @@ class ComputeInactiveImageTestCase(BaseTestCase):
|
|||
return {'id': id, 'min_disk': None, 'min_ram': None,
|
||||
'name': 'fake_name',
|
||||
'status': 'deleted',
|
||||
'min_ram': 0,
|
||||
'min_disk': 0,
|
||||
'properties': {'kernel_id': 'fake_kernel_id',
|
||||
'ramdisk_id': 'fake_ramdisk_id',
|
||||
'something_else': 'meow'}}
|
||||
|
|
@ -11770,78 +11772,175 @@ class CheckRequestedImageTestCase(test.TestCase):
|
|||
|
||||
def test_no_image_specified(self):
|
||||
self.compute_api._check_requested_image(self.context, None, None,
|
||||
self.instance_type)
|
||||
self.instance_type, None)
|
||||
|
||||
def test_image_status_must_be_active(self):
|
||||
image = dict(id='123', status='foo')
|
||||
|
||||
self.assertRaises(exception.ImageNotActive,
|
||||
self.compute_api._check_requested_image, self.context,
|
||||
image['id'], image, self.instance_type)
|
||||
image['id'], image, self.instance_type, None)
|
||||
|
||||
image['status'] = 'active'
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
|
||||
def test_image_min_ram_check(self):
|
||||
image = dict(id='123', status='active', min_ram='65')
|
||||
|
||||
self.assertRaises(exception.FlavorMemoryTooSmall,
|
||||
self.compute_api._check_requested_image, self.context,
|
||||
image['id'], image, self.instance_type)
|
||||
image['id'], image, self.instance_type, None)
|
||||
|
||||
image['min_ram'] = '64'
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
|
||||
def test_image_min_disk_check(self):
|
||||
image = dict(id='123', status='active', min_disk='2')
|
||||
|
||||
self.assertRaises(exception.FlavorDiskTooSmall,
|
||||
self.compute_api._check_requested_image, self.context,
|
||||
image['id'], image, self.instance_type)
|
||||
image['id'], image, self.instance_type, None)
|
||||
|
||||
image['min_disk'] = '1'
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
|
||||
def test_image_too_large(self):
|
||||
image = dict(id='123', status='active', size='1073741825')
|
||||
|
||||
self.assertRaises(exception.FlavorDiskTooSmall,
|
||||
self.compute_api._check_requested_image, self.context,
|
||||
image['id'], image, self.instance_type)
|
||||
image['id'], image, self.instance_type, None)
|
||||
|
||||
image['size'] = '1073741824'
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
|
||||
def test_root_gb_zero_disables_size_check(self):
|
||||
self.instance_type['root_gb'] = 0
|
||||
image = dict(id='123', status='active', size='1073741825')
|
||||
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
|
||||
def test_root_gb_zero_disables_min_disk(self):
|
||||
self.instance_type['root_gb'] = 0
|
||||
image = dict(id='123', status='active', min_disk='2')
|
||||
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
|
||||
def test_config_drive_option(self):
|
||||
image = {'id': 1, 'status': 'active'}
|
||||
image['properties'] = {'img_config_drive': 'optional'}
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
image['properties'] = {'img_config_drive': 'mandatory'}
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type)
|
||||
image, self.instance_type, None)
|
||||
image['properties'] = {'img_config_drive': 'bar'}
|
||||
self.assertRaises(exception.InvalidImageConfigDrive,
|
||||
self.compute_api._check_requested_image,
|
||||
self.context, image['id'], image, self.instance_type)
|
||||
self.context, image['id'], image, self.instance_type,
|
||||
None)
|
||||
|
||||
def test_volume_blockdevicemapping(self):
|
||||
# We should allow a root volume which is larger than the flavor root
|
||||
# disk.
|
||||
# We should allow a root volume created from an image whose min_disk is
|
||||
# larger than the flavor root disk.
|
||||
image_uuid = str(uuid.uuid4())
|
||||
image = dict(id=image_uuid, status='active',
|
||||
size=self.instance_type.root_gb * units.Gi,
|
||||
min_disk=self.instance_type.root_gb + 1)
|
||||
|
||||
volume_uuid = str(uuid.uuid4())
|
||||
root_bdm = block_device_obj.BlockDeviceMapping(
|
||||
source_type='volume', destination_type='volume',
|
||||
volume_id=volume_uuid, volume_size=self.instance_type.root_gb + 1)
|
||||
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type, root_bdm)
|
||||
|
||||
def test_volume_blockdevicemapping_min_disk(self):
|
||||
# A bdm object volume smaller than the image's min_disk should not be
|
||||
# allowed
|
||||
image_uuid = str(uuid.uuid4())
|
||||
image = dict(id=image_uuid, status='active',
|
||||
size=self.instance_type.root_gb * units.Gi,
|
||||
min_disk=self.instance_type.root_gb + 1)
|
||||
|
||||
volume_uuid = str(uuid.uuid4())
|
||||
root_bdm = block_device_obj.BlockDeviceMapping(
|
||||
source_type='image', destination_type='volume',
|
||||
image_id=image_uuid, volume_id=volume_uuid,
|
||||
volume_size=self.instance_type.root_gb)
|
||||
|
||||
self.assertRaises(exception.FlavorDiskTooSmall,
|
||||
self.compute_api._check_requested_image,
|
||||
self.context, image_uuid, image, self.instance_type,
|
||||
root_bdm)
|
||||
|
||||
def test_volume_blockdevicemapping_min_disk_no_size(self):
|
||||
# We should allow a root volume whose size is not given
|
||||
image_uuid = str(uuid.uuid4())
|
||||
image = dict(id=image_uuid, status='active',
|
||||
size=self.instance_type.root_gb * units.Gi,
|
||||
min_disk=self.instance_type.root_gb)
|
||||
|
||||
volume_uuid = str(uuid.uuid4())
|
||||
root_bdm = block_device_obj.BlockDeviceMapping(
|
||||
source_type='volume', destination_type='volume',
|
||||
volume_id=volume_uuid, volume_size=None)
|
||||
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type, root_bdm)
|
||||
|
||||
def test_image_blockdevicemapping(self):
|
||||
# Test that we can succeed when passing bdms, and the root bdm isn't a
|
||||
# volume
|
||||
image_uuid = str(uuid.uuid4())
|
||||
image = dict(id=image_uuid, status='active',
|
||||
size=self.instance_type.root_gb * units.Gi, min_disk=0)
|
||||
|
||||
root_bdm = block_device_obj.BlockDeviceMapping(
|
||||
source_type='image', destination_type='local', image_id=image_uuid)
|
||||
|
||||
self.compute_api._check_requested_image(self.context, image['id'],
|
||||
image, self.instance_type, root_bdm)
|
||||
|
||||
def test_image_blockdevicemapping_too_big(self):
|
||||
# We should do a size check against flavor if we were passed bdms but
|
||||
# the root bdm isn't a volume
|
||||
image_uuid = str(uuid.uuid4())
|
||||
image = dict(id=image_uuid, status='active',
|
||||
size=(self.instance_type.root_gb + 1) * units.Gi,
|
||||
min_disk=0)
|
||||
|
||||
root_bdm = block_device_obj.BlockDeviceMapping(
|
||||
source_type='image', destination_type='local', image_id=image_uuid)
|
||||
|
||||
self.assertRaises(exception.FlavorDiskTooSmall,
|
||||
self.compute_api._check_requested_image,
|
||||
self.context, image['id'],
|
||||
image, self.instance_type, root_bdm)
|
||||
|
||||
def test_image_blockdevicemapping_min_disk(self):
|
||||
# We should do a min_disk check against flavor if we were passed bdms
|
||||
# but the root bdm isn't a volume
|
||||
image_uuid = str(uuid.uuid4())
|
||||
image = dict(id=image_uuid, status='active',
|
||||
size=0, min_disk=self.instance_type.root_gb + 1)
|
||||
|
||||
root_bdm = block_device_obj.BlockDeviceMapping(
|
||||
source_type='image', destination_type='local', image_id=image_uuid)
|
||||
|
||||
self.assertRaises(exception.FlavorDiskTooSmall,
|
||||
self.compute_api._check_requested_image,
|
||||
self.context, image['id'],
|
||||
image, self.instance_type, root_bdm)
|
||||
|
||||
|
||||
class ComputeHooksTestCase(test.BaseHookTestCase):
|
||||
|
|
|
|||
|
|
@ -2257,15 +2257,16 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
vm_state=vm_states.ACTIVE, cell_name='fake-cell',
|
||||
launched_at=timeutils.utcnow(),
|
||||
system_metadata=orig_system_metadata,
|
||||
image_ref='foo',
|
||||
expected_attrs=['system_metadata'])
|
||||
get_flavor.return_value = test_flavor.fake_flavor
|
||||
flavor = instance.get_flavor()
|
||||
image_href = ''
|
||||
image_href = 'foo'
|
||||
image = {"min_ram": 10, "min_disk": 1,
|
||||
"properties": {'architecture': arch.X86_64}}
|
||||
admin_pass = ''
|
||||
files_to_inject = []
|
||||
bdms = []
|
||||
bdms = objects.BlockDeviceMappingList()
|
||||
|
||||
_get_image.return_value = (None, image)
|
||||
bdm_get_by_instance_uuid.return_value = bdms
|
||||
|
|
@ -2284,7 +2285,7 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
|
||||
_check_auto_disk_config.assert_called_once_with(image=image)
|
||||
_checks_for_create_and_rebuild.assert_called_once_with(self.context,
|
||||
None, image, flavor, {}, [])
|
||||
None, image, flavor, {}, [], None)
|
||||
self.assertNotEqual(orig_system_metadata, instance.system_metadata)
|
||||
|
||||
@mock.patch.object(objects.Instance, 'save')
|
||||
|
|
@ -2309,7 +2310,7 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
'vm_mode': 'xen'}}
|
||||
admin_pass = ''
|
||||
files_to_inject = []
|
||||
bdms = []
|
||||
bdms = objects.BlockDeviceMappingList()
|
||||
|
||||
instance = fake_instance.fake_instance_obj(self.context,
|
||||
vm_state=vm_states.ACTIVE, cell_name='fake-cell',
|
||||
|
|
@ -2342,7 +2343,7 @@ class _ComputeAPIUnitTestMixIn(object):
|
|||
|
||||
_check_auto_disk_config.assert_called_once_with(image=new_image)
|
||||
_checks_for_create_and_rebuild.assert_called_once_with(self.context,
|
||||
None, new_image, flavor, {}, [])
|
||||
None, new_image, flavor, {}, [], None)
|
||||
self.assertEqual(vm_mode.XEN, instance.vm_mode)
|
||||
|
||||
def _test_check_injected_file_quota_onset_file_limit_exceeded(self,
|
||||
|
|
|
|||
|
|
@ -236,11 +236,13 @@ class CellsConductorAPIRPCRedirect(test.NoDBTestCase):
|
|||
@mock.patch.object(compute_api.API, '_check_and_transform_bdm')
|
||||
@mock.patch.object(compute_api.API, '_get_image')
|
||||
@mock.patch.object(compute_api.API, '_validate_and_build_base_options')
|
||||
def test_build_instances(self, _validate, _get_image, _check_bdm,
|
||||
@mock.patch.object(compute_api.API, '_checks_for_create_and_rebuild')
|
||||
def test_build_instances(self, _checks_for_create_and_rebuild,
|
||||
_validate, _get_image, _check_bdm,
|
||||
_provision, _record_action_start):
|
||||
_get_image.return_value = (None, 'fake-image')
|
||||
_validate.return_value = ({}, 1)
|
||||
_check_bdm.return_value = 'bdms'
|
||||
_check_bdm.return_value = objects.BlockDeviceMappingList()
|
||||
_provision.return_value = 'instances'
|
||||
|
||||
self.compute_api.create(self.context, 'fake-flavor', 'fake-image')
|
||||
|
|
@ -309,7 +311,7 @@ class CellsConductorAPIRPCRedirect(test.NoDBTestCase):
|
|||
"properties": {'architecture': 'x86_64'}}
|
||||
admin_pass = ''
|
||||
files_to_inject = []
|
||||
bdms = []
|
||||
bdms = objects.BlockDeviceMappingList()
|
||||
|
||||
_get_image.return_value = (None, image)
|
||||
bdm_get_by_instance_uuid.return_value = bdms
|
||||
|
|
|
|||
Loading…
Reference in New Issue