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
(cherry picked from commit b77fb72aa9)
This commit is contained in:
Gorka Eguileor 2021-03-18 17:19:34 +01:00
parent 0c9512f941
commit 4600957444
6 changed files with 138 additions and 67 deletions

View File

@ -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

View File

@ -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)

View File

@ -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)

View File

@ -1688,18 +1688,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()

View File

@ -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:

View File

@ -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.