Set correct Content-Length on cached remote images

Override the image metadata size attribute with the actual
cached image file size when setting the Content-Length header
on the response to a GET of a cache remote image.

This avoids an non-initial GET on a remote image failing when
caching is enabled, as described in this bug:

  Bug 900959

Change-Id: I7656bc971d982cce78ce1ed4cf7ea509f8bbdfa8

Authors

@@ -9,6 +9,7 @@ Cory Wright <corywright@gmail.com>
 Dan Prince <dan.prince@rackspace.com>
 Donal Lafferty <donal.lafferty@citrix.com>
 Eldar Nugaev <enugaev@griddynamics.com>
+Eoghan Glynn <eglynn@redhat.com>
 Ewan Mellor <ewan.mellor@citrix.com>
 Hengqing Hu <hudayou@hotmail.com>
 Isaku Yamahata <yamahata@valinux.co.jp>

glance/api/middleware/cache.py

@@ -80,6 +80,11 @@ def process_request(self, request):
             try:
                 image_meta = registry.get_image_metadata(context, image_id)
 
+                if not image_meta['size']:
+                    # override image size metadata with the actual cached
+                    # file size, see LP Bug #900959
+                    image_meta['size'] = self.cache.get_image_size(image_id)
+
                 response = webob.Response(request=request)
                 return self.serializer.show(response, {
                     'image_iterator': image_iterator,

glance/image_cache/__init__.py

@@ -272,6 +272,15 @@ def open_for_read(self, image_id):
         """
         return self.driver.open_for_read(image_id)
 
+    def get_image_size(self, image_id):
+        """
+        Return the size of the image file for an image with supplied
+        identifier.
+
+        :param image_id: Image ID
+        """
+        return self.driver.get_image_size(image_id)
+
     def get_queued_images(self):
         """
         Returns a list of image IDs that are in the queue. The

glance/image_cache/drivers/base.py

@@ -198,6 +198,27 @@ def open_for_read(self, image_id):
         """
         raise NotImplementedError
 
+    def get_image_filepath(self, image_id, cache_status='active'):
+        """
+        This crafts an absolute path to a specific entry
+
+        :param image_id: Image ID
+        :param cache_status: Status of the image in the cache
+        """
+        if cache_status == 'active':
+            return os.path.join(self.base_dir, str(image_id))
+        return os.path.join(self.base_dir, cache_status, str(image_id))
+
+    def get_image_size(self, image_id):
+        """
+        Return the size of the image file for an image with supplied
+        identifier.
+
+        :param image_id: Image ID
+        """
+        path = self.get_image_filepath(image_id)
+        return os.path.getsize(path)
+
     def get_queued_images(self):
         """
         Returns a list of image IDs that are in the queue. The

glance/image_cache/drivers/sqlite.py

@@ -375,17 +375,6 @@ def get_db(self):
         finally:
             conn.close()
 
-    def get_image_filepath(self, image_id, cache_status='active'):
-        """
-        This crafts an absolute path to a specific entry
-
-        :param image_id: Image ID
-        :param cache_status: Status of the image in the cache
-        """
-        if cache_status == 'active':
-            return os.path.join(self.base_dir, str(image_id))
-        return os.path.join(self.base_dir, cache_status, str(image_id))
-
     def queue_image(self, image_id):
         """
         This adds a image to be cache to the queue.

glance/image_cache/drivers/xattr.py

@@ -314,17 +314,6 @@ def open_for_read(self, image_id):
         path = self.get_image_filepath(image_id)
         inc_xattr(path, 'hits', 1)
 
-    def get_image_filepath(self, image_id, cache_status='active'):
-        """
-        This crafts an absolute path to a specific entry
-
-        :param image_id: Image ID
-        :param cache_status: Status of the image in the cache
-        """
-        if cache_status == 'active':
-            return os.path.join(self.base_dir, str(image_id))
-        return os.path.join(self.base_dir, cache_status, str(image_id))
-
     def queue_image(self, image_id):
         """
         This adds a image to be cache to the queue.

glance/tests/functional/test_cache_middleware.py

@@ -26,8 +26,10 @@
 import json
 import os
 import shutil
+import thread
 import time
 
+import BaseHTTPServer
 import httplib2
 
 from glance.tests import functional
@@ -39,6 +41,25 @@
 FIVE_KB = 5 * 1024
 
 
+class RemoteImageHandler(BaseHTTPServer.BaseHTTPRequestHandler):
+    def do_GET(s):
+        """
+        Respond to an image GET request with fake image content.
+        """
+        if 'images' in s.path:
+            s.send_response(200)
+            s.send_header('Content-Type', 'application/octet-stream')
+            s.send_header('Content-Length', FIVE_KB)
+            s.end_headers()
+            image_data = '*' * FIVE_KB
+            s.wfile.write(image_data)
+            self.wfile.close()
+            return
+        else:
+            self.send_error(404, 'File Not Found: %s' % self.path)
+            return
+
+
 class BaseCacheMiddlewareTest(object):
 
     @skip_if_disabled
@@ -118,6 +139,61 @@ def test_cache_middleware_transparent(self):
 
         self.stop_servers()
 
+    @skip_if_disabled
+    def test_cache_remote_image(self):
+        """
+        We test that caching is no longer broken for remote images
+        """
+        self.cleanup()
+        self.start_servers(**self.__dict__.copy())
+
+        api_port = self.api_port
+        registry_port = self.registry_port
+
+        # set up "remote" image server
+        server_class = BaseHTTPServer.HTTPServer
+        remote_server = server_class(('127.0.0.1', 0), RemoteImageHandler)
+        remote_ip, remote_port = remote_server.server_address
+
+        def serve_requests(httpd):
+            httpd.serve_forever()
+
+        thread.start_new_thread(serve_requests, (remote_server,))
+
+        # Add a remote image and verify a 200 OK is returned
+        remote_uri = 'http://%s:%d/images/2' % (remote_ip, remote_port)
+        headers = {'X-Image-Meta-Name': 'Image2',
+                   'X-Image-Meta-Is-Public': 'True',
+                   'X-Image-Meta-Location': remote_uri}
+        path = "http://%s:%d/v1/images" % ("0.0.0.0", self.api_port)
+        http = httplib2.Http()
+        response, content = http.request(path, 'POST', headers=headers)
+        self.assertEqual(response.status, 201)
+        data = json.loads(content)
+        self.assertEqual(data['image']['size'], 0)
+
+        image_id = data['image']['id']
+
+        # Grab the image
+        path = "http://%s:%d/v1/images/%s" % ("0.0.0.0", self.api_port,
+                                              image_id)
+        http = httplib2.Http()
+        response, content = http.request(path, 'GET')
+        self.assertEqual(response.status, 200)
+
+        # Grab the image again to ensure it can be served out from
+        # cache with the correct size
+        path = "http://%s:%d/v1/images/%s" % ("0.0.0.0", self.api_port,
+                                              image_id)
+        http = httplib2.Http()
+        response, content = http.request(path, 'GET')
+        self.assertEqual(response.status, 200)
+        self.assertEqual(int(response['content-length']), FIVE_KB)
+
+        remote_server.shutdown()
+
+        self.stop_servers()
+
 
 class BaseCacheManageMiddlewareTest(object):
 

glance/tests/unit/test_image_cache.py

@@ -29,17 +29,13 @@
 from glance.tests import utils as test_utils
 from glance.tests.utils import skip_if_disabled, xattr_writes_supported
 
-FIXTURE_DATA = '*' * 1024
+FIXTURE_LENGTH = 1024
+FIXTURE_DATA = '*' * FIXTURE_LENGTH
 
 
 class ImageCacheTestCase(object):
 
-    @skip_if_disabled
-    def test_is_cached(self):
-        """
-        Verify is_cached(1) returns 0, then add something to the cache
-        and verify is_cached(1) returns 1.
-        """
+    def _setup_fixture_file(self):
         FIXTURE_FILE = StringIO.StringIO(FIXTURE_DATA)
 
         self.assertFalse(self.cache.is_cached(1))
@@ -48,18 +44,22 @@ def test_is_cached(self):
 
         self.assertTrue(self.cache.is_cached(1))
 
+    @skip_if_disabled
+    def test_is_cached(self):
+        """
+        Verify is_cached(1) returns 0, then add something to the cache
+        and verify is_cached(1) returns 1.
+        """
+        self._setup_fixture_file()
+
     @skip_if_disabled
     def test_read(self):
         """
         Verify is_cached(1) returns 0, then add something to the cache
         and verify after a subsequent read from the cache that
         is_cached(1) returns 1.
         """
-        FIXTURE_FILE = StringIO.StringIO(FIXTURE_DATA)
-
-        self.assertFalse(self.cache.is_cached(1))
-
-        self.assertTrue(self.cache.cache_image_file(1, FIXTURE_FILE))
+        self._setup_fixture_file()
 
         buff = StringIO.StringIO()
         with self.cache.open_for_read(1) as cache_file:
@@ -74,11 +74,7 @@ def test_open_for_read(self):
         Test convenience wrapper for opening a cache file via
         its image identifier.
         """
-        FIXTURE_FILE = StringIO.StringIO(FIXTURE_DATA)
-
-        self.assertFalse(self.cache.is_cached(1))
-
-        self.assertTrue(self.cache.cache_image_file(1, FIXTURE_FILE))
+        self._setup_fixture_file()
 
         buff = StringIO.StringIO()
         with self.cache.open_for_read(1) as cache_file:
@@ -88,17 +84,23 @@ def test_open_for_read(self):
         self.assertEqual(FIXTURE_DATA, buff.getvalue())
 
     @skip_if_disabled
-    def test_delete(self):
+    def test_get_image_size(self):
         """
-        Test delete method that removes an image from the cache
+        Test convenience wrapper for querying cache file size via
+        its image identifier.
         """
-        FIXTURE_FILE = StringIO.StringIO(FIXTURE_DATA)
+        self._setup_fixture_file()
 
-        self.assertFalse(self.cache.is_cached(1))
+        size = self.cache.get_image_size(1)
 
-        self.assertTrue(self.cache.cache_image_file(1, FIXTURE_FILE))
+        self.assertEqual(FIXTURE_LENGTH, size)
 
-        self.assertTrue(self.cache.is_cached(1))
+    @skip_if_disabled
+    def test_delete(self):
+        """
+        Test delete method that removes an image from the cache
+        """
+        self._setup_fixture_file()
 
         self.cache.delete_cached_image(1)