Merge "Fix leftovers after backup abort"

cinder/backup/chunkeddriver.py

@@ -559,7 +559,8 @@ class ChunkedBackupDriver(driver.BackupDriver):
             # First of all, we check the status of this backup. If it
             # has been changed to delete or has been deleted, we cancel the
             # backup process to do forcing delete.
-            backup.refresh()
+            with backup.as_read_deleted():
+                backup.refresh()
             if backup.status in (fields.BackupStatus.DELETING,
                                  fields.BackupStatus.DELETED):
                 is_backup_canceled = True

cinder/backup/manager.py

@@ -405,21 +405,29 @@ class BackupManager(manager.ThreadPoolManager):
             self.db.volume_update(context, volume_id,
                                   {'status': previous_status,
                                    'previous_status': 'backing-up'})
-        backup.status = fields.BackupStatus.AVAILABLE
-        backup.size = volume['size']
 
-        if updates:
-            backup.update(updates)
-        backup.save()
+        # _run_backup method above updated the status for the backup, so it
+        # will reflect latest status, even if it is deleted
+        completion_msg = 'finished'
+        if backup.status in (fields.BackupStatus.DELETING,
+                             fields.BackupStatus.DELETED):
+            completion_msg = 'aborted'
+        else:
+            backup.status = fields.BackupStatus.AVAILABLE
+            backup.size = volume['size']
 
-        # Handle the num_dependent_backups of parent backup when child backup
-        # has created successfully.
-        if backup.parent_id:
-            parent_backup = objects.Backup.get_by_id(context,
-                                                     backup.parent_id)
-            parent_backup.num_dependent_backups += 1
-            parent_backup.save()
-        LOG.info('Create backup finished. backup: %s.', backup.id)
+            if updates:
+                backup.update(updates)
+            backup.save()
+
+            # Handle the num_dependent_backups of parent backup when child
+            # backup has created successfully.
+            if backup.parent_id:
+                parent_backup = objects.Backup.get_by_id(context,
+                                                         backup.parent_id)
+                parent_backup.num_dependent_backups += 1
+                parent_backup.save()
+        LOG.info('Create backup %s. backup: %s.', completion_msg, backup.id)
         self._notify_about_backup_usage(context, backup, "create.end")
 
     def _run_backup(self, context, backup, volume):
@@ -471,7 +479,8 @@ class BackupManager(manager.ThreadPoolManager):
                                     backup_device.is_snapshot, force=True,
                                     ignore_errors=True)
         finally:
-            backup = objects.Backup.get_by_id(context, backup.id)
+            with backup.as_read_deleted():
+                backup.refresh()
             self._cleanup_temp_volumes_snapshots_when_backup_created(
                 context, backup)
         return updates

cinder/objects/base.py

@@ -298,6 +298,34 @@ class CinderPersistentObject(object):
         finally:
             self._context = original_context
 
+    @contextlib.contextmanager
+    def as_read_deleted(self, mode='yes'):
+        """Context manager to make OVO with modified read deleted context.
+
+        This temporarily modifies the context embedded in an object to
+        have a different `read_deleted` parameter.
+
+        Parameter mode accepts most of the same parameters as our `model_query`
+        DB method.  We support 'yes', 'no', and 'only'.
+
+        usage:
+
+           with obj.as_read_deleted():
+               obj.refresh()
+           if obj.status = 'deleted':
+               ...
+        """
+        if self._context is None:
+            raise exception.OrphanedObjectError(method='as_read_deleted',
+                                                objtype=self.obj_name())
+
+        original_mode = self._context.read_deleted
+        self._context.read_deleted = mode
+        try:
+            yield
+        finally:
+            self._context.read_deleted = original_mode
+
     @classmethod
     def _get_expected_attrs(cls, context, *args, **kwargs):
         return None

cinder/tests/unit/backup/drivers/test_backup_nfs.py

@@ -250,6 +250,38 @@ class BackupNFSSwiftBasedTestCase(test.TestCase):
         backup = objects.Backup.get_by_id(self.ctxt, FAKE_BACKUP_ID)
         self.assertEqual(backup['container'], UPDATED_CONTAINER_NAME)
 
