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