Clean old temporary tracking

On Xena we added the use_quota DB field to volumes and snapshots to
unify the tracking of temporary resources, but we still had to keep
compatibility code for the old mechanisms (due to rolling
upgrades).

This patch removes compatibility code with the old mechanism and adds
additional cleanup code to remove the tracking in the volume metadata.

Change-Id: I3f9ed65b0fe58f7b7a0867c0e5ebc0ac3c703b05

cinder/cmd/manage.py

@@ -159,13 +159,8 @@ class DbCommands(object):
     #    db.service_uuids_online_data_migration,
     online_migrations: Tuple[Callable[[context.RequestContext, int],
                                       Tuple[int, int]], ...] = (
-        # TODO: (Z Release) Remove next line and this comment
+        # TODO: (D Release) Remove next line and this comment
-        # TODO: (Y Release) Uncomment next line and remove this comment
+        db.remove_temporary_admin_metadata_data_migration,
-        # db.remove_temporary_admin_metadata_data_migration,
-
-        # TODO: (Y Release) Remove next 2 line and this comment
-        db.volume_use_quota_online_data_migration,
-        db.snapshot_use_quota_online_data_migration,
     )
 
     def __init__(self):

cinder/db/api.py

@@ -1978,17 +1978,6 @@ def attachment_specs_update_or_create(context,
 ###################
 
 
-# TODO: (Y Release) remove method and this comment
+# TODO: (D Release) remove method and this comment
-def volume_use_quota_online_data_migration(context, max_count):
+def remove_temporary_admin_metadata_data_migration(context, max_count):
-    return IMPL.volume_use_quota_online_data_migration(context, max_count)
+    IMPL.remove_temporary_admin_metadata_data_migration(context, max_count)
-
-
-# TODO: (Y Release) remove method and this comment
-def snapshot_use_quota_online_data_migration(context, max_count):
-    return IMPL.snapshot_use_quota_online_data_migration(context, max_count)
-
-
-# TODO: (Z Release) remove method and this comment
-# TODO: (Y Release) uncomment method
-# def remove_temporary_admin_metadata_data_migration(context, max_count):
-#     IMPL.remove_temporary_admin_metadata_data_migration(context, max_count)

cinder/db/migrations/versions/9ab1b092a404_make_use_quota_non_nullable.py

@@ -0,0 +1,53 @@
+# 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.
+
+"""Make use_quota non nullable
+
+Revision ID: 9ab1b092a404
+Revises: b8660621f1b9
+Create Date: 2021-10-22 16:23:17.080934
+"""
+
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = '9ab1b092a404'
+down_revision = 'b8660621f1b9'
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # It's safe to set them as non nullable because when we run db sync on this
+    # release the online migrations from the previous release must already have
+    # been run.
+    connection = op.get_bind()
+    # SQLite doesn't support dropping/altering tables, so we use a workaround
+    if connection.engine.name == 'sqlite':
+        with op.batch_alter_table('volumes') as batch_op:
+            batch_op.alter_column('use_quota',
+                                  existing_type=sa.BOOLEAN,
+                                  nullable=False, server_default=sa.true())
+        with op.batch_alter_table('snapshots') as batch_op:
+            batch_op.alter_column('use_quota',
+                                  existing_type=sa.BOOLEAN,
+                                  nullable=False, server_default=sa.true())
+
+    else:
+        op.alter_column('volumes', 'use_quota',
+                        existing_type=sa.BOOLEAN,
+                        nullable=False, server_default=sa.true())
+        op.alter_column('snapshots', 'use_quota',
+                        existing_type=sa.BOOLEAN,
+                        nullable=False, server_default=sa.true())

cinder/db/sqlalchemy/api.py

@@ -2079,32 +2079,10 @@ def _volume_data_get_for_project(
         context, func.count(model.id), func.sum(model.size), read_deleted="no"
     ).filter_by(project_id=project_id)
 
-    # When calling the method for quotas we don't count volumes that are the
+    # By default we skip temporary resources creted for internal usage and
-    # destination of a migration since they were not accounted for quotas or
+    # migration destination volumes.
-    # reservations in the first place.
-    # Also skip temporary volumes that have 'temporary' admin_metadata key set
-    # to True.
     if skip_internal:
-        # TODO: (Y release) replace everything inside this if with:
+        query = query.filter(model.use_quota)
-        #       query = query.filter(model.use_quota)
-        admin_model = models.VolumeAdminMetadata
-        query = query.filter(
-            and_(
-                or_(
-                    model.migration_status.is_(None),
-                    ~model.migration_status.startswith('target:'),
-                ),
-                ~model.use_quota.is_(False),
-                ~sql.exists().where(
-                    and_(
-                        model.id == admin_model.volume_id,
-                        ~admin_model.deleted,
-                        admin_model.key == 'temporary',
-                        admin_model.value == 'True',
-                    )
-                ),
-            )
-        )
 
     if host:
         query = query.filter(_filter_host(model.host, host))
@@ -4094,9 +4072,7 @@ def _snapshot_data_get_for_project(
         read_deleted="no",
     )
     if skip_internal:
-        # TODO: (Y release) replace next line with:
+        query = query.filter(models.Snapshot.use_quota)
-        #        query = query.filter(models.Snapshot.use_quota)
-        query = query.filter(~models.Snapshot.use_quota.is_(False))
 
     if volume_type_id or host:
         query = query.join(models.Snapshot.volume)
@@ -8615,86 +8591,17 @@ def worker_destroy(context, **filters):
 ###############################
 
 
-# TODO: (Y Release) remove method and this comment
+# TODO: (A Release) remove method and this comment
 @enginefacade.writer
-def volume_use_quota_online_data_migration(context, max_count):
+def remove_temporary_admin_metadata_data_migration(context, max_count):
-    def calculate_use_quota(volume):
+    query = model_query(context,
-        is_migrating = (volume.migration_status or '').startswith('target:')
+                        models.VolumeAdminMetadata).filter_by(key='temporary')
-        is_temporary = False
-        if volume.volume_admin_metadata:
-            for admin_meta in volume.volume_admin_metadata:
-                if (admin_meta.key == 'temporary') and (
-                    admin_meta.value == 'True'
-                ):
-                    is_temporary = True
-                    break
-        return not (is_migrating or is_temporary)
-
-    return use_quota_online_data_migration(
-        context,
-        max_count,
-        'Volume',
-        calculate_use_quota,
-    )
-
-
-# TODO: (Y Release) remove method and this comment
-@enginefacade.writer
-def snapshot_use_quota_online_data_migration(context, max_count):
-    # Temp snapshots are created in
-    # - cinder.volume.manager.VolumeManager._create_backup_snapshot
-    # - cinder.volume.driver.BaseVD.driver _create_temp_snapshot
-    #
-    # But we don't have a "good" way to know which ones are temporary as the
-    # only identification is the display_name that can be "forged" by users.
-    # Most users are not doing rolling upgrades so we'll assume there are no
-    # temporary snapshots, not even volumes with display_name:
-    # - '[revert] volume %s backup snapshot' % resource.volume_id
-    # - 'backup-snap-%s' % resource.volume_id
-    return use_quota_online_data_migration(
-        context,
-        max_count,
-        'Snapshot',
-        lambda snapshot: True,
-    )
-
-
-# TODO: (Y Release) remove method and this comment
-def use_quota_online_data_migration(
-    context,
-    max_count,
-    resource_name,
-    calculate_use_quota,
-):
-    updated = 0
-    query = model_query(context, getattr(models, resource_name)).filter_by(
-        use_quota=None
-    )
-    if resource_name == 'Volume':
-        query = query.options(joinedload(models.Volume.volume_admin_metadata))
     total = query.count()
-    resources = query.limit(max_count).with_for_update().all()
+    updated = query.limit(max_count).update(
-    for resource in resources:
+        models.VolumeAdminMetadata.delete_values())
-        resource.use_quota = calculate_use_quota(resource)
-        updated += 1
-
     return total, updated
 
 
-# TODO: (Z Release) remove method and this comment
-# TODO: (Y Release) uncomment method
-# @enginefacade.writer
-# def remove_temporary_admin_metadata_data_migration(context, max_count):
-#     query = model_query(
-#         context, models.VolumeAdminMetadata,
-#     ).filter_by(key='temporary')
-#     total = query.count()
-#     updated = query.limit(max_count).update(
-#         models.VolumeAdminMetadata.delete_values)
-#
-#     return total, updated
-
-
 ###############################
 
 

cinder/db/sqlalchemy/models.py

@@ -325,11 +325,11 @@ class Volume(BASE, CinderBase):
 
     id = sa.Column(sa.String(36), primary_key=True)
     _name_id = sa.Column(sa.String(36))  # Don't access/modify this directly!
-    # TODO: (Y release) Change nullable to False
     use_quota = Column(
         sa.Boolean,
-        nullable=True,
+        nullable=False,
         default=True,
+        server_default=sa.true(),
         doc='Ignore volume in quota usage',
     )
 
@@ -917,11 +917,11 @@ class Snapshot(BASE, CinderBase):
     )
 
     id = sa.Column(sa.String(36), primary_key=True)
