Merge "Change snapshots type with volume retype"

cinder/quota_utils.py

@@ -45,12 +45,20 @@ class GenericProjectInfo(object):
 
 
 def get_volume_type_reservation(ctxt, volume, type_id,
-                                reserve_vol_type_only=False):
+                                reserve_vol_type_only=False,
+                                negative=False):
     from cinder import quota
     QUOTAS = quota.QUOTAS
     # Reserve quotas for the given volume type
     try:
         reserve_opts = {'volumes': 1, 'gigabytes': volume['size']}
+        # When retyping a volume it may contain snapshots (if we are not
+        # migrating it) and we need to account for its snapshots' size
+        if volume.snapshots:
+            reserve_opts['snapshots'] = len(volume.snapshots)
+            if not CONF.no_snapshot_gb_quota:
+                reserve_opts['gigabytes'] += sum(snap.volume_size
+                                                 for snap in volume.snapshots)
         QUOTAS.add_volume_type_opts(ctxt,
                                     reserve_opts,
                                     type_id)
@@ -59,6 +67,12 @@ def get_volume_type_reservation(ctxt, volume, type_id,
         if reserve_vol_type_only:
             reserve_opts.pop('volumes')
             reserve_opts.pop('gigabytes')
+            reserve_opts.pop('snapshots', None)
+
+        if negative:
+            for key, value in reserve_opts.items():
+                reserve_opts[key] = -value
+
         # Note that usually the project_id on the volume will be the same as
         # the project_id in the context. But, if they are different then the
         # reservations must be recorded against the project_id that owns the

cinder/tests/unit/test_quota.py

@@ -18,6 +18,7 @@
 import datetime
 from unittest import mock
 
+import ddt
 from oslo_config import cfg
 from oslo_utils import timeutils
 
@@ -34,6 +35,8 @@ from cinder import quota
 from cinder import quota_utils
 from cinder.scheduler import rpcapi as scheduler_rpcapi
 from cinder.tests.unit import fake_constants as fake
+from cinder.tests.unit import fake_snapshot
+from cinder.tests.unit import fake_volume
 import cinder.tests.unit.image.fake
 from cinder.tests.unit import test
 from cinder.tests.unit import utils as tests_utils
@@ -1916,6 +1919,7 @@ class QuotaReserveSqlAlchemyTestCase(test.TestCase):
                                        delta=-2 * 1024), ])
 
 
+@ddt.ddt
 class QuotaVolumeTypeReservationTestCase(test.TestCase):
 
     def setUp(self):
@@ -1932,32 +1936,32 @@ class QuotaVolumeTypeReservationTestCase(test.TestCase):
                                      mock_add_volume_type_opts,
                                      mock_reserve):
         my_context = FakeContext('MyProject', None)
-        volume = {'name': 'my_vol_name',
+        volume = fake_volume.fake_volume_obj(my_context,
-                  'id': 'my_vol_id',
+                                             name= 'my_vol_name',
-                  'size': '1',
+                                             id= 'my_vol_id',
-                  'project_id': 'vol_project_id',
+                                             size= 1,
-                  }
+                                             project_id= 'vol_project_id')
-        reserve_opts = {'volumes': 1, 'gigabytes': volume['size']}
         quota_utils.get_volume_type_reservation(my_context,
                                                 volume,
                                                 self.volume_type['id'])
+        reserve_opts = {'volumes': 1, 'gigabytes': volume.size}
         mock_add_volume_type_opts.assert_called_once_with(
             my_context,
             reserve_opts,
             self.volume_type['id'])
         mock_reserve.assert_called_once_with(my_context,
                                              project_id='vol_project_id',
-                                             gigabytes='1',
+                                             gigabytes=1,
                                              volumes=1)
 
     @mock.patch.object(quota.QUOTAS, 'reserve')
     def test_volume_type_reservation_with_type_only(self, mock_reserve):
         my_context = FakeContext('MyProject', None)
-        volume = {'name': 'my_vol_name',
+        volume = fake_volume.fake_volume_obj(my_context,
-                  'id': 'my_vol_id',
+                                             name='my_vol_name',
-                  'size': '1',
+                                             id='my_vol_id',
-                  'project_id': 'vol_project_id',
+                                             size=1,
-                  }
+                                             project_id='vol_project_id')
         quota_utils.get_volume_type_reservation(my_context,
                                                 volume,
                                                 self.volume_type['id'],
@@ -1965,7 +1969,58 @@ class QuotaVolumeTypeReservationTestCase(test.TestCase):
         vtype_volume_quota = "%s_%s" % ('volumes', self.volume_type['name'])
         vtype_size_quota = "%s_%s" % ('gigabytes', self.volume_type['name'])
         reserve_opts = {vtype_volume_quota: 1,
-                        vtype_size_quota: volume['size']}
+                        vtype_size_quota: volume.size}
         mock_reserve.assert_called_once_with(my_context,
                                              project_id='vol_project_id',
                                              **reserve_opts)
