From: john-griffith Date: Sun, 2 Dec 2012 07:01:49 +0000 (-0700) Subject: Convert volume_type id from int to uuid. X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=51a438c8f3f0d13c6779602264d72cde82feab9c;p=openstack-build%2Fcinder-build.git Convert volume_type id from int to uuid. This converts the volume_type id from int to uuid. In addition, this also corrects an issue of deleting volume_types by name. Even though the client/api is taking the id, it was converting to the name of the volume_type for the db access. We also want to continue enforcing that the name is unique on creation so as to avoid any confusion and for clarity in Horizon. The api has also been enhanced to allow you to specify the type by name or by uuid. This does NOT modify the things like the volume details display field (currently shows the type 'name' not 'uuid'), these types of changes will go in to V2 of the API. Implements blueprint vol-type-to-uuid Change-Id: I1c54ff2a1e0c5df5891408fc11b15176db4028c3 --- diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py index 40b3af40d..acf824fa3 100644 --- a/cinder/api/v1/volumes.py +++ b/cinder/api/v1/volumes.py @@ -294,12 +294,21 @@ class VolumeController(wsgi.Controller): req_volume_type = volume.get('volume_type', None) if req_volume_type: - try: - kwargs['volume_type'] = volume_types.get_volume_type_by_name( - context, req_volume_type) - except exception.VolumeTypeNotFound: - explanation = 'Volume type not found.' - raise exc.HTTPNotFound(explanation=explanation) + if not uuidutils.is_uuid_like(req_volume_type): + try: + kwargs['volume_type'] = \ + volume_types.get_volume_type_by_name( + context, req_volume_type) + except exception.VolumeTypeNotFound: + explanation = 'Volume type not found.' + raise exc.HTTPNotFound(explanation=explanation) + else: + try: + kwargs['volume_type'] = volume_types.get_volume_type( + context, req_volume_type) + except exception.VolumeTypeNotFound: + explanation = 'Volume type not found.' + raise exc.HTTPNotFound(explanation=explanation) kwargs['metadata'] = volume.get('metadata', None) diff --git a/cinder/db/api.py b/cinder/db/api.py index 8c4a4cace..0f5e11fcd 100644 --- a/cinder/db/api.py +++ b/cinder/db/api.py @@ -353,9 +353,9 @@ def volume_type_get_by_name(context, name): return IMPL.volume_type_get_by_name(context, name) -def volume_type_destroy(context, name): +def volume_type_destroy(context, id): """Delete a volume type.""" - return IMPL.volume_type_destroy(context, name) + return IMPL.volume_type_destroy(context, id) def volume_get_active_by_window(context, begin, end=None, project_id=None): diff --git a/cinder/db/sqlalchemy/api.py b/cinder/db/sqlalchemy/api.py index c22729022..78033be7e 100644 --- a/cinder/db/sqlalchemy/api.py +++ b/cinder/db/sqlalchemy/api.py @@ -1313,13 +1313,21 @@ def volume_type_create(context, values): {'extra_specs' : {'k1': 'v1', 'k2': 'v2', ...}} """ + if not values.get('id'): + values['id'] = str(uuid.uuid4()) + session = get_session() with session.begin(): try: volume_type_get_by_name(context, values['name'], session) - raise exception.VolumeTypeExists(name=values['name']) + raise exception.VolumeTypeExists(id=values['name']) except exception.VolumeTypeNotFoundByName: pass + try: + volume_type_get(context, values['id'], session) + raise exception.VolumeTypeExists(id=values['id']) + except exception.VolumeTypeNotFound: + pass try: values['extra_specs'] = _metadata_refs(values.get('extra_specs'), models.VolumeTypeExtraSpecs) @@ -1383,19 +1391,18 @@ def volume_type_get_by_name(context, name, session=None): @require_admin_context -def volume_type_destroy(context, name): +def volume_type_destroy(context, id): + volume_type_get(context, id) + session = get_session() with session.begin(): - volume_type_ref = volume_type_get_by_name(context, name, - session=session) - volume_type_id = volume_type_ref['id'] session.query(models.VolumeTypes).\ - filter_by(id=volume_type_id).\ + filter_by(id=id).\ update({'deleted': True, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}) session.query(models.VolumeTypeExtraSpecs).\ - filter_by(volume_type_id=volume_type_id).\ + filter_by(volume_type_id=id).\ update({'deleted': True, 'deleted_at': timeutils.utcnow(), 'updated_at': literal_column('updated_at')}) diff --git a/cinder/db/sqlalchemy/migrate_repo/versions/004_volume_type_to_uuid.py b/cinder/db/sqlalchemy/migrate_repo/versions/004_volume_type_to_uuid.py new file mode 100644 index 000000000..d755853a0 --- /dev/null +++ b/cinder/db/sqlalchemy/migrate_repo/versions/004_volume_type_to_uuid.py @@ -0,0 +1,155 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# 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. + +import uuid + +from cinder.openstack.common import log as logging +from migrate import ForeignKeyConstraint +from sqlalchemy import Integer, MetaData, String, Table + +LOG = logging.getLogger(__name__) + + +def upgrade(migrate_engine): + """Convert volume_type_id to UUID.""" + meta = MetaData() + meta.bind = migrate_engine + + volumes = Table('volumes', meta, autoload=True) + volume_types = Table('volume_types', meta, autoload=True) + extra_specs = Table('volume_type_extra_specs', meta, autoload=True) + + fkey_remove_list = [volumes.c.volume_type_id, + volume_types.c.id, + extra_specs.c.volume_type_id] + + for column in fkey_remove_list: + fkeys = list(column.foreign_keys) + if fkeys: + fkey_name = fkeys[0].constraint.name + fkey = ForeignKeyConstraint(columns=[column], + refcolumns=[volume_types.c.id], + name=fkey_name) + + try: + fkey.drop() + except Exception: + if migrate_engine.url.get_dialect().name.startswith('sqlite'): + pass + else: + raise + + volumes.c.volume_type_id.alter(String(36)) + volume_types.c.id.alter(String(36)) + extra_specs.c.volume_type_id.alter(String(36)) + + vtype_list = list(volume_types.select().execute()) + for t in vtype_list: + new_id = str(uuid.uuid4()) + + volumes.update().\ + where(volumes.c.volume_type_id == t['id']).\ + values(volume_type_id=new_id).execute() + + extra_specs.update().\ + where(extra_specs.c.volume_type_id == t['id']).\ + values(volume_type_id=new_id).execute() + + volume_types.update().\ + where(volume_types.c.id == t['id']).\ + values(id=new_id).execute() + + for column in fkey_remove_list: + fkeys = list(column.foreign_keys) + if fkeys: + fkey_name = fkeys[0].constraint.name + fkey = ForeignKeyConstraint(columns=[column], + refcolumns=[volume_types.c.id], + name=fkey_name) + try: + fkey.create() + LOG.info('Created foreign key %s' % fkey_name) + except Exception: + if migrate_engine.url.get_dialect().name.startswith('sqlite'): + pass + else: + raise + + +def downgrade(migrate_engine): + """Convert volume_type from UUID back to int.""" + meta = MetaData() + meta.bind = migrate_engine + + volumes = Table('volumes', meta, autoload=True) + volume_types = Table('volume_types', meta, autoload=True) + extra_specs = Table('volume_type_extra_specs', meta, autoload=True) + + fkey_remove_list = [volumes.c.volume_type_id, + volume_types.c.id, + extra_specs.c.volume_type_id] + + for column in fkey_remove_list: + fkeys = list(column.foreign_keys) + if fkeys: + fkey_name = fkeys[0].constraint.name + fkey = ForeignKeyConstraint(columns=[column], + refcolumns=[volume_types.c.id], + name=fkey_name) + + try: + fkey.drop() + except Exception: + if migrate_engine.url.get_dialect().name.startswith('sqlite'): + pass + else: + raise + + volumes.c.volume_type_id.alter(Integer) + volume_types.c.id.alter(Integer) + extra_specs.c.volume_type_id.alter(Integer) + + vtype_list = list(volume_types.select().execute()) + new_id = 1 + + for t in vtype_list: + volumes.update().\ + where(volumes.c.volume_type_id == t['id']).\ + values(volume_type_id=new_id).execute() + + extra_specs.update().\ + where(extra_specs.c.volume_type_id == t['id']).\ + values(volume_type_id=new_id).execute() + + volume_types.update().\ + where(volume_types.c.id == t['id']).\ + values(id=new_id).execute() + + new_id += 1 + + for column in fkey_remove_list: + fkeys = list(column.foreign_keys) + if fkeys: + fkey_name = fkeys[0].constraint.name + fkey = ForeignKeyConstraint(columns=[column], + refcolumns=[volume_types.c.id], + name=fkey_name) + try: + fkey.create() + LOG.info('Created foreign key %s' % fkey_name) + except Exception: + if migrate_engine.url.get_dialect().name.startswith('sqlite'): + pass + else: + raise diff --git a/cinder/db/sqlalchemy/models.py b/cinder/db/sqlalchemy/models.py index 6207e214d..542d03586 100644 --- a/cinder/db/sqlalchemy/models.py +++ b/cinder/db/sqlalchemy/models.py @@ -155,7 +155,7 @@ class Volume(BASE, CinderBase): provider_location = Column(String(255)) provider_auth = Column(String(255)) - volume_type_id = Column(Integer) + volume_type_id = Column(String(36)) class VolumeMetadata(BASE, CinderBase): @@ -175,7 +175,7 @@ class VolumeMetadata(BASE, CinderBase): class VolumeTypes(BASE, CinderBase): """Represent possible volume_types of volumes offered.""" __tablename__ = "volume_types" - id = Column(Integer, primary_key=True) + id = Column(String(36), primary_key=True) name = Column(String(255)) volumes = relationship(Volume, @@ -192,7 +192,7 @@ class VolumeTypeExtraSpecs(BASE, CinderBase): id = Column(Integer, primary_key=True) key = Column(String(255)) value = Column(String(255)) - volume_type_id = Column(Integer, + volume_type_id = Column(String(36), ForeignKey('volume_types.id'), nullable=False) volume_type = relationship( diff --git a/cinder/exception.py b/cinder/exception.py index c9dc09a19..a857cf69e 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -372,7 +372,7 @@ class KeyPairExists(Duplicate): class VolumeTypeExists(Duplicate): - message = _("Volume Type %(name)s already exists.") + message = _("Volume Type %(id)s already exists.") class MigrationError(CinderException): diff --git a/cinder/tests/api/openstack/fakes.py b/cinder/tests/api/openstack/fakes.py index 3cabca28d..95433c1ff 100644 --- a/cinder/tests/api/openstack/fakes.py +++ b/cinder/tests/api/openstack/fakes.py @@ -195,7 +195,7 @@ def stub_volume(id, **kwargs): 'display_description': 'displaydesc', 'created_at': datetime.datetime(1, 1, 1, 1, 1, 1), 'snapshot_id': None, - 'volume_type_id': 'fakevoltype', + 'volume_type_id': '3e196c20-3c06-11e2-81c1-0800200c9a66', 'volume_metadata': [], 'volume_type': {'name': 'vol_type_name'}} diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py index cbce5c078..baa46f98c 100644 --- a/cinder/tests/api/v1/test_volumes.py +++ b/cinder/tests/api/v1/test_volumes.py @@ -95,7 +95,6 @@ class VolumeApiTest(test.TestCase): vol_type = FLAGS.default_volume_type db.volume_type_create(context.get_admin_context(), dict(name=vol_type, extra_specs={})) - db_vol_type = db.volume_type_get_by_name(context.get_admin_context(), vol_type) diff --git a/cinder/tests/test_migrations.py b/cinder/tests/test_migrations.py index 9f23a1426..f6eaf6c6e 100644 --- a/cinder/tests/test_migrations.py +++ b/cinder/tests/test_migrations.py @@ -303,3 +303,33 @@ class TestMigrations(test.TestCase): self.assertEqual(version, migration_api.db_version(engine, TestMigrations.REPOSITORY)) + + def test_migration_004(self): + """Test that volume_type_id migration works correctly.""" + for (key, engine) in self.engines.items(): + migration_api.version_control(engine, + TestMigrations.REPOSITORY, + migration.INIT_VERSION) + migration_api.upgrade(engine, TestMigrations.REPOSITORY, 3) + metadata = sqlalchemy.schema.MetaData() + metadata.bind = engine + + migration_api.upgrade(engine, TestMigrations.REPOSITORY, 4) + volumes = sqlalchemy.Table('volumes', + metadata, + autoload=True) + volume_types = sqlalchemy.Table('volume_types', + metadata, + autoload=True) + extra_specs = sqlalchemy.Table('volume_type_extra_specs', + metadata, + autoload=True) + + self.assertTrue(isinstance(volumes.c.volume_type_id.type, + sqlalchemy.types.VARCHAR)) + self.assertTrue(isinstance(volume_types.c.id.type, + sqlalchemy.types.VARCHAR)) + self.assertTrue(isinstance(extra_specs.c.volume_type_id.type, + sqlalchemy.types.VARCHAR)) + + self.assertTrue(extra_specs.c.volume_type_id.foreign_keys) diff --git a/cinder/tests/test_volume_types.py b/cinder/tests/test_volume_types.py index 3395cc447..8b639690e 100644 --- a/cinder/tests/test_volume_types.py +++ b/cinder/tests/test_volume_types.py @@ -49,9 +49,9 @@ class VolumeTypeTestCase(test.TestCase): """Ensure volume types can be created and deleted.""" prev_all_vtypes = volume_types.get_all_types(self.ctxt) - volume_types.create(self.ctxt, - self.vol_type1_name, - self.vol_type1_specs) + type_ref = volume_types.create(self.ctxt, + self.vol_type1_name, + self.vol_type1_specs) new = volume_types.get_volume_type_by_name(self.ctxt, self.vol_type1_name) @@ -67,7 +67,7 @@ class VolumeTypeTestCase(test.TestCase): len(new_all_vtypes), 'drive type was not created') - volume_types.destroy(self.ctxt, self.vol_type1_name) + volume_types.destroy(self.ctxt, type_ref['id']) new_all_vtypes = volume_types.get_all_types(self.ctxt) self.assertEqual(prev_all_vtypes, new_all_vtypes, @@ -82,9 +82,9 @@ class VolumeTypeTestCase(test.TestCase): def test_get_default_volume_type(self): """Ensures default volume type can be retrieved.""" - volume_types.create(self.ctxt, - fake_flags.def_vol_type, - {}) + type_ref = volume_types.create(self.ctxt, + fake_flags.def_vol_type, + {}) default_vol_type = volume_types.get_default_volume_type() self.assertEqual(default_vol_type.get('name'), fake_flags.def_vol_type) @@ -98,15 +98,15 @@ class VolumeTypeTestCase(test.TestCase): def test_non_existent_vol_type_shouldnt_delete(self): """Ensures that volume type creation fails with invalid args.""" - self.assertRaises(exception.VolumeTypeNotFoundByName, + self.assertRaises(exception.VolumeTypeNotFound, volume_types.destroy, self.ctxt, "sfsfsdfdfs") def test_repeated_vol_types_shouldnt_raise(self): """Ensures that volume duplicates don't raise.""" new_name = self.vol_type1_name + "dup" - volume_types.create(self.ctxt, new_name) - volume_types.destroy(self.ctxt, new_name) - volume_types.create(self.ctxt, new_name) + type_ref = volume_types.create(self.ctxt, new_name) + volume_types.destroy(self.ctxt, type_ref['id']) + type_ref = volume_types.create(self.ctxt, new_name) def test_invalid_volume_types_params(self): """Ensures that volume type creation fails with invalid args.""" diff --git a/cinder/tests/test_volume_types_extra_specs.py b/cinder/tests/test_volume_types_extra_specs.py index af0a332f3..f8d4fac99 100644 --- a/cinder/tests/test_volume_types_extra_specs.py +++ b/cinder/tests/test_volume_types_extra_specs.py @@ -45,9 +45,9 @@ class VolumeTypeExtraSpecsTestCase(test.TestCase): def tearDown(self): # Remove the volume type from the database db.volume_type_destroy(context.get_admin_context(), - self.vol_type1['name']) + self.vol_type1['id']) db.volume_type_destroy(context.get_admin_context(), - self.vol_type2_noextra['name']) + self.vol_type2_noextra['id']) super(VolumeTypeExtraSpecsTestCase, self).tearDown() def test_volume_type_specs_get(self): diff --git a/cinder/volume/volume_types.py b/cinder/volume/volume_types.py index f4ac4dc14..82c513b9c 100644 --- a/cinder/volume/volume_types.py +++ b/cinder/volume/volume_types.py @@ -34,22 +34,23 @@ LOG = logging.getLogger(__name__) def create(context, name, extra_specs={}): """Creates volume types.""" try: - db.volume_type_create(context, - dict(name=name, - extra_specs=extra_specs)) + type_ref = db.volume_type_create(context, + dict(name=name, + extra_specs=extra_specs)) except exception.DBError, e: LOG.exception(_('DB error: %s') % e) raise exception.VolumeTypeCreateFailed(name=name, extra_specs=extra_specs) + return type_ref -def destroy(context, name): +def destroy(context, id): """Marks volume types as deleted.""" - if name is None: - msg = _("name cannot be None") + if id is None: + msg = _("id cannot be None") raise exception.InvalidVolumeType(reason=msg) else: - db.volume_type_destroy(context, name) + db.volume_type_destroy(context, id) def get_all_types(context, inactive=0, search_opts={}):