Merge "Move ImageTooLarge check to Compute API"

5 files changed, 208 insertions, 174 deletions

diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py
index d6259be63..0816e0a61 100644
--- a/nova/api/openstack/compute/servers.py
+++ b/nova/api/openstack/compute/servers.py
@@ -905,32 +905,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}
@@ -938,7 +924,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:
@@ -1291,10 +1285,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 ca488a797..0315c8593 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',
@@ -1929,6 +1958,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)