Merge "Remove race condition (in InstanceTypes)"

3 files changed, 119 insertions, 28 deletions

diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 081d6ede8..eb34ee301 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -3460,35 +3460,29 @@ def instance_type_create(context, values):
     {'extra_specs' : {'k1': 'v1', 'k2': 'v2', ...}}
 
     """
-    session = get_session()
-    with session.begin():
-        try:
-            instance_type_get_by_name(context, values['name'], session)
-            raise exception.InstanceTypeExists(name=values['name'])
-        except exception.InstanceTypeNotFoundByName:
-            pass
-        try:
-            instance_type_get_by_flavor_id(context, values['flavorid'],
-                                           session)
+    specs = values.get('extra_specs')
+    specs_refs = []
+    if specs:
+        for k, v in specs.iteritems():
+            specs_ref = models.InstanceTypeExtraSpecs()
+            specs_ref['key'] = k
+            specs_ref['value'] = v
+            specs_refs.append(specs_ref)
+
+    values['extra_specs'] = specs_refs
+    instance_type_ref = models.InstanceTypes()
+    instance_type_ref.update(values)
+
+    try:
+        instance_type_ref.save()
+    except db_exc.DBDuplicateEntry as e:
+        if 'flavorid' in e.columns:
             raise exception.InstanceTypeIdExists(flavor_id=values['flavorid'])
-        except exception.FlavorNotFound:
-            pass
-        try:
-            specs = values.get('extra_specs')
-            specs_refs = []
-            if specs:
-                for k, v in specs.iteritems():
-                    specs_ref = models.InstanceTypeExtraSpecs()
-                    specs_ref['key'] = k
-                    specs_ref['value'] = v
-                    specs_refs.append(specs_ref)
-            values['extra_specs'] = specs_refs
-            instance_type_ref = models.InstanceTypes()
-            instance_type_ref.update(values)
-            instance_type_ref.save(session=session)
-        except Exception, e:
-            raise db_exc.DBError(e)
-        return _dict_with_extra_specs(instance_type_ref)
+        raise exception.InstanceTypeExists(name=values['name'])
+    except Exception, e:
+        raise db_exc.DBError(e)
+
+    return _dict_with_extra_specs(instance_type_ref)
 
 
 def _dict_with_extra_specs(inst_type_query):
diff --git a/nova/db/sqlalchemy/migrate_repo/versions/172_add_instance_type_uc.py b/nova/db/sqlalchemy/migrate_repo/versions/172_add_instance_type_uc.py
new file mode 100644
index 000000000..1709f1130
--- /dev/null
+++ b/nova/db/sqlalchemy/migrate_repo/versions/172_add_instance_type_uc.py
@@ -0,0 +1,52 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright (c) 2013 Boris Pavlovic (boris@pavlovic.me).
+# 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 UniqueConstraint
+from sqlalchemy import MetaData, Table
+
+from nova.db.sqlalchemy import utils
+
+
+UC_FLAVOR = 'uniq_flavorid_x_deleted'
+FLAVOR_COLUMNS = ('flavorid', 'deleted')
+
+UC_NAME = 'uniq_name_x_deleted'
+NAME_COLUMNS = ('name', 'deleted')
+
+TABLE_NAME = 'instance_types'
+
+
+def upgrade(migrate_engine):
+    meta = MetaData(bind=migrate_engine)
+    t = Table(TABLE_NAME, meta, autoload=True)
+
+    utils.drop_old_duplicate_entries_from_table(migrate_engine, TABLE_NAME,
+                                                True, *FLAVOR_COLUMNS)
+    uc = UniqueConstraint(*FLAVOR_COLUMNS, table=t, name=UC_FLAVOR)
+    uc.create()
+
+    utils.drop_old_duplicate_entries_from_table(migrate_engine, TABLE_NAME,
+                                                True, *NAME_COLUMNS)
+    uc = UniqueConstraint(*NAME_COLUMNS, table=t, name=UC_NAME)
+    uc.create()
+
+
+def downgrade(migrate_engine):
+    utils.drop_unique_constraint(migrate_engine, TABLE_NAME, UC_FLAVOR,
+                                 *FLAVOR_COLUMNS)
+    utils.drop_unique_constraint(migrate_engine, TABLE_NAME, UC_NAME,
+                                 *NAME_COLUMNS)
diff --git a/nova/tests/test_migrations.py b/nova/tests/test_migrations.py
index 8a8dc93f4..98c50aaa7 100644
--- a/nova/tests/test_migrations.py
+++ b/nova/tests/test_migrations.py
@@ -1106,6 +1106,51 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn):
                 self.assertEqual(result['value'], original['value'])
                 self.assertEqual(result['created_at'], None)
 
+    def _pre_upgrade_172(self, engine):
+        instance_types = get_table(engine, 'instance_types')
+        data = [
+            dict(id=21, name='uc_name0', memory_mb=128, vcpus=1,
+                 root_gb=10, ephemeral_gb=0, flavorid="uc_flavor1", swap=0,
+                 rxtx_factor=1.0, vcpu_weight=1, disabled=False,
+                 is_public=True, deleted=0),
+            dict(id=22, name='uc_name1', memory_mb=128, vcpus=1,
+                 root_gb=10, ephemeral_gb=0, flavorid="uc_flavor1", swap=0,
+                 rxtx_factor=1.0, vcpu_weight=1, disabled=False,
+                 is_public=True, deleted=0),
+            dict(id=23, name='uc_name2', memory_mb=128, vcpus=1,
+                 root_gb=10, ephemeral_gb=0, flavorid="uc_flavor2", swap=0,
+                 rxtx_factor=1.0, vcpu_weight=1, disabled=False,
+                 is_public=True, deleted=0),
+            dict(id=24, name='uc_name2', memory_mb=128, vcpus=1,
+                 root_gb=10, ephemeral_gb=0, flavorid="uc_flavor3", swap=0,
+                 rxtx_factor=1.0, vcpu_weight=1, disabled=False,
+                 is_public=True, deleted=0),
+        ]
+        engine.execute(instance_types.insert(), data)
+        return data
+
+    def _check_172(self, engine, data):
+        instance_types = get_table(engine, 'instance_types')
+
+        not_deleted = instance_types.c.deleted != instance_types.c.id
+
+        # There is only one instance_type with flavor `uc_flavor1`
+        uc_flavor1_rows = instance_types.select().\
+                    where(instance_types.c.flavorid == 'uc_flavor1').\
+                    where(not_deleted).\
+                    execute().\
+                    fetchall()
+
+        self.assertEqual(1, len(uc_flavor1_rows))
+
+        # There is only one instance_type with name `uc_name2`
+        uc_name2_rows = instance_types.select().\
+                    where(instance_types.c.name == 'uc_name2').\
+                    where(not_deleted).\
+                    execute().\
+                    fetchall()
+        self.assertEqual(1, len(uc_name2_rows))
+
 
 class TestBaremetalMigrations(BaseMigrationTestCase, CommonTestsMixIn):
     """Test sqlalchemy-migrate migrations."""