-    # TODO: (Y release) Change nullable to False
     use_quota = Column(
         sa.Boolean,
-        nullable=True,
+        nullable=False,
         default=True,
+        server_default=sa.true(),
         doc='Ignore volume in quota usage',
     )
 

cinder/objects/snapshot.py

@@ -13,7 +13,6 @@
 #    under the License.
 
 from oslo_config import cfg
-from oslo_utils import versionutils
 from oslo_versionedobjects import fields
 
 from cinder import db
@@ -53,8 +52,7 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject,
         'user_id': fields.StringField(nullable=True),
         'project_id': fields.StringField(nullable=True),
 
-        # TODO: (Y release) Change nullable to False
+        'use_quota': fields.BooleanField(default=True, nullable=False),
-        'use_quota': fields.BooleanField(default=True, nullable=True),
         'volume_id': fields.UUIDField(nullable=True),
         'cgsnapshot_id': fields.UUIDField(nullable=True),
         'group_snapshot_id': fields.UUIDField(nullable=True),
@@ -113,15 +111,6 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject,
             self._orig_metadata = (dict(self.metadata)
                                    if self.obj_attr_is_set('metadata') else {})
 
-    # TODO: (Y release) remove method
-    @classmethod
-    def _obj_from_primitive(cls, context, objver, primitive):
-        primitive['versioned_object.data'].setdefault('use_quota', True)
-        obj = super(Snapshot, Snapshot)._obj_from_primitive(context, objver,
-                                                            primitive)
-        obj._reset_metadata_tracking()
-        return obj
-
     def obj_what_changed(self):
         changes = super(Snapshot, self).obj_what_changed()
         if hasattr(self, 'metadata') and self.metadata != self._orig_metadata:
