Improve quota usage for temporary resources

Cinder creates temporary resources, volumes and snapshots, during some
of its operations, and these resources aren't counted towards quota
usage.

Cinder currently has a problem to track quota usage is when deleting
temporary resources.

Determining which volumes are temporary is a bit inconvenient because we
have to check the migration status as well as the admin metadata, so
they have been the source of several bugs, though they should be
properly tracked now.

For snapshots we don't have any way to track which ones are temporary,
which creates some issues:

- Quota sync mechanism will count them as normal snapshots.

- Manually deleting temporary snapshots after an operation fails will
  mess the quota.

- If we are using snapshots instead of clones for backups of in-use
  volumes the quota will be messed on completion.

This patch proposes the introduction of a new field for those database
resource tables where we create temporary resources: volumes and
snaphots.

The field will be called "use_quota" and will be set to False for
temporary resources to indicate that we don't want them to be counted
towards quota on deletion.

Instead of using "temporary" as the field name "use_quota" was used to
allow other cases that should not do quota in the future.

Moving from our current mechanism to the new one is a multi-release
process because we need to have backward compatibility code for rolling
upgrades.

This patch adds everything needed to complete the multi-release process
so that anybody can submit next release patches.  To do so the patch
adds backward compatible code adding the feature in this release and
TODO comments with the exact changes that need to be done for the next
2 releases.

The removal of the compatibility code will be done in the next release,
and in the one after that we'll remove the temporary metadata rows that
may still exist in the database.

With this new field we'll be able to make our DB queries more efficient
for quota usage calculations, reduce the chances of introducing new
quota usage bugs in the future, and allow users to filter in/out
temporary volumes on listings.

Closes-Bug: #1923828
Closes-Bug: #1923829
Closes-Bug: #1923830
Implements: blueprint temp-resources
Change-Id: I98bd4d7a54906b613daaf14233d749da1e1531d5

cinder/cmd/manage.py

@@ -147,13 +147,21 @@ class DbCommands(object):
     # NOTE: Online migrations cannot depend on having Cinder services running.
     # Migrations can be called during Fast-Forward Upgrades without having any
     # Cinder services up.
-    # NOTE; Online migrations must be removed at the beginning of the next
+    # NOTE: Online migrations must be removed at the beginning of the next
     # release to the one they've been introduced.  A comment with the release
     # a migration is introduced and the one where it must be removed must
     # preceed any element of the "online_migrations" tuple, like this:
     #    # Added in Queens remove in Rocky
     #    db.service_uuids_online_data_migration,
