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 c04548923dbc2279f3496f1ec2512d8e650c2553..f8d82c1f96595765560d10d286703e39399226ec 100644 (file)
--- 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 0a97f8b0d2cb910f504434b5f5a732b808306272..a446c3802aafe5e8204e6f095a3b923c8a2f6a8b 100644 (file)
--- 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,