Merge lp:~justin-fathomdb/nova/bug607541 into lp:~hudson-openstack/nova/trunk

[justinsb]

Fix bug 607541. Exceptions mapped to 404/401 error codes as they were before the twisted rewrite.

[Jesse Andrews (anotherjesse)]

lgtm

we might want the body content to have a human readable message eventually...

review: Approve

[justinsb (justin-fathomdb)]

Re: Human Readable; I agree, but we should probably try to match S3 if that's what we're emulating here.

e.g. euca-upload-bundle performs this check instead of checking the response code directly:
 except S3ResponseError, s3error:
        s3error_string = '%s' % (s3error)
        if (s3error_string.find("404") >= 0):

I'd imagine other clients might also have written similarly bizarre logic. I'm not sure exactly what S3 does.

Anyway, this patch is really just restoring the behavior that was there before the twisted rewrite.

Long term, I'd hope that we'd determine our own better way of uploading images than the EC2/S3 approach.

[Todd Willey (xtoddx)]

We typically make TODO(name): comments so that they have a clear owner. If you don't want it, and nobody in IRC wants it, I'd be okay with TODO(unassigned): or somesuch. Not sure how everyone else feels about it though.

Otherwise lgtm

[Vish Ishaya (vishvananda)]

agreed on the TODO note. Plz make the change and I'll approve for merging

[Jay Pipes (jaypipes)]

Hi!

Nice work, Justin!

One note, though, is that I don't see a test case included that verifies the bug and the fix. Could you add one to /nova/tests/objectstore_unittest.py?

If you need assistance creating the test case, gimme a holla on IRC.

Cheers!
jay

review: Needs Fixing

[lp:~justin-fathomdb/nova/bug607541]

153. By justinsb on 2010-07-22

Nobody wants to take on this twisted cleanup. It works for now, but could be much nicer if twisted has a nice hook-point for exception mapping

[Soren Hansen (soren)]

> We typically make TODO(name): comments so that they have a clear owner. If
> you don't want it, and nobody in IRC wants it, I'd be okay with
> TODO(unassigned): or somesuch. Not sure how everyone else feels about it
> though.

Any reason not to just use the bug tracker for this?

[Soren Hansen (soren)]

I'm happy to accept ownership of the twisted cleanup. I've just been caught up at OSCON and traveling and whatnot. Normalcy should be restored starting tomorrow. :)

=== modified file 'nova/objectstore/handler.py'
--- nova/objectstore/handler.py	2010-07-20 01:26:19 +0000
+++ nova/objectstore/handler.py	2010-07-22 18:53:45 +0000
@@ -116,7 +116,21 @@
         logging.debug("Authentication Failure: %s" % ex)
         raise exception.NotAuthorized
 
-class S3(Resource):
+class ErrorHandlingResource(Resource):
+    """Maps exceptions to 404 / 401 codes.  Won't work for exceptions thrown after NOT_DONE_YET is returned."""
+    # TODO(unassigned) (calling-all-twisted-experts): This needs to be plugged in to the right place in twisted...
+    #   This doesn't look like it's the right place (consider exceptions in getChild; or after NOT_DONE_YET is returned     
+    def render(self, request):
+        try:
+            return Resource.render(self, request)
+        except exception.NotFound:
+            request.setResponseCode(404)
+            return ''
+        except exception.NotAuthorized:
+            request.setResponseCode(403)
+            return ''
+
+class S3(ErrorHandlingResource):
     """Implementation of an S3-like storage server based on local files."""
     def getChild(self, name, request):
         request.context = get_context(request)
@@ -136,7 +150,7 @@
         }})
         return server.NOT_DONE_YET
 
-class BucketResource(Resource):
+class BucketResource(ErrorHandlingResource):
     def __init__(self, name):
         Resource.__init__(self)
         self.name = name
@@ -186,7 +200,7 @@
         return ''
 
 
-class ObjectResource(Resource):
+class ObjectResource(ErrorHandlingResource):
     def __init__(self, bucket, name):
         Resource.__init__(self)
         self.bucket = bucket
@@ -227,7 +241,7 @@
         request.setResponseCode(204)
         return ''
 
-class ImageResource(Resource):
+class ImageResource(ErrorHandlingResource):
     isLeaf = True
 
     def getChild(self, name, request):