diff --git a/glance/registry/__init__.py b/glance/registry/__init__.py index 3219687c2b..a4e4a80b69 100644 --- a/glance/registry/__init__.py +++ b/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 " diff --git a/glance/registry/client.py b/glance/registry/client.py index af78a7a33e..336c825f05 100644 --- a/glance/registry/client.py +++ b/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 diff --git a/glance/registry/db/api.py b/glance/registry/db/api.py index 8bc6a5dfd4..c509623673 100644 --- a/glance/registry/db/api.py +++ b/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): diff --git a/glance/registry/db/migrate_repo/schema.py b/glance/registry/db/migrate_repo/schema.py index a5b7a8a68c..202eceb782 100644 --- a/glance/registry/db/migrate_repo/schema.py +++ b/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 diff --git a/glance/registry/db/migrate_repo/versions/005_size_big_integer.py b/glance/registry/db/migrate_repo/versions/005_size_big_integer.py new file mode 100644 index 0000000000..81c1199bd7 --- /dev/null +++ b/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()) diff --git a/glance/registry/db/migrate_repo/versions/005_key_to_name.py b/glance/registry/db/migrate_repo/versions/006_key_to_name.py similarity index 100% rename from glance/registry/db/migrate_repo/versions/005_key_to_name.py rename to glance/registry/db/migrate_repo/versions/006_key_to_name.py diff --git a/glance/registry/db/migrate_repo/versions/005_mysql_downgrade.sql b/glance/registry/db/migrate_repo/versions/006_mysql_downgrade.sql similarity index 100% rename from glance/registry/db/migrate_repo/versions/005_mysql_downgrade.sql rename to glance/registry/db/migrate_repo/versions/006_mysql_downgrade.sql diff --git a/glance/registry/db/migrate_repo/versions/005_mysql_upgrade.sql b/glance/registry/db/migrate_repo/versions/006_mysql_upgrade.sql similarity index 100% rename from glance/registry/db/migrate_repo/versions/005_mysql_upgrade.sql rename to glance/registry/db/migrate_repo/versions/006_mysql_upgrade.sql diff --git a/glance/registry/db/migrate_repo/versions/005_sqlite_downgrade.sql b/glance/registry/db/migrate_repo/versions/006_sqlite_downgrade.sql similarity index 88% rename from glance/registry/db/migrate_repo/versions/005_sqlite_downgrade.sql rename to glance/registry/db/migrate_repo/versions/006_sqlite_downgrade.sql index eb8b17aa9e..6be0adaecc 100644 --- a/glance/registry/db/migrate_repo/versions/005_sqlite_downgrade.sql +++ b/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; diff --git a/glance/registry/db/migrate_repo/versions/005_sqlite_upgrade.sql b/glance/registry/db/migrate_repo/versions/006_sqlite_upgrade.sql similarity index 100% rename from glance/registry/db/migrate_repo/versions/005_sqlite_upgrade.sql rename to glance/registry/db/migrate_repo/versions/006_sqlite_upgrade.sql diff --git a/glance/registry/db/models.py b/glance/registry/db/models.py index 3fbdccaad1..972854044a 100644 --- a/glance/registry/db/models.py +++ b/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) diff --git a/glance/registry/server.py b/glance/registry/server.py index 3d440d0379..52d17e28fa 100644 --- a/glance/registry/server.py +++ b/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. " diff --git a/glance/server.py b/glance/server.py index 263d027e12..0072a84e7c 100644 --- a/glance/server.py +++ b/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) diff --git a/tests/functional/__init__.py b/tests/functional/__init__.py index fea33e98ac..c4cff106b4 100644 --- a/tests/functional/__init__.py +++ b/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): """ diff --git a/tests/functional/test_curl_api.py b/tests/functional/test_curl_api.py index c935993896..b791e9a310 100644 --- a/tests/functional/test_curl_api.py +++ b/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)) diff --git a/tests/stubs.py b/tests/stubs.py index 0aef1c1737..d696e85972 100644 --- a/tests/stubs.py +++ b/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 diff --git a/tests/unit/test_api.py b/tests/unit/test_api.py index 6ca0041431..4c3593ef23 100644 --- a/tests/unit/test_api.py +++ b/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)