Fix unsaved exception in backup/drivers

When an exception occurs during exception handling, it loses the
information of the first exception.

We should use the excutils.save_and_reraise_exception() in some cases.

Change-Id: I5d0ea53ba6c52138c71cca61aedbdf06338f2a7d
Closes-Bug: #1292380

cinder/backup/drivers/ceph.py

@@ -53,6 +53,7 @@ from oslo.config import cfg
 
 from cinder.backup.driver import BackupDriver
 from cinder import exception
+from cinder.openstack.common import excutils
 from cinder.openstack.common import log as logging
 from cinder.openstack.common import strutils
 from cinder.openstack.common import units
@@ -667,21 +668,19 @@ class CephBackupDriver(BackupDriver):
                 source_rbd_image.remove_snap(from_snap)
 
         except exception.BackupRBDOperationFailed:
-            LOG.debug("Differential backup transfer failed")
+            with excutils.save_and_reraise_exception():
+                LOG.debug("Differential backup transfer failed")
 
-            # Clean up if image was created as part of this operation
+                # Clean up if image was created as part of this operation
-            if image_created:
+                if image_created:
-                self._try_delete_base_image(backup_id, volume_id,
+                    self._try_delete_base_image(backup_id, volume_id,
-                                            base_name=base_name)
+                                                base_name=base_name)
 
-            # Delete snapshot
+                # Delete snapshot
-            LOG.debug("Deleting diff backup snapshot='%(snapshot)s' of "
+                LOG.debug("Deleting diff backup snapshot='%(snapshot)s' of "
-                      "source volume='%(volume)s'." %
+                          "source volume='%(volume)s'." %
-                      {'snapshot': new_snap, 'volume': volume_id})
+                          {'snapshot': new_snap, 'volume': volume_id})
-            source_rbd_image.remove_snap(new_snap)
+                source_rbd_image.remove_snap(new_snap)
-
-            # Re-raise the exception so that caller can try another approach
-            raise
 
     def _file_is_rbd(self, volume_file):
         """Returns True if the volume_file is actually an RBD image."""
@@ -881,9 +880,9 @@ class CephBackupDriver(BackupDriver):
             try:
                 self._backup_metadata(backup)
             except exception.BackupOperationError:
-                # Cleanup.
+                with excutils.save_and_reraise_exception():
-                self.delete(backup)
+                    # Cleanup.
-                raise
+                    self.delete(backup)
 
         LOG.debug("Backup '%(backup_id)s' of volume %(volume_id)s finished."
                   % {'backup_id': backup_id, 'volume_id': volume_id})

cinder/backup/drivers/swift.py

@@ -41,6 +41,7 @@ from oslo.config import cfg
 
 from cinder.backup.driver import BackupDriver
 from cinder import exception
+from cinder.openstack.common import excutils
 from cinder.openstack.common import log as logging
 from cinder.openstack.common import timeutils
 from cinder.openstack.common import units
@@ -349,10 +350,11 @@ class SwiftBackupDriver(BackupDriver):
             try:
                 self._backup_metadata(backup, object_meta)
             except Exception as err:
-                LOG.exception(_("Backup volume metadata to swift failed: %s")
+                with excutils.save_and_reraise_exception():
-                              % six.text_type(err))
+                    LOG.exception(
-                self.delete(backup)
+                        _("Backup volume metadata to swift failed: %s") %
-                raise
+                        six.text_type(err))
+                    self.delete(backup)
 
         self._finalize_backup(backup, container, object_meta)
 

cinder/tests/test_backup_ceph.py

@@ -14,6 +14,7 @@
 #    under the License.
 """ Tests for Ceph backup service."""
 
+import contextlib
 import hashlib
 import mock
 import os
@@ -452,6 +453,148 @@ class BackupCephTestCase(test.TestCase):
                             self.assertEqual(checksum.digest(),
                                              self.checksum.digest())
 
