Merge lp:~jaypipes/glance/bug704854 into lp:~glance-coresec/glance/cactus-trunk

[Jay Pipes]

This adds a test case for LP Bug 704854 -- Exception
raised by Registry server gets eaten by API server.

The test involves spinning up an API server and a registry
server, firing cURL and glance-upload against the API
server with invalid requests, and verifying that appropriate
exception messages are contained in the response.

I should probably rebase this commit considering all the previous
commits weren't actually addressing the issue. The fact that I
had glance-api and glance-registry installed on my local machine
was causing the test runs to improperly return a passing result.

It just so happens that the glance-api and glance-registry that
I had installed to my /usr/local/bin were the very same programs
that were from a previous branch I had locally where I fixed the
root cause of this issue, which was that the sqlalchemy @validates
decorator does NOT fire for *new* objects, only existing ones, which
resulted in image_create() improperly storing NULL data in type, name,
and other non-nullable fields in the database. This then set off
a domino effect which caused the next call from
glance.server._upload_and_activate() to die a horrible death due to
the @validates decorator then firing on the already-created Image
object. This horrible death was improperly being raised from the
glance.client as a BadRequest instead of exception.Invalid, which
caused the API server to ignore the text in the actual Invalid
exception coming from the registry server.

In short, this patch finally fixes the root of the problem by
placing a validate_image() function guard which throws exception.Invalid
for any invalid data coming into the _image_update() method in the db
API. It also adds a bunch of logging statements and ensures that
exceptions throughout the call stack between the API server to the
glance.registry.Client to the Registry server are properly handled and
that the text of those exceptions isn't thrown away willy-nilly.

[Rick Harris (rconradharris)]

Jay-

Sorry I'm late on reviewing this, overall this looks really good. Surprised by the decorator only validating on update (major WTF).

Femto-nits:

> 170 + type = values.get('type', None)

We probably shouldn't be using `type` as a variable name (since it's a builtin). `type_` seems ok, `image_type` seems better. (Was burned by this recently :-)

> 320 + logger.debug("Uploading image data for image %(image_id)s "
> 321 + "to %(store_name)s store"
> 322 + % {'image_id': image_id,
> 323 + 'store_name': image_store})

Certainly not a problem, but is there a reason you didn't use the ` % locals()` idiom here?

+ with tempfile.NamedTemporaryFile() as f:

Rather than just `f`, I think `temp_file` or `tmp` would be a little clearer and help with grep-ability.

(Should we eliminate single-letter variable names entirely since they are usually a bad idea? Should we special case the counting variables `i` and `j` since they are *soo* common/idiomatic?)

review: Needs Fixing

[Jay Pipes (jaypipes)]

> Jay-
>
> Sorry I'm late on reviewing this, overall this looks really good. Surprised by
> the decorator only validating on update (major WTF).
>
> Femto-nits:
>
> > 170 + type = values.get('type', None)
>
> We probably shouldn't be using `type` as a variable name (since it's a
> builtin). `type_` seems ok, `image_type` seems better. (Was burned by this
> recently :-)

ya.

