From: Zhiteng Huang Date: Fri, 5 Jul 2013 08:46:34 +0000 (-0700) Subject: Refactor reschedule in exception handling of volume manager X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=40aef764bb24330221b03bafc0e6d85fa493e383;p=openstack-build%2Fcinder-build.git Refactor reschedule in exception handling of volume manager Previous exception handling has a pitfall that may potentially clear the sys.exc_info() (by calling SQLalchmey to update db). This patch refactors some part of exception handling in create_volume() to make sure sys.exc_info() is retrieved so that we can log and reraise it; also we make sure exception be reraised at the end of exception handling no matter the request is rescheduled or not. As a side effect, we fixed a bug in unittest which didn't provide correct argument to db API but previously this exception has been wrongly consumed by volume manager's exception handling (not reraise exception when request is rescheduled). fix bug: 1197648 Change-Id: Idce5d06f8be1fb6018012503ec7f844898a21b25 --- diff --git a/cinder/tests/test_volume.py b/cinder/tests/test_volume.py index c04548923..f8d82c1f9 100644 --- a/cinder/tests/test_volume.py +++ b/cinder/tests/test_volume.py @@ -1267,9 +1267,9 @@ class VolumeTestCase(test.TestCase): image_location): raise exception.CinderException('fake exception') - def fake_reschedule_or_reraise(context, volume_id, exc_info, - snapshot_id, image_id, request_spec, - filter_properties, allow_reschedule): + def fake_reschedule_or_error(context, volume_id, exc_info, + snapshot_id, image_id, request_spec, + filter_properties): self.assertFalse(context.is_admin) self.assertFalse('admin' in context.roles) #compare context passed in with the context we saved @@ -1284,13 +1284,14 @@ class VolumeTestCase(test.TestCase): #create one copy of context for future comparison self.saved_ctxt = ctxt.deepcopy() - self.stubs.Set(self.volume, '_reschedule_or_reraise', - fake_reschedule_or_reraise) + self.stubs.Set(self.volume, '_reschedule_or_error', + fake_reschedule_or_error) self.stubs.Set(self.volume, '_create_volume', fake_create_volume) volume_src = self._create_volume() - self.volume.create_volume(ctxt, volume_src['id']) + self.assertRaises(exception.CinderException, + self.volume.create_volume, ctxt, volume_src['id']) def test_create_volume_from_sourcevol(self): """Test volume can be created from a source volume.""" @@ -1339,23 +1340,26 @@ class VolumeTestCase(test.TestCase): def test_create_volume_from_sourcevol_failed_clone(self): """Test src vol status will be restore by error handling code.""" def fake_error_create_cloned_volume(volume, src_vref): - db.volume_update(context, src_vref['id'], {'status': 'error'}) + db.volume_update(self.context, src_vref['id'], {'status': 'error'}) raise exception.CinderException('fake exception') - def fake_reschedule_or_reraise(context, volume_id, exc_info, - snapshot_id, image_id, request_spec, - filter_properties, allow_reschedule): + def fake_reschedule_or_error(context, volume_id, exc_info, + snapshot_id, image_id, request_spec, + filter_properties): pass - self.stubs.Set(self.volume, '_reschedule_or_reraise', - fake_reschedule_or_reraise) + self.stubs.Set(self.volume, '_reschedule_or_error', + fake_reschedule_or_error) self.stubs.Set(self.volume.driver, 'create_cloned_volume', fake_error_create_cloned_volume) volume_src = self._create_volume() self.volume.create_volume(self.context, volume_src['id']) volume_dst = self._create_volume(0, source_volid=volume_src['id']) - self.volume.create_volume(self.context, volume_dst['id'], - source_volid=volume_src['id']) + self.assertRaises(exception.CinderException, + self.volume.create_volume, + self.context, + volume_dst['id'], None, None, None, None, None, + volume_src['id']) self.assertEqual(volume_src['status'], 'creating') self.volume.delete_volume(self.context, volume_dst['id']) self.volume.delete_volume(self.context, volume_src['id']) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 0a97f8b0d..a446c3802 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -209,7 +209,7 @@ class VolumeManager(manager.SchedulerDependentManager): filter_properties=None, allow_reschedule=True, snapshot_id=None, image_id=None, source_volid=None): """Creates and exports the volume.""" - context_before_elevated = context.deepcopy() + context_saved = context.deepcopy() context = context.elevated() if filter_properties is None: filter_properties = {} @@ -272,18 +272,37 @@ class VolumeManager(manager.SchedulerDependentManager): {'status': 'error'}) return except Exception: + exc_info = sys.exc_info() # restore source volume status before reschedule + # FIXME(zhiteng) do all the clean-up before reschedule if sourcevol_ref is not None: self.db.volume_update(context, sourcevol_ref['id'], {'status': sourcevol_ref['status']}) - exc_info = sys.exc_info() + rescheduled = False # try to re-schedule volume: - self._reschedule_or_reraise(context_before_elevated, - volume_id, exc_info, - snapshot_id, image_id, - request_spec, filter_properties, - allow_reschedule) - return + if allow_reschedule: + rescheduled = self._reschedule_or_error(context_saved, + volume_id, + exc_info, + snapshot_id, + image_id, + request_spec, + filter_properties) + + if rescheduled: + # log the original build error + self._log_original_error(exc_info) + msg = (_('Creating %(volume_id)s %(snapshot_id)s ' + '%(image_id)s was rescheduled due to ' + '%(reason)s') + % {'volume_id': volume_id, + 'snapshot_id': snapshot_id, + 'image_id': image_id, + 'reason': unicode(exc_info[1])}) + raise exception.CinderException(msg) + else: + # not re-scheduling + raise exc_info[0], exc_info[1], exc_info[2] if model_update: volume_ref = self.db.volume_update( @@ -358,17 +377,11 @@ class VolumeManager(manager.SchedulerDependentManager): LOG.error(_('Error: %s') % traceback.format_exception(type_, value, tb)) - def _reschedule_or_reraise(self, context, volume_id, exc_info, - snapshot_id, image_id, request_spec, - filter_properties, allow_reschedule): - """Try to re-schedule the create or re-raise the original error to - error out the volume. - """ - if not allow_reschedule: - raise exc_info[0], exc_info[1], exc_info[2] - + def _reschedule_or_error(self, context, volume_id, exc_info, + snapshot_id, image_id, request_spec, + filter_properties): + """Try to re-schedule the request.""" rescheduled = False - try: method_args = (CONF.volume_topic, volume_id, snapshot_id, image_id, request_spec, filter_properties) @@ -378,18 +391,12 @@ class VolumeManager(manager.SchedulerDependentManager): self.scheduler_rpcapi.create_volume, method_args, exc_info) - except Exception: rescheduled = False LOG.exception(_("volume %s: Error trying to reschedule create"), volume_id) - if rescheduled: - # log the original build error - self._log_original_error(exc_info) - else: - # not re-scheduling - raise exc_info[0], exc_info[1], exc_info[2] + return rescheduled def _reschedule(self, context, request_spec, filter_properties, volume_id, scheduler_method, method_args,