+    @common_mocks
+    @mock.patch('fcntl.fcntl', spec=True)
+    @mock.patch('subprocess.Popen', spec=True)
+    def test_backup_volume_from_rbd_fail(self, mock_popen, mock_fnctl):
+        """Test of when an exception occurs in an exception handler.
+
+        In _backup_rbd(), after an exception.BackupRBDOperationFailed
+        occurs in self._rbd_diff_transfer(), we want to check the
+        process when the second exception occurs in
+        self._try_delete_base_image().
+        """
+        backup_name = self.service._get_backup_base_name(self.backup_id,
+                                                         diff_format=True)
+
+        def mock_write_data():
+            self.volume_file.seek(0)
+            data = self.volume_file.read(self.data_length)
+            self.callstack.append('write')
+            checksum.update(data)
+            test_file.write(data)
+
+        def mock_read_data():
+            self.callstack.append('read')
+            return self.volume_file.read(self.data_length)
+
+        self._setup_mock_popen(mock_popen,
+                               ['out', 'err'],
+                               p1hook=mock_read_data,
+                               p2hook=mock_write_data)
+
+        self.mock_rbd.RBD.list = mock.Mock()
+        self.mock_rbd.RBD.list.return_value = [backup_name]
+
+        with contextlib.nested(
+                mock.patch.object(self.service, 'get_backup_snaps'),
+                mock.patch.object(self.service, '_rbd_diff_transfer')
+        ) as (_unused, mock_rbd_diff_transfer):
+            def mock_rbd_diff_transfer_side_effect(src_name, src_pool,
+                                                   dest_name, dest_pool,
+                                                   src_user, src_conf,
+                                                   dest_user, dest_conf,
+                                                   src_snap, from_snap):
+                raise exception.BackupRBDOperationFailed(_('mock'))
+
+            # Raise a pseudo exception.BackupRBDOperationFailed.
+            mock_rbd_diff_transfer.side_effect \
+                = mock_rbd_diff_transfer_side_effect
+            with contextlib.nested(
+                    mock.patch.object(self.service, '_full_backup'),
+                    mock.patch.object(self.service, '_try_delete_base_image')
+            ) as (_unused, mock_try_delete_base_image):
+                def mock_try_delete_base_image_side_effect(backup_id,
+                                                           volume_id,
+                                                           base_name):
+                    raise self.service.rbd.ImageNotFound(_('mock'))
+
+                # Raise a pesudo exception rbd.ImageNotFound.
+                mock_try_delete_base_image.side_effect \
+                    = mock_try_delete_base_image_side_effect
+                with mock.patch.object(self.service, '_backup_metadata'):
+                    with tempfile.NamedTemporaryFile() as test_file:
+                        checksum = hashlib.sha256()
+                        image = self.service.rbd.Image()
+                        meta = rbddriver.RBDImageMetadata(image,
+                                                          'pool_foo',
+                                                          'user_foo',
+                                                          'conf_foo')
+                        rbdio = rbddriver.RBDImageIOWrapper(meta)
+
+                        # We expect that the second exception is
+                        # notified.
+                        self.assertRaises(
+                            self.service.rbd.ImageNotFound,
+                            self.service.backup,
+                            self.backup, rbdio)
+
+    @common_mocks
+    @mock.patch('fcntl.fcntl', spec=True)
+    @mock.patch('subprocess.Popen', spec=True)
+    def test_backup_volume_from_rbd_fail2(self, mock_popen, mock_fnctl):
+        """Test of when an exception occurs in an exception handler.
+
+        In backup(), after an exception.BackupOperationError occurs in
+        self._backup_metadata(), we want to check the process when the
+        second exception occurs in self.delete().
+        """
+        backup_name = self.service._get_backup_base_name(self.backup_id,
+                                                         diff_format=True)
+
+        def mock_write_data():
+            self.volume_file.seek(0)
+            data = self.volume_file.read(self.data_length)
+            self.callstack.append('write')
+            checksum.update(data)
+            test_file.write(data)
+
+        def mock_read_data():
+            self.callstack.append('read')
+            return self.volume_file.read(self.data_length)
+
+        self._setup_mock_popen(mock_popen,
+                               ['out', 'err'],
+                               p1hook=mock_read_data,
+                               p2hook=mock_write_data)
+
+        self.mock_rbd.RBD.list = mock.Mock()
+        self.mock_rbd.RBD.list.return_value = [backup_name]
+
+        with contextlib.nested(
+                mock.patch.object(self.service, 'get_backup_snaps'),
+                mock.patch.object(self.service, '_rbd_diff_transfer'),
+                mock.patch.object(self.service, '_full_backup'),
+                mock.patch.object(self.service, '_backup_metadata')
+        ) as (_unused1, _u2, _u3, mock_backup_metadata):
+
+            def mock_backup_metadata_side_effect(backup):
+                raise exception.BackupOperationError(_('mock'))
+
+            # Raise a pseudo exception.BackupOperationError.
+            mock_backup_metadata.side_effect = mock_backup_metadata_side_effect
+            with mock.patch.object(self.service, 'delete') as mock_delete:
+                def mock_delete_side_effect(backup):
+                    raise self.service.rbd.ImageBusy()
+
+                # Raise a pseudo exception rbd.ImageBusy.
+                mock_delete.side_effect = mock_delete_side_effect
+                with tempfile.NamedTemporaryFile() as test_file:
+                    checksum = hashlib.sha256()
+                    image = self.service.rbd.Image()
+                    meta = rbddriver.RBDImageMetadata(image,
+                                                      'pool_foo',
+                                                      'user_foo',
+                                                      'conf_foo')
+                    rbdio = rbddriver.RBDImageIOWrapper(meta)
+
+                    # We expect that the second exception is
+                    # notified.
+                    self.assertRaises(
+                        self.service.rbd.ImageBusy,
+                        self.service.backup,
+                        self.backup, rbdio)
+
     @common_mocks
     def test_backup_vol_length_0(self):
         volume_id = str(uuid.uuid4())