> > 320 + logger.debug("Uploading image data for image %(image_id)s
> "
> > 321 + "to %(store_name)s store"
> > 322 + % {'image_id': image_id,
> > 323 + 'store_name': image_store})
>
> Certainly not a problem, but is there a reason you didn't use the ` %
> locals()` idiom here?

nope, will fix. :)

> + with tempfile.NamedTemporaryFile() as f:

Copied from Swift ;) I'll rename to temp_file.

> Rather than just `f`, I think `temp_file` or `tmp` would be a little clearer
> and help with grep-ability.
>
> (Should we eliminate single-letter variable names entirely since they are
> usually a bad idea? Should we special case the counting variables `i` and `j`
> since they are *soo* common/idiomatic?)

[Jay Pipes (jaypipes)]

fixes made. pls re-review.

[Rick Harris (rconradharris)]

Looks good, thanks for the fixes!

review: Approve

[Devin Carlen (devcamcar)]

lgtm!

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~jaypipes/glance/bug704854 into lp:glance failed. Below is the output from the failed tests.

running test
running egg_info
creating glance.egg-info
writing glance.egg-info/PKG-INFO
writing top-level names to glance.egg-info/top_level.txt
writing dependency_links to glance.egg-info/dependency_links.txt
writing manifest file 'glance.egg-info/SOURCES.txt'
reading manifest file 'glance.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching 'LICENSE'
warning: no files found matching 'ChangeLog'
warning: no files found matching 'tests/test_data.py'
writing manifest file 'glance.egg-info/SOURCES.txt'
running build_ext

Tests raises BadRequest for invalid store header ... ok
Tests raises BadRequest for invalid store header ... ok
Tests creates a queued image for no body and no loc header ... ok
test_delete_image (tests.unit.test_api.TestGlanceAPI) ... ok
test_delete_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
Test for HEAD /images/<ID> ... ok
test_show_image_basic (tests.unit.test_api.TestGlanceAPI) ... ok
test_show_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
Tests that the /images POST registry API creates the image ... ok
Tests proper exception is raised if a bad status is set ... ok
Tests that the /images DELETE registry API deletes the image ... ok
Tests proper exception is raised if attempt to delete non-existing ... ok
Tests that the /images/detail registry API returns ... ok
Tests that the /images registry API returns list of ... ok
Tests that the root registry API returns "index", ... ok
Tests that the /images PUT registry API updates the image ... ok
Tests proper exception is raised if attempt to update non-existing ... ok
Test ClientConnectionError raised ... ok
Tests proper exception is raised if image with ID already exists ... ok
Tests that we can add image metadata and returns the new id ... ok
Tests a bad status is set to a proper one by server ... ok
Tests BadRequest raised when supplying bad store name in meta ... ok
Tests can add image by passing image data as file ... ok
Tests can add image by passing image data as string ... ok
Tests add image by passing image data as string w/ no size attr ... ok
Tests that we can add image metadata with properties ... ok
Tests client returns image as queued ... ok
Tests that image metadata is deleted properly ... ok
Tests cannot delete non-existing image ... ok
Test a simple file backend retrieval works as expected ... ok
Tests that the detailed info about public images returned ... ok
Test correct set of public image returned ... ok
Tests that the detailed info about an image returned ... ok
Tests that NotFound is raised when getting a non-existing image ... ok
Test retrieval of a non-existing image returns a 404 ... ok
Tests that the /images PUT registry API updates the image ... ok
Tests non existing image update doesn't work ... ok
Tests proper exception is raised if image with ID already exists ... ok
Tests that we can add image metadata and returns the new id ... ok
Tests proper exception is raised if a bad status is set ... ok
Tests that we can add image metadata with properties ... ok
Tests t...

[Jay Pipes (jaypipes)]

I'm getting SERIOUSLY annoyed with not being able to reproduce shit that Hudson keeps barfing on. Grr.

[Jay Pipes (jaypipes)]

approving debugging statements in test...

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~jaypipes/glance/bug704854 into lp:glance failed. Below is the output from the failed tests.

running test
running egg_info
creating glance.egg-info
writing glance.egg-info/PKG-INFO
writing top-level names to glance.egg-info/top_level.txt
writing dependency_links to glance.egg-info/dependency_links.txt
writing manifest file 'glance.egg-info/SOURCES.txt'
reading manifest file 'glance.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching 'LICENSE'
warning: no files found matching 'ChangeLog'
warning: no files found matching 'tests/test_data.py'
writing manifest file 'glance.egg-info/SOURCES.txt'
running build_ext

Tests raises BadRequest for invalid store header ... ok
Tests raises BadRequest for invalid store header ... ok
Tests creates a queued image for no body and no loc header ... ok
test_delete_image (tests.unit.test_api.TestGlanceAPI) ... ok
test_delete_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
Test for HEAD /images/<ID> ... ok
test_show_image_basic (tests.unit.test_api.TestGlanceAPI) ... ok
test_show_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
Tests that the /images POST registry API creates the image ... ok
Tests proper exception is raised if a bad status is set ... ok
Tests that the /images DELETE registry API deletes the image ... ok
Tests proper exception is raised if attempt to delete non-existing ... ok
Tests that the /images/detail registry API returns ... ok
Tests that the /images registry API returns list of ... ok
Tests that the root registry API returns "index", ... ok
Tests that the /images PUT registry API updates the image ... ok
Tests proper exception is raised if attempt to update non-existing ... ok
Test ClientConnectionError raised ... ok
Tests proper exception is raised if image with ID already exists ... ok
Tests that we can add image metadata and returns the new id ... ok
Tests a bad status is set to a proper one by server ... ok
Tests BadRequest raised when supplying bad store name in meta ... ok
Tests can add image by passing image data as file ... ok
Tests can add image by passing image data as string ... ok
Tests add image by passing image data as string w/ no size attr ... ok
Tests that we can add image metadata with properties ... ok
Tests client returns image as queued ... ok
Tests that image metadata is deleted properly ... ok
Tests cannot delete non-existing image ... ok
Test a simple file backend retrieval works as expected ... ok
Tests that the detailed info about public images returned ... ok
Test correct set of public image returned ... ok
Tests that the detailed info about an image returned ... ok
Tests that NotFound is raised when getting a non-existing image ... ok
Test retrieval of a non-existing image returns a 404 ... ok
Tests that the /images PUT registry API updates the image ... ok
Tests non existing image update doesn't work ... ok
Tests proper exception is raised if image with ID already exists ... ok
Tests that we can add image metadata and returns the new id ... ok
Tests proper exception is raised if a bad status is set ... ok
Tests that we can add image metadata with properties ... ok
Tests t...

[Vish Ishaya (vishvananda)]

argparse isn't in python 2.6, no?
On Feb 23, 2011, at 12:48 PM, OpenStack Hudson wrote:

> The attempt to merge lp:~jaypipes/glance/bug704854 into lp:glance failed. Below is the output from the failed tests.
>
> running test
> running egg_info
> creating glance.egg-info
> writing glance.egg-info/PKG-INFO
> writing top-level names to glance.egg-info/top_level.txt
> writing dependency_links to glance.egg-info/dependency_links.txt
> writing manifest file 'glance.egg-info/SOURCES.txt'
> reading manifest file 'glance.egg-info/SOURCES.txt'
> reading manifest template 'MANIFEST.in'
> warning: no files found matching 'LICENSE'
> warning: no files found matching 'ChangeLog'
> warning: no files found matching 'tests/test_data.py'
> writing manifest file 'glance.egg-info/SOURCES.txt'
> running build_ext
>
> Tests raises BadRequest for invalid store header ... ok
> Tests raises BadRequest for invalid store header ... ok
> Tests creates a queued image for no body and no loc header ... ok
> test_delete_image (tests.unit.test_api.TestGlanceAPI) ... ok
> test_delete_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
> Test for HEAD /images/<ID> ... ok
> test_show_image_basic (tests.unit.test_api.TestGlanceAPI) ... ok
> test_show_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
> Tests that the /images POST registry API creates the image ... ok
> Tests proper exception is raised if a bad status is set ... ok
> Tests that the /images DELETE registry API deletes the image ... ok
> Tests proper exception is raised if attempt to delete non-existing ... ok
> Tests that the /images/detail registry API returns ... ok
> Tests that the /images registry API returns list of ... ok
> Tests that the root registry API returns "index", ... ok
> Tests that the /images PUT registry API updates the image ... ok
> Tests proper exception is raised if attempt to update non-existing ... ok
> Test ClientConnectionError raised ... ok
> Tests proper exception is raised if image with ID already exists ... ok
> Tests that we can add image metadata and returns the new id ... ok
> Tests a bad status is set to a proper one by server ... ok
> Tests BadRequest raised when supplying bad store name in meta ... ok
> Tests can add image by passing image data as file ... ok
> Tests can add image by passing image data as string ... ok
> Tests add image by passing image data as string w/ no size attr ... ok
> Tests that we can add image metadata with properties ... ok
> Tests client returns image as queued ... ok
> Tests that image metadata is deleted properly ... ok
> Tests cannot delete non-existing image ... ok
> Test a simple file backend retrieval works as expected ... ok
> Tests that the detailed info about public images returned ... ok
> Test correct set of public image returned ... ok
> Tests that the detailed info about an image returned ... ok
> Tests that NotFound is raised when getting a non-existing image ... ok
> Test retrieval of a non-existing image returns a 404 ... ok
> Tests that the /images PUT registry API updates the image ... ok
> Tests non existing image update doesn't work ... ok
> Tests proper exception is raised if image with ID already exists ... o...

[Jay Pipes (jaypipes)]

No it's not.

On Wed, Feb 23, 2011 at 3:58 PM, Vish Ishaya <email address hidden> wrote:
> argparse isn't in python 2.6, no?
> On Feb 23, 2011, at 12:48 PM, OpenStack Hudson wrote:
>
>> The attempt to merge lp:~jaypipes/glance/bug704854 into lp:glance failed. Below is the output from the failed tests.
>>
>> running test
>> running egg_info
>> creating glance.egg-info
>> writing glance.egg-info/PKG-INFO
>> writing top-level names to glance.egg-info/top_level.txt
>> writing dependency_links to glance.egg-info/dependency_links.txt
>> writing manifest file 'glance.egg-info/SOURCES.txt'
>> reading manifest file 'glance.egg-info/SOURCES.txt'
>> reading manifest template 'MANIFEST.in'
>> warning: no files found matching 'LICENSE'
>> warning: no files found matching 'ChangeLog'
>> warning: no files found matching 'tests/test_data.py'
>> writing manifest file 'glance.egg-info/SOURCES.txt'
>> running build_ext
>>
>> Tests raises BadRequest for invalid store header ... ok
>> Tests raises BadRequest for invalid store header ... ok
>> Tests creates a queued image for no body and no loc header ... ok
>> test_delete_image (tests.unit.test_api.TestGlanceAPI) ... ok
>> test_delete_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
>> Test for HEAD /images/<ID> ... ok
>> test_show_image_basic (tests.unit.test_api.TestGlanceAPI) ... ok
>> test_show_non_exists_image (tests.unit.test_api.TestGlanceAPI) ... ok
>> Tests that the /images POST registry API creates the image ... ok
>> Tests proper exception is raised if a bad status is set ... ok
>> Tests that the /images DELETE registry API deletes the image ... ok
>> Tests proper exception is raised if attempt to delete non-existing ... ok
>> Tests that the /images/detail registry API returns ... ok
>> Tests that the /images registry API returns list of ... ok
>> Tests that the root registry API returns "index", ... ok
>> Tests that the /images PUT registry API updates the image ... ok
>> Tests proper exception is raised if attempt to update non-existing ... ok
>> Test ClientConnectionError raised ... ok
>> Tests proper exception is raised if image with ID already exists ... ok
>> Tests that we can add image metadata and returns the new id ... ok
>> Tests a bad status is set to a proper one by server ... ok
>> Tests BadRequest raised when supplying bad store name in meta ... ok
>> Tests can add image by passing image data as file ... ok
>> Tests can add image by passing image data as string ... ok
>> Tests add image by passing image data as string w/ no size attr ... ok
>> Tests that we can add image metadata with properties ... ok
>> Tests client returns image as queued ... ok
>> Tests that image metadata is deleted properly ... ok
>> Tests cannot delete non-existing image ... ok
>> Test a simple file backend retrieval works as expected ... ok
>> Tests that the detailed info about public images returned ... ok
>> Test correct set of public image returned ... ok
>> Tests that the detailed info about an image returned ... ok
>> Tests that NotFound is raised when getting a non-existing image ... ok
>> Test retrieval of a non-existing image returns a 404 ... ok
>> Tests that the /images PUT registry API updat...

=== modified file 'MANIFEST.in'
--- MANIFEST.in	2011-02-02 01:43:16 +0000
+++ MANIFEST.in	2011-02-23 20:37:56 +0000
@@ -8,4 +8,5 @@
 include run_tests.py
 include glance/registry/db/migrate_repo/migrate.cfg
 graft doc
+graft etc
 graft tools

=== modified file 'bin/glance-api'
--- bin/glance-api	2011-02-09 20:24:59 +0000
+++ bin/glance-api	2011-02-23 20:37:56 +0000
@@ -26,9 +26,13 @@
 import os
 import sys
 
-ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
-
-sys.path.append(ROOT_DIR)
+# If ../glance/__init__.py exists, add ../ to Python search path, so that
+# it will override what happens to be installed in /usr/(local/)lib/python...
+possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]),
+                                   os.pardir,
+                                   os.pardir))
+if os.path.exists(os.path.join(possible_topdir, 'glance', '__init__.py')):
+    sys.path.insert(0, possible_topdir)
 
 from glance import version
 from glance.common import config

=== modified file 'bin/glance-combined'
--- bin/glance-combined	2011-02-09 20:24:59 +0000
+++ bin/glance-combined	2011-02-23 20:37:56 +0000
@@ -28,9 +28,13 @@
 import os
 import sys
 
-ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
-
-sys.path.append(ROOT_DIR)
+# If ../glance/__init__.py exists, add ../ to Python search path, so that
+# it will override what happens to be installed in /usr/(local/)lib/python...
+possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]),
+                                   os.pardir,
+                                   os.pardir))
+if os.path.exists(os.path.join(possible_topdir, 'glance', '__init__.py')):
+    sys.path.insert(0, possible_topdir)
 
 from glance import version
 from glance.common import config

=== modified file 'bin/glance-control'
--- bin/glance-control	2011-02-16 07:43:11 +0000
+++ bin/glance-control	2011-02-23 20:37:56 +0000
@@ -31,6 +31,14 @@
 import sys
 import time
 
+# If ../glance/__init__.py exists, add ../ to Python search path, so that
+# it will override what happens to be installed in /usr/(local/)lib/python...
+possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]),
+                                   os.pardir,
+                                   os.pardir))
+if os.path.exists(os.path.join(possible_topdir, 'glance', '__init__.py')):
+    sys.path.insert(0, possible_topdir)
+
 from glance import version
 from glance.common import config
 

=== modified file 'bin/glance-manage'
--- bin/glance-manage	2011-02-02 18:47:08 +0000
+++ bin/glance-manage	2011-02-23 20:37:56 +0000
@@ -30,9 +30,13 @@
 import os
 import sys
 
-ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
-
-sys.path.append(ROOT_DIR)
+# If ../glance/__init__.py exists, add ../ to Python search path, so that
+# it will override what happens to be installed in /usr/(local/)lib/python...
+possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]),
+                                   os.pardir,
+                                   os.pardir))
+if os.path.exists(os.path.join(possible_topdir, 'glance', '__init__.py')):
+    sys.path.insert(0, possible_topdir)
 
 from glance import version as glance_version
 from glance.common import config

=== modified file 'bin/glance-registry'
--- bin/glance-registry	2011-02-09 20:24:59 +0000
+++ bin/glance-registry	2011-02-23 20:37:56 +0000
@@ -26,9 +26,13 @@
 import os
 import sys
 
-ROOT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
-
-sys.path.append(ROOT_DIR)
+# If ../glance/__init__.py exists, add ../ to Python search path, so that
+# it will override what happens to be installed in /usr/(local/)lib/python...
+possible_topdir = os.path.normpath(os.path.join(os.path.abspath(sys.argv[0]),
+                                   os.pardir,
+                                   os.pardir))
+if os.path.exists(os.path.join(possible_topdir, 'glance', '__init__.py')):
+    sys.path.insert(0, possible_topdir)
 
 from glance import version
 from glance.common import config

=== modified file 'bin/glance-upload'
--- bin/glance-upload	2011-02-02 01:43:16 +0000
+++ bin/glance-upload	2011-02-23 20:37:56 +0000
@@ -53,10 +53,12 @@
     parser.add_argument('filename', help='file to upload into Glance')
     parser.add_argument('name', help='name of image')
     parser.add_argument('--host', metavar='HOST', default='127.0.0.1',
-                        help='Location of Glance Server (default: 127.0.0.1)')
+                        help='Location of Glance Server (default: %default)')
+    parser.add_argument('--port', metavar='PORT', type=int, default=9292,
+                        help='Port of Glance Server (default: %default)')
     parser.add_argument('--type', metavar='TYPE', default='raw',
                         help='Type of Image [kernel, ramdisk, machine, raw] '
-                             '(default: raw)')
+                             '(default: %default)')
     parser.add_argument('--kernel', metavar='KERNEL',
                         help='ID of kernel associated with this machine image')
     parser.add_argument('--ramdisk', metavar='RAMDISK',
@@ -77,7 +79,7 @@
         else:
             die("kernel and ramdisk required for machine image")
 
-    client = Client(args.host, 9292)
+    client = Client(args.host, args.port)
     with open(args.filename) as f:
         new_meta = client.add_image(meta, f)
 

=== modified file 'glance/client.py'
--- glance/client.py	2011-02-05 02:32:31 +0000
+++ glance/client.py	2011-02-23 20:37:56 +0000
@@ -153,7 +153,7 @@
             elif status_code == httplib.CONFLICT:
                 raise exception.Duplicate(res.read())
             elif status_code == httplib.BAD_REQUEST:
-                raise exception.BadInputError(res.read())
+                raise exception.Invalid(res.read())
             else:
                 raise Exception("Unknown error occurred! %s" % res.__dict__)
 

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py	2011-02-11 00:12:51 +0000
+++ glance/registry/db/api.py	2011-02-23 20:37:56 +0000
@@ -144,6 +144,30 @@
             del values[attr]
 
 
+def validate_image(values):
+    """
+    Validates the incoming data and raises a Invalid exception
+    if anything is out of order.
+
+    :param values: Mapping of image metadata to check
+    """
+
+    image_type = values.get('type', None)
+    if not image_type:
+        msg = "Image type is required."
+        raise exception.Invalid(msg)
+    if image_type not in ('machine', 'kernel', 'ramdisk', 'raw', 'vhd'):
+        msg = "Invalid image type '%s' for image." % image_type
+        raise exception.Invalid(msg)
+    status = values.get('status', None)
+    if not status:
+        msg = "Image status is required."
+        raise exception.Invalid(msg)
+    if status not in ('active', 'queued', 'killed', 'saving'):
+        msg = "Invalid image status '%s' for image." % status
+        raise exception.Invalid(msg)
+
+
 def _image_update(context, values, image_id):
     """Used internally by image_create and image_update
 
@@ -151,6 +175,12 @@
     :param values: A dict of attributes to set
     :param image_id: If None, create the image, otherwise, find and update it
     """
+
+    # Validate the attributes before we go any further. From my investigation,
+    # the @validates decorator does not validate on new records, only on
+    # existing records, which is, well, idiotic.
+    validate_image(values)
+
     session = get_session()
     with session.begin():
         _drop_protected_attrs(models.Image, values)

=== modified file 'glance/registry/db/models.py'
--- glance/registry/db/models.py	2011-02-17 18:34:28 +0000
+++ glance/registry/db/models.py	2011-02-23 20:37:56 +0000
@@ -101,19 +101,6 @@
     is_public = Column(Boolean, nullable=False, default=False)
     location = Column(Text)
 
-    @validates('type')
-    def validate_type(self, key, type):
-        if not type in ('machine', 'kernel', 'ramdisk', 'raw', 'vhd'):
-            raise exception.Invalid(
-                "Invalid image type '%s' for image." % type)
-        return type
-
-    @validates('status')
-    def validate_status(self, key, status):
-        if not status in ('active', 'queued', 'killed', 'saving'):
-            raise exception.Invalid("Invalid status '%s' for image." % status)
-        return status
-
 
 class ImageProperty(BASE, ModelBase):
     """Represents an image properties in the datastore"""

=== modified file 'glance/registry/server.py'
--- glance/registry/server.py	2011-02-03 15:23:48 +0000
+++ glance/registry/server.py	2011-02-23 20:37:56 +0000
@@ -19,6 +19,7 @@
 """
 
 import json
+import logging
 
 import routes
 from webob import exc
@@ -28,6 +29,9 @@
 from glance.registry.db import api as db_api
 
 
+logger = logging.getLogger('glance.registry.server')
+
+
 class Controller(wsgi.Controller):
     """Controller for the reference implementation registry server"""
 
@@ -118,9 +122,13 @@
             image_data = db_api.image_create(context, image_data)
             return dict(image=make_image_dict(image_data))
         except exception.Duplicate:
-            return exc.HTTPConflict()
-        except exception.Invalid:
-            return exc.HTTPBadRequest()
+            msg = ("Image with identifier %s already exists!" % id)
+            logger.error(msg)
+            return exc.HTTPConflict(msg)
+        except exception.Invalid, e:
+            msg = ("Failed to add image metadata. Got error: %(e)s" % locals())
+            logger.error(msg)
+            return exc.HTTPBadRequest(msg)
 
     def update(self, req, id):
         """Updates an existing image with the registry.
@@ -140,7 +148,9 @@
             updated_image = db_api.image_update(context, id, image_data)
             return dict(image=make_image_dict(updated_image))
         except exception.NotFound:
-            return exc.HTTPNotFound()
+            raise exc.HTTPNotFound(body='Image not found',
+                               request=req,
+                               content_type='text/plain')
 
 
 class API(wsgi.Router):

=== modified file 'glance/server.py'
--- glance/server.py	2011-02-16 10:01:21 +0000
+++ glance/server.py	2011-02-23 20:37:56 +0000
@@ -49,6 +49,9 @@
 from glance import utils
 
 
+logger = logging.getLogger('glance.server')
+
+
 class Controller(wsgi.Controller):
 
     """
@@ -188,10 +191,13 @@
         except exception.Duplicate:
             msg = "An image with identifier %s already exists"\
                   % image_meta['id']
-            logging.error(msg)
-            raise HTTPConflict(msg, request=req)
-        except exception.Invalid:
-            raise HTTPBadRequest()
+            logger.error(msg)
+            raise HTTPConflict(msg, request=req, content_type="text/plain")
+        except exception.Invalid, e:
+            msg = ("Failed to reserve image. Got error: %(e)s" % locals())
+            for line in msg.split('\n'):
+                logger.error(line)
+            raise HTTPBadRequest(msg, request=req, content_type="text/plain")
 
     def _upload(self, req, image_meta):
         """
@@ -211,17 +217,22 @@
             raise HTTPBadRequest(
                 "Content-Type must be application/octet-stream")
 
-        image_store = req.headers.get(
+        store_name = req.headers.get(
             'x-image-meta-store', self.options['default_store'])
 
-        store = self.get_store_or_400(req, image_store)
+        store = self.get_store_or_400(req, store_name)
 
         image_meta['status'] = 'saving'
+        image_id = image_meta['id']
+        logger.debug("Updating image metadata for image %s"
+                     % image_id)
         registry.update_image_metadata(self.options,
                                        image_meta['id'],
                                        image_meta)
 
         try:
+            logger.debug("Uploading image data for image %(image_id)s "
+                         "to %(store_name)s store" % locals())
             location, size = store.add(image_meta['id'],
                                        req.body_file,
                                        self.options)
@@ -230,12 +241,14 @@
             # the new size of the image
             if image_meta.get('size', 0) != size:
                 image_meta['size'] = size
+                logger.debug("Updating image metadata for image %s"
+                             % image_id)
                 registry.update_image_metadata(self.options,
                                                image_meta['id'],
                                                image_meta)
             return location
         except exception.Duplicate, e:
-            logging.error("Error adding image to store: %s", str(e))
+            logger.error("Error adding image to store: %s", str(e))
             raise HTTPConflict(str(e), request=req)
 
     def _activate(self, req, image_meta, location):
@@ -277,7 +290,7 @@
         try:
             self._kill(req, image_meta)
         except Exception, e:
-            logging.error("Unable to kill image %s: %s",
+            logger.error("Unable to kill image %s: %s",
                           image_meta['id'], repr(e))
 
     def _upload_and_activate(self, req, image_meta):
@@ -367,14 +380,21 @@
             raise HTTPConflict("Cannot upload to an unqueued image")
 
         new_image_meta = utils.get_image_meta_from_headers(req)
-        image_meta = registry.update_image_metadata(self.options,
-                                                    id,
-                                                    new_image_meta)
-
-        if has_body:
-            self._upload_and_activate(req, image_meta)
-
-        return dict(image=image_meta)
+        try:
+            image_meta = registry.update_image_metadata(self.options,
+                                                        id,
+                                                        new_image_meta)
+
+            if has_body:
+                self._upload_and_activate(req, image_meta)
+
+            return dict(image=image_meta)
+        except exception.Invalid, e:
+            msg = ("Failed to update image metadata. Got error: %(e)s"
+                   % locals())
+            for line in msg.split('\n'):
+                logger.error(line)
+            raise HTTPBadRequest(msg, request=req, content_type="text/plain")
 
     def delete(self, req, id):
         """
@@ -407,8 +427,9 @@
         try:
             return registry.get_image_metadata(self.options, id)
         except exception.NotFound:
-            raise HTTPNotFound(body='Image not found',
-                               request=request,
+            msg = "Image with identifier %s not found" % id
+            logger.debug(msg)
+            raise HTTPNotFound(msg, request=request,
                                content_type='text/plain')
 
     def get_store_or_400(self, request, store_name):
@@ -424,10 +445,10 @@
         try:
             return get_backend_class(store_name)
         except UnsupportedBackend:
-            raise HTTPBadRequest(body='Requested store %s not available '
-                                 'for storage on this Glance node'
-                                 % store_name,
-                                 request=request,
+            msg = ("Requested store %s not available on this Glance server"
+                   % store_name)
+            logger.error(msg)
+            raise HTTPBadRequest(msg, request=request,
                                  content_type='text/plain')
 
 

=== modified file 'tests/unit/test_clients.py'
--- tests/unit/test_clients.py	2011-01-28 21:54:34 +0000
+++ tests/unit/test_clients.py	2011-02-23 20:37:56 +0000
@@ -204,7 +204,7 @@
                    'location': "file:///tmp/glance-tests/2",
                   }
 
-        self.assertRaises(exception.BadInputError,
+        self.assertRaises(exception.Invalid,
                           self.client.add_image,
                           fixture)
 
@@ -567,7 +567,7 @@
 
         image_data_fixture = r"chunk00000remainder"
 
-        self.assertRaises(exception.BadInputError,
+        self.assertRaises(exception.Invalid,
                           self.client.add_image,
                           fixture,
                           image_data_fixture)

=== added file 'tests/unit/test_misc.py'
--- tests/unit/test_misc.py	1970-01-01 00:00:00 +0000
+++ tests/unit/test_misc.py	2011-02-23 20:37:56 +0000
@@ -0,0 +1,178 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack, LLC
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import os
+import shutil
+import signal
+import subprocess
+import tempfile
+import time
+import unittest
+
+
+def execute(cmd):
+    env = os.environ.copy()
+    # Make sure that we use the programs in the
+    # current source directory's bin/ directory.
+    env['PATH'] = os.path.join(os.getcwd(), 'bin') + ':' + env['PATH']
+    process = subprocess.Popen(cmd,
+                               shell=True,
+                               stdin=subprocess.PIPE,
+                               stdout=subprocess.PIPE,
+                               stderr=subprocess.PIPE,
+                               env=env)
+    result = process.communicate()
+    (out, err) = result
+    exitcode = process.returncode
+    if process.returncode != 0:
+        msg = "Command %(cmd)s did not succeed. Returned an exit "\
+              "code of %(exitcode)d."\
+              "\n\nSTDOUT: %(out)s"\
+              "\n\nSTDERR: %(err)s" % locals()
+        raise RuntimeError(msg)
+    return exitcode, out, err
+
+
+class TestMiscellaneous(unittest.TestCase):
+
+    """Some random tests for various bugs and stuff"""
+
+    def tearDown(self):
+        self._cleanup_test_servers()
+
+    def _cleanup_test_servers(self):
+        # Clean up any leftover test servers...
+        pid_files = ('glance-api.pid', 'glance-registry.pid')
+        for pid_file in pid_files:
+            if os.path.exists(pid_file):
+                pid = int(open(pid_file).read().strip())
+                try:
+                    os.killpg(pid, signal.SIGTERM)
+                except:
+                    pass  # Ignore if the process group is dead
+                os.unlink(pid_file)
+
+    def test_exception_not_eaten_from_registry_to_api(self):
+        """
+        A test for LP bug #704854 -- Exception thrown by registry
+        server is consumed by API server.
+
+        We start both servers daemonized.
+
+        We then use curl to try adding an image that does not
+        meet validation requirements on the registry server and test
+        that the error returned from the API server to curl is appropriate
+
+        We also fire the glance-upload tool against the API server
+        and verify that glance-upload doesn't eat the exception either...
+        """
+
+        self._cleanup_test_servers()
+
+        # Port numbers hopefully not used by anything...
+        api_port = 32001
+        reg_port = 32000
+        image_dir = "/tmp/test.images.%d" % api_port
+        if os.path.exists(image_dir):
+            shutil.rmtree(image_dir)
+
+        # A config file to use just for this test...we don't want
+        # to trample on currently-running Glance servers, now do we?
+        with tempfile.NamedTemporaryFile() as conf_file:
+            conf_contents = """[DEFAULT]
+verbose = True
+debug = True
+
+[app:glance-api]
+paste.app_factory = glance.server:app_factory
+filesystem_store_datadir=%(image_dir)s
+default_store = file
+bind_host = 0.0.0.0
+bind_port = %(api_port)s
+registry_host = 0.0.0.0
+registry_port = %(reg_port)s
+
+[app:glance-registry]
+paste.app_factory = glance.registry.server:app_factory
+bind_host = 0.0.0.0
+bind_port = %(reg_port)s
+sql_connection = sqlite://
+sql_idle_timeout = 3600
+""" % locals()
+            conf_file.write(conf_contents)
+            conf_file.flush()
+            conf_file_name = conf_file.name
+
+            venv = ""
+            if 'VIRTUAL_ENV' in os.environ:
+                venv = "tools/with_venv.sh "
+
+            # Start up the API and default registry server
+            cmd = venv + "./bin/glance-control api start "\
+                         "%s --pid-file=glance-api.pid" % conf_file_name
+            exitcode, out, err = execute(cmd)
+
+            self.assertEquals(0, exitcode)
+            self.assertTrue("Starting glance-api with" in out)
+
+            cmd = venv + "./bin/glance-control registry start "\
+                         "%s --pid-file=glance-registry.pid" % conf_file_name
+            exitcode, out, err = execute(cmd)
+
+            self.assertEquals(0, exitcode)
+            self.assertTrue("Starting glance-registry with" in out)
+
+            time.sleep(2)  # Gotta give some time for spin up...
+
+            cmd = "curl -g http://0.0.0.0:%d/images" % api_port
+
+            exitcode, out, err = execute(cmd)
+
+            self.assertEquals(0, exitcode)
+            self.assertEquals('{"images": []}', out.strip())
+
+            cmd = "curl -X POST -H 'Content-Type: application/octet-stream' "\
+                  "-dinvalid http://0.0.0.0:%d/images" % api_port
+            ignored, out, err = execute(cmd)
+
+            self.assertTrue('Image type is required' in out,
+                            "Could not find 'Image type is required' "
+                            "in output: %s" % out)
+
+            cmd = "./bin/glance-upload --port=%(api_port)d "\
+                  "--type=invalid %(conf_file_name)s 'my image'" % locals()
+
+            # Normally, we would simply self.assertRaises() here, but
+            # we also want to check that the Invalid image type is in
+            # the exception text...
+            hit_exception = False
+            try:
+                ignored, out, err = execute(cmd)
+            except RuntimeError, e:
+                hit_exception = True
+                self.assertTrue('Invalid image type' in str(e),
+                                "Could not find 'Invalid image type' in "
+                                "result from glance-upload:\n%(e)s" % locals())
+            self.assertTrue(hit_exception)
+
+            # Spin down the API and default registry server
+            cmd = "./bin/glance-control api stop "\
+                  "%s --pid-file=glance-api.pid" % conf_file_name
+            ignored, out, err = execute(cmd)
+            cmd = "./bin/glance-control registry stop "\
+                  "%s --pid-file=glance-registry.pid" % conf_file_name
+            ignored, out, err = execute(cmd)