diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py index 40b3af40d74..acf824fa3ae 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 8c4a4caceec..0f5e11fcd51 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 c2272902217..78033be7eaa 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 00000000000..d755853a075 --- /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 6207e214da7..542d0358603 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 c9dc09a1913..a857cf69e99 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 3cabca28d41..95433c1ffb6 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 cbce5c07880..baa46f98cfa 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 9f23a142665..f6eaf6c6eef 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 3395cc447dd..8b639690eee 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 af0a332f3ff..f8d4fac998a 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 f4ac4dc14a0..82c513b9c44 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={}):