cinder/tests/test_backup_swift.py

@@ -131,6 +131,62 @@ class BackupSwiftTestCase(test.TestCase):
                           service.backup,
                           backup, self.volume_file)
 
+    def test_backup_backup_metadata_fail(self):
+        """Test of when an exception occurs in backup().
+
+        In backup(), after an exception occurs in
+        self._backup_metadata(), we want to check the process of an
+        exception handler.
+        """
+        self._create_backup_db_entry()
+        self.flags(backup_compression_algorithm='none')
+        service = SwiftBackupDriver(self.ctxt)
+        self.volume_file.seek(0)
+        backup = db.backup_get(self.ctxt, 123)
+
+        def fake_backup_metadata(self, backup, object_meta):
+            raise exception.BackupDriverException(message=_('fake'))
+
+        # Raise a pseudo exception.BackupDriverException.
+        self.stubs.Set(SwiftBackupDriver, '_backup_metadata',
+                       fake_backup_metadata)
+
+        # We expect that an exception be notified directly.
+        self.assertRaises(exception.BackupDriverException,
+                          service.backup,
+                          backup, self.volume_file)
+
+    def test_backup_backup_metadata_fail2(self):
+        """Test of when an exception occurs in an exception handler.
+
+        In backup(), after an exception occurs in
+        self._backup_metadata(), we want to check the process when the
+        second exception occurs in self.delete().
+        """
+        self._create_backup_db_entry()
+        self.flags(backup_compression_algorithm='none')
+        service = SwiftBackupDriver(self.ctxt)
+        self.volume_file.seek(0)
+        backup = db.backup_get(self.ctxt, 123)
+
+        def fake_backup_metadata(self, backup, object_meta):
+            raise exception.BackupDriverException(message=_('fake'))
+
+        # Raise a pseudo exception.BackupDriverException.
+        self.stubs.Set(SwiftBackupDriver, '_backup_metadata',
+                       fake_backup_metadata)
+
+        def fake_delete(self, backup):
+            raise exception.BackupOperationError()
+
+        # Raise a pseudo exception.BackupOperationError.
+        self.stubs.Set(SwiftBackupDriver, 'delete', fake_delete)
+
+        # We expect that the second exception is notified.
+        self.assertRaises(exception.BackupOperationError,
+                          service.backup,
+                          backup, self.volume_file)
+
     def test_restore(self):
         self._create_backup_db_entry()
         service = SwiftBackupDriver(self.ctxt)