Merge lp:~jaypipes/glance/bug700162 into lp:~hudson-openstack/glance/trunk

[Jay Pipes]

Fixes LP Bug #700162: Images greater than 2GB cannot be uploaded
using glance.client.Client.

Fixes LP Bug #700162: Images greater than 2GB cannot be uploaded
using glance.client.Client.

This patch introduces chunked-transfer encoding to the client
classes. When the glance.client.BaseClient.do_request() method
is called and the method is either POST or PUT and the body
argument is a file-like object, then the do_request() method
builds a chunked-transfer encoded request and send()s chunks of
the body file-like object over the httplib connection.

Not sure how we fully test this, since we'd have to pack up
a very large file for use in testing, but I'm open to suggestions :)

[Rick Harris (rconradharris)]

Perfect timing, hit this bug just now and there is already a patch :)

Overall, your fix looks really good. I had to make a couple of small tweaks to get it fully-working. My fixes are here:

https://code.launchpad.net/~rconradharris/glance/bug700162

> Not sure how we fully test this, since we'd have to pack up
> a very large file for use in testing, but I'm open to suggestion

I just tested the code functionally by uploading ubuntu-lucid into Glance. Without the fix I get a MemoryError, with it, it uploads successfully.

review: Needs Fixing

[Ewan Mellor (ewanmellor)]

For test, how about uploading a pattern through it? Just incrementing integers would do the trick. You can make a file-like object that generates the pattern, and stub out the actual send routine, and check that the pattern makes it through.

If the unit test rlimited itself (resource.setrlimit) then it wouldn't even need to stream too much -- just cap yourself at 100 MB and stream 200 MB through.

[Ewan Mellor (ewanmellor)]

lgtm.

[Jay Pipes (jaypipes)]

On Fri, Jan 21, 2011 at 12:00 AM, Rick Harris <email address hidden> wrote:
> Review: Needs Fixing
> Perfect timing, hit this bug just now and there is already a patch :)
>
> Overall, your fix looks really good.  I had to make a couple of small tweaks to get it fully-working. My fixes are here:
>
> https://code.launchpad.net/~rconradharris/glance/bug700162

Swett, thanks Rick! :) I'll merge your patch into mine and push shortly.

>> Not sure how we fully test this, since we'd have to pack up
>> a very large file for use in testing, but I'm open to suggestion
>
> I just tested the code functionally by uploading ubuntu-lucid into Glance. Without the fix I get a MemoryError, with it, it uploads successfully.

Great! I like Ewan's idea for testing below; I'll try to integrate
that idea in too.

-jay

[Jay Pipes (jaypipes)]

> Overall, your fix looks really good. I had to make a couple of small tweaks
> to get it fully-working. My fixes are here:
>
> https://code.launchpad.net/~rconradharris/glance/bug700162

Hmm, looks like you may not have run the test suite after making your changes :) I'll fix up the few errors that have popped up!

-jay

[Rick Harris (rconradharris)]

Shoot, yeah, had tunnel-vision trying to get nova-compute working on DomU. Thanks for catching that.

[Jay Pipes (jaypipes)]

OK all, please re-review. Tests now all pass and it seems that the bug is fixed...

[Rick Harris (rconradharris)]

lgtm, let's get this merged asap! :D

review: Approve

[Devin Carlen (devcamcar)]

lgtm

review: Approve

=== modified file 'glance/client.py'
--- glance/client.py	2011-01-07 06:13:33 +0000
+++ glance/client.py	2011-01-21 17:38:25 +0000
@@ -70,6 +70,8 @@
 
     """A base client class"""
 
+    CHUNKSIZE = 65536
+
     def __init__(self, host, port, use_ssl):
         """
         Creates a new client to some service.
@@ -103,12 +105,41 @@
         :param action: part of URL after root netloc
         :param body: string of data to send, or None (default)
         :param headers: mapping of key/value pairs to add as headers
+
+        :note
+
+        If the body param has a read attribute, and method is either
+        POST or PUT, this method will automatically conduct a chunked-transfer
+        encoding and use the body as a file object, transferring chunks
+        of data using the connection's send() method. This allows large
+        objects to be transferred efficiently without buffering the entire
+        body in memory.
         """
         try:
             connection_type = self.get_connection_type()
             headers = headers or {}
             c = connection_type(self.host, self.port)
-            c.request(method, action, body, headers)
+
+            # Do a simple request or a chunked request, depending
+            # on whether the body param is a file-like object and
+            # the method is PUT or POST
+            if hasattr(body, 'read') and method.lower() in ('post', 'put'):
+                # Chunk it, baby...
+                c.putrequest(method, action)
+
+                for header, value in headers.items():
+                    c.putheader(header, value)
+                c.putheader('Transfer-Encoding', 'chunked')
+                c.endheaders()
+
+                chunk = body.read(self.CHUNKSIZE)
+                while chunk:
+                    c.send('%x\r\n%s\r\n' % (len(chunk), chunk))
+                    chunk = body.read(self.CHUNKSIZE)
+                c.send('0\r\n\r\n')
+            else:
+                # Simple request...
+                c.request(method, action, body, headers)
             res = c.getresponse()
             status_code = self.get_status_code(res)
             if status_code == httplib.OK:
@@ -185,8 +216,6 @@
         :retval Tuple containing (image_meta, image_blob)
         :raises exception.NotFound if image is not found
         """
-        # TODO(jaypipes): Handle other registries than Registry...
-
         res = self.do_request("GET", "/images/%s" % image_id)
 
         image = util.get_image_meta_from_headers(res)
@@ -219,24 +248,13 @@
         if image_meta is None:
             image_meta = {}
 
-        if image_data:
-            if hasattr(image_data, 'read'):
-                # TODO(jaypipes): This is far from efficient. Implement
-                # chunked transfer encoding if size is not in image_meta
-                body = image_data.read()
-            else:
-                body = image_data
-        else:
-            body = None
-
-        if not 'size' in image_meta.keys():
-            if body:
-                image_meta['size'] = len(body)
-
         headers = util.image_meta_to_http_headers(image_meta)
 
         if image_data:
+            body = image_data
             headers['content-type'] = 'application/octet-stream'
+        else:
+            body = None
 
         res = self.do_request("POST", "/images", body, headers)
         data = json.loads(res.read())

=== modified file 'glance/server.py'
--- glance/server.py	2011-01-18 17:58:23 +0000
+++ glance/server.py	2011-01-21 17:38:25 +0000
@@ -324,7 +324,9 @@
         """
         image_meta = self._reserve(req)
 
-        if req.body:
+        has_body = req.content_length or 'transfer-encoding' in req.headers
+
+        if has_body:
             self._upload_and_activate(req, image_meta)
         else:
             if 'x-image-meta-location' in req.headers:

=== modified file 'tests/stubs.py'
--- tests/stubs.py	2011-01-18 17:58:23 +0000
+++ tests/stubs.py	2011-01-21 17:38:25 +0000
@@ -260,6 +260,22 @@
         def close(self):
             return True
 
+        def putrequest(self, method, url):
+            self.req = webob.Request.blank("/" + url.lstrip("/"))
+            self.req.method = method
+
+        def putheader(self, key, value):
+            self.req.headers[key] = value
+
+        def endheaders(self):
+            pass
+
+        def send(self, data):
+            # send() is called during chunked-transfer encoding, and
+            # data is of the form %x\r\n%s\r\n. Strip off the %x and
+            # only write the actual data in tests.
+            self.req.body += data.split("\r\n")[1]
+
         def request(self, method, url, body=None, headers={}):
             self.req = webob.Request.blank("/" + url.lstrip("/"))
             self.req.method = method