Merge trunk, resolve conflicts, and 005 -> 006 the migration scripts.

glance/registry/__init__.py

@@ -63,13 +63,13 @@ def add_image_metadata(options, image_meta):
     return new_image_meta
 
 
-def update_image_metadata(options, image_id, image_meta):
+def update_image_metadata(options, image_id, image_meta, purge_props=False):
     if options['debug']:
         logger.debug("Updating image metadata for image %s...", image_id)
         _debug_print_metadata(image_meta)
 
     c = get_registry_client(options)
-    new_image_meta = c.update_image(image_id, image_meta)
+    new_image_meta = c.update_image(image_id, image_meta, purge_props)
 
     if options['debug']:
         logger.debug("Returned image metadata from call to "

glance/registry/client.py

@@ -87,7 +87,7 @@ class RegistryClient(BaseClient):
         data = json.loads(res.read())
         return data['image']
 
-    def update_image(self, image_id, image_metadata):
+    def update_image(self, image_id, image_metadata, purge_props=False):
         """
         Updates Registry's information about an image
         """
@@ -96,7 +96,10 @@ class RegistryClient(BaseClient):
 
         body = json.dumps(image_metadata)
 
-        res = self.do_request("PUT", "/images/%s" % image_id, body)
+        headers = {}
+        if purge_props:
+            headers["X-Glance-Registry-Purge-Props"] = "true"
+        res = self.do_request("PUT", "/images/%s" % image_id, body, headers)
         data = json.loads(res.read())
         image = data['image']
         return image

glance/registry/db/api.py

@@ -21,6 +21,7 @@
 Defines interface for DB access
 """
 
+import logging
 
 from sqlalchemy import create_engine
 from sqlalchemy.ext.declarative import declarative_base
@@ -59,14 +60,19 @@ def configure_db(options):
     """
     global _ENGINE
     if not _ENGINE:
+        debug = config.get_option(
+            options, 'debug', type='bool', default=False)
         verbose = config.get_option(
             options, 'verbose', type='bool', default=False)
         timeout = config.get_option(
             options, 'sql_idle_timeout', type='int', default=3600)
         _ENGINE = create_engine(options['sql_connection'],
-                                echo=verbose,
-                                echo_pool=verbose,
                                 pool_recycle=timeout)
+        logger = logging.getLogger('sqlalchemy.engine')
+        if debug:
+            logger.setLevel(logging.DEBUG)
+        elif verbose:
+            logger.setLevel(logging.INFO)
         register_models()
 
 
@@ -97,16 +103,16 @@ def unregister_models():
 
 def image_create(context, values):
     """Create an image from the values dictionary."""
-    return _image_update(context, values, None)
+    return _image_update(context, values, None, False)
 
 
-def image_update(context, image_id, values):
+def image_update(context, image_id, values, purge_props=False):
     """Set the given properties on an image and update it.
 
     Raises NotFound if image does not exist.
 
     """
-    return _image_update(context, values, image_id)
+    return _image_update(context, values, image_id, purge_props)
 
 
 def image_destroy(context, image_id):
@@ -188,7 +194,7 @@ def validate_image(values):
             raise exception.Invalid(msg)
 
 
-def _image_update(context, values, image_id):
+def _image_update(context, values, image_id, purge_props=False):
     """Used internally by image_create and image_update
 
     :param context: Request context
@@ -227,12 +233,14 @@ def _image_update(context, values, image_id):
 
         image_ref.save(session=session)
 
-        _set_properties_for_image(context, image_ref, properties, session)
+        _set_properties_for_image(context, image_ref, properties, purge_props,
+                                  session)
 
     return image_get(context, image_ref.id)
 
 
-def _set_properties_for_image(context, image_ref, properties, session=None):
+def _set_properties_for_image(context, image_ref, properties,
+                              purge_props=False, session=None):
     """
     Create or update a set of image_properties for a given image
 
@@ -256,10 +264,11 @@ def _set_properties_for_image(context, image_ref, properties, session=None):
         else:
             image_property_create(context, prop_values, session=session)
 
-    for name in orig_properties.keys():
-        if not name in properties:
-            prop_ref = orig_properties[name]
-            image_property_delete(context, prop_ref, session=session)
+    if purge_props:
+        for key in orig_properties.keys():
+            if not key in properties:
+                prop_ref = orig_properties[key]
+                image_property_delete(context, prop_ref, session=session)
 
 
 def image_property_create(context, values, session=None):

glance/registry/db/migrate_repo/schema.py

@@ -47,6 +47,9 @@ DateTime = lambda: sqlalchemy.types.DateTime(timezone=False)
 Integer = lambda: sqlalchemy.types.Integer()
 
 
+BigInteger = lambda: sqlalchemy.types.BigInteger()
+
+
 def from_migration_import(module_name, fromlist):
     """Import a migration file and return the module
 

glance/registry/db/migrate_repo/versions/005_size_big_integer.py

@@ -0,0 +1,99 @@
+# 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.
+
+from migrate.changeset import *
+from sqlalchemy import *
+
+from glance.registry.db.migrate_repo.schema import (
+    Boolean, DateTime, BigInteger, Integer, String,
+    Text, from_migration_import)
+
+
+def get_images_table(meta):
+    """
+    Returns the Table object for the images table that
+    corresponds to the images table definition of this version.
+    """
+    images = Table('images', meta,
+        Column('id', Integer(), primary_key=True, nullable=False),
+        Column('name', String(255)),
+        Column('disk_format', String(20)),
+        Column('container_format', String(20)),
+        Column('size', BigInteger()),
+        Column('status', String(30), nullable=False),
+        Column('is_public', Boolean(), nullable=False, default=False,
+               index=True),
+        Column('location', Text()),
+        Column('created_at', DateTime(), nullable=False),
+        Column('updated_at', DateTime()),
+        Column('deleted_at', DateTime()),
+        Column('deleted', Boolean(), nullable=False, default=False,
+               index=True),
+        mysql_engine='InnoDB',
+        useexisting=True)
+
+    return images
+
+
+def get_image_properties_table(meta):
+    """
+    No changes to the image properties table from 002...
+    """
+    (define_image_properties_table,) = from_migration_import(
+        '002_add_image_properties_table', ['define_image_properties_table'])
+
+    image_properties = define_image_properties_table(meta)
+    return image_properties
+
+
+def upgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+
+    # No changes to SQLite stores are necessary, since
+    # there is no BIG INTEGER type in SQLite. Unfortunately,
+    # running the Python 005_size_big_integer.py migration script
+    # on a SQLite datastore results in an error in the sa-migrate
+    # code that does the workarounds for SQLite not having
+    # ALTER TABLE MODIFY COLUMN ability
+
+    dialect = migrate_engine.url.get_dialect().name
+
+    if not dialect.startswith('sqlite'):
+        (get_images_table,) = from_migration_import(
+            '003_add_disk_format', ['get_images_table'])
+
+        images = get_images_table(meta)
+        images.columns['size'].alter(type=BigInteger())
+
+
+def downgrade(migrate_engine):
+    meta = MetaData()
+    meta.bind = migrate_engine
+
+    # No changes to SQLite stores are necessary, since
+    # there is no BIG INTEGER type in SQLite. Unfortunately,
+    # running the Python 005_size_big_integer.py migration script
+    # on a SQLite datastore results in an error in the sa-migrate
+    # code that does the workarounds for SQLite not having
+    # ALTER TABLE MODIFY COLUMN ability
+
+    dialect = migrate_engine.url.get_dialect().name
+
+    if not dialect.startswith('sqlite'):
+        images = get_images_table(meta)
+        images.columns['size'].alter(type=Integer())

glance/registry/db/migrate_repo/versions/006_sqlite_downgrade.sql

@@ -36,10 +36,10 @@ CREATE TABLE image_properties (
 	UNIQUE (image_id, key), 
 	FOREIGN KEY(image_id) REFERENCES images (id)
 );
-CREATE INDEX ix_image_properties_name ON image_properties (name);
+CREATE INDEX ix_image_properties_key ON image_properties (key);
 
 INSERT INTO image_properties (id, image_id, key, value, created_at, updated_at, deleted_at, deleted)
-SELECT id, image_id, name, value, created_at, updated_at, deleted_at, deleted
+SELECT id, image_id, key, value, created_at, updated_at, deleted_at, deleted
 FROM image_properties_backup;
 
 DROP TABLE image_properties_backup;

glance/registry/db/models.py

@@ -24,7 +24,7 @@ import sys
 import datetime
 
 from sqlalchemy.orm import relationship, backref, exc, object_mapper, validates
-from sqlalchemy import Column, Integer, String
+from sqlalchemy import Column, Integer, String, BigInteger
 from sqlalchemy import ForeignKey, DateTime, Boolean, Text
 from sqlalchemy import UniqueConstraint
 from sqlalchemy.ext.declarative import declarative_base
@@ -100,7 +100,7 @@ class Image(BASE, ModelBase):
     name = Column(String(255))
     disk_format = Column(String(20))
     container_format = Column(String(20))
-    size = Column(Integer)
+    size = Column(BigInteger)
     status = Column(String(30), nullable=False)
     is_public = Column(Boolean, nullable=False, default=False)
     location = Column(Text)

glance/registry/server.py

@@ -157,11 +157,16 @@ class Controller(wsgi.Controller):
         """
         image_data = json.loads(req.body)['image']
 
+        purge_props = req.headers.get("X-Glance-Registry-Purge-Props", "false")
         context = None
         try:
             logger.debug("Updating image %(id)s with metadata: %(image_data)r"
                          % locals())
-            updated_image = db_api.image_update(context, id, image_data)
+            if purge_props == "true":
+                updated_image = db_api.image_update(context, id, image_data,
+                                                        True)
+            else:
+                updated_image = db_api.image_update(context, id, image_data)
             return dict(image=make_image_dict(updated_image))
         except exception.Invalid, e:
             msg = ("Failed to update image metadata. "

glance/server.py

@@ -418,7 +418,8 @@ class Controller(wsgi.Controller):
         try:
             image_meta = registry.update_image_metadata(self.options,
                                                         id,
-                                                        new_image_meta)
+                                                        new_image_meta,
+                                                        True)
             if has_body:
                 image_meta = self._upload_and_activate(req, image_meta)
 
@@ -449,7 +450,12 @@ class Controller(wsgi.Controller):
         """
         image = self.get_image_meta_or_404(req, id)
 
-        delete_from_backend(image['location'])
+        # The image's location field may be None in the case
+        # of a saving or queued image, therefore don't ask a backend
+        # to delete the image if the backend doesn't yet store it.
+        # See https://bugs.launchpad.net/glance/+bug/747799
+        if image['location']:
+            delete_from_backend(image['location'])
 
         registry.delete_image_metadata(self.options, id)
 

tests/functional/__init__.py

@@ -32,6 +32,7 @@ import socket
 import tempfile
 import time
 import unittest
+import urlparse
 
 from tests.utils import execute, get_unused_port
 
@@ -60,7 +61,7 @@ class FunctionalTest(unittest.TestCase):
 
         self.image_dir = "/tmp/test.%d/images" % self.test_id
 
-        self.sql_connection = os.environ.get('GLANCE_SQL_CONNECTION',
+        self.sql_connection = os.environ.get('GLANCE_TEST_SQL_CONNECTION',
                                              "sqlite://")
         self.pid_files = [self.api_pid_file,
                           self.registry_pid_file]
@@ -68,6 +69,41 @@ class FunctionalTest(unittest.TestCase):
 
     def tearDown(self):
         self.cleanup()
+        # We destroy the test data store between each test case,
+        # and recreate it, which ensures that we have no side-effects
+        # from the tests
+        self._reset_database()
+
+    def _reset_database(self):
+        conn_string = self.sql_connection
+        conn_pieces = urlparse.urlparse(conn_string)
+        if conn_string.startswith('sqlite'):
+            # We can just delete the SQLite database, which is
+            # the easiest and cleanest solution
+            db_path = conn_pieces.path.strip('/')
+            if db_path and os.path.exists(db_path):
+                os.unlink(db_path)
+            # No need to recreate the SQLite DB. SQLite will
+            # create it for us if it's not there...
+        elif conn_string.startswith('mysql'):
+            # We can execute the MySQL client to destroy and re-create
+            # the MYSQL database, which is easier and less error-prone
+            # than using SQLAlchemy to do this via MetaData...trust me.
+            database = conn_pieces.path.strip('/')
+            loc_pieces = conn_pieces.netloc.split('@')
+            host = loc_pieces[1]
+            auth_pieces = loc_pieces[0].split(':')
+            user = auth_pieces[0]
+            password = ""
+            if len(auth_pieces) > 1:
+                if auth_pieces[1].strip():
+                    password = "-p%s" % auth_pieces[1]
+            sql = ("drop database if exists %(database)s; "
+                   "create database %(database)s;") % locals()
+            cmd = ("mysql -u%(user)s %(password)s -h%(host)s "
+                   "-e\"%(sql)s\"") % locals()
+            exitcode, out, err = execute(cmd)
+            self.assertEqual(0, exitcode)
 
     def cleanup(self):
         """

tests/functional/test_curl_api.py

@@ -25,6 +25,7 @@ from tests import functional
 from tests.utils import execute
 
 FIVE_KB = 5 * 1024
+FIVE_GB = 5 * 1024 * 1024 * 1024
 
 
 class TestCurlApi(functional.FunctionalTest):
@@ -324,3 +325,62 @@ class TestCurlApi(functional.FunctionalTest):
         self.assertEqual('x86_64', image['properties']['arch'])
 
         self.stop_servers()
+
+    def test_size_greater_2G_mysql(self):
+        """
+        A test against the actual datastore backend for the registry
+        to ensure that the image size property is not truncated.
+
+        :see https://bugs.launchpad.net/glance/+bug/739433
+        """
+
+        self.cleanup()
+        api_port, reg_port, conf_file = self.start_servers()
+
+        # 1. POST /images with public image named Image1
+        # attribute and a size of 5G. Use the HTTP engine with an
+        # X-Image-Meta-Location attribute to make Glance forego
+        # "adding" the image data.
+        # Verify a 200 OK is returned
+        cmd = ("curl -i -X POST "
+               "-H 'Expect: ' "  # Necessary otherwise sends 100 Continue
+               "-H 'X-Image-Meta-Location: http://example.com/fakeimage' "
+               "-H 'X-Image-Meta-Size: %d' "
+               "-H 'X-Image-Meta-Name: Image1' "
+               "-H 'X-Image-Meta-Is-Public: True' "
+               "http://0.0.0.0:%d/images") % (FIVE_GB, api_port)
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 201 Created", status_line)
+
+        # Get the ID of the just-added image. This may NOT be 1, since the
+        # database in the environ variable TEST_GLANCE_CONNECTION may not
+        # have been cleared between test cases... :(
+        new_image_uri = None
+        for line in lines:
+            if line.startswith('Location:'):
+                new_image_uri = line[line.find(':') + 1:].strip()
+
+        self.assertTrue(new_image_uri is not None,
+                        "Could not find a new image URI!")
+
+        # 2. HEAD /images
+        # Verify image size is what was passed in, and not truncated
+        cmd = "curl -i -X HEAD %s" % new_image_uri
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 200 OK", status_line)
+        self.assertTrue("X-Image-Meta-Size: %d" % FIVE_GB in out,
+                        "Size was supposed to be %d. Got:\n%s."
+                        % (FIVE_GB, out))

tests/stubs.py

@@ -321,6 +321,7 @@ def stub_out_registry_db_image_api(stubs):
             values['checksum'] = values.get('checksum')
             values['deleted'] = False
             values['properties'] = values.get('properties', {})
+            values['location'] = values.get('location')
             values['created_at'] = datetime.datetime.utcnow()
             values['updated_at'] = datetime.datetime.utcnow()
             values['deleted_at'] = None
@@ -344,7 +345,7 @@ def stub_out_registry_db_image_api(stubs):
             self.images.append(values)
             return values
 
-        def image_update(self, _context, image_id, values):
+        def image_update(self, _context, image_id, values, purge_props=False):
 
             image = self.image_get(_context, image_id)
             copy_image = image.copy()
@@ -353,16 +354,17 @@ def stub_out_registry_db_image_api(stubs):
             props = []
             orig_properties = image['properties']
 
-            if 'properties' in values.keys():
-                for k, v in values['properties'].items():
-                    p = {}
-                    p['name'] = k
-                    p['value'] = v
-                    p['deleted'] = False
-                    p['created_at'] = datetime.datetime.utcnow()
-                    p['updated_at'] = datetime.datetime.utcnow()
-                    p['deleted_at'] = None
-                    props.append(p)
+            if purge_props == False:
+                if 'properties' in values.keys():
+                    for k, v in values['properties'].items():
+                        p = {}
+                        p['name'] = k
+                        p['value'] = v
+                        p['deleted'] = False
+                        p['created_at'] = datetime.datetime.utcnow()
+                        p['updated_at'] = datetime.datetime.utcnow()
+                        p['deleted_at'] = None
+                        props.append(p)
 
             orig_properties = orig_properties + props
             values['properties'] = orig_properties

tests/unit/test_api.py

@@ -582,3 +582,35 @@ class TestGlanceAPI(unittest.TestCase):
         req.method = 'DELETE'
         res = req.get_response(self.api)
         self.assertEquals(res.status_int, webob.exc.HTTPNotFound.code)
+
+    def test_delete_queued_image(self):
+        """
+        Here, we try to delete an image that is in the queued state.
+        Bug #747799 demonstrated that trying to DELETE an image
+        that had had its save process killed manually results in failure
+        because the location attribute is None.
+        """
+        # Add an image the way that glance-upload adds an image...
+        # by reserving a place in the database for an image without
+        # really any attributes or information on the image and then
+        # later doing an update with the image body and other attributes.
+        # We will stop the process after the reservation stage, then
+        # try to delete the image.
+        fixture_headers = {'x-image-meta-store': 'file',
+                           'x-image-meta-name': 'fake image #3'}
+
+        req = webob.Request.blank("/images")
+        req.method = 'POST'
+        for k, v in fixture_headers.iteritems():
+            req.headers[k] = v
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, httplib.CREATED)
+
+        res_body = json.loads(res.body)['image']
+        self.assertEquals('queued', res_body['status'])
+
+        # Now try to delete the image...
+        req = webob.Request.blank("/images/3")
+        req.method = 'DELETE'
+        res = req.get_response(self.api)
+        self.assertEquals(res.status_int, 200)