@@ -129,14 +118,6 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject,
 
         return changes
 
-    def obj_make_compatible(self, primitive, target_version):
-        """Make a Snapshot representation compatible with a target version."""
-        super(Snapshot, self).obj_make_compatible(primitive, target_version)
-        target_version = versionutils.convert_version_to_tuple(target_version)
-        # TODO: (Y release) remove next 2 lines & method if nothing else below
-        if target_version < (1, 6):
-            primitive.pop('use_quota', None)
-
     @classmethod
     def _from_db_object(cls, context, snapshot, db_snapshot,
                         expected_attrs=None):
@@ -199,8 +180,6 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject,
             updates['volume_type_id'] = (
                 volume_types.get_default_volume_type()['id'])
 
-        # TODO: (Y release) remove setting use_quota default, it's set by ORM
-        updates.setdefault('use_quota', True)
         db_snapshot = db.snapshot_create(self._context, updates)
         self._from_db_object(self._context, self, db_snapshot)
 

cinder/objects/volume.py

@@ -14,7 +14,6 @@
 
 from oslo_config import cfg
 from oslo_log import log as logging
-from oslo_utils import versionutils
 from oslo_versionedobjects import fields
 
 from cinder import db
@@ -85,8 +84,7 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject,
         'user_id': fields.StringField(nullable=True),
         'project_id': fields.StringField(nullable=True),
 
-        # TODO: (Y release) Change nullable to False
+        'use_quota': fields.BooleanField(default=True, nullable=False),
-        'use_quota': fields.BooleanField(default=True, nullable=True),
         'snapshot_id': fields.UUIDField(nullable=True),
 
         'cluster_name': fields.StringField(nullable=True),
@@ -236,8 +234,6 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject,
 
     @classmethod
     def _obj_from_primitive(cls, context, objver, primitive):
-        # TODO: (Y release) remove next line
-        cls._ensure_use_quota_is_set(primitive['versioned_object.data'])
         obj = super(Volume, Volume)._obj_from_primitive(context, objver,
                                                         primitive)
         obj._reset_metadata_tracking()
@@ -269,14 +265,6 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject,
 
         return changes
 
-    def obj_make_compatible(self, primitive, target_version):
-        """Make a Volume representation compatible with a target version."""
-        super(Volume, self).obj_make_compatible(primitive, target_version)
-        target_version = versionutils.convert_version_to_tuple(target_version)
-        # TODO: (Y release) remove next 2 lines & method if nothing else below
-        if target_version < (1, 9):
-            primitive.pop('use_quota', None)
-
     @classmethod
     def _from_db_object(cls, context, volume, db_volume, expected_attrs=None):
         if expected_attrs is None:
@@ -298,6 +286,13 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject,
             metadata = db_volume.get('volume_admin_metadata', [])
             volume.admin_metadata = {item['key']: item['value']
                                      for item in metadata}
