From 74960fb8de3a84d0f3a5a56f15143647b4182f01 Mon Sep 17 00:00:00 2001 From: Avishay Traeger Date: Wed, 17 Jul 2013 08:17:14 +0300 Subject: [PATCH] Fix extend_volume error handling. If the async call to the manager/driver failed, the API still updated the quota and volume size in the DB. Solution is to move these tasks down to the manager, where we know if the extend succeeded. Change-Id: I668fd659830bd6d410be64a1f5116377b08a9e96 Fixes: bug 1201814 --- cinder/tests/test_volume.py | 52 +++++++++++++++++++++++++++++++++++-- cinder/volume/api.py | 34 +----------------------- cinder/volume/manager.py | 44 ++++++++++++++++++++++++++----- 3 files changed, 89 insertions(+), 41 deletions(-) diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index 32274011b..7204749b1 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -1228,7 +1228,7 @@ class VolumeTestCase(test.TestCase): self.assertEqual(snapshots[2].id, u'4') def test_extend_volume(self): - """Test volume can be extended.""" + """Test volume can be extended at API level.""" # create a volume and assign to host volume = self._create_volume(2) self.volume.create_volume(self.context, volume['id']) @@ -1255,7 +1255,55 @@ class VolumeTestCase(test.TestCase): volume_api.extend(self.context, volume, 3) volume = db.volume_get(context.get_admin_context(), volume['id']) - self.assertEquals(volume['size'], 3) + self.assertEquals(volume['status'], 'extending') + + # clean up + self.volume.delete_volume(self.context, volume['id']) + + def test_extend_volume_manager(self): + """Test volume can be extended at the manager level.""" + def fake_reserve(context, expire=None, project_id=None, **deltas): + return ['RESERVATION'] + + def fake_reserve_exc(context, expire=None, project_id=None, **deltas): + raise exception.OverQuota(overs=['gigabytes'], + quotas={'gigabytes': 20}, + usages={'gigabytes': {'reserved': 5, + 'in_use': 15}}) + + def fake_extend_exc(volume, new_size): + raise exception.CinderException('fake exception') + + volume = self._create_volume(2) + self.volume.create_volume(self.context, volume['id']) + + # Test quota exceeded + self.stubs.Set(QUOTAS, 'reserve', fake_reserve_exc) + self.stubs.Set(QUOTAS, 'commit', lambda x, y, project_id=None: True) + self.stubs.Set(QUOTAS, 'rollback', lambda x, y: True) + volume['status'] = 'extending' + self.volume.extend_volume(self.context, volume['id'], '4') + volume = db.volume_get(context.get_admin_context(), volume['id']) + self.assertEquals(volume['size'], 2) + self.assertEquals(volume['status'], 'error_extending') + + # Test driver exception + self.stubs.Set(QUOTAS, 'reserve', fake_reserve) + self.stubs.Set(self.volume.driver, 'extend_volume', fake_extend_exc) + volume['status'] = 'extending' + self.volume.extend_volume(self.context, volume['id'], '4') + volume = db.volume_get(context.get_admin_context(), volume['id']) + self.assertEquals(volume['size'], 2) + self.assertEquals(volume['status'], 'error_extending') + + # Test driver success + self.stubs.Set(self.volume.driver, 'extend_volume', + lambda x, y: True) + volume['status'] = 'extending' + self.volume.extend_volume(self.context, volume['id'], '4') + volume = db.volume_get(context.get_admin_context(), volume['id']) + self.assertEquals(volume['size'], 4) + self.assertEquals(volume['status'], 'available') # clean up self.volume.delete_volume(self.context, volume['id']) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index fdb4169f7..c00573113 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -811,41 +811,9 @@ class API(base.Base): "extended: %(new_size)s)") % {'new_size': new_size, 'size': volume['size']}) raise exception.InvalidInput(reason=msg) - try: - reservations = QUOTAS.reserve(context, gigabytes=+size_increase) - except exception.OverQuota as exc: - overs = exc.kwargs['overs'] - usages = exc.kwargs['usages'] - quotas = exc.kwargs['quotas'] - - def _consumed(name): - return (usages[name]['reserved'] + usages[name]['in_use']) - - if 'gigabytes' in overs: - msg = _("Quota exceeded for %(s_pid)s, " - "tried to extend volume by " - "%(s_size)sG, (%(d_consumed)dG of %(d_quota)dG " - "already consumed)") - LOG.warn(msg % {'s_pid': context.project_id, - 's_size': size_increase, - 'd_consumed': _consumed('gigabytes'), - 'd_quota': quotas['gigabytes']}) - raise exception.VolumeSizeExceedsAvailableQuota() self.update(context, volume, {'status': 'extending'}) - - try: - self.volume_rpcapi.extend_volume(context, volume, new_size) - except Exception: - with excutils.save_and_reraise_exception(): - try: - self.update(context, volume, {'status': 'error_extending'}) - finally: - QUOTAS.rollback(context, reservations) - - self.update(context, volume, {'size': new_size}) - QUOTAS.commit(context, reservations) - self.update(context, volume, {'status': 'available'}) + self.volume_rpcapi.extend_volume(context, volume, new_size) class HostAPI(base.Base): diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 7c8500797..d658b8d12 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -802,14 +802,46 @@ class VolumeManager(manager.SchedulerDependentManager): extra_usage_info=extra_usage_info, host=self.host) def extend_volume(self, context, volume_id, new_size): - volume_ref = self.db.volume_get(context, volume_id) + volume = self.db.volume_get(context, volume_id) + size_increase = (int(new_size)) - volume['size'] + + try: + reservations = QUOTAS.reserve(context, gigabytes=+size_increase) + except exception.OverQuota as exc: + self.db.volume_update(context, volume['id'], + {'status': 'error_extending'}) + overs = exc.kwargs['overs'] + usages = exc.kwargs['usages'] + quotas = exc.kwargs['quotas'] + + def _consumed(name): + return (usages[name]['reserved'] + usages[name]['in_use']) + + if 'gigabytes' in overs: + msg = _("Quota exceeded for %(s_pid)s, " + "tried to extend volume by " + "%(s_size)sG, (%(d_consumed)dG of %(d_quota)dG " + "already consumed)") + LOG.error(msg % {'s_pid': context.project_id, + 's_size': size_increase, + 'd_consumed': _consumed('gigabytes'), + 'd_quota': quotas['gigabytes']}) + return try: - LOG.info(_("volume %s: extending"), volume_ref['name']) - self.driver.extend_volume(volume_ref, new_size) - LOG.info(_("volume %s: extended successfully"), volume_ref['name']) + LOG.info(_("volume %s: extending"), volume['name']) + self.driver.extend_volume(volume, new_size) + LOG.info(_("volume %s: extended successfully"), volume['name']) except Exception: LOG.exception(_("volume %s: Error trying to extend volume"), volume_id) - self.db.volume_update(context, volume_ref['id'], - {'status': 'error_extending'}) + try: + self.db.volume_update(context, volume['id'], + {'status': 'error_extending'}) + finally: + QUOTAS.rollback(context, reservations) + return + + QUOTAS.commit(context, reservations) + self.db.volume_update(context, volume['id'], {'size': int(new_size), + 'status': 'available'}) -- 2.45.2