+    def test_backup_cancel(self):
+        """Test the backup abort mechanism when backup is force deleted."""
+        count = set()
+
+        def my_refresh():
+            # This refresh method will abort the backup after 1 chunk
+            count.add(len(count) + 1)
+            if len(count) == 2:
+                backup.destroy()
+            original_refresh()
+
+        volume_id = fake.VOLUME_ID
+        self._create_backup_db_entry(volume_id=volume_id,
+                                     container=None,
+                                     backup_id=FAKE_BACKUP_ID)
+        service = nfs.NFSBackupDriver(self.ctxt)
+        self.volume_file.seek(0)
+        backup = objects.Backup.get_by_id(self.ctxt, FAKE_BACKUP_ID)
+        original_refresh = backup.refresh
+
+        # We cannot mock refresh method in backup object directly because
+        # mock will raise AttributeError on context manager exit.
+        with mock.patch('cinder.objects.base.CinderPersistentObject.refresh',
+                        side_effect=my_refresh), \
+                mock.patch.object(service, 'delete_object',
+                                  side_effect=service.delete_object) as delete:
+            # Driver shouldn't raise the NotFound exception
+            service.backup(backup, self.volume_file)
+
+            # Ensure we called the delete_backup method when abort is detected
+            self.assertEqual(1, delete.call_count)
+
     @mock.patch('cinder.backup.drivers.posix.PosixBackupDriver.'
                 'update_container_name',
                 return_value='testcontainer1')

cinder/tests/unit/backup/test_backup.py

@@ -588,6 +588,31 @@ class BackupTestCase(BaseBackupTest):
         self.assertEqual(fields.BackupStatus.ERROR, backup['status'])
         self.assertTrue(mock_run_backup.called)
 
+    @mock.patch('cinder.backup.manager.BackupManager._run_backup')
+    def test_create_backup_aborted(self, run_backup_mock):
+        """Test error handling when abort occurs during backup creation."""
+        def my_run_backup(*args, **kwargs):
+            backup.destroy()
+            with backup.as_read_deleted():
+                original_refresh()
+
+        run_backup_mock.side_effect = my_run_backup
+        vol_id = self._create_volume_db_entry(size=1)
+        backup = self._create_backup_db_entry(volume_id=vol_id)
+        original_refresh = backup.refresh
+
+        self.backup_mgr.create_backup(self.ctxt, backup)
+
+        self.assertTrue(run_backup_mock.called)
+
+        vol = objects.Volume.get_by_id(self.ctxt, vol_id)
+        self.assertEqual('available', vol.status)
+        self.assertEqual('backing-up', vol['previous_status'])
+        # Make sure we didn't set the backup to available after it was deleted
+        with backup.as_read_deleted():
+            backup.refresh()
+        self.assertEqual(fields.BackupStatus.DELETED, backup.status)
+
     @mock.patch('cinder.backup.manager.BackupManager._run_backup',
                 side_effect=FakeBackupException(str(uuid.uuid4())))
     def test_create_backup_with_snapshot_error(self, mock_run_backup):

cinder/tests/unit/objects/test_base.py

@@ -59,6 +59,7 @@ class TestCinderObjectVersionHistory(test_objects.BaseObjectsTestCase):
                           history.add, '1.0', {'Backup': '1.0'})
 
 
+@ddt.ddt
 class TestCinderObject(test_objects.BaseObjectsTestCase):
     """Tests methods from CinderObject."""
 
@@ -160,6 +161,21 @@ class TestCinderObject(test_objects.BaseObjectsTestCase):
 
         MyTestObject.cinder_ovo_cls_init.assert_called_once_with()
 
+    def test_as_read_deleted_default(self):
+        volume = objects.Volume(context=self.context)
+        self.assertEqual('no', volume._context.read_deleted)
+        with volume.as_read_deleted():
+            self.assertEqual('yes', volume._context.read_deleted)
+        self.assertEqual('no', volume._context.read_deleted)
+
+    @ddt.data('yes', 'no', 'only')
+    def test_as_read_deleted_modes(self, mode):
+        volume = objects.Volume(context=self.context)
+        self.assertEqual('no', volume._context.read_deleted)
+        with volume.as_read_deleted(mode=mode):
+            self.assertEqual(mode, volume._context.read_deleted)
+        self.assertEqual('no', volume._context.read_deleted)
+
 
 class TestCinderComparableObject(test_objects.BaseObjectsTestCase):
     def test_comparable_objects(self):

releasenotes/notes/fix-abort-backup-df196e9dcb992586.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    We no longer leave orphaned chunks on the backup backend or leave a
+    temporary volume/snapshot when aborting a backup.