+            # TODO: (A release): Remove code of temporary admin metadata delete
+            if 'temporary' in volume.admin_metadata:
+                volume.admin_metadata.pop('temporary')
+                # Admin metadata deletion requires admin context, but since
+                # read also requires it we know our context is admin.
+                db.volume_admin_metadata_delete(context,
+                                                volume.id, 'temporary')
         if 'glance_metadata' in expected_attrs:
             metadata = db_volume.get('volume_glance_metadata', [])
             volume.glance_metadata = {item['key']: item['value']
@@ -350,20 +345,6 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject,
         volume.obj_reset_changes()
         return volume
 
-    # TODO: (Z release): Remove method and leave the default of False from DB
-    @staticmethod
-    def _ensure_use_quota_is_set(updates, warning=False):
-        if updates.get('use_quota') is None:
-            use_quota = not (
-                (updates.get('migration_status') or ''
-                 ).startswith('target:') or
-                (updates.get('admin_metadata') or {}
-                 ).get('temporary') == 'True')
-            if warning and not use_quota:
-                LOG.warning('Ooooops, we forgot to set the use_quota field to '
-                            'False!!  Fix code here')
-            updates['use_quota'] = use_quota
-
     def populate_consistencygroup(self):
         """Populate CG fields based on group fields.
 
@@ -404,19 +385,11 @@ class Volume(cleanable.CinderCleanableObject, base.CinderObject,
             updates['volume_type_id'] = (
                 volume_types.get_default_volume_type()['id'])
 
-        # TODO: (Y release) Remove this call since we should have already made
-        # all methods in Cinder make the call with the right values.
-        self._ensure_use_quota_is_set(updates, warning=True)
-
         db_volume = db.volume_create(self._context, updates)
         expected_attrs = self._get_expected_attrs(self._context)
         self._from_db_object(self._context, self, db_volume, expected_attrs)
 
     def save(self):
-        # TODO: (Y release) Remove this online migration code
-        # Pass self directly since it's a CinderObjectDictCompat
-        self._ensure_use_quota_is_set(self)
-
         updates = self.cinder_obj_get_changes()
         if updates:
             # NOTE(xyang): Allow this to pass if 'consistencygroup' is

cinder/tests/unit/db/test_migrations.py

@@ -187,6 +187,10 @@ class MigrationsWalk(
         # Increasing resource column max length to 300 is acceptable, since
         # it's a backward compatible change.
         'b8660621f1b9',
+        # Making use_quota non-nullable is acceptable since on the last release
+        # we added an online migration to set the value, but we also provide
+        # a default on the OVO, the ORM, and the DB engine.
+        '9ab1b092a404',
     ]
     FORBIDDEN_METHODS = ('alembic.operations.Operations.alter_column',
                          'alembic.operations.Operations.drop_column',
@@ -306,6 +310,13 @@ class MigrationsWalk(
             self.assertIsInstance(table.c.resource.type, self.VARCHAR_TYPE)
             self.assertEqual(300, table.c.resource.type.length)
 
+    def _check_9ab1b092a404(self, connection):
+        """Test use_quota is non-nullable."""
+        volumes = db_utils.get_table(connection, 'volumes')
+        self.assertFalse(volumes.c.use_quota.nullable)
+        snapshots = db_utils.get_table(connection, 'snapshots')
+        self.assertFalse(snapshots.c.use_quota.nullable)
+
 
 class TestMigrationsWalkSQLite(
     MigrationsWalk,

cinder/tests/unit/objects/test_objects.py

@@ -45,9 +45,9 @@ object_data = {
     'RequestSpec': '1.5-2f6efbb86107ee70cc1bb07f4bdb4ec7',
     'Service': '1.6-e881b6b324151dd861e09cdfffcdaccd',
     'ServiceList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e',
-    'Snapshot': '1.6-a2a1b62ae7e8d2794359ae59aff47ff6',
+    'Snapshot': '1.6-457ae45840b208c8fdfe399daaf1f745',
     'SnapshotList': '1.0-15ecf022a68ddbb8c2a6739cfc9f8f5e',
-    'Volume': '1.9-4e25e166fa38bfcf039dcac1b19465b1',
+    'Volume': '1.9-37de6d473e44d3f9f6d946fe93a3cece',
     'VolumeList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e',
     'VolumeAttachment': '1.3-e6a3f7c5590d19f1e3ff6f819fbe6593',
     'VolumeAttachmentList': '1.1-15ecf022a68ddbb8c2a6739cfc9f8f5e',

cinder/tests/unit/objects/test_snapshot.py

@@ -22,7 +22,6 @@ import pytz
 from cinder.db.sqlalchemy import models
 from cinder import exception
 from cinder import objects
-from cinder.objects import base as ovo_base
 from cinder.objects import fields
 from cinder.tests.unit import fake_constants as fake
 from cinder.tests.unit import fake_snapshot
@@ -230,17 +229,6 @@ class TestSnapshot(test_objects.BaseObjectsTestCase):
             mock.call(self.context,
                       fake.SNAPSHOT_ID)])
 
-    @ddt.data('1.38', '1.39')
-    def test_obj_make_compatible_use_quota_added(self, version):
-        snapshot = objects.Snapshot(self.context, use_quota=False)
-
-        serializer = ovo_base.CinderObjectSerializer(version)
-        primitive = serializer.serialize_entity(self.context, snapshot)
-
-        converted_snapshot = objects.Snapshot.obj_from_primitive(primitive)
-        expected = version != '1.39'
-        self.assertIs(expected, converted_snapshot.use_quota)
-
 
 class TestSnapshotList(test_objects.BaseObjectsTestCase):
     @mock.patch('cinder.objects.volume.Volume.get_by_id')

cinder/tests/unit/objects/test_volume.py

@@ -21,7 +21,6 @@ import pytz
 from cinder import context
 from cinder import exception
 from cinder import objects
-from cinder.objects import base as ovo_base
 from cinder.objects import fields
 from cinder.tests.unit.consistencygroup import fake_consistencygroup
 from cinder.tests.unit import fake_constants as fake
@@ -67,68 +66,25 @@ class TestVolume(test_objects.BaseObjectsTestCase):
                           objects.Volume.get_by_id, self.context, 123)
 
     @mock.patch('cinder.db.volume_create')
-    # TODO: (Y release) remove ddt.data and ddt.unpack decorators
+    def test_create(self, volume_create):
-    @ddt.data(
-        ({}, True),  # default value
-        ({'use_quota': True}, True),  # Normal init
-        ({'use_quota': False}, False),
-        ({'migration_status': 'target:'}, False),  # auto detect migrating
-        ({'migration_status': 'migrating:'}, True),  # auto detect normal
-        ({'admin_metadata': {'temporary': True}}, False),  # temp
-        ({'admin_metadata': {'something': True}}, True),  # normal
-    )
-    @ddt.unpack
-    def test_create(self, ovo, expected, volume_create):
         db_volume = fake_volume.fake_db_volume()
         volume_create.return_value = db_volume
-        volume = objects.Volume(context=self.context, **ovo)
+        volume = objects.Volume(context=self.context)
         volume.create()
         self.assertEqual(db_volume['id'], volume.id)
 
-        use_quota = volume_create.call_args[0][1]['use_quota']
-        # TODO: (Y release) remove next line
-        self.assertIs(expected, use_quota)
-
     @mock.patch('cinder.db.volume_update')
-    # TODO: (Y release) replace ddt.data and ddt.unpack decorators with
+    @ddt.data(False, True)
-    #       @ddt.data(False, True)
+    def test_save(self, test_cg, volume_update):
-    @ddt.data(
+        db_volume = fake_volume.fake_db_volume()
-        (False, {}, True),
-        (True, {}, True),
-        (False, {'use_quota': True}, True),
-        (False, {'use_quota': False}, False),
-        (False, {'migration_status': 'target:'}, False),
-        (False, {'migration_status': 'migrating:'}, True),
-        (False,
-         {'volume_admin_metadata': [{'key': 'temporary', 'value': True}]},
-         False),
-        (False,
-         {'volume_admin_metadata': [{'key': 'something', 'value': True}]},
-         True),
-    )
-    @ddt.unpack
-    def test_save(self, test_cg, ovo, expected, volume_update):
-        use_quota = ovo.pop('use_quota', None)
-        db_volume = fake_volume.fake_db_volume(**ovo)
-        # TODO: (Y release) remove expected_attrs
-        if 'volume_admin_metadata' in ovo:
-            expected_attrs = ['admin_metadata']
-        else:
-            expected_attrs = []
         volume = objects.Volume._from_db_object(self.context,
-                                                objects.Volume(), db_volume,
+                                                objects.Volume(), db_volume)
-                                                expected_attrs=expected_attrs)
         volume.display_name = 'foobar'
         if test_cg:
             volume.consistencygroup = None
-        # TODO: (Y release) remove next 2 lines
-        if use_quota is not None:
-            volume.use_quota = use_quota
         volume.save()
-        # TODO: (Y release) remove use_quota
         volume_update.assert_called_once_with(self.context, volume.id,
-                                              {'display_name': 'foobar',
+                                              {'display_name': 'foobar'})
-                                               'use_quota': expected})
 
     def test_save_error(self):
         db_volume = fake_volume.fake_db_volume()
@@ -153,10 +109,8 @@ class TestVolume(test_objects.BaseObjectsTestCase):
                           'metadata': {'key1': 'value1'}},
                          volume.obj_get_changes())
         volume.save()
-        # TODO: (Y release) remove use_quota
         volume_update.assert_called_once_with(self.context, volume.id,
-                                              {'display_name': 'foobar',
+                                              {'display_name': 'foobar'})
-                                               'use_quota': True})
         metadata_update.assert_called_once_with(self.context, volume.id,
                                                 {'key1': 'value1'}, True)
 
@@ -621,29 +575,6 @@ class TestVolume(test_objects.BaseObjectsTestCase):
                                              migration_status=migration_status)
         self.assertIs(expected, volume.is_migration_target())
 
-    @ddt.data(
-        # We could lose value during rolling upgrade if we added a new temp
-        # type in this upgrade and didn't take it into consideration
-        ('1.38', {'use_quota': False}, True),
-        # On rehydration we auto calculate use_quota value if not present
-        ('1.38', {'migration_status': 'target:123'}, False),
-        # Both versions in X
-        ('1.39', {'use_quota': True}, True),
-        # In X we don't recalculate, since we transmit the field
-        ('1.39', {'migration_status': 'target:123', 'use_quota': True}, True),
-    )
-    @ddt.unpack
-    def test_obj_make_compatible_use_quota_added(self, version, ovo, expected):
-        volume = objects.Volume(self.context, **ovo)
-
-        # When serializing to v1.38 we'll lose the use_quota value so it will
-        # be recalculated based on the Volume values
-        serializer = ovo_base.CinderObjectSerializer(version)
-        primitive = serializer.serialize_entity(self.context, volume)
-
-        converted_volume = objects.Volume.obj_from_primitive(primitive)
-        self.assertIs(expected, converted_volume.use_quota)
-
 
 @ddt.ddt
 class TestVolumeList(test_objects.BaseObjectsTestCase):

cinder/tests/unit/test_db_api.py

@@ -562,7 +562,8 @@ class DBAPIVolumeTestCase(BaseTest):
                                      'size': ONE_HUNDREDS,
                                      'host': 'h-%d' % i,
                                      'volume_type_id': fake.VOLUME_TYPE_ID,
-                                     'migration_status': 'target:vol-id'})
+                                     'migration_status': 'target:vol-id',
+                                     'use_quota': False})
         # This one will not be counted
         db.volume_create(self.ctxt, {'project_id': 'project',
                                      'size': ONE_HUNDREDS,
@@ -591,7 +592,7 @@ class DBAPIVolumeTestCase(BaseTest):
                                      'size': ONE_HUNDREDS,
                                      'host': 'h-%d' % i,
                                      'volume_type_id': fake.VOLUME_TYPE_ID,
-                                     'admin_metadata': {'temporary': 'True'}})
+                                     'use_quota': False})
 
         with sqlalchemy_api.main_context_manager.reader.using(self.ctxt):
             result = sqlalchemy_api._volume_data_get_for_project(
@@ -3902,96 +3903,75 @@ class DBAPIGroupTypeTestCase(BaseTest):
 
 
 class OnlineMigrationTestCase(BaseTest):
-    # TODO: (Y Release) remove method and this comment
+    # TODO: (A Release) remove method and this comment
     @mock.patch.object(sqlalchemy_api,
-                       'snapshot_use_quota_online_data_migration')
+                       'remove_temporary_admin_metadata_data_migration')
-    def test_db_snapshot_use_quota_online_data_migration(self, migration_mock):
+    def test_db_remove_temporary_admin_metadata_data_migration(self,
+                                                               migration_mock):
+        """Test that DB layer method properly calls implementation layer."""
         params = (mock.sentinel.ctxt, mock.sentinel.max_count)
-        db.snapshot_use_quota_online_data_migration(*params)
+        db.remove_temporary_admin_metadata_data_migration(*params)
         migration_mock.assert_called_once_with(*params)
 
-    # TODO: (Y Release) remove method and this comment
+    # TODO: (A Release) remove method and this comment
-    @mock.patch.object(sqlalchemy_api,
-                       'volume_use_quota_online_data_migration')
-    def test_db_volume_use_quota_online_data_migration(self, migration_mock):
-        params = (mock.sentinel.ctxt, mock.sentinel.max_count)
-        db.volume_use_quota_online_data_migration(*params)
-        migration_mock.assert_called_once_with(*params)
-
-    # TODO: (Y Release) remove method and this comment
-    @mock.patch.object(sqlalchemy_api, 'use_quota_online_data_migration')
-    def test_snapshot_use_quota_online_data_migration(self, migration_mock):
-        sqlalchemy_api.snapshot_use_quota_online_data_migration(
-            self.ctxt, mock.sentinel.max_count)
-        migration_mock.assert_called_once_with(self.ctxt,
-                                               mock.sentinel.max_count,
-                                               'Snapshot',
-                                               mock.ANY)
-        calculation_method = migration_mock.call_args[0][3]
-        # Confirm we always set the field to True regardless of what we pass
-        self.assertTrue(calculation_method(None))
-
-    # TODO: (Y Release) remove method and this comment
-    @mock.patch.object(sqlalchemy_api, 'use_quota_online_data_migration')
-    def test_volume_use_quota_online_data_migration(self, migration_mock):
-
-        class FakeAdminMeta:
-            def __init__(self, value):
-                self.key = 'temporary'
-                self.value = value
-        sqlalchemy_api.volume_use_quota_online_data_migration(
-            self.ctxt, mock.sentinel.max_count)
-        migration_mock.assert_called_once_with(self.ctxt,
-                                               mock.sentinel.max_count,
-                                               'Volume',
-                                               mock.ANY)
-        calculation_method = migration_mock.call_args[0][3]
-
-        # Confirm we set use_quota field to False for temporary volumes
-        temp_volume = mock.Mock(volume_admin_metadata=[FakeAdminMeta('True')])
-        self.assertFalse(calculation_method(temp_volume))
-
-        # Confirm we set use_quota field to False for migrating volumes
-        migration_dest_volume = mock.Mock(migration_status='target:123',
-                                          volume_admin_metadata=[])
-        self.assertFalse(calculation_method(migration_dest_volume))
-
-        # Confirm we set use_quota field to True for non-migrating volumes
-        non_migrating_volume = mock.Mock(migration_status=None,
-                                         volume_admin_metadata=[])
-        self.assertTrue(calculation_method(non_migrating_volume))
-
-        # Confirm we set use_quota field to True in other cases
-        volume = mock.Mock(volume_admin_metadata=[FakeAdminMeta('False')],
-                           migration_status='success')
-        self.assertTrue(calculation_method(volume))
-
-    # TODO: (Y Release) remove method and this comment
     @mock.patch.object(sqlalchemy_api, 'models')
     @mock.patch.object(sqlalchemy_api, 'model_query')
-    def test_use_quota_online_data_migration(self, query_mock, models_mock):
+    def test_remove_temporary_admin_metadata_data_migration_mocked(
-        calculate_method = mock.Mock()
+            self, query_mock, models_mock):
-        resource1 = mock.Mock()
+        """Test method implementation."""
-        resource2 = mock.Mock()
+        result = sqlalchemy_api.remove_temporary_admin_metadata_data_migration(
-        query = query_mock.return_value.filter_by.return_value
+            self.ctxt, mock.sentinel.max_count)
-        query_all = query.limit.return_value.with_for_update.return_value.all
-        query_all.return_value = [resource1, resource2]
-
-        result = sqlalchemy_api.use_quota_online_data_migration(
-            self.ctxt, mock.sentinel.max_count, 'resource_name',
-            calculate_method)
 
         query_mock.assert_called_once_with(self.ctxt,
-                                           models_mock.resource_name)
+                                           models_mock.VolumeAdminMetadata)
-        query_mock.return_value.filter_by.assert_called_once_with(
+        filter_by = query_mock.return_value.filter_by
-            use_quota=None)
+        filter_by.assert_called_once_with(key='temporary')
+        query = filter_by.return_value
         query.count.assert_called_once_with()
         query.limit.assert_called_once_with(mock.sentinel.max_count)
-        query.limit.return_value.with_for_update.assert_called_once_with()
+        del_vals_mock = models_mock.VolumeAdminMetadata.delete_values
-        query_all.assert_called_once_with()
+        del_vals_mock.assert_called_once_with()
 
-        calculate_method.assert_has_calls((mock.call(resource1),
+        update = query.limit.return_value.update
-                                           mock.call(resource2)))
+        update.assert_called_once_with(del_vals_mock.return_value)
-        self.assertEqual(calculate_method.return_value, resource1.use_quota)
+
-        self.assertEqual(calculate_method.return_value, resource2.use_quota)
+        self.assertEqual((query.count.return_value, update.return_value),
-        self.assertEqual((query.count.return_value, 2), result)
+                         result)
+
+    # TODO: (D Release) remove method and this comment
+    def test_remove_temporary_admin_metadata_data_migration(self):
+        """Test migration's full implementation."""
+        if not utils.is_db_dialect('mysql'):
+            raise test.testtools.TestCase.skipException(
+                'Only MySQL supports UPDATE on a LIMIT query')
+
+        vol1_admin_meta = {'temporary_not': 'false'}
+        vol1 = utils.create_volume(self.ctxt, display_name='normal',
+                                   admin_metadata=vol1_admin_meta)
+
+        vol2_meta = {'temporary': True}
+        vol2 = utils.create_volume(self.ctxt, display_name='metadata',
+                                   metadata=vol2_meta)
+
+        vol3_admin_meta = {'temporary': 'true', 'temp': 'true'}
+        vol3 = utils.create_volume(
+            self.ctxt, display_name='admin_metadata', use_quota=False,
+            admin_metadata=vol3_admin_meta)
+
+        # Should only  remove "temporary" admin metadata
+        result = sqlalchemy_api.remove_temporary_admin_metadata_data_migration(
+            self.ctxt, 4)
+        self.assertEqual(1, result)
+
+        vol1.refresh()
+        self.assertEqual(({}, vol1_admin_meta),
+                         (vol1.metadata, vol1.admin_metadata))
+
+        vol2.refresh()
+        self.assertEqual((vol2_meta, {}),
+                         (vol2.metadata, vol2.admin_metadata))
+
+        vol3.refresh()
+        vol3_admin_meta.pop('temporary')
+        self.assertEqual(({}, vol3_admin_meta),
+                         (vol3.metadata, vol2.admin_metadata))

cinder/tests/unit/utils.py

@@ -29,6 +29,7 @@ import oslo_versionedobjects
 from cinder.common import constants
 from cinder import context
 from cinder import db
+from cinder.db.sqlalchemy import api as sqlalchemy_api
 from cinder import exception
 from cinder import objects
 from cinder.objects import fields
@@ -42,6 +43,12 @@ def get_test_admin_context():
     return context.get_admin_context()
 
 
+def is_db_dialect(dialect_name):
+    db_engine = sqlalchemy_api.get_engine()
+    dialect = db_engine.url.get_dialect()
+    return dialect_name == dialect.name
+
+
 def obj_attr_is_set(obj_class):
     """Method to allow setting the ID on an OVO on creation."""
     original_method = obj_class.obj_attr_is_set

cinder/tests/unit/volume/flows/test_create_volume_flow.py

@@ -1867,9 +1867,7 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
                           self.mock_image_service)
 
         self.assertTrue(mock_cleanup_cg.called)
