From bb247ef96bf551c1b3b0ea0f3f9ed88eb16751a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Ribaud?= Date: Wed, 6 Mar 2024 10:56:19 +0100 Subject: [PATCH] Amend DB model add a unique constraint. This update adds a constraint between instance_uuid and share_id to make sure it will be unique. It aims to avoid potential conflicts from simultaneous API creation requests, which could result in the creation of duplicate records in the database. Manila is the OpenStack Shared Filesystems service. These series of patches implement changes required in Nova to allow the shares provided by Manila to be associated with and attached to instances using virtiofs. Implements: blueprint libvirt-virtiofs-attach-manila-shares Change-Id: I419fd40baf9acf487154eebc77c181633ea53272 --- ...03_add_constraint_instance_share_avoid_.py | 75 ++++++++++++++++++ nova/db/main/models.py | 10 +++ nova/tests/unit/db/main/test_api.py | 77 +++++++++++++++++++ nova/tests/unit/db/main/test_migrations.py | 10 +++ 4 files changed, 172 insertions(+) create mode 100644 nova/db/main/migrations/versions/d60bddf7a903_add_constraint_instance_share_avoid_.py diff --git a/nova/db/main/migrations/versions/d60bddf7a903_add_constraint_instance_share_avoid_.py b/nova/db/main/migrations/versions/d60bddf7a903_add_constraint_instance_share_avoid_.py new file mode 100644 index 0000000000..4f2a157390 --- /dev/null +++ b/nova/db/main/migrations/versions/d60bddf7a903_add_constraint_instance_share_avoid_.py @@ -0,0 +1,75 @@ +# 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. + +"""add_constraint_instance_share_avoid_duplicates + +Revision ID: d60bddf7a903 +Revises: 13863f4e1612 +Create Date: 2024-03-06 17:05:29.361678 +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'd60bddf7a903' +down_revision = '13863f4e1612' +branch_labels = None +depends_on = None + + +def upgrade(): + op.drop_table("share_mapping") + op.create_table( + "share_mapping", + sa.Column("created_at", sa.DateTime), + sa.Column("updated_at", sa.DateTime), + sa.Column( + "id", + sa.BigInteger().with_variant(sa.Integer, "sqlite"), + primary_key=True, + autoincrement=True, + nullable=False, + ), + sa.Column("uuid", sa.String(36)), + sa.Column( + "instance_uuid", + sa.String(length=36), + sa.ForeignKey( + "instances.uuid", name="share_mapping_instance_uuid_fkey" + ), + ), + sa.Column("share_id", sa.String(length=36)), + sa.Column("status", sa.String(length=32)), + sa.Column("tag", sa.String(48)), + sa.Column("export_location", sa.Text), + sa.Column("share_proto", sa.String(32)), + sa.UniqueConstraint( + "instance_uuid", + "share_id", + name="uniq_key_pairs0instance_uuid0share_id", + ), + sa.UniqueConstraint( + "instance_uuid", + "tag", + name="uniq_key_pairs0instance_uuid0tag", + ), + sa.Index("share_idx", "share_id"), + sa.Index( + "share_mapping_instance_uuid_share_id_idx", + "instance_uuid", + "share_id", + ), + mysql_engine="InnoDB", + mysql_charset="utf8", + ) diff --git a/nova/db/main/models.py b/nova/db/main/models.py index 3fbe209049..903ef267c9 100644 --- a/nova/db/main/models.py +++ b/nova/db/main/models.py @@ -761,6 +761,16 @@ class ShareMapping(BASE, NovaBase): sa.Index('share_idx', 'share_id'), sa.Index('share_mapping_instance_uuid_share_id_idx', 'instance_uuid', 'share_id'), + sa.UniqueConstraint( + "instance_uuid", + "share_id", + name="uniq_key_pairs0instance_uuid0share_id", + ), + sa.UniqueConstraint( + "instance_uuid", + "tag", + name="uniq_key_pairs0instance_uuid0tag", + ), ) # sqlite> create table my_table(id bigint primary key AUTOINCREMENT, # name text); diff --git a/nova/tests/unit/db/main/test_api.py b/nova/tests/unit/db/main/test_api.py index 87fcd00b3b..d8dcd5734c 100644 --- a/nova/tests/unit/db/main/test_api.py +++ b/nova/tests/unit/db/main/test_api.py @@ -4288,6 +4288,83 @@ class ShareMappingDBApiTestCase(test.TestCase): self._compare( share, expected_share_mappings_after_update[share.share_id]) + def test_share_mapping_duplicate(self): + ctxt = context.get_admin_context() + expected_share_mappings = self.create_test_data(ctxt) + + share_mappings = db.share_mapping_get_all(ctxt) + self.assertEqual(len(share_mappings), 2) + for share in share_mappings: + self._compare( + share, expected_share_mappings[share.share_id]) + + # Assuming we have a race condition, make sure we cannot create a + # duplicate entry with the same instance_uuid and share_id + with mock.patch( + "nova.db.main.api.share_mapping_get_by_instance_uuid_and_share_id" + ) as mock_db: + mock_db.return_value = None + + exc = self.assertRaises( + db_exc.DBDuplicateEntry, + db.share_mapping_update, + ctxt, + uuid='fake-uuid3', + instance_uuid='fake-instance-uuid1', + share_id='1', + status='attached', + tag='fake-tag3', + export_location='fake-export_location1', + share_proto='NFS' + ) + + self.assertIn( + ( + "UNIQUE constraint failed: " + "share_mapping.instance_uuid, share_mapping.share_id" + ), + str(exc), + ) + + def test_share_mapping_using_same_tag(self): + ctxt = context.get_admin_context() + expected_share_mappings = self.create_test_data(ctxt) + + share_mappings = db.share_mapping_get_all(ctxt) + self.assertEqual(len(share_mappings), 2) + for share in share_mappings: + self._compare( + share, expected_share_mappings[share.share_id]) + + # Avoid the same tag name with the same instance. Resulting with: + # libvirt.libvirtError: unsupported configuration: filesystem target + # 'my-share' specified twice + with mock.patch( + "nova.db.main.api.share_mapping_get_by_instance_uuid_and_share_id" + ) as mock_db: + mock_db.return_value = None + + exc = self.assertRaises( + db_exc.DBDuplicateEntry, + db.share_mapping_update, + ctxt, + uuid='fake-uuid3', + instance_uuid='fake-instance-uuid1', + share_id='3', + status='attached', + tag='fake-tag1', + export_location='fake-export_location1', + share_proto='NFS' + ) + + self.assertIn( + ( + "UNIQUE constraint failed: " + "share_mapping.instance_uuid, share_mapping.tag" + ), + str(exc), + ) + def test_share_mapping_get_by_share_id(self): ctxt = context.get_admin_context() expected_share_mappings = self.create_test_data(ctxt) diff --git a/nova/tests/unit/db/main/test_migrations.py b/nova/tests/unit/db/main/test_migrations.py index f3f46758d8..8619124d29 100644 --- a/nova/tests/unit/db/main/test_migrations.py +++ b/nova/tests/unit/db/main/test_migrations.py @@ -364,6 +364,16 @@ class NovaMigrationsWalk( self.assertForeignKeyExists( connection, 'share_mapping', 'instance_uuid') + def _pre_upgrade_d60bddf7a903(self, connection): + # Verifying the presence or absence of the uniqueness constraint + # does not seem trivial, especially for SQLite. Instead of checking + # here, the test "test_share_mapping_duplicate" ensures that we cannot + # introduce two identical keys into the share_mapping table. + pass + + def _check_d60bddf7a903(self, connection): + pass + def test_single_base_revision(self): """Ensure we only have a single base revision.