-    online_migrations = tuple()
+    online_migrations = (
+        # TODO: (Z Release) Remove next line and this comment
+        # TODO: (Y Release) Uncomment next line and remove this comment
+        # 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):
         pass

cinder/db/api.py

@@ -1940,3 +1940,19 @@ def conditional_update(context, model, values, expected_values, filters=(),
     return IMPL.conditional_update(context, model, values, expected_values,
                                    filters, include_deleted, project_only,
                                    order)
+
+
+# TODO: (Y Release) remove method and this comment
+def volume_use_quota_online_data_migration(context, max_count):
+    IMPL.volume_use_quota_online_data_migration(context, max_count)
+
+
+# TODO: (Y Release) remove method and this comment
+def snapshot_use_quota_online_data_migration(context, max_count):
+    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/sqlalchemy/api.py

@@ -1592,10 +1592,13 @@ def _volume_data_get_for_project(context, project_id, volume_type_id=None,
     # 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)
         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',
@@ -3271,13 +3274,19 @@ def snapshot_get_all_by_project(context, project_id, filters=None, marker=None,
 
 @require_context
 def _snapshot_data_get_for_project(context, project_id, volume_type_id=None,
-                                   session=None, host=None):
+                                   session=None, host=None,
+                                   skip_internal=True):
     authorize_project_context(context, project_id)
     query = model_query(context,
                         func.count(models.Snapshot.id),
                         func.sum(models.Snapshot.volume_size),
                         read_deleted="no",
                         session=session)
+    if skip_internal:
+        # TODO: (Y release) replace next line with:
+        #        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('volume')
         if volume_type_id:
@@ -3294,8 +3303,11 @@ def _snapshot_data_get_for_project(context, project_id, volume_type_id=None,
 @require_context
 def snapshot_data_get_for_project(context, project_id,
                                   volume_type_id=None, host=None):
+    # This method doesn't support filtering temporary resources (use_quota
+    # field) and defaults to returning all snapshots because all callers (quota
+    # sync methods and os-host API extension) require all the snapshots.
     return _snapshot_data_get_for_project(context, project_id, volume_type_id,
-                                          host=host)
+                                          host=host, skip_internal=False)
 
 
 @require_context
@@ -7377,3 +7389,67 @@ def conditional_update(context, model, values, expected_values, filters=(),
     # Return True if we were able to change any DB entry, False otherwise
     result = query.update(values, **update_args)
     return 0 != result
+
+
+# TODO: (Y Release) remove method and this comment
+@enginefacade.writer
+def volume_use_quota_online_data_migration(context, max_count):
+    def calculate_use_quota(volume):
+        return not (volume.migration_status.startswith('target:') or
+                    volume.admin_metadata.get('temporary') == 'True')
+
+    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
+@enginefacade.writer
+def use_quota_online_data_migration(context, max_count,
+                                    resource_name, calculate_use_quota):
+    updated = 0
+    session = get_session()
+    with session.begin():
+        query = model_query(context,
+                            getattr(models, resource_name),
+                            session=session).filter_by(
+                                use_quota=None)
+        total = query.count()
+        resources = query.limit(max_count).with_for_update().all()
+        for resource in resources:
+            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):
+#     session = get_session()
+#     with session.begin():
+#         query = model_query(context,
+#                             models.VolumeAdminMetadata,
+#                             session=session).filter_by(key='temporary')
+#         total = query.count()
+#         updated = query.limit(max_count).update(
+#             models.VolumeAdminMetadata.delete_values)
+#
+#     return total, updated

cinder/db/sqlalchemy/migrate_repo/versions/145_add_use_quota_fields.py

@@ -0,0 +1,34 @@
+# Copyright 2021 Red Hat, Inc.
+# 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.
+
+import sqlalchemy as sa
+
+
+def upgrade(migrate_engine):
+    """Update volumes and snapshots tables with use_quota field.
+
+    Add use_quota field to both volumes and snapshots table to fast and easily
+    identify resources that must be counted for quota usages.
+    """
+    # Existing resources will be left with None value to allow rolling upgrades
+    # with the online data migration pattern, since they will identify the
+    # resources that don't have the field set/known yet.
+    meta = sa.MetaData(bind=migrate_engine)
+    for table_name in ('volumes', 'snapshots'):
+        table = sa.Table(table_name, meta, autoload=True)
+
+        if not hasattr(table.c, 'use_quota'):
+            column = sa.Column('use_quota', sa.Boolean, nullable=True)
+            table.create_column(column)

cinder/db/sqlalchemy/models.py

@@ -252,6 +252,9 @@ 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, default=True,
+                       doc='Ignore volume in quota usage')
 
     @property
     def name_id(self):
@@ -755,6 +758,9 @@ class Snapshot(BASE, CinderBase):
     """Represents a snapshot of volume."""
     __tablename__ = 'snapshots'
     id = sa.Column(sa.String(36), primary_key=True)
+    # TODO: (Y release) Change nullable to False
+    use_quota = Column(sa.Boolean, nullable=True, default=True,
+                       doc='Ignore volume in quota usage')
 
     @property
     def name(self):
@@ -823,6 +829,7 @@ class Backup(BASE, CinderBase):
     """Represents a backup of a volume to Swift."""
     __tablename__ = 'backups'
     id = sa.Column(sa.String(36), primary_key=True)
+    # Backups don't have use_quota field since we don't have temporary backups
 
     @property
     def name(self):

cinder/objects/base.py

@@ -135,6 +135,11 @@ def add(self, ver, updates):
 # '1.35' and the self['<versioname>'] = { to self['1.35'] = {
 
 
+# TODO: (Z release) remove up to next TODO  and update
+#       CinderObjectVersionsHistory (was added in X release)
+OBJ_VERSIONS.add('1.39', {'Volume': '1.9', 'Snapshot': '1.6'})
+
+
 class CinderObjectRegistry(base.VersionedObjectRegistry):
     def registration_hook(self, cls, index):
         """Hook called when registering a class.

cinder/objects/snapshot.py

@@ -13,6 +13,7 @@
 #    under the License.
 
 from oslo_config import cfg
+from oslo_utils import versionutils
 from oslo_versionedobjects import fields
 
 from cinder import db
@@ -38,7 +39,8 @@ class Snapshot(cleanable.CinderCleanableObject, base.CinderObject,
     # Version 1.3: SnapshotStatusField now includes "unmanaging"
     # Version 1.4: SnapshotStatusField now includes "backing-up"
     # Version 1.5: SnapshotStatusField now includes "restoring"
-    VERSION = '1.5'
+    # Version 1.6: Added use_quota
+    VERSION = '1.6'
 
     # NOTE(thangp): OPTIONAL_FIELDS are fields that would be lazy-loaded. They
     # are typically the relationship in the sqlalchemy object.
@@ -51,6 +53,8 @@ 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=True),
         'volume_id': fields.UUIDField(nullable=True),
         'cgsnapshot_id': fields.UUIDField(nullable=True),
         'group_snapshot_id': fields.UUIDField(nullable=True),
@@ -109,13 +113,30 @@ def _reset_metadata_tracking(self, fields=None):
             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:
             changes.add('metadata')
 
         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):
@@ -178,6 +199,8 @@ def create(self):
             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)