-        # Online migration of the use_quota field
+        mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 1})
-        mock_volume_update.assert_any_call(self.ctxt, volume.id,
-                                           {'size': 1, 'use_quota': True})
         self.assertEqual(volume_size, volume.size)
 
     @mock.patch('cinder.image.image_utils.check_available_space')
@@ -2006,9 +2004,7 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
         )
 
         # The volume size should be reduced to virtual_size and then put back
-        # Online migration of the use_quota field
+        mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 2})
-        mock_volume_update.assert_any_call(self.ctxt, volume.id,
-                                           {'size': 2, 'use_quota': True})
         mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 10})
 
         # Make sure created a new cache entry
@@ -2086,9 +2082,7 @@ class CreateVolumeFlowManagerImageCacheTestCase(test.TestCase):
         # The volume size should be reduced to virtual_size and then put back,
         # especially if there is an exception while creating the volume.
         self.assertEqual(2, mock_volume_update.call_count)
-        # Online migration of the use_quota field
+        mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 2})
-        mock_volume_update.assert_any_call(self.ctxt, volume.id,
-                                           {'size': 2, 'use_quota': True})
         mock_volume_update.assert_any_call(self.ctxt, volume.id, {'size': 10})
 
         # Make sure we didn't try and create a cache entry

cinder/tests/unit/volume/test_volume.py

