Merge lp:~donal-lafferty/glance/fix739433 into lp:~glance-coresec/glance/cactus-trunk

[Donal Lafferty]

Provide revised schema and migration scripts for turning 'size' column in 'images' table to BIGINT. This overcomes a 2 gig limit on images sizes that can be downloaded from Glance.

[Devin Carlen (devcamcar)]

lgtm

review: Approve

[Jay Pipes (jaypipes)]

Donal, I took this branch and made changes to make the migration script Python-based, instead of just MySQL .sql migrate files. The reason is because we also needed PostgreSQL to migrate.

There is an issue with sa-migrate and SQLite, however, that produced an error in the sa-migrate workaround code for SQLite not having ALTER TABLE MODIFY COLUMN abilities. But, since SQLite doesn't have a concept of BIGINT, I simply made sure that the migrate script doesn't alter anything for SQLite engines.

Please merge lp:~jaypipes/glance/bug739433 into your branch locally and bzr commit && bzr push. While you do that, please do set this Merge Request to Work In Progress status, then back to Needs Review when you've pushed. Thanks for your patience on this.

[Donal Lafferty (donal-lafferty)]

Updated code to reflect changes from lp:~jaypipes/glance/bug739433

Ran migration unit tests locally. Noted that making use of MySQL for tests required updating the vitualenv to include an installation of MySQLdb. This is because MySQLdb is not a dependency for the test vitualenv. E.g. after tests fail, use

sudo pip install mysql-python -e ./.glance-venv

Then run the tests again.

[Jay Pipes (jaypipes)]

approve from me :)

review: Approve

=== modified file 'glance/registry/db/migrate_repo/schema.py'
--- glance/registry/db/migrate_repo/schema.py	2011-02-02 23:44:56 +0000
+++ glance/registry/db/migrate_repo/schema.py	2011-04-05 21:27:46 +0000
@@ -47,6 +47,9 @@
 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
 

=== added file 'glance/registry/db/migrate_repo/versions/005_size_big_integer.py'
--- glance/registry/db/migrate_repo/versions/005_size_big_integer.py	1970-01-01 00:00:00 +0000
+++ glance/registry/db/migrate_repo/versions/005_size_big_integer.py	2011-04-05 21:27:46 +0000
@@ -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())

=== modified file 'glance/registry/db/models.py'
--- glance/registry/db/models.py	2011-03-08 15:22:44 +0000
+++ glance/registry/db/models.py	2011-04-05 21:27:46 +0000
@@ -24,7 +24,7 @@
 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 @@
     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)

=== modified file 'tests/functional/__init__.py'
--- tests/functional/__init__.py	2011-03-25 18:59:56 +0000
+++ tests/functional/__init__.py	2011-04-05 21:27:46 +0000
@@ -32,6 +32,7 @@
 import tempfile
 import time
 import unittest
+import urlparse
 
 from tests.utils import execute, get_unused_port
 
@@ -60,7 +61,7 @@
 
         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 @@
 
     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):
         """

=== modified file 'tests/functional/test_curl_api.py'
--- tests/functional/test_curl_api.py	2011-03-28 18:02:21 +0000
+++ tests/functional/test_curl_api.py	2011-04-05 21:27:46 +0000
@@ -25,6 +25,7 @@
 from tests.utils import execute
 
 FIVE_KB = 5 * 1024
+FIVE_GB = 5 * 1024 * 1024 * 1024
 
 
 class TestCurlApi(functional.FunctionalTest):
@@ -324,3 +325,62 @@
         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))