From b77fb72aa9a58668b2c39f8c9eff06f920d76a74 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 18 Mar 2021 17:19:34 +0100 Subject: [PATCH] Change snapshots type with volume retype Both volumes and snapshots have a volume_type_id field in the DB, but when we migrate a volume we leave the snapshots with the old type. This can only happen for retypes that don't do migration, since we cannot do a retype with migration if the volume has snapshots. Leaving the snapshots with the old time makes a mess of the quota usage when we do the retype as well as when we delete the snapshots. This patch fixes the quota issue by making sure the snapshots are retyped as well. This means that we will check quotas for the snapshots when retyping a volume that have them and we will properly reserve and set the quota on retype. Closes-Bug: #1877164 Change-Id: I90e9f85d192e1f2fee4ec8615a5bc95851a90f8e --- cinder/quota_utils.py | 16 +++- cinder/tests/unit/test_quota.py | 81 ++++++++++++++++--- .../unit/volume/test_volume_migration.py | 78 +++++++++--------- cinder/volume/api.py | 16 +--- cinder/volume/manager.py | 7 +- ...etype-with-snapshots-2d9fc7b2c75f899d.yaml | 7 ++ 6 files changed, 138 insertions(+), 67 deletions(-) create mode 100644 releasenotes/notes/quota-on-retype-with-snapshots-2d9fc7b2c75f899d.yaml diff --git a/cinder/quota_utils.py b/cinder/quota_utils.py index 6efe2e0fb02..4480496071c 100644 --- a/cinder/quota_utils.py +++ b/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 diff --git a/cinder/tests/unit/test_quota.py b/cinder/tests/unit/test_quota.py index 337561a17ab..18e59062a21 100644 --- a/cinder/tests/unit/test_quota.py +++ b/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', - 'id': 'my_vol_id', - 'size': '1', - 'project_id': 'vol_project_id', - } - reserve_opts = {'volumes': 1, 'gigabytes': volume['size']} + volume = fake_volume.fake_volume_obj(my_context, + name= 'my_vol_name', + id= 'my_vol_id', + size= 1, + project_id= 'vol_project_id') 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', - 'id': 'my_vol_id', - 'size': '1', - 'project_id': 'vol_project_id', - } + volume = fake_volume.fake_volume_obj(my_context, + name='my_vol_name', + id='my_vol_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) diff --git a/cinder/tests/unit/volume/test_volume_migration.py b/cinder/tests/unit/volume/test_volume_migration.py index dc827df7fe1..4ececef902e 100644 --- a/cinder/tests/unit/volume/test_volume_migration.py +++ b/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, - user_id=self.user_context.user_id, - project_id=self.user_context.project_id, - ctxt=self.user_context) + snapshot = create_snapshot(volume.id, size=volume.size, + user_id=self.user_context.user_id, + project_id=self.user_context.project_id, + 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} - QUOTAS.add_volume_type_opts(self.context, - reserve_opts, - vol_type['id']) - if reserve_vol_type_only: - 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) + reservations = quota_utils.get_volume_type_reservation( + self.context, volume, new_type.id, reserve_vol_type_only) + old_reservations = quota_utils.get_volume_type_reservation( + self.context, volume, old_vol_type['id'], reserve_vol_type_only, + negative=True) - old_reserve_opts = {'volumes': -1, 'gigabytes': -volume.size} - 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) + old_usage = db.quota_usage_get_all_by_project(elevated, project_id) 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: - usage = db.quota_usage_get(elevated, project_id, 'volumes') - new_total_volumes_in_use = usage.in_use - usage = db.quota_usage_get(elevated, project_id, 'gigabytes') - new_total_gigabytes_in_use = usage.in_use - except exception.QuotaUsageNotFound: - new_total_volumes_in_use = 0 - new_total_gigabytes_in_use = 0 - self.assertEqual(total_volumes_in_use, new_total_volumes_in_use) - self.assertEqual(total_gigabytes_in_use, - new_total_gigabytes_in_use) + new_usage = db.quota_usage_get_all_by_project(elevated, project_id) + for resource in ('volumes', 'gigabytes', 'snapshots'): + empty = {'in_use': 0, 'reserved': 0} + # Global resource hasn't changed + self.assertEqual(old_usage.get(resource, empty)['in_use'], + new_usage.get(resource, empty)['in_use']) + # The new type was empty before + self.assertEqual( + 0, old_usage.get(resource + '_new', empty)['in_use']) + # Old type resources have been moved to the new one + 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) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 4bcad1c14fd..a29c8be5576 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -1690,18 +1690,10 @@ class API(base.Base): # Get old reservations try: - reserve_opts = {'volumes': -1, 'gigabytes': -volume.size} - QUOTAS.add_volume_type_opts(context, - reserve_opts, - 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) + old_reservations = quota_utils.get_volume_type_reservation( + context, volume, volume.volume_type_id, + reserve_vol_type_only=True, negative=True) + except Exception: volume.status = volume.previous_status volume.save() diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index a5248a50336..0d3ac8aba43 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -200,7 +200,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, @@ -3048,6 +3048,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: diff --git a/releasenotes/notes/quota-on-retype-with-snapshots-2d9fc7b2c75f899d.yaml b/releasenotes/notes/quota-on-retype-with-snapshots-2d9fc7b2c75f899d.yaml new file mode 100644 index 00000000000..c1432385165 --- /dev/null +++ b/releasenotes/notes/quota-on-retype-with-snapshots-2d9fc7b2c75f899d.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + `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.