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

[Jay Pipes]

Removes the call to webob.Request.make_body_seekable() in the
general images controller to prevent the image from being copied
into memory. In the S3 controller, which needs a seekable file-like
object when calling boto.s3.Key.set_contents_from_file(), we work
around this by writing chunks of the request body to a tempfile on
the API node, then stream this tempfile to S3.

[Josh Kearney (jk0)]

Verified in our lab. Thanks Jay!

review: Approve

[Matt Dietz (cerberus)]

I like it

review: Approve

[Rick Harris (rconradharris)]

Great job churning this patch out so quickly, Jay. Much appreciated.

In a follow-up patch we might want to consider deleting the temporary file once we're done with it.

Since we're using Python 2.6, we can add a delete=True to the NamedTemporaryFile initializer and that will automatically delete the file when we close it. Something like:

with NamedTemporaryFile(delete=True) as f:
  do a bunch of stuff

review: Approve

[Jay Pipes (jaypipes)]

Good suggestion, Rick. I'll do that in a later commit.

Cheers,
jay

=== modified file 'glance/api/v1/images.py'
--- glance/api/v1/images.py	2011-08-02 17:43:16 +0000
+++ glance/api/v1/images.py	2011-08-03 19:09:46 +0000
@@ -335,7 +335,6 @@
         try:
             logger.debug("Uploading image data for image %(image_id)s "
                          "to %(store_name)s store", locals())
-            req.make_body_seekable()
             location, size, checksum = store.add(image_meta['id'],
                                                  req.body_file)
 

=== modified file 'glance/store/s3.py'
--- glance/store/s3.py	2011-08-01 22:10:31 +0000
+++ glance/store/s3.py	2011-08-03 19:09:46 +0000
@@ -18,7 +18,9 @@
 """Storage backend for S3 or Storage Servers that follow the S3 Protocol"""
 
 import logging
+import hashlib
 import httplib
+import tempfile
 import urlparse
 
 from glance.common import config
@@ -305,12 +307,41 @@
 
         key = bucket_obj.new_key(obj_name)
 
+        # We need to wrap image_file, which is a reference to the
+        # webob.Request.body_file, with a seekable file-like object,
+        # otherwise the call to set_contents_from_file() will die
+        # with an error about Input object has no method 'seek'. We
+        # might want to call webob.Request.make_body_seekable(), but
+        # unfortunately, that method copies the entire image into
+        # memory and results in LP Bug #818292 occurring. So, here
+        # we write temporary file in as memory-efficient manner as
+        # possible and then supply the temporary file to S3. We also
+        # take this opportunity to calculate the image checksum while
+        # writing the tempfile, so we don't need to call key.compute_md5()
+
+        logger.debug("Writing request body file to temporary "
+                     "file for %s", loc.get_uri())
+        temp_file = tempfile.NamedTemporaryFile()
+
+        checksum = hashlib.md5()
+        chunk = image_file.read(self.CHUNKSIZE)
+        while chunk:
+            checksum.update(chunk)
+            temp_file.write(chunk)
+            chunk = image_file.read(self.CHUNKSIZE)
+        temp_file.flush()
+
+        logger.debug("Uploading temporary file to S3 for %s", loc.get_uri())
+
         # OK, now upload the data into the key
-        obj_md5, _base64_digest = key.compute_md5(image_file)
-        key.set_contents_from_file(image_file, replace=False)
+        key.set_contents_from_file(open(temp_file.name, 'r+b'), replace=False)
         size = key.size
-
-        return (loc.get_uri(), size, obj_md5)
+        checksum_hex = checksum.hexdigest()
+
+        logger.debug("Wrote %(size)d bytes to S3 key named %(obj_name)s with "
+                     "checksum %(checksum_hex)s" % locals())
+
+        return (loc.get_uri(), size, checksum_hex)
 
     def delete(self, location):
         """

=== modified file 'glance/tests/functional/test_s3.py'
--- glance/tests/functional/test_s3.py	2011-08-02 22:10:52 +0000
+++ glance/tests/functional/test_s3.py	2011-08-03 19:09:46 +0000
@@ -50,7 +50,8 @@
     # Test machines can set the GLANCE_TEST_MIGRATIONS_CONF variable
     # to override the location of the config file for migration testing
     CONFIG_FILE_PATH = os.environ.get('GLANCE_TEST_S3_CONF',
-                                      os.path.join('tests', 'functional',
+                                      os.path.join('glance', 'tests',
+                                                   'functional',
                                                    'test_s3.conf'))
 
     def setUp(self):

=== modified file 'glance/tests/unit/test_s3_store.py'
--- glance/tests/unit/test_s3_store.py	2011-08-02 15:43:05 +0000
+++ glance/tests/unit/test_s3_store.py	2011-08-03 19:09:46 +0000
@@ -77,7 +77,8 @@
 
         def set_contents_from_file(self, fp, replace=False, **kwargs):
             self.data = StringIO.StringIO()
-            self.data.write(fp.getvalue())
+            for bytes in fp:
+                self.data.write(bytes)
             self.size = self.data.len
             # Reset the buffer to start
             self.data.seek(0)