Handle missing temp volume and snapshot during cleanup

When backup service is started, we try to clean up temp volumes
and snapshots in previously failed backups. If the temp volume
or snapshot is already deleted, we will get VolumeNotFound or
SnapshotNotFound exceptions. These exceptions should be handled.
Also temp_volume_id and temp_snapshot_id should be set to None
after they are deleted.

Change-Id: Ia20834bcc89040364bce71fc66c32c1777a5ac11
Closes-Bug: #1484774

diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py
index e9449ba34be1beeebb89252a6954127bfddf81de..4c00315211b8ebd99b4875512e3b0be275c562d0 100644 (file)
--- a/cinder/backup/manager.py
+++ b/cinder/backup/manager.py
@@ -273,22 +273,38 @@ class BackupManager(manager.SchedulerDependentManager):
                           "backup %s.", backup.id)
                 continue
             if backup.temp_volume_id and backup.status == 'error':
-                temp_volume = self.db.volume_get(ctxt,
-                                                 backup.temp_volume_id)
-                # The temp volume should be deleted directly thru the
-                # the volume driver, not thru the volume manager.
-                mgr.driver.delete_volume(temp_volume)
-                self.db.volume_destroy(ctxt, temp_volume['id'])
+                try:
+                    temp_volume = self.db.volume_get(ctxt,
+                                                     backup.temp_volume_id)
+                    # The temp volume should be deleted directly thru the
+                    # the volume driver, not thru the volume manager.
+                    mgr.driver.delete_volume(temp_volume)
+                    self.db.volume_destroy(ctxt, temp_volume['id'])
+                except exception.VolumeNotFound:
+                    LOG.debug("Could not find temp volume %(vol)s to clean up "
+                              "for backup %(backup)s.",
+                              {'vol': backup.temp_volume_id,
+                               'backup': backup.id})
+                backup.temp_volume_id = None
+                backup.save()
             if backup.temp_snapshot_id and backup.status == 'error':
-                temp_snapshot = objects.Snapshot.get_by_id(
-                    ctxt, backup.temp_snapshot_id)
-                # The temp snapshot should be deleted directly thru the
-                # volume driver, not thru the volume manager.
-                mgr.driver.delete_snapshot(temp_snapshot)
-                with temp_snapshot.obj_as_admin():
-                    self.db.volume_glance_metadata_delete_by_snapshot(
-                        ctxt, temp_snapshot.id)
-                    temp_snapshot.destroy()
+                try:
+                    temp_snapshot = objects.Snapshot.get_by_id(
+                        ctxt, backup.temp_snapshot_id)
+                    # The temp snapshot should be deleted directly thru the
+                    # volume driver, not thru the volume manager.
+                    mgr.driver.delete_snapshot(temp_snapshot)
+                    with temp_snapshot.obj_as_admin():
+                        self.db.volume_glance_metadata_delete_by_snapshot(
+                            ctxt, temp_snapshot.id)
+                        temp_snapshot.destroy()
+                except exception.SnapshotNotFound:
+                    LOG.debug("Could not find temp snapshot %(snap)s to clean "
+                              "up for backup %(backup)s.",
+                              {'snap': backup.temp_snapshot_id,
+                               'backup': backup.id})
+                backup.temp_snapshot_id = None
+                backup.save()
 
     def create_backup(self, context, backup):
         """Create volume backups using configured backup service."""

diff --git a/cinder/tests/unit/test_backup.py b/cinder/tests/unit/test_backup.py
index 5276010b1bd483773935426a0d95f9a4c4354a06..327af6e59d294e0ebc2791852c80f725231678ea 100644 (file)
--- a/cinder/tests/unit/test_backup.py
+++ b/cinder/tests/unit/test_backup.py
@@ -230,9 +230,8 @@ class BackupTestCase(BaseBackupTest):
 
     @mock.patch.object(db, 'volume_get')
     @ddt.data(KeyError, exception.VolumeNotFound)
-    def test_cleanup_temp_volumes_snapshots(self,
-                                            err,
-                                            mock_volume_get):
+    def test_cleanup_temp_volumes_snapshots_volume_not_found(
+            self, err, mock_volume_get):
         """Ensure we handle missing volume for a backup."""
         mock_volume_get.side_effect = [err]
 
@@ -242,6 +241,44 @@ class BackupTestCase(BaseBackupTest):
         self.assertIsNone(self.backup_mgr._cleanup_temp_volumes_snapshots(
             backups))
 
+    @mock.patch.object(lvm.LVMVolumeDriver, 'delete_snapshot')
+    def test_cleanup_temp_snapshot_not_found(self,
+                                             mock_delete_snapshot):
+        """Ensure we handle missing temp snapshot for a backup."""
+        vol1_id = self._create_volume_db_entry()
+        self._create_volume_attach(vol1_id)
+        db.volume_update(self.ctxt, vol1_id, {'status': 'backing-up'})
+        backup1 = self._create_backup_db_entry(status='error',
+                                               volume_id=vol1_id,
+                                               temp_snapshot_id='fake')
+        backups = [backup1]
+        self.assertEqual('fake', backups[0].temp_snapshot_id)
+        self.assertIsNone(self.backup_mgr._cleanup_temp_volumes_snapshots(
+            backups))
+        self.assertFalse(mock_delete_snapshot.called)
+        self.assertIsNone(backups[0].temp_snapshot_id)
+        backup1.destroy()
+        db.volume_destroy(self.ctxt, vol1_id)
+
+    @mock.patch.object(lvm.LVMVolumeDriver, 'delete_volume')
+    def test_cleanup_temp_volume_not_found(self,
+                                           mock_delete_volume):
+        """Ensure we handle missing temp volume for a backup."""
+        vol1_id = self._create_volume_db_entry()
+        self._create_volume_attach(vol1_id)
+        db.volume_update(self.ctxt, vol1_id, {'status': 'backing-up'})
+        backup1 = self._create_backup_db_entry(status='error',
+                                               volume_id=vol1_id,
+                                               temp_volume_id='fake')
+        backups = [backup1]
+        self.assertEqual('fake', backups[0].temp_volume_id)
+        self.assertIsNone(self.backup_mgr._cleanup_temp_volumes_snapshots(
+            backups))
+        self.assertFalse(mock_delete_volume.called)
+        self.assertIsNone(backups[0].temp_volume_id)
+        backup1.destroy()
+        db.volume_destroy(self.ctxt, vol1_id)
+
     def test_create_backup_with_bad_volume_status(self):
         """Test creating a backup from a volume with a bad status."""
         vol_id = self._create_volume_db_entry(status='restoring', size=1)