Fix leftovers after backup abort

When aborting a backup on any chunked driver we will be leaving chunks
in the backend without Cinder knowing so and with no way of deleting
them from Cinder.  In this case the only way to delete them is going to
the storage itself and deleting them manually.

Another issue that will happen if we are using a temporary resource for
the backup, be it a volume or a snapshot, is that it will not be cleaned
up and will be left for us to manually issue the delete through the
Cinder API.

The first issue is caused by the chunked driver's assumption that the
`refresh` method in an OVO will ignore the context's `read_deleted`
configuration and always read the record, which is not true.  And since
it doesn't work when the record is deleted there will be leftovers if
the status of the backup transitions to deleted during the processing of
a chunk.

The second issue is caused by the same thing, but in this case is when
the backup manager refreshes the backup OVO to know the temporary
resource it needs to clean up.

This patches fixes the incorrect behavior of the backup abort mechanism
to prevent leaving things behind.

Closes-Bug: #1746559
Change-Id: Idcfdbf815f404982d26618710a291054f19be736

cinder/backup/chunkeddriver.py

@@ -516,7 +516,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

@@ -394,21 +394,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):
@@ -451,7 +459,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

@@ -297,6 +297,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

@@ -220,6 +220,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

@@ -567,6 +567,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.