Merge "Remove race condition (in InstanceTypeProjects)"

3 files changed, 86 insertions, 10 deletions

diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index 6d51deb1c..354341998 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -3714,25 +3714,20 @@ def instance_type_access_get_by_flavor_id(context, flavor_id):
 @require_admin_context
 def instance_type_access_add(context, flavor_id, project_id):
     """Add given tenant to the flavor access list."""
-    # NOTE(boris-42): There is a race condition in this method and it will be
-    #                 rewritten after bp/db-unique-keys implementation.
     session = get_session()
     with session.begin():
         instance_type_ref = instance_type_get_by_flavor_id(context, flavor_id,
                                                            session=session)
         instance_type_id = instance_type_ref['id']
-        access_ref = _instance_type_access_query(context, session=session).\
-                        filter_by(instance_type_id=instance_type_id).\
-                        filter_by(project_id=project_id).\
-                        first()
-        if access_ref:
-            raise exception.FlavorAccessExists(flavor_id=flavor_id,
-                                               project_id=project_id)
 
         access_ref = models.InstanceTypeProjects()
         access_ref.update({"instance_type_id": instance_type_id,
                            "project_id": project_id})
-        access_ref.save(session=session)
+        try:
+            access_ref.save(session=session)
+        except db_exc.DBDuplicateEntry:
+            raise exception.FlavorAccessExists(flavor_id=flavor_id,
+                                               project_id=project_id)
         return access_ref
 
 
diff --git a/nova/db/sqlalchemy/migrate_repo/versions/174_add_instance_type_access_uc.py b/nova/db/sqlalchemy/migrate_repo/versions/174_add_instance_type_access_uc.py
new file mode 100644
index 000000000..402fe6ce9
--- /dev/null
+++ b/nova/db/sqlalchemy/migrate_repo/versions/174_add_instance_type_access_uc.py
@@ -0,0 +1,42 @@
+# 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_NAME = 'uniq_instance_type_id_x_project_id_x_deleted'
+UC_COLUMNS = ('instance_type_id', 'project_id', 'deleted')
+
+TABLE_NAME = 'instance_type_projects'
+
+
+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, *UC_COLUMNS)
+    uc = UniqueConstraint(*UC_COLUMNS, table=t, name=UC_NAME)
+    uc.create()
+
+
+def downgrade(migrate_engine):
+    utils.drop_unique_constraint(migrate_engine, TABLE_NAME, UC_NAME,
+                                 *UC_COLUMNS)
diff --git a/nova/tests/test_migrations.py b/nova/tests/test_migrations.py
index 94854140d..749494ea7 100644
--- a/nova/tests/test_migrations.py
+++ b/nova/tests/test_migrations.py
@@ -1190,6 +1190,45 @@ class TestNovaMigrations(BaseMigrationTestCase, CommonTestsMixIn):
         # Ensure the number of unique keypairs is correct
         self.assertEqual(len(rows), 2)
 
+    def _pre_upgrade_174(self, engine):
+        instance_types = get_table(engine, 'instance_types')
+        instance_type_projects = get_table(engine, 'instance_type_projects')
+
+        instance_type_data = [
+            dict(id=31, name='itp_name0', memory_mb=128, vcpus=1,
+                 root_gb=10, ephemeral_gb=0, flavorid="itp_flavor1", swap=0,
+                 rxtx_factor=1.0, vcpu_weight=1, disabled=False,
+                 is_public=True, deleted=0)
+        ]
+        instance_type_projects_data = [
+            dict(project_id='pr1', instance_type_id=31, deleted=0),
+            dict(project_id='pr1', instance_type_id=31, deleted=0),
+            dict(project_id='pr2', instance_type_id=31, deleted=0)
+        ]
+
+        engine.execute(instance_types.insert(), instance_type_data)
+        engine.execute(instance_type_projects.insert(),
+                       instance_type_projects_data)
+
+    def _check_174(self, engine, data):
+        it_projects = get_table(engine, 'instance_type_projects')
+
+        def get_(project_id, it_id, deleted):
+            deleted_value = 0 if not deleted else it_projects.c.id
+            return it_projects.select().\
+                        where(it_projects.c.project_id == project_id).\
+                        where(it_projects.c.instance_type_id == it_id).\
+                        where(it_projects.c.deleted == deleted_value).\
+                        execute().\
+                        fetchall()
+
+        self.assertEqual(1, len(get_('pr1', '31', False)))
+        self.assertEqual(1, len(get_('pr1', '31', True)))
+        self.assertEqual(1, len(get_('pr2', '31', False)))
+        self.assertRaises(sqlalchemy.exc.IntegrityError,
+                          it_projects.insert().execute,
+                          dict(instance_type=31, project_id='pr1', deleted=0))
+
 
 class TestBaremetalMigrations(BaseMigrationTestCase, CommonTestsMixIn):
     """Test sqlalchemy-migrate migrations."""