From 454bdf0f0fc5d0eb58fa87855700995345c93fc6 Mon Sep 17 00:00:00 2001 From: Xing Yang Date: Sun, 9 Aug 2015 22:41:21 -0400 Subject: [PATCH] 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 --- cinder/backup/manager.py | 46 +++++++++++++++++++++----------- cinder/tests/unit/test_backup.py | 43 ++++++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 18 deletions(-) diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index e9449ba34..4c0031521 100644 --- 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 5276010b1..327af6e59 100644 --- 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) -- 2.45.2