From 642c986f0636d52a9ba279c87e25082b4aa9b3b8 Mon Sep 17 00:00:00 2001 From: Matthew Booth Date: Wed, 22 Jul 2015 14:56:52 +0100 Subject: [PATCH] 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 --- nova/compute/api.py | 102 +++++++++++--- nova/tests/unit/compute/test_compute.py | 127 ++++++++++++++++-- nova/tests/unit/compute/test_compute_api.py | 11 +- nova/tests/unit/compute/test_compute_cells.py | 8 +- 4 files changed, 204 insertions(+), 44 deletions(-) diff --git a/nova/compute/api.py b/nova/compute/api.py index 594a6db3b697..dac3ac0a2417 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -32,6 +32,7 @@ from oslo_serialization import jsonutils from oslo_utils import excutils from oslo_utils import strutils from oslo_utils import timeutils +from oslo_utils import units from oslo_utils import uuidutils import six from six.moves import range @@ -646,7 +647,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 @@ -663,15 +665,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, @@ -791,10 +841,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, @@ -802,7 +853,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, @@ -834,9 +885,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, @@ -1101,7 +1149,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) @@ -1121,6 +1169,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) @@ -2363,8 +2417,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) @@ -3246,15 +3301,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 diff --git a/nova/tests/unit/compute/test_compute.py b/nova/tests/unit/compute/test_compute.py index 7e2d732522a3..82d6ef857a0c 100644 --- a/nova/tests/unit/compute/test_compute.py +++ b/nova/tests/unit/compute/test_compute.py @@ -11189,6 +11189,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'}} @@ -11561,78 +11563,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): diff --git a/nova/tests/unit/compute/test_compute_api.py b/nova/tests/unit/compute/test_compute_api.py index f9c22c5bff4f..b4c2e60373b0 100644 --- a/nova/tests/unit/compute/test_compute_api.py +++ b/nova/tests/unit/compute/test_compute_api.py @@ -2294,15 +2294,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 @@ -2321,7 +2322,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') @@ -2346,7 +2347,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', @@ -2379,7 +2380,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, diff --git a/nova/tests/unit/compute/test_compute_cells.py b/nova/tests/unit/compute/test_compute_cells.py index f3449ba54589..cf9f2aaf3c9d 100644 --- a/nova/tests/unit/compute/test_compute_cells.py +++ b/nova/tests/unit/compute/test_compute_cells.py @@ -330,11 +330,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') @@ -404,7 +406,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