Skip to content

Commit

Permalink
Refactor reschedule in exception handling of volume manager
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
Zhiteng Huang committed Jul 8, 2013
1 parent e958394 commit 40aef76
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 39 deletions.
32 changes: 18 additions & 14 deletions cinder/tests/test_volume.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1267,9 +1267,9 @@ def fake_create_volume(context, volume_ref, snapshot_ref,
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
Expand All @@ -1284,13 +1284,14 @@ def fake_reschedule_or_reraise(context, volume_id, exc_info,
#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."""
Expand Down Expand Up @@ -1339,23 +1340,26 @@ def fake_create_cloned_volume(volume, src_vref):
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'])
Expand Down
57 changes: 32 additions & 25 deletions cinder/volume/manager.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ def create_volume(self, context, volume_id, request_spec=None,
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 = {}
Expand Down Expand Up @@ -272,18 +272,37 @@ def create_volume(self, context, volume_id, request_spec=None,
{'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(
Expand Down Expand Up @@ -358,17 +377,11 @@ def _log_original_error(self, exc_info):
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)
Expand All @@ -378,18 +391,12 @@ def _reschedule_or_reraise(self, context, volume_id, exc_info,
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,
Expand Down

0 comments on commit 40aef76

Please sign in to comment.