From: lisali Date: Tue, 7 Jul 2015 07:34:32 +0000 (+0800) Subject: Fix restore point if backup base is diff-format in ceph X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=e7c7355589c29c16af2bc73ce06164a72ff5af09;p=openstack-build%2Fcinder-build.git Fix restore point if backup base is diff-format in ceph In Ceph when backup base is diff-format and use this backup to restore volumes not rbd, need to find out the correct restore point. This is fixed in cinder/backup/drivers/ceph.py _diff_restore_allowed. Before the change: when backup is diff-format, and destination volume is not rbd, the function returns restore_point as None. As a result, backup restore uses wrong image/snapshot to do restore. With the change, we always get the right restore point, regardless of the kind of restore we'll do differential or not. Change-Id: I006c05d246f59fc6aff597543bedc68589d37576 Closes-Bug: #1472088 --- diff --git a/cinder/backup/drivers/ceph.py b/cinder/backup/drivers/ceph.py index 089a5add7..e197eb3fa 100644 --- a/cinder/backup/drivers/ceph.py +++ b/cinder/backup/drivers/ceph.py @@ -1033,41 +1033,45 @@ class CephBackupDriver(driver.BackupDriver): def _diff_restore_allowed(self, base_name, backup, volume, volume_file, rados_client): - """Determine whether a differential restore is possible/allowed. + """Determine if differential restore is possible and restore point. + + Determine whether a differential restore is possible/allowed, + and find out the restore point if backup base is diff-format. In order for a differential restore to be performed we need: * destination volume must be RBD * destination volume must have zero extents * backup base image must exist * backup must have a restore point + * target volume is different from source volume of backup Returns True if differential restore is allowed, False otherwise. + Return the restore point if back base is diff-format. """ - not_allowed = (False, None) - - if self._file_is_rbd(volume_file): - # NOTE(dosaboy): base_name here must be diff format. - rbd_exists, base_name = self._rbd_image_exists(base_name, - backup['volume_id'], - rados_client) + # NOTE(dosaboy): base_name here must be diff format. + rbd_exists, base_name = self._rbd_image_exists(base_name, + backup['volume_id'], + rados_client) + + if not rbd_exists: + return False, None + + # Get the restore point. If no restore point is found, we assume + # that the backup was not performed using diff/incremental methods + # so we enforce full copy. + restore_point = self._get_restore_point(base_name, backup['id']) + + if restore_point: + if self._file_is_rbd(volume_file): + + # If the volume we are restoring to is the volume the backup + # was made from, force a full restore since a diff will not + # work in this case. + if volume['id'] == backup['volume_id']: + LOG.debug("Destination volume is same as backup source " + "volume %s - forcing full copy.", volume['id']) + return False, restore_point - if not rbd_exists: - return not_allowed - - # Get the restore point. If no restore point is found, we assume - # that the backup was not performed using diff/incremental methods - # so we enforce full copy. - restore_point = self._get_restore_point(base_name, backup['id']) - - # If the volume we are restoring to is the volume the backup was - # made from, force a full restore since a diff will not work in - # this case. - if volume['id'] == backup['volume_id']: - LOG.debug("Destination volume is same as backup source volume " - "%s - forcing full copy.", volume['id']) - return False, restore_point - - if restore_point: # If the destination volume has extents we cannot allow a diff # restore. if self._rbd_has_extents(volume_file.rbd_image): @@ -1077,14 +1081,14 @@ class CephBackupDriver(driver.BackupDriver): return False, restore_point return True, restore_point - else: - LOG.info(_LI("No restore point found for " - "backup='%(backup)s' of " - "volume %(volume)s - forcing full copy."), - {'backup': backup['id'], - 'volume': backup['volume_id']}) - - return not_allowed + else: + LOG.info(_LI("No restore point found for backup=" + "'%(backup)s' of volume %(volume)s " + "although base image is found - " + "forcing full copy."), + {'backup': backup['id'], + 'volume': backup['volume_id']}) + return False, restore_point def _restore_volume(self, backup, volume, volume_file): """Restore volume from backup using diff transfer if possible. diff --git a/cinder/tests/unit/test_backup_ceph.py b/cinder/tests/unit/test_backup_ceph.py index b5e9d4399..4e51cce32 100644 --- a/cinder/tests/unit/test_backup_ceph.py +++ b/cinder/tests/unit/test_backup_ceph.py @@ -772,94 +772,149 @@ class BackupCephTestCase(test.TestCase): self.assertEqual(RAISED_EXCEPTIONS, [MockImageNotFoundException]) @common_mocks - def test_diff_restore_allowed(self): + def test_diff_restore_allowed_with_image_not_exists(self): + '''Test diff restore not allowed when backup not diff-format.''' not_allowed = (False, None) backup_base = 'backup.base' - restore_point = 'backup.snap.1' rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) args_vols_different = [backup_base, self.backup, self.alt_volume, rbd_io, self.mock_rados] - args_vols_same = [backup_base, self.backup, self.volume, rbd_io, - self.mock_rados] - # 1. destination volume is not an rbd - with mock.patch.object(self.service, '_file_is_rbd') as \ - mock_file_is_rbd: - mock_file_is_rbd.return_value = False + with mock.patch.object(self.service, '_rbd_image_exists') as \ + mock_rbd_image_exists: + mock_rbd_image_exists.return_value = (False, backup_base) + resp = self.service._diff_restore_allowed(*args_vols_different) - self.assertEqual(resp, not_allowed) - self.assertTrue(mock_file_is_rbd.called) - # 1. destination volume is an rbd - # 2. backup base is not diff-format - with mock.patch.object(self.service, '_file_is_rbd') as \ - mock_file_is_rbd: - mock_file_is_rbd.return_value = True - with mock.patch.object(self.service, '_rbd_image_exists') as \ - mock_rbd_image_exists: - mock_rbd_image_exists.return_value = (False, backup_base) - resp = self.service._diff_restore_allowed(*args_vols_different) - self.assertEqual(resp, not_allowed) - self.assertTrue(mock_file_is_rbd.called) + self.assertEqual(not_allowed, resp) + mock_rbd_image_exists.assert_called_once_with( + backup_base, + self.backup['volume_id'], + self.mock_rados) + + @common_mocks + def test_diff_restore_allowed_with_no_restore_point(self): + '''Test diff restore not allowed when no restore point found. + + Detail conditions: + 1. backup base is diff-format + 2. restore point does not exist + ''' + not_allowed = (False, None) + backup_base = 'backup.base' + rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) + args_vols_different = [backup_base, self.backup, self.alt_volume, + rbd_io, self.mock_rados] + + with mock.patch.object(self.service, '_rbd_image_exists') as \ + mock_rbd_image_exists: + mock_rbd_image_exists.return_value = (True, backup_base) + with mock.patch.object(self.service, '_get_restore_point') as \ + mock_get_restore_point: + mock_get_restore_point.return_value = None + + args = args_vols_different + resp = self.service._diff_restore_allowed(*args) + + self.assertEqual(not_allowed, resp) self.assertTrue(mock_rbd_image_exists.called) + mock_get_restore_point.assert_called_once_with( + backup_base, + self.backup['id']) - # 1. destination volume is an rbd - # 2. backup base is diff-format - # 3. restore point does not exist - # 4. source and destination volumes are different - with mock.patch.object(self.service, '_file_is_rbd') as \ - mock_file_is_rbd: - mock_file_is_rbd.return_value = True - with mock.patch.object(self.service, '_rbd_image_exists') as \ - mock_rbd_image_exists: - mock_rbd_image_exists.return_value = (True, backup_base) - with mock.patch.object(self.service, '_get_restore_point') as \ - mock_get_restore_point: - mock_get_restore_point.return_value = None + @common_mocks + def test_diff_restore_allowed_with_not_rbd(self): + '''Test diff restore not allowed when destination volume is not rbd. + + Detail conditions: + 1. backup base is diff-format + 2. restore point exists + 3. destination volume is not an rbd. + ''' + backup_base = 'backup.base' + restore_point = 'backup.snap.1' + rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) + args_vols_different = [backup_base, self.backup, self.alt_volume, + rbd_io, self.mock_rados] + + with mock.patch.object(self.service, '_rbd_image_exists') as \ + mock_rbd_image_exists: + mock_rbd_image_exists.return_value = (True, backup_base) + with mock.patch.object(self.service, '_get_restore_point') as \ + mock_get_restore_point: + mock_get_restore_point.return_value = restore_point + with mock.patch.object(self.service, '_file_is_rbd') as \ + mock_file_is_rbd: + mock_file_is_rbd.return_value = False args = args_vols_different resp = self.service._diff_restore_allowed(*args) - self.assertEqual(resp, not_allowed) - self.assertTrue(mock_file_is_rbd.called) + self.assertEqual((False, restore_point), resp) self.assertTrue(mock_rbd_image_exists.called) self.assertTrue(mock_get_restore_point.called) + mock_file_is_rbd.assert_called_once_with( + rbd_io) - # 1. destination volume is an rbd - # 2. backup base is diff-format - # 3. restore point does not exist - # 4. source and destination volumes are the same - with mock.patch.object(self.service, '_file_is_rbd') as \ - mock_file_is_rbd: - mock_file_is_rbd.return_value = True - with mock.patch.object(self.service, '_rbd_image_exists') as \ - mock_rbd_image_exists: - mock_rbd_image_exists.return_value = (True, backup_base) - with mock.patch.object(self.service, '_get_restore_point') as \ - mock_get_restore_point: - mock_get_restore_point.return_value = None + @common_mocks + def test_diff_restore_allowed_with_same_volume(self): + '''Test diff restore not allowed when volumes are same. + + Detail conditions: + 1. backup base is diff-format + 2. restore point exists + 3. destination volume is an rbd + 4. source and destination volumes are the same + ''' + backup_base = 'backup.base' + restore_point = 'backup.snap.1' + rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) + args_vols_same = [backup_base, self.backup, self.volume, rbd_io, + self.mock_rados] + + with mock.patch.object(self.service, '_rbd_image_exists') as \ + mock_rbd_image_exists: + mock_rbd_image_exists.return_value = (True, backup_base) + with mock.patch.object(self.service, '_get_restore_point') as \ + mock_get_restore_point: + mock_get_restore_point.return_value = restore_point + with mock.patch.object(self.service, '_file_is_rbd') as \ + mock_file_is_rbd: + mock_file_is_rbd.return_value = True resp = self.service._diff_restore_allowed(*args_vols_same) - self.assertEqual(resp, not_allowed) - self.assertTrue(mock_file_is_rbd.called) + self.assertEqual((False, restore_point), resp) self.assertTrue(mock_rbd_image_exists.called) self.assertTrue(mock_get_restore_point.called) + self.assertTrue(mock_file_is_rbd.called) - # 1. destination volume is an rbd - # 2. backup base is diff-format - # 3. restore point exists - # 4. source and destination volumes are different - # 5. destination volume has data on it - full copy is mandated - with mock.patch.object(self.service, '_file_is_rbd') as \ - mock_file_is_rbd: - mock_file_is_rbd.return_value = True - with mock.patch.object(self.service, '_rbd_image_exists') as \ - mock_rbd_image_exists: - mock_rbd_image_exists.return_value = (True, backup_base) - with mock.patch.object(self.service, '_get_restore_point') as \ - mock_get_restore_point: - mock_get_restore_point.return_value = restore_point + @common_mocks + def test_diff_restore_allowed_with_has_extents(self): + '''Test diff restore not allowed when destination volume has data. + + Detail conditions: + 1. backup base is diff-format + 2. restore point exists + 3. destination volume is an rbd + 4. source and destination volumes are different + 5. destination volume has data on it - full copy is mandated + ''' + backup_base = 'backup.base' + restore_point = 'backup.snap.1' + rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) + args_vols_different = [backup_base, self.backup, self.alt_volume, + rbd_io, self.mock_rados] + + with mock.patch.object(self.service, '_rbd_image_exists') as \ + mock_rbd_image_exists: + mock_rbd_image_exists.return_value = (True, backup_base) + with mock.patch.object(self.service, '_get_restore_point') as \ + mock_get_restore_point: + mock_get_restore_point.return_value = restore_point + with mock.patch.object(self.service, '_file_is_rbd') as \ + mock_file_is_rbd: + mock_file_is_rbd.return_value = True with mock.patch.object(self.service, '_rbd_has_extents') \ as mock_rbd_has_extents: mock_rbd_has_extents.return_value = True @@ -867,26 +922,39 @@ class BackupCephTestCase(test.TestCase): args = args_vols_different resp = self.service._diff_restore_allowed(*args) - self.assertEqual(resp, (False, restore_point)) - self.assertTrue(mock_file_is_rbd.called) + self.assertEqual((False, restore_point), resp) self.assertTrue(mock_rbd_image_exists.called) self.assertTrue(mock_get_restore_point.called) - self.assertTrue(mock_rbd_has_extents.called) + self.assertTrue(mock_file_is_rbd.called) + mock_rbd_has_extents.assert_called_once_with( + rbd_io.rbd_image) - # 1. destination volume is an rbd - # 2. backup base is diff-format - # 3. restore point exists - # 4. source and destination volumes are different - # 5. destination volume no data on it - with mock.patch.object(self.service, '_file_is_rbd') as \ - mock_file_is_rbd: - mock_file_is_rbd.return_value = True - with mock.patch.object(self.service, '_rbd_image_exists') as \ - mock_rbd_image_exists: - mock_rbd_image_exists.return_value = (True, backup_base) - with mock.patch.object(self.service, '_get_restore_point') as \ - mock_restore_point: - mock_restore_point.return_value = restore_point + @common_mocks + def test_diff_restore_allowed_with_no_extents(self): + '''Test diff restore allowed when no data in destination volume. + + Detail conditions: + 1. backup base is diff-format + 2. restore point exists + 3. destination volume is an rbd + 4. source and destination volumes are different + 5. destination volume no data on it + ''' + backup_base = 'backup.base' + restore_point = 'backup.snap.1' + rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) + args_vols_different = [backup_base, self.backup, self.alt_volume, + rbd_io, self.mock_rados] + + with mock.patch.object(self.service, '_rbd_image_exists') as \ + mock_rbd_image_exists: + mock_rbd_image_exists.return_value = (True, backup_base) + with mock.patch.object(self.service, '_get_restore_point') as \ + mock_get_restore_point: + mock_get_restore_point.return_value = restore_point + with mock.patch.object(self.service, '_file_is_rbd') as \ + mock_file_is_rbd: + mock_file_is_rbd.return_value = True with mock.patch.object(self.service, '_rbd_has_extents') \ as mock_rbd_has_extents: mock_rbd_has_extents.return_value = False @@ -894,11 +962,11 @@ class BackupCephTestCase(test.TestCase): args = args_vols_different resp = self.service._diff_restore_allowed(*args) - self.assertEqual(resp, (True, restore_point)) - self.assertTrue(mock_restore_point.called) - self.assertTrue(mock_rbd_has_extents.called) + self.assertEqual((True, restore_point), resp) self.assertTrue(mock_rbd_image_exists.called) + self.assertTrue(mock_get_restore_point.called) self.assertTrue(mock_file_is_rbd.called) + self.assertTrue(mock_rbd_has_extents.called) @common_mocks @mock.patch('fcntl.fcntl', spec=True)