+
+    @ddt.data({'count_snaps': True, 'negative': True},
+              {'count_snaps': True, 'negative': False},
+              {'count_snaps': False, 'negative': True},
+              {'count_snaps': False, 'negative': False})
+    @ddt.unpack
+    @mock.patch.object(quota.QUOTAS, 'reserve')
+    def test_volume_type_reservation_snapshots_with_type_only(self,
+                                                              mock_reserve,
+                                                              count_snaps,
+                                                              negative):
+        """Volume type reservations on volume with snapshots
+
+        Test that when the volume has snapshots it takes them into account,
+        and even calculates the quota correctly depending on
+        no_snapshot_gb_quota configuration option.
+
+        It should work for negative and positive quotas.
+        """
+        self.override_config('no_snapshot_gb_quota', not count_snaps)
+        my_context = FakeContext('MyProject', None)
+        snaps = [fake_snapshot.fake_db_snapshot(volume_size=1),
+                 fake_snapshot.fake_db_snapshot(volume_size=2)]
+        volume = fake_volume.fake_volume_obj(my_context,
+                                             expected_attrs=['snapshots'],
+                                             name='my_vol_name',
+                                             id=fake.VOLUME_ID,
+                                             size=1,
+                                             project_id=fake.PROJECT_ID,
+                                             snapshots=snaps)
+        quota_utils.get_volume_type_reservation(my_context,
+                                                volume,
+                                                self.volume_type['id'],
+                                                reserve_vol_type_only=True,
+                                                negative=negative)
+
+        factor = -1 if negative else 1
+        if count_snaps:
+            snaps_size = (snaps[0]['volume_size'] + snaps[1]['volume_size'])
+        else:
+            snaps_size = 0
+        vtype_volume_quota = "volumes_%s" % self.volume_type['name']
+        vtype_snapshot_quota = "snapshots_%s" % self.volume_type['name']
+        vtype_size_quota = "%s_%s" % ('gigabytes', self.volume_type['name'])
+        reserve_opts = {vtype_volume_quota: factor * 1,
+                        vtype_snapshot_quota: factor * 2,
+                        vtype_size_quota: factor * (volume['size'] +
+                                                    snaps_size)}
+        mock_reserve.assert_called_once_with(my_context,
+                                             project_id=fake.PROJECT_ID,
+                                             **reserve_opts)

cinder/tests/unit/volume/test_volume_migration.py

@@ -31,6 +31,7 @@ from cinder import exception
 from cinder import objects
 from cinder.objects import fields
 from cinder import quota
+from cinder import quota_utils
 from cinder.tests.unit.api import fakes
 from cinder.tests.unit import fake_constants as fake
 from cinder.tests.unit import fake_volume
@@ -783,41 +784,23 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
         volume.previous_status = 'available'
         volume.save()
         if snap:
-            create_snapshot(volume.id, size=volume.size,
+            snapshot = create_snapshot(volume.id, size=volume.size,
-                            user_id=self.user_context.user_id,
+                                       user_id=self.user_context.user_id,
-                            project_id=self.user_context.project_id,
+                                       project_id=self.user_context.project_id,
-                            ctxt=self.user_context)
+                                       volume_type_id=volume.volume_type_id,
+                                       ctxt=self.user_context)
         if driver or diff_equal:
             host_obj = {'host': CONF.host, 'capabilities': {}}
         else:
             host_obj = {'host': 'newhost', 'capabilities': {}}
 
-        reserve_opts = {'volumes': 1, 'gigabytes': volume.size}
+        reservations = quota_utils.get_volume_type_reservation(
-        QUOTAS.add_volume_type_opts(self.context,
+            self.context, volume, new_type.id, reserve_vol_type_only)
-                                    reserve_opts,
+        old_reservations = quota_utils.get_volume_type_reservation(
-                                    vol_type['id'])
+            self.context, volume, old_vol_type['id'], reserve_vol_type_only,
-        if reserve_vol_type_only:
+            negative=True)
-            reserve_opts.pop('volumes')
-            reserve_opts.pop('gigabytes')
-            try:
-                usage = db.quota_usage_get(elevated, project_id, 'volumes')
-                total_volumes_in_use = usage.in_use
-                usage = db.quota_usage_get(elevated, project_id, 'gigabytes')
-                total_gigabytes_in_use = usage.in_use
-            except exception.QuotaUsageNotFound:
-                total_volumes_in_use = 0
-                total_gigabytes_in_use = 0
-        reservations = QUOTAS.reserve(self.context,
-                                      project_id=project_id,
-                                      **reserve_opts)
 
-        old_reserve_opts = {'volumes': -1, 'gigabytes': -volume.size}
+        old_usage = db.quota_usage_get_all_by_project(elevated, project_id)
-        QUOTAS.add_volume_type_opts(self.context,
-                                    old_reserve_opts,
-                                    old_vol_type['id'])
-        old_reservations = QUOTAS.reserve(self.context,
-                                          project_id=project_id,
-                                          **old_reserve_opts)
 
         with mock.patch.object(self.volume.driver, 'retype') as _retype,\
                 mock.patch.object(volume_types, 'volume_types_diff') as _diff,\