@@ -424,6 +424,7 @@ class VolumeTestCase(base.BaseVolumeTestCase):
             self.context,
             availability_zone=CONF.storage_availability_zone,
             migration_status='target:123',
+            use_quota=False,
             **self.volume_params)
         volume_id = volume['id']
 

cinder/volume/driver.py

@@ -1353,8 +1353,6 @@ class BaseVD(object, metaclass=abc.ABCMeta):
             'availability_zone': volume.availability_zone,
             'volume_type_id': volume.volume_type_id,
             'use_quota': False,  # Don't count for quota
-            # TODO: (Y release) Remove admin_metadata and only use use_quota
-            'admin_metadata': {'temporary': 'True'},
         }
         kwargs.update(volume_options or {})
         temp_vol_ref = objects.Volume(context=context.elevated(), **kwargs)

cinder/volume/flows/manager/create_volume.py

@@ -1371,8 +1371,7 @@ def get_flow(context, manager, db, driver, scheduler_rpcapi, host, volume,
     volume_flow.add(ExtractVolumeSpecTask(db))
     # Temporary volumes created during migration should not be notified
     end_notify_suffix = None
-    # TODO: (Y release) replace check with: if volume.use_quota:
+    if volume.use_quota:
-    if volume.use_quota or not volume.is_migration_target():
         volume_flow.add(NotifyVolumeActionTask(db, 'create.start'))
         end_notify_suffix = 'create.end'
     volume_flow.add(CreateVolumeFromSpecTask(manager,

cinder/volume/manager.py

@@ -982,19 +982,7 @@ class VolumeManager(manager.CleanableManager,
         # and should not modify it when deleted.  These temporary volumes are
         # created for volume migration between backends and for backups (from
         # in-use volume or snapshot).
-        # TODO: (Y release) replace until the if do_quota (including comments)
+        if volume.use_quota:
-        #       with: do_quota = volume.use_quota
-        # The status 'deleting' is not included, because it only applies to
-        # the source volume to be deleted after a migration. No quota
-        # needs to be handled for it.
-        is_migrating = volume.migration_status not in (None, 'error',
-                                                       'success')
-        # Get admin_metadata (needs admin context) to detect temporary volume.
-        with volume.obj_as_admin():
-            do_quota = not (volume.use_quota is False or is_migrating or
-                            volume.admin_metadata.get('temporary') == 'True')
-
-        if do_quota:
             notification = 'unmanage.' if unmanage_only else 'delete.'
             self._notify_about_volume_usage(context, volume,
                                             notification + 'start')
@@ -1043,7 +1031,7 @@ class VolumeManager(manager.CleanableManager,
 
         # If deleting source/destination volume in a migration or a temp
         # volume for backup, we should skip quotas.
-        if do_quota:
+        if volume.use_quota:
             # Get reservations
             try:
                 reservations = None
@@ -1064,7 +1052,7 @@ class VolumeManager(manager.CleanableManager,
 
         # If deleting source/destination volume in a migration or a temp
         # volume for backup, we should skip quotas.
-        if do_quota:
+        if volume.use_quota:
             self._notify_about_volume_usage(context, volume,
                                             notification + 'end')
             # Commit the reservations