From 3f67de92cfcfc61ca26156961e1f2d4d2ebded66 Mon Sep 17 00:00:00 2001 From: KIYOHIRO ADACHI Date: Fri, 14 Mar 2014 16:04:57 +0900 Subject: [PATCH] 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 | 31 ++++--- cinder/backup/drivers/swift.py | 10 ++- cinder/tests/test_backup_ceph.py | 143 ++++++++++++++++++++++++++++++ cinder/tests/test_backup_swift.py | 56 ++++++++++++ 4 files changed, 220 insertions(+), 20 deletions(-) diff --git a/cinder/backup/drivers/ceph.py b/cinder/backup/drivers/ceph.py index 2d97d1dd7..6546dfe43 100644 --- a/cinder/backup/drivers/ceph.py +++ b/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 - if image_created: - self._try_delete_base_image(backup_id, volume_id, - base_name=base_name) + # Clean up if image was created as part of this operation + if image_created: + self._try_delete_base_image(backup_id, volume_id, + base_name=base_name) - # Delete snapshot - LOG.debug("Deleting diff backup snapshot='%(snapshot)s' of " - "source volume='%(volume)s'." % - {'snapshot': new_snap, 'volume': volume_id}) - source_rbd_image.remove_snap(new_snap) - - # Re-raise the exception so that caller can try another approach - raise + # Delete snapshot + LOG.debug("Deleting diff backup snapshot='%(snapshot)s' of " + "source volume='%(volume)s'." % + {'snapshot': new_snap, 'volume': volume_id}) + source_rbd_image.remove_snap(new_snap) 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. - self.delete(backup) - raise + with excutils.save_and_reraise_exception(): + # Cleanup. + self.delete(backup) LOG.debug("Backup '%(backup_id)s' of volume %(volume_id)s finished." % {'backup_id': backup_id, 'volume_id': volume_id}) diff --git a/cinder/backup/drivers/swift.py b/cinder/backup/drivers/swift.py index 8eb83f574..a36e9cad9 100644 --- a/cinder/backup/drivers/swift.py +++ b/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") - % six.text_type(err)) - self.delete(backup) - raise + with excutils.save_and_reraise_exception(): + LOG.exception( + _("Backup volume metadata to swift failed: %s") % + six.text_type(err)) + self.delete(backup) self._finalize_backup(backup, container, object_meta) diff --git a/cinder/tests/test_backup_ceph.py b/cinder/tests/test_backup_ceph.py index 074b46477..ed328ef13 100644 --- a/cinder/tests/test_backup_ceph.py +++ b/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()) diff --git a/cinder/tests/test_backup_swift.py b/cinder/tests/test_backup_swift.py index 202d175e5..d02af3b3d 100644 --- a/cinder/tests/test_backup_swift.py +++ b/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) -- 2.45.2