From 1fa3101fceab27c8ac70c091bac0d9e7bcc184b4 Mon Sep 17 00:00:00 2001 From: Rick Harris Date: Mon, 20 May 2013 20:01:08 +0000 Subject: Move ImageTooLarge check to Compute API The existing ImageTooLarge check is in the compute manager, meaning that a validation error puts the instance into an ERROR state without the ability to communicate the fault back to the user. This patch moves the check to the API where we can return the appropriate error to the user. Change-Id: I2183449351e3b70f98d0552ea247184a3641d85a --- nova/api/openstack/compute/servers.py | 31 ++-- nova/compute/api.py | 15 +- nova/compute/manager.py | 62 +------ nova/tests/api/openstack/compute/test_servers.py | 58 ++++++ nova/tests/compute/test_compute.py | 216 ++++++++++++----------- 5 files changed, 208 insertions(+), 174 deletions(-) diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 44d8dce3b..512f8cb7a 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -902,32 +902,18 @@ class Controller(wsgi.Controller): raise exc.HTTPRequestEntityTooLarge( explanation=error.format_message(), headers={'Retry-After': 0}) - except exception.InstanceTypeMemoryTooSmall as error: - raise exc.HTTPBadRequest(explanation=error.format_message()) - except exception.InstanceTypeNotFound as error: - raise exc.HTTPBadRequest(explanation=error.format_message()) - except exception.InstanceTypeDiskTooSmall as error: - raise exc.HTTPBadRequest(explanation=error.format_message()) - except exception.InvalidMetadata as error: - raise exc.HTTPBadRequest(explanation=error.format_message()) except exception.InvalidMetadataSize as error: raise exc.HTTPRequestEntityTooLarge( explanation=error.format_message()) - except exception.InvalidRequest as error: - raise exc.HTTPBadRequest(explanation=error.format_message()) except exception.ImageNotFound as error: msg = _("Can not find requested image") raise exc.HTTPBadRequest(explanation=msg) - except exception.ImageNotActive as error: - raise exc.HTTPBadRequest(explanation=error.format_message()) except exception.FlavorNotFound as error: msg = _("Invalid flavorRef provided.") raise exc.HTTPBadRequest(explanation=msg) except exception.KeypairNotFound as error: msg = _("Invalid key_name provided.") raise exc.HTTPBadRequest(explanation=msg) - except exception.SecurityGroupNotFound as error: - raise exc.HTTPBadRequest(explanation=error.format_message()) except rpc_common.RemoteError as err: msg = "%(err_type)s: %(err_msg)s" % {'err_type': err.exc_type, 'err_msg': err.value} @@ -935,7 +921,15 @@ class Controller(wsgi.Controller): except UnicodeDecodeError as error: msg = "UnicodeError: %s" % unicode(error) raise exc.HTTPBadRequest(explanation=msg) - # Let the caller deal with unhandled exceptions. + except (exception.ImageNotActive, + exception.ImageTooLarge, + exception.InstanceTypeDiskTooSmall, + exception.InstanceTypeMemoryTooSmall, + exception.InstanceTypeNotFound, + exception.InvalidMetadata, + exception.InvalidRequest, + exception.SecurityGroupNotFound) as error: + raise exc.HTTPBadRequest(explanation=error.format_message()) # If the caller wanted a reservation_id, return it if ret_resv_id: @@ -1288,10 +1282,11 @@ class Controller(wsgi.Controller): except exception.ImageNotFound: msg = _("Cannot find image for rebuild") raise exc.HTTPBadRequest(explanation=msg) - except (exception.InvalidMetadata, - exception.InstanceTypeMemoryTooSmall, + except (exception.ImageNotActive, + exception.ImageTooLarge, exception.InstanceTypeDiskTooSmall, - exception.ImageNotActive) as error: + exception.InstanceTypeMemoryTooSmall, + exception.InvalidMetadata) as error: raise exc.HTTPBadRequest(explanation=error.format_message()) instance = self._get_server(context, req, id) diff --git a/nova/compute/api.py b/nova/compute/api.py index 12c1cf579..b5a69ea37 100644 --- a/nova/compute/api.py +++ b/nova/compute/api.py @@ -500,12 +500,23 @@ class API(base.Base): if not image: # Image checks don't apply when building from volume return + if image['status'] != 'active': raise exception.ImageNotActive(image_id=image_id) + if instance_type['memory_mb'] < int(image.get('min_ram') or 0): raise exception.InstanceTypeMemoryTooSmall() - if instance_type['root_gb'] < int(image.get('min_disk') or 0): - raise exception.InstanceTypeDiskTooSmall() + + # 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.ImageTooLarge() + + if int(image.get('min_disk') or 0) > root_gb: + raise exception.InstanceTypeDiskTooSmall() def _get_image(self, context, image_href): if not image_href: diff --git a/nova/compute/manager.py b/nova/compute/manager.py index 91a814f98..699007057 100755 --- a/nova/compute/manager.py +++ b/nova/compute/manager.py @@ -889,7 +889,13 @@ class ComputeManager(manager.SchedulerDependentManager): raise exception.BuildAbortException(instance_uuid=instance['uuid'], reason=msg) - return self._get_and_check_image_metadata(context, instance) + if instance['image_ref']: + image_meta = _get_image_meta(context, instance['image_ref']) + else: + # Instance was started from volume - so no image ref + image_meta = {} + + return image_meta def _build_instance(self, context, request_spec, filter_properties, requested_networks, injected_files, admin_password, is_first_time, @@ -1084,60 +1090,6 @@ class ComputeManager(manager.SchedulerDependentManager): if self.driver.instance_exists(instance['name']): raise exception.InstanceExists(name=instance['name']) - def _get_and_check_image_metadata(self, context, instance): - """Get the image metadata and do basic sanity checks on said image - like ensuring the image is smaller than the maximum size allowed by - the instance_type. - - The image stored in Glance is potentially compressed, so we use two - checks to ensure that the size isn't exceeded: - - 1) This one - checks compressed size, this a quick check to - eliminate any images which are obviously too large - - 2) Check uncompressed size in nova.virt.xenapi.vm_utils. This - is a slower check since it requires uncompressing the entire - image, but is accurate because it reflects the image's - actual size. - """ - if instance['image_ref']: - image_meta = _get_image_meta(context, instance['image_ref']) - else: # Instance was started from volume - so no image ref - return {} - - try: - size_bytes = int(image_meta['size']) - except (KeyError, TypeError, ValueError): - # Disregard missing field or bad data rather than put the instance - # into an ERROR state that can't be communicated back to the user - return image_meta - - instance_type = flavors.extract_instance_type(instance) - allowed_size_gb = instance_type['root_gb'] - - # 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 not allowed_size_gb: - return image_meta - - allowed_size_bytes = allowed_size_gb * 1024 * 1024 * 1024 - - image_id = image_meta['id'] - LOG.debug(_("image_id=%(image_id)s, image_size_bytes=" - "%(size_bytes)d, allowed_size_bytes=" - "%(allowed_size_bytes)d") % locals(), - instance=instance) - - if size_bytes > allowed_size_bytes: - LOG.info(_("Image '%(image_id)s' size %(size_bytes)d exceeded" - " instance_type allowed size " - "%(allowed_size_bytes)d") - % locals(), instance=instance) - raise exception.ImageTooLarge() - - return image_meta - def _start_building(self, context, instance): """Save the host and launched_on fields and log appropriately.""" LOG.audit(_('Starting instance...'), context=context, diff --git a/nova/tests/api/openstack/compute/test_servers.py b/nova/tests/api/openstack/compute/test_servers.py index 993451263..11eeff143 100644 --- a/nova/tests/api/openstack/compute/test_servers.py +++ b/nova/tests/api/openstack/compute/test_servers.py @@ -1451,6 +1451,35 @@ class ServersControllerTest(test.TestCase): self.assertRaises(webob.exc.HTTPBadRequest, self.controller._action_rebuild, req, FAKE_UUID, body) + def test_rebuild_instance_image_too_large(self): + # make image size larger than our instance disk size + size = str(1000 * (1024 ** 3)) + + def fake_get_image(self, context, image_href): + return dict(id='76fa36fc-c930-4bf3-8c8a-ea2a2420deb6', + name='public image', is_public=True, + status='active', size=size) + + self.stubs.Set(fake._FakeImageService, 'show', fake_get_image) + + self.stubs.Set(db, 'instance_get_by_uuid', + fakes.fake_instance_get(vm_state=vm_states.ACTIVE)) + image_uuid = '76fa36fc-c930-4bf3-8c8a-ea2a2420deb6' + image_href = 'http://localhost/v2/fake/images/%s' % image_uuid + body = { + 'rebuild': { + 'name': 'new_name', + 'imageRef': image_href, + }, + } + + req = fakes.HTTPRequest.blank('/v2/fake/servers/a/action') + req.method = 'POST' + req.body = jsonutils.dumps(body) + req.headers["content-type"] = "application/json" + self.assertRaises(webob.exc.HTTPBadRequest, + self.controller._action_rebuild, req, FAKE_UUID, body) + def test_rebuild_instance_with_deleted_image(self): def fake_get_image(self, context, image_href): return dict(id='76fa36fc-c930-4bf3-8c8a-ea2a2420deb6', @@ -1908,6 +1937,35 @@ class ServersControllerCreateTest(test.TestCase): 'Image 76fa36fc-c930-4bf3-8c8a-ea2a2420deb6 is not active.'): self.controller.create(req, body) + def test_create_server_image_too_large(self): + image_uuid = '76fa36fc-c930-4bf3-8c8a-ea2a2420deb6' + + # Get the fake image service so we can set the status to deleted + (image_service, image_id) = glance.get_remote_image_service( + context, image_uuid) + + image = image_service.show(context, image_id) + + orig_size = image['size'] + new_size = str(1000 * (1024 ** 3)) + image_service.update(context, image_uuid, {'size': new_size}) + + self.addCleanup(image_service.update, context, image_uuid, + {'size': orig_size}) + + req = fakes.HTTPRequest.blank('/v2/fake/servers') + req.method = 'POST' + body = dict(server=dict(name='server_test', + imageRef=image_uuid, + flavorRef=2)) + req.body = jsonutils.dumps(body) + + req.headers["content-type"] = "application/json" + with testtools.ExpectedException( + webob.exc.HTTPBadRequest, + 'Image is larger than instance type allows'): + self.controller.create(req, body) + def test_create_instance_invalid_negative_min(self): self.ext_mgr.extensions = {'os-multiple-create': 'fake'} image_href = '76fa36fc-c930-4bf3-8c8a-ea2a2420deb6' diff --git a/nova/tests/compute/test_compute.py b/nova/tests/compute/test_compute.py index 933d40d59..285261e18 100644 --- a/nova/tests/compute/test_compute.py +++ b/nova/tests/compute/test_compute.py @@ -4888,6 +4888,26 @@ class ComputeAPITestCase(BaseTestCase): inst_type, self.fake_image['id']) db.instance_destroy(self.context, refs[0]['uuid']) + def test_create_with_too_large_image(self): + # Test an instance type with too little disk space. + + inst_type = flavors.get_default_instance_type() + inst_type['root_gb'] = 1 + + self.fake_image['size'] = '1073741825' + + self.stubs.Set(fake_image._FakeImageService, 'show', self.fake_show) + + self.assertRaises(exception.ImageTooLarge, + self.compute_api.create, self.context, + inst_type, self.fake_image['id']) + + # Reduce image to 1 GB limit and ensure it works + self.fake_image['size'] = '1073741824' + (refs, resv_id) = self.compute_api.create(self.context, + inst_type, self.fake_image['id']) + db.instance_destroy(self.context, refs[0]['uuid']) + def test_create_just_enough_ram_and_disk(self): # Test an instance type with just enough ram and disk space. @@ -5642,6 +5662,29 @@ class ComputeAPITestCase(BaseTestCase): instance, self.fake_image['id'], 'new_password') db.instance_destroy(self.context, instance['uuid']) + def test_rebuild_with_too_large_image(self): + instance = jsonutils.to_primitive( + self._create_fake_instance(params={'image_ref': '1'})) + + def fake_extract_instance_type(_inst): + return dict(memory_mb=64, root_gb=1) + + self.stubs.Set(flavors, 'extract_instance_type', + fake_extract_instance_type) + + self.fake_image['size'] = '1073741825' + self.stubs.Set(fake_image._FakeImageService, 'show', self.fake_show) + + self.assertRaises(exception.ImageTooLarge, + self.compute_api.rebuild, self.context, + instance, self.fake_image['id'], 'new_password') + + # Reduce image to 1 GB limit and ensure it works + self.fake_image['size'] = '1073741824' + self.compute_api.rebuild(self.context, + instance, self.fake_image['id'], 'new_password') + db.instance_destroy(self.context, instance['uuid']) + def _stub_out_reboot(self, device_name): def fake_reboot_instance(rpcapi, context, instance, block_device_info, @@ -8910,105 +8953,6 @@ class ComputeInjectedFilesTestCase(BaseTestCase): injected_files=expected) -class GetAndCheckImageMetadataTest(test.TestCase): - NOT_PRESENT = object() - - def setUp(self): - super(GetAndCheckImageMetadataTest, self).setUp() - self.context = context.RequestContext('fakeuser', 'fakeproject') - self.compute = importutils.import_object(CONF.compute_manager) - - def assertCheckSucceedsWithWonkySize(self, size): - instance = {'uuid': 'fakeuuid', 'image_ref': 'fakeimage'} - image_meta = {'id': 'imageid'} - if size is not self.NOT_PRESENT: - image_meta['size'] = size - - self.mox.StubOutWithMock(compute_manager, '_get_image_meta') - compute_manager._get_image_meta(self.context, - instance['image_ref']).AndReturn(image_meta) - - self.mox.ReplayAll() - - image_meta2 = self.compute._get_and_check_image_metadata( - self.context, instance) - - self.assertEqual(image_meta, image_meta2) - - def test_size_not_present_on_image(self): - self.assertCheckSucceedsWithWonkySize(self.NOT_PRESENT) - - def test_invalid_image_size(self): - self.assertCheckSucceedsWithWonkySize('invalid') - - def test_image_size_present_but_empty(self): - self.assertCheckSucceedsWithWonkySize('') - - def test_image_size_is_none(self): - self.assertCheckSucceedsWithWonkySize(None) - - def test_root_gb_zero(self): - instance = {'uuid': 'fakeuuid', 'image_ref': 'fakeimage'} - self.mox.StubOutWithMock(flavors, 'extract_instance_type') - flavors.extract_instance_type(instance).AndReturn({'root_gb': 0}) - - self.mox.StubOutWithMock(compute_manager, '_get_image_meta') - - image_meta = {'id': 'imageid', 'size': '123'} - compute_manager._get_image_meta(self.context, - instance['image_ref']).AndReturn(image_meta) - - self.mox.ReplayAll() - - image_meta2 = self.compute._get_and_check_image_metadata( - self.context, instance) - - self.assertEqual(image_meta, image_meta2) - - def test_image_ref_is_none(self): - # Instance was started from volume - so no image ref - instance = {'uuid': 'fakeuuid', 'image_ref': None} - image_meta2 = self.compute._get_and_check_image_metadata( - self.context, instance) - - self.assertEqual({}, image_meta2) - - def test_image_not_too_large(self): - instance = {'uuid': 'fakeuuid', 'image_ref': 'fakeimage'} - - self.mox.StubOutWithMock(flavors, 'extract_instance_type') - flavors.extract_instance_type(instance).AndReturn({'root_gb': 1}) - - image_meta = {'id': 'imageid', 'size': '1073741824'} - self.mox.StubOutWithMock(compute_manager, '_get_image_meta') - compute_manager._get_image_meta(self.context, - instance['image_ref']).AndReturn(image_meta) - - self.mox.ReplayAll() - - image_meta2 = self.compute._get_and_check_image_metadata( - self.context, instance) - - self.assertEqual(image_meta, image_meta2) - - def test_image_too_large(self): - instance = {'uuid': 'fakeuuid', 'image_ref': 'fakeimage'} - - self.mox.StubOutWithMock(flavors, 'extract_instance_type') - flavors.extract_instance_type(instance).AndReturn({'root_gb': 1}) - - image_meta = {'id': 'imageid', 'size': '1073741825'} - self.mox.StubOutWithMock(compute_manager, '_get_image_meta') - compute_manager._get_image_meta(self.context, - instance['image_ref']).AndReturn(image_meta) - - self.mox.ReplayAll() - - self.assertRaises(exception.ImageTooLarge, - self.compute._get_and_check_image_metadata, - self.context, instance) - - class CheckConfigDriveTestCase(test.TestCase): # NOTE(sirp): `TestCase` is far too heavyweight for this test, this should # probably derive from a `test.FastTestCase` that omits DB and env @@ -9060,3 +9004,77 @@ class CheckConfigDriveTestCase(test.TestCase): def test_value_is_image_id(self): self.assertCheck((2, None), 2) + + +class CheckRequestedImageTestCase(test.TestCase): + def setUp(self): + super(CheckRequestedImageTestCase, self).setUp() + self.compute_api = compute.API() + self.context = context.RequestContext( + 'fake_user_id', 'fake_project_id') + + self.instance_type = flavors.get_default_instance_type() + self.instance_type['memory_mb'] = 64 + self.instance_type['root_gb'] = 1 + + def test_no_image_specified(self): + self.compute_api._check_requested_image(self.context, None, {}, + self.instance_type) + + 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['status'] = 'active' + self.compute_api._check_requested_image(self.context, image['id'], + image, self.instance_type) + + def test_image_min_ram_check(self): + image = dict(id='123', status='active', min_ram='65') + + self.assertRaises(exception.InstanceTypeMemoryTooSmall, + self.compute_api._check_requested_image, self.context, + image['id'], image, self.instance_type) + + image['min_ram'] = '64' + self.compute_api._check_requested_image(self.context, image['id'], + image, self.instance_type) + + def test_image_min_disk_check(self): + image = dict(id='123', status='active', min_disk='2') + + self.assertRaises(exception.InstanceTypeDiskTooSmall, + self.compute_api._check_requested_image, self.context, + image['id'], image, self.instance_type) + + image['min_disk'] = '1' + self.compute_api._check_requested_image(self.context, image['id'], + image, self.instance_type) + + def test_image_too_large(self): + image = dict(id='123', status='active', size='1073741825') + + self.assertRaises(exception.ImageTooLarge, + self.compute_api._check_requested_image, self.context, + image['id'], image, self.instance_type) + + image['size'] = '1073741824' + self.compute_api._check_requested_image(self.context, image['id'], + image, self.instance_type) + + 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) + + 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) -- cgit