Merge "Remove race condition (in Networks)"

4 files changed, 76 insertions, 9 deletions

diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index c717871d1..b83b8e839 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -2070,12 +2070,6 @@ def network_count_reserved_ips(context, network_id):
 
 @require_admin_context
 def network_create_safe(context, values):
-    if values.get('vlan'):
-        if model_query(context, models.Network, read_deleted="no")\
-                      .filter_by(vlan=values['vlan'])\
-                      .first():
-            raise exception.DuplicateVlan(vlan=values['vlan'])
-
     network_ref = models.Network()
     network_ref['uuid'] = str(uuid.uuid4())
     network_ref.update(values)
@@ -2083,8 +2077,8 @@ def network_create_safe(context, values):
     try:
         network_ref.save()
         return network_ref
-    except IntegrityError:
-        return None
+    except db_session.DBDuplicateEntry:
+        raise exception.DuplicateVlan(vlan=values['vlan'])
 
 
 @require_admin_context
@@ -2323,7 +2317,10 @@ def network_update(context, network_id, values):
     with session.begin():
         network_ref = network_get(context, network_id, session=session)
         network_ref.update(values)
-        network_ref.save(session=session)
+        try:
+            network_ref.save(session=session)
+        except db_session.DBDuplicateEntry:
+            raise exception.DuplicateVlan(vlan=values['vlan'])
         return network_ref
 
 
diff --git a/nova/db/sqlalchemy/migrate_repo/versions/158_add_networks_uc.py b/nova/db/sqlalchemy/migrate_repo/versions/158_add_networks_uc.py
new file mode 100644
index 000000000..18644d140
--- /dev/null
+++ b/nova/db/sqlalchemy/migrate_repo/versions/158_add_networks_uc.py
@@ -0,0 +1,40 @@
+# 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_vlan_x_deleted"
+COLUMNS = ('vlan', 'deleted')
+TABLE_NAME = 'networks'
+
+
+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, *COLUMNS)
+    uc = UniqueConstraint(*COLUMNS, table=t, name=UC_NAME)
+    uc.create()
+
+
+def downgrade(migrate_engine):
+    utils.drop_unique_constraint(migrate_engine, TABLE_NAME, UC_NAME, *COLUMNS)
diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py
index cf81cca74..346e0b2b7 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -334,6 +334,16 @@ class DbApiTestCase(test.TestCase):
         self.assertRaises(exception.DuplicateVlan,
                           db.network_create_safe, ctxt, values2)
 
+    def test_network_update_with_duplicate_vlan(self):
+        ctxt = context.get_admin_context()
+        values1 = {'host': 'localhost', 'project_id': 'project1', 'vlan': 1}
+        values2 = {'host': 'something', 'project_id': 'project1', 'vlan': 2}
+        network_ref = db.network_create_safe(ctxt, values1)
+        db.network_create_safe(ctxt, values2)
+        self.assertRaises(exception.DuplicateVlan,
+                          db.network_update,
+                          ctxt, network_ref["id"], values2)
+
     def test_instance_update_with_instance_uuid(self):
         # test instance_update() works when an instance UUID is passed.
         ctxt = context.get_admin_context()
diff --git a/nova/tests/test_migrations.py b/nova/tests/test_migrations.py
index fafe3348f..6f55ce846 100644
--- a/nova/tests/test_migrations.py
+++ b/nova/tests/test_migrations.py
@@ -860,6 +860,26 @@ class TestNovaMigrations(BaseMigrationTestCase):
         # recheck the 149 data
         self._check_149(engine, data)
 
+    def _prerun_158(self, engine):
+        networks = get_table(engine, 'networks')
+        data = [
+            {'vlan': 1, 'deleted': 0},
+            {'vlan': 1, 'deleted': 0},
+            {'vlan': 1, 'deleted': 0},
+        ]
+
+        for item in data:
+            networks.insert().values(item).execute()
+        return data
+
+    def _check_158(self, engine, data):
+        networks = get_table(engine, 'networks')
+        rows = networks.select().\
+                    where(networks.c.deleted != networks.c.id).\
+                    execute().\
+                    fetchall()
+        self.assertEqual(len(rows), 1)
+
 
 class TestBaremetalMigrations(BaseMigrationTestCase):
     """Test sqlalchemy-migrate migrations."""