@@ -874,17 +857,22 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
 
         # Get new in_use after retype, it should not be changed.
         if reserve_vol_type_only:
-            try:
+            new_usage = db.quota_usage_get_all_by_project(elevated, project_id)
-                usage = db.quota_usage_get(elevated, project_id, 'volumes')
+            for resource in ('volumes', 'gigabytes', 'snapshots'):
-                new_total_volumes_in_use = usage.in_use
+                empty = {'in_use': 0, 'reserved': 0}
-                usage = db.quota_usage_get(elevated, project_id, 'gigabytes')
+                # Global resource hasn't changed
-                new_total_gigabytes_in_use = usage.in_use
+                self.assertEqual(old_usage.get(resource, empty)['in_use'],
-            except exception.QuotaUsageNotFound:
+                                 new_usage.get(resource, empty)['in_use'])
-                new_total_volumes_in_use = 0
+                # The new type was empty before
-                new_total_gigabytes_in_use = 0
+                self.assertEqual(
-            self.assertEqual(total_volumes_in_use, new_total_volumes_in_use)
+                    0, old_usage.get(resource + '_new', empty)['in_use'])
-            self.assertEqual(total_gigabytes_in_use,
+                # Old type resources have been moved to the new one
-                             new_total_gigabytes_in_use)
+                self.assertEqual(
+                    old_usage.get(resource + '_old', empty)['in_use'],
+                    new_usage.get(resource + '_new', empty)['in_use'])
+                # The old type is empty now
+                self.assertEqual(
+                    0, new_usage.get(resource + '_old', empty)['in_use'])
 
         # check properties
         if driver or diff_equal:
@@ -903,6 +891,12 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
                                                 new_type,
                                                 returned_diff,
                                                 host_obj)
+            # When retyping a volume with snapshots the snapshots should be
+            # retyped as well
+            if snap:
+                snapshot.refresh()
+                self.assertEqual(new_type.id, snapshot.volume_type_id)
+
         elif not exc:
             self.assertEqual(old_vol_type['id'], volume.volume_type_id)
             self.assertEqual('retyping', volume.status)
@@ -951,6 +945,10 @@ class VolumeMigrationTestCase(base.BaseVolumeTestCase):
     def test_retype_volume_migration_equal_types(self):
         self._retype_volume_exec(False, diff_equal=True)
 
+    def test_retype_volume_migration_equal_types_snaps(self):
+        self._retype_volume_exec(False, snap=True, diff_equal=True,
+                                 reserve_vol_type_only=True)
+
     def test_retype_volume_with_type_only(self):
         self._retype_volume_exec(True, reserve_vol_type_only=True)
 

cinder/volume/api.py

@@ -1688,18 +1688,10 @@ class API(base.Base):
 
         # Get old reservations
         try:
-            reserve_opts = {'volumes': -1, 'gigabytes': -volume.size}
+            old_reservations = quota_utils.get_volume_type_reservation(
-            QUOTAS.add_volume_type_opts(context,
+                context, volume, volume.volume_type_id,
-                                        reserve_opts,
+                reserve_vol_type_only=True, negative=True)
-                                        volume.volume_type_id)
+
-            # NOTE(wanghao): We don't need to reserve volumes and gigabytes
-            # quota for retyping operation since they didn't changed, just
-            # reserve volume_type and type gigabytes is fine.
-            reserve_opts.pop('volumes')
-            reserve_opts.pop('gigabytes')
-            old_reservations = QUOTAS.reserve(context,
-                                              project_id=volume.project_id,
-                                              **reserve_opts)
         except Exception:
             volume.status = volume.previous_status
             volume.save()

cinder/volume/manager.py

@@ -202,7 +202,7 @@ class VolumeManager(manager.CleanableManager,
     _VOLUME_CLONE_SKIP_PROPERTIES = {
         'id', '_name_id', 'name_id', 'name', 'status',
         'attach_status', 'migration_status', 'volume_type',
-        'consistencygroup', 'volume_attachment', 'group'}
+        'consistencygroup', 'volume_attachment', 'group', 'snapshots'}
 
     def _get_service(self,
                      host: str = None,
@@ -3050,6 +3050,11 @@ class VolumeManager(manager.CleanableManager,
             volume.update(model_update)
             volume.save()
 
+            # We need to make the snapshots match the volume's type
+            for snap in volume.snapshots:
+                snap.volume_type_id = new_type_id
+                snap.save()
+
         if old_reservations:
             QUOTAS.commit(context, old_reservations, project_id=project_id)
         if new_reservations:

releasenotes/notes/quota-on-retype-with-snapshots-2d9fc7b2c75f899d.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    `Bug #1877164 <https://bugs.launchpad.net/cinder/+bug/1877164>`_: Fix
+    retyping volume with snapshots leaves the snapshots with the old type,
+    making the quotas wrong inmediately for snapshots, and breaking them even
+    more after those snapshots are deleted.