From 5248653c7b14a6a01f2d5886d3d5c32d48e6ad34 Mon Sep 17 00:00:00 2001 From: Boris Pavlovic Date: Mon, 25 Mar 2013 00:50:54 +0400 Subject: Remove race condition (in InstanceTypeProjects) Soft delete all duplicate rows with the same (project_id, instance_type_id) except one with the biggest value in `id` column. Create UC on columns (project_id, instance_type_id, deleted) Add tests for migration Fix instance_type_access_add method Replace Select then Insert -> Try to Insert. blueprint db-enforce-unique-keys Change-Id: Id70509337e78f3bf778501f2c77aa03263580ef0 --- nova/db/sqlalchemy/api.py | 15 +++----- .../versions/174_add_instance_type_access_uc.py | 42 ++++++++++++++++++++++ nova/tests/test_migrations.py | 39 ++++++++++++++++++++ 3 files changed, 86 insertions(+), 10 deletions(-) create mode 100644 nova/db/sqlalchemy/migrate_repo/versions/174_add_instance_type_access_uc.py diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py index 838d0e79b..94047cfa1 100644 --- a/nova/db/sqlalchemy/api.py +++ b/nova/db/sqlalchemy/api.py @@ -3634,25 +3634